From d9e24a18811af8692e5c7ce2b4f4aaf09e48cdaa Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 4 Apr 2026 20:12:49 -0700 Subject: [PATCH 1/5] fix: [#635] accept ${VAR} shell/dotenv syntax for env interpolation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/docs/configure/mcp-servers.md | 11 +++ packages/opencode/src/config/paths.ts | 10 ++- .../test/config/paths-parsetext.test.ts | 75 +++++++++++++++++++ 3 files changed, 95 insertions(+), 1 deletion(-) diff --git a/docs/docs/configure/mcp-servers.md b/docs/docs/configure/mcp-servers.md index 59547953fe..d01ef360e7 100644 --- a/docs/docs/configure/mcp-servers.md +++ b/docs/docs/configure/mcp-servers.md @@ -20,6 +20,17 @@ Run an MCP server as a local subprocess: } ``` +### Environment variable interpolation + +Both syntaxes work anywhere in the config: + +| Syntax | Example | +|--------|---------| +| `{env:VAR}` | `"API_KEY": "{env:MY_API_KEY}"` | +| `${VAR}` | `"API_KEY": "${MY_API_KEY}"` (shell / dotenv / VS Code style) | + +If the variable is not set, it resolves to an empty string. Bare `$VAR` (without braces) is **not** interpolated — use `${VAR}` or `{env:VAR}`. + | Field | Type | Description | |-------|------|-------------| | `type` | `"local"` | Local subprocess server | diff --git a/packages/opencode/src/config/paths.ts b/packages/opencode/src/config/paths.ts index 1629df5179..4296330fc3 100644 --- a/packages/opencode/src/config/paths.ts +++ b/packages/opencode/src/config/paths.ts @@ -84,11 +84,19 @@ export namespace ConfigPaths { return typeof input === "string" ? path.dirname(input) : input.dir } - /** Apply {env:VAR} and {file:path} substitutions to config text. */ + /** Apply {env:VAR}, ${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 — accept ${VAR} shell/dotenv syntax as alias for {env:VAR} + // Users arriving from Claude Code / VS Code / dotenv / docker-compose expect this + // convention. Only matches POSIX identifier names to avoid collisions with random + // ${...} content. See issue #635. + text = text.replace(/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g, (_, varName) => { + return process.env[varName] || "" + }) + // 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 index b3c8d92c87..edddc2da70 100644 --- a/packages/opencode/test/config/paths-parsetext.test.ts +++ b/packages/opencode/test/config/paths-parsetext.test.ts @@ -97,6 +97,81 @@ 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("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() From 31fdbcfa293bf07833aa496c4caa17c91e188b8d Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 4 Apr 2026 20:34:01 -0700 Subject: [PATCH 2/5] fix: address consensus review findings for PR #655 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/docs/configure/mcp-servers.md | 11 ++++-- .../src/cli/cmd/tui/component/tips.tsx | 2 +- packages/opencode/src/config/paths.ts | 13 +++++-- .../test/config/paths-parsetext.test.ts | 38 +++++++++++++++++++ 4 files changed, 55 insertions(+), 9 deletions(-) diff --git a/docs/docs/configure/mcp-servers.md b/docs/docs/configure/mcp-servers.md index d01ef360e7..5677788d42 100644 --- a/docs/docs/configure/mcp-servers.md +++ b/docs/docs/configure/mcp-servers.md @@ -24,13 +24,16 @@ Run an MCP server as a local subprocess: Both syntaxes work anywhere in the config: -| Syntax | Example | -|--------|---------| -| `{env:VAR}` | `"API_KEY": "{env:MY_API_KEY}"` | -| `${VAR}` | `"API_KEY": "${MY_API_KEY}"` (shell / dotenv / VS Code style) | +| Syntax | Injection mode | Example | +|--------|----------------|---------| +| `${VAR}` | String-safe (JSON-escaped) | `"API_KEY": "${MY_API_KEY}"` — shell / dotenv / VS Code style | +| `{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, 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/cli/cmd/tui/component/tips.tsx b/packages/opencode/src/cli/cmd/tui/component/tips.tsx index 818edfed01..4f30372b39 100644 --- 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 index 4296330fc3..0532c74502 100644 --- a/packages/opencode/src/config/paths.ts +++ b/packages/opencode/src/config/paths.ts @@ -84,7 +84,7 @@ export namespace ConfigPaths { return typeof input === "string" ? path.dirname(input) : input.dir } - /** Apply {env:VAR}, ${VAR}, and {file:path} substitutions to config text. */ + /** 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] || "" @@ -92,10 +92,15 @@ export namespace ConfigPaths { // altimate_change start — accept ${VAR} shell/dotenv syntax as alias for {env:VAR} // Users arriving from Claude Code / VS Code / dotenv / docker-compose expect this // convention. Only matches POSIX identifier names to avoid collisions with random - // ${...} content. See issue #635. - text = text.replace(/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g, (_, varName) => { - return process.env[varName] || "" + // ${...} content. Value is JSON-escaped so it can't break out of the enclosing + // string — use {env:VAR} for raw unquoted injection. Docker-compose convention: + // $${VAR} escapes to literal ${VAR}. See issue #635. + text = text.replace(/(? { + const value = process.env[varName] || "" + return JSON.stringify(value).slice(1, -1) }) + // Unescape: $${VAR} → ${VAR} (user-authored literal preservation, docker-compose style) + text = text.replace(/\$\$(\{[A-Za-z_][A-Za-z0-9_]*\})/g, "$$$1") // altimate_change end const fileMatches = Array.from(text.matchAll(/\{file:[^}]+\}/g)) diff --git a/packages/opencode/test/config/paths-parsetext.test.ts b/packages/opencode/test/config/paths-parsetext.test.ts index edddc2da70..8f0aa4f38b 100644 --- a/packages/opencode/test/config/paths-parsetext.test.ts +++ b/packages/opencode/test/config/paths-parsetext.test.ts @@ -152,6 +152,44 @@ describe("ConfigPaths.parseText: ${VAR} substitution (shell/dotenv alias)", () = } }) + 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("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("works inside MCP environment config (issue #635 regression)", async () => { process.env.OPENCODE_TEST_GITLAB_TOKEN = "glpat-xxxxx" try { From 90d56f622bc94fe407a45797238aab8f6c9cd5fd Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 4 Apr 2026 20:39:38 -0700 Subject: [PATCH 3/5] feat: add ${VAR:-default} syntax for fallback values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/docs/configure/mcp-servers.md | 5 +- packages/opencode/src/config/paths.ts | 14 +++-- .../test/config/paths-parsetext.test.ts | 60 +++++++++++++++++++ 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/docs/docs/configure/mcp-servers.md b/docs/docs/configure/mcp-servers.md index 5677788d42..e1c50d5d49 100644 --- a/docs/docs/configure/mcp-servers.md +++ b/docs/docs/configure/mcp-servers.md @@ -26,11 +26,12 @@ Both syntaxes work anywhere in the config: | Syntax | Injection mode | Example | |--------|----------------|---------| -| `${VAR}` | String-safe (JSON-escaped) | `"API_KEY": "${MY_API_KEY}"` — shell / dotenv / VS Code style | +| `${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, it resolves to an empty string. Bare `$VAR` (without braces) is **not** interpolated — use `${VAR}` or `{env:VAR}`. +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. diff --git a/packages/opencode/src/config/paths.ts b/packages/opencode/src/config/paths.ts index 0532c74502..a79d8430eb 100644 --- a/packages/opencode/src/config/paths.ts +++ b/packages/opencode/src/config/paths.ts @@ -93,14 +93,18 @@ export namespace ConfigPaths { // Users arriving from Claude Code / VS Code / dotenv / docker-compose expect this // convention. Only matches POSIX identifier names to avoid collisions with random // ${...} content. Value is JSON-escaped so it can't break out of the enclosing - // string — use {env:VAR} for raw unquoted injection. Docker-compose convention: - // $${VAR} escapes to literal ${VAR}. See issue #635. - text = text.replace(/(? { - const value = process.env[varName] || "" + // string — use {env:VAR} for raw unquoted injection. Supports ${VAR:-default} + // for fallback values (docker-compose / POSIX shell convention: default used when + // the variable is unset OR empty). Docker-compose convention: $${VAR} escapes to + // literal ${VAR}. See issue #635. + text = text.replace(/(? { + const envValue = process.env[varName] + const value = envValue !== undefined && envValue !== "" ? envValue : (fallback ?? "") return JSON.stringify(value).slice(1, -1) }) // Unescape: $${VAR} → ${VAR} (user-authored literal preservation, docker-compose style) - text = text.replace(/\$\$(\{[A-Za-z_][A-Za-z0-9_]*\})/g, "$$$1") + // Handles both ${VAR} and ${VAR:-default} forms. + text = text.replace(/\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})/g, "$$$1") // altimate_change end const fileMatches = Array.from(text.matchAll(/\{file:[^}]+\}/g)) diff --git a/packages/opencode/test/config/paths-parsetext.test.ts b/packages/opencode/test/config/paths-parsetext.test.ts index 8f0aa4f38b..012d51fb17 100644 --- a/packages/opencode/test/config/paths-parsetext.test.ts +++ b/packages/opencode/test/config/paths-parsetext.test.ts @@ -178,6 +178,66 @@ describe("ConfigPaths.parseText: ${VAR} substitution (shell/dotenv alias)", () = } }) + 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 { From a9367e1e8b3539cd151b99463e7d4dc671c6f217 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 4 Apr 2026 20:43:06 -0700 Subject: [PATCH 4/5] feat: add config_env_interpolation telemetry event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../opencode/src/altimate/telemetry/index.ts | 19 ++++++++ packages/opencode/src/config/paths.ts | 43 +++++++++++++++++-- .../opencode/test/telemetry/telemetry.test.ts | 3 +- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index c30b0c2f06..33d296e834 100644 --- 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/config/paths.ts b/packages/opencode/src/config/paths.ts index a79d8430eb..5fb0af702f 100644 --- a/packages/opencode/src/config/paths.ts +++ b/packages/opencode/src/config/paths.ts @@ -86,9 +86,16 @@ export namespace ConfigPaths { /** Apply {env:VAR} and {file:path} substitutions to config text. */ async function substitute(text: string, input: ParseSource, missing: "error" | "empty" = "error") { + // altimate_change start — track interpolation stats for telemetry + let legacyBraceRefs = 0 + let legacyBraceUnresolved = 0 text = text.replace(/\{env:([^}]+)\}/g, (_, varName) => { - return process.env[varName] || "" + legacyBraceRefs++ + const v = process.env[varName] + if (v === undefined || v === "") legacyBraceUnresolved++ + return v || "" }) + // altimate_change end // altimate_change start — accept ${VAR} shell/dotenv syntax as alias for {env:VAR} // Users arriving from Claude Code / VS Code / dotenv / docker-compose expect this // convention. Only matches POSIX identifier names to avoid collisions with random @@ -97,14 +104,44 @@ export namespace ConfigPaths { // for fallback values (docker-compose / POSIX shell convention: default used when // the variable is unset OR empty). Docker-compose convention: $${VAR} escapes to // literal ${VAR}. See issue #635. + let dollarRefs = 0 + let dollarUnresolved = 0 + let dollarDefaulted = 0 text = text.replace(/(? { + dollarRefs++ const envValue = process.env[varName] - const value = envValue !== undefined && envValue !== "" ? envValue : (fallback ?? "") + const resolved = envValue !== undefined && envValue !== "" + if (!resolved && fallback !== undefined) dollarDefaulted++ + if (!resolved && fallback === undefined) dollarUnresolved++ + const value = resolved ? envValue : (fallback ?? "") return JSON.stringify(value).slice(1, -1) }) // Unescape: $${VAR} → ${VAR} (user-authored literal preservation, docker-compose style) // Handles both ${VAR} and ${VAR:-default} forms. - text = text.replace(/\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})/g, "$$$1") + let dollarEscaped = 0 + text = text.replace(/\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})/g, (_, rest) => { + dollarEscaped++ + return "$" + rest + }) + // 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)) diff --git a/packages/opencode/test/telemetry/telemetry.test.ts b/packages/opencode/test/telemetry/telemetry.test.ts index 8500ac6c3a..45966c5106 100644 --- 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) }) }) From 45dd7bddd07234b2cf1ff3409b180f7b396fdebb Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sat, 4 Apr 2026 20:55:19 -0700 Subject: [PATCH 5/5] fix: address P1 double-substitution from cubic/coderabbit review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (? { - legacyBraceRefs++ - const v = process.env[varName] - if (v === undefined || v === "") legacyBraceUnresolved++ - return v || "" - }) - // altimate_change end - // altimate_change start — accept ${VAR} shell/dotenv syntax as alias for {env:VAR} - // Users arriving from Claude Code / VS Code / dotenv / docker-compose expect this - // convention. Only matches POSIX identifier names to avoid collisions with random - // ${...} content. Value is JSON-escaped so it can't break out of the enclosing - // string — use {env:VAR} for raw unquoted injection. Supports ${VAR:-default} - // for fallback values (docker-compose / POSIX shell convention: default used when - // the variable is unset OR empty). Docker-compose convention: $${VAR} escapes to - // literal ${VAR}. See issue #635. + // 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 - text = text.replace(/(? { - dollarRefs++ - const envValue = process.env[varName] - const resolved = envValue !== undefined && envValue !== "" - if (!resolved && fallback !== undefined) dollarDefaulted++ - if (!resolved && fallback === undefined) dollarUnresolved++ - const value = resolved ? envValue : (fallback ?? "") - return JSON.stringify(value).slice(1, -1) - }) - // Unescape: $${VAR} → ${VAR} (user-authored literal preservation, docker-compose style) - // Handles both ${VAR} and ${VAR:-default} forms. let dollarEscaped = 0 - text = text.replace(/\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})/g, (_, rest) => { - dollarEscaped++ - return "$" + rest - }) + let legacyBraceRefs = 0 + let legacyBraceUnresolved = 0 + text = text.replace( + /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(? { + 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) { diff --git a/packages/opencode/test/config/paths-parsetext.test.ts b/packages/opencode/test/config/paths-parsetext.test.ts index 012d51fb17..b91d48562a 100644 --- a/packages/opencode/test/config/paths-parsetext.test.ts +++ b/packages/opencode/test/config/paths-parsetext.test.ts @@ -250,6 +250,37 @@ describe("ConfigPaths.parseText: ${VAR} substitution (shell/dotenv alias)", () = } }) + 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 {