fix(providers): MCP env var expansion supports ${VAR} brace syntax

[kombiz]

Summary

• Problem: expandEnvVarsInRecord in packages/providers/src/claude/provider.ts only matched the bare $VAR_NAME form. The ${VAR_NAME} brace syntax — standard in shell, Docker Compose, and MCP server documentation — was silently passed through unexpanded, causing MCP servers to receive literal ${...} strings instead of resolved secrets.
• Why it matters: Users copy MCP config examples from official tooling that uses ${VAR} form. The silent failure (no error, garbage value) makes this particularly hard to debug.
• What changed: Extended the regex in expandEnvVarsInRecord to a two-branch alternation that handles both $VAR and ${VAR} in a single pass; added 4 new test cases; updated the MCP servers guide to document both forms.
• What did not change (scope boundary): command, args, and url fields are intentionally not expanded (security boundary). Lowercase variable names remain unsupported. No other env expansion systems (workflow variable substitution) were touched.

UX Journey

Before

User writes MCP config:
  { "env": { "API_KEY": "${MY_API_KEY}" } }

  User                    Archon                   MCP Server
  ────                    ──────                   ──────────
  sets MY_API_KEY=secret
  runs workflow ────────▶ loads mcp.json
                          expandEnvVarsInRecord()
                          regex: /\$([A-Z_][A-Z0-9_]*)/g
                          "${MY_API_KEY}" → no match
                          passes literal through ──────────▶ receives "${MY_API_KEY}"
                                                              (broken — auth fails silently)

After

User writes MCP config:
  { "env": { "API_KEY": "${MY_API_KEY}" } }

  User                    Archon                   MCP Server
  ────                    ──────                   ──────────
  sets MY_API_KEY=secret
  runs workflow ────────▶ loads mcp.json
                          expandEnvVarsInRecord()
                          [~] regex: /\$(?:\{([A-Z_][A-Z0-9_]*)\}|([A-Z_][A-Z0-9_]*))/g
                          "${MY_API_KEY}" → matches brace branch → "secret"
                          passes resolved value ───────────▶ receives "secret" ✓

Architecture Diagram

Before

dag-executor.ts
    └── loadMcpConfig()
            └── expandEnvVars()
                    └── expandEnvVarsInRecord()
                            └── /\$([A-Z_][A-Z0-9_]*)/g
                                (only matches bare $VAR)

After

dag-executor.ts
    └── loadMcpConfig()
            └── expandEnvVars()
                    └── expandEnvVarsInRecord()
                            [~] └── /\$(?:\{([A-Z_][A-Z0-9_]*)\}|([A-Z_][A-Z0-9_]*))/g
                                    (matches both $VAR and ${VAR})

Connection inventory:

From	To	Status	Notes
dag-executor.ts	loadMcpConfig	unchanged	call site unchanged
loadMcpConfig	expandEnvVars	unchanged	call site unchanged
expandEnvVars	expandEnvVarsInRecord	unchanged	call site unchanged
expandEnvVarsInRecord	regex engine	modified	alternation regex added

Label Snapshot

• Risk: risk: low
• Size: size: XS
• Scope: providers
• Module: providers:claude

Change Metadata

• Change type: bug
• Primary scope: providers

Linked Issue

• Closes MCP env var expansion misses ${VAR} brace syntax #1612
• Supersedes fix(providers): expand ${VAR} brace syntax in MCP env var expansion #1621 (archon/task-fix-issue-1612-v2 — had merge conflicts with dev; this is an equivalent, conflict-free implementation)

Validation Evidence (required)

bun run type-check   # ✅ All 10 packages pass
bun run lint         # ✅ 0 errors, 0 warnings (zero-tolerance policy)
bun run format:check # ✅ Pass
bun run test         # ✅ ~3,538 tests pass across all packages, 0 failures

• Evidence: All 4 new tests for ${VAR} brace syntax are in packages/workflows/src/dag-executor.test.ts and pass. The targeted run bun test packages/workflows/src/dag-executor.test.ts --test-name-pattern "loadMcpConfig" shows 15/15 pass (11 existing + 4 new).
• No commands skipped.

Security Impact (required)

• New permissions/capabilities? No
• New external network calls? No
• Secrets/tokens handling changed? Yes — the fix correctly resolves env var references that were previously passed as literals. MCP servers that use ${VAR} syntax will now receive the actual secret value instead of the unexpanded string. This is the intended behavior; the prior behavior was a bug.
• File system access scope changed? No

The expansion is scoped to env and headers fields only — same as before. command, args, and url are intentionally excluded (security boundary, unchanged).

Compatibility / Migration

• Backward compatible? Yes — $VAR expansion is unchanged; ${VAR} previously passed through as-is (broken), now resolves correctly.
• Config/env changes? No
• Database migration needed? No

Human Verification (required)

• Verified scenarios:
  • ${VAR} in env values expands to the env value
  • $VAR and ${VAR} mixed in the same string both expand correctly in one pass
  • ${MISSING_VAR} is reported in missingVars and replaced with ''
  • ${VAR} in headers values expands correctly
  • All pre-existing $VAR tests continue to pass (no regression)
• Edge cases checked: missing var in brace form; mixed syntax in single string value
• What was not verified: manual end-to-end test with a live MCP server (automated tests cover the expansion logic thoroughly)

Side Effects / Blast Radius (required)

• Affected subsystems/workflows: Only expandEnvVarsInRecord in @archon/providers. Called only from expandEnvVars → loadMcpConfig → dag-executor.ts. No other call sites.
• Potential unintended effects: None identified. The regex change is additive — it adds a new matching branch before the existing one; the existing $VAR branch behavior is preserved exactly.
• Guardrails: The 4 new tests serve as regression guards. missingVars reporting is unchanged for both forms.

Rollback Plan (required)

• Fast rollback: git revert c9c8c88f — single commit, no migrations, no config changes.
• Feature flags or config toggles: None.
• Observable failure symptoms: MCP servers receiving ${VAR_NAME} literal strings instead of resolved values; auth failures in MCP-dependent workflow nodes.

Risks and Mitigations

• Risk: Regex alternation introduces subtle precedence issue where brace form is matched when bare form was intended.
  • Mitigation: The brace branch is \{([A-Z_][A-Z0-9_]*)\} — it requires a closing }, so ${VAR (unclosed) won't match either branch and passes through unchanged (same behavior as before). Test coverage includes both forms.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c2a7da7e-13d2-4494-8b6d-489832923967

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[kombiz]

🔍 Comprehensive PR Review

PR: #1650 — fix(providers): MCP env var expansion supports `${VAR}` brace syntax
Reviewed by: 3 specialized agents (code-review, test-coverage, docs-impact)
Date: 2026-05-11

---

Summary

This XS PR extends expandEnvVarsInRecord in packages/providers/src/claude/provider.ts to support ${VAR} brace syntax alongside the existing $VAR form. The regex change is logically correct, the 4 new tests cover all meaningful cases, and the doc update is accurate. No blocking issues found.

Verdict: ✅ APPROVE

Severity	Count
🔴 CRITICAL	0
🟠 HIGH	0
🟡 MEDIUM	1
🟢 LOW	4

---

🟡 Medium Issue (Optional — Recommended Fix)

Boundary test missing for ${VAR} in command/args fields

📍 packages/workflows/src/dag-executor.test.ts:2399

The existing security boundary test 'does not expand vars in command or args fields' only uses bare $TEST_CMD_445 syntax. If a future refactor accidentally expanded command/args for brace syntax, it would go undetected.

Fix is 2 lines — extend the existing test's args array:

View suggested fix

it('does not expand vars in command or args fields', async () => {
  process.env.TEST_CMD_445 = 'should-not-expand';
  const config = {
    svc: {
      command: '$TEST_CMD_445',
      args: ['$TEST_CMD_445', '${TEST_CMD_445}'],  // add brace variant
    },
  };
  await writeFile(join(testDir, 'mcp.json'), JSON.stringify(config));

  const result = await loadMcpConfig('mcp.json', testDir);
  const server = result.servers.svc as Record<string, unknown>;
  expect(server.command).toBe('$TEST_CMD_445');
  expect(server.args).toEqual(['$TEST_CMD_445', '${TEST_CMD_445}']);  // both unchanged

  delete process.env.TEST_CMD_445;
});

---

🟢 Low Issues

View 4 low-priority suggestions

Issue	Location	Agent	Suggestion
varName === undefined guard is unreachable dead code (TypeScript narrowing workaround)	provider.ts:226	code-review, test-coverage	Add a comment: // varName is always string — regex alternation guarantees one group matches, or replace with (braced ?? bare) as string
New tests use inline delete without try/finally	dag-executor.test.ts:2317, 2335–2336, 2367	code-review	Consistent with pre-existing pattern in same describe block; leave as-is or refactor whole block to afterEach
No code example in mcp-servers.md shows ${VAR} in practice	mcp-servers.md	docs-impact	Add one mixed-syntax example to the "Environment Variable Expansion" section (e.g., "DATABASE_URL": "${DATABASE_URL}")
Duplicate of first finding (reported by 2 agents)	provider.ts:226	test-coverage	See above

---

✅ What's Good

• Regex correctness: /\$(?:\{([A-Z_][A-Z0-9_]*)\}|([A-Z_][A-Z0-9_]*))/g correctly alternates between brace and bare forms. Non-capturing outer group is the right choice.
• Minimal scope: Only expandEnvVarsInRecord is touched. The security boundary (command, args, url not expanded) is preserved.
• Symmetric test coverage: 4 new tests mirror the 4 pre-existing bare-syntax tests — including a mixed-syntax test that validates the alternation doesn't cause offset corruption.
• Env var naming: _1612 suffixes prevent cross-test pollution with pre-existing _445 variables.
• Doc included in PR: mcp-servers.md update is accurate with both forms documented and the security boundary noted.
• No new abstractions: Pure extension of an existing function. No new interfaces, modules, or config keys.
• CLAUDE.md compliance: No any types, no console.log, immutable result construction, zero ESLint disable comments.

---

📋 Suggested Follow-up Issues

Issue Title	Priority
Harden loadMcpConfig tests: use afterEach for env var cleanup in loadMcpConfig describe block	P3
Add ${VAR} mixed-syntax example to mcp-servers.md Environment Variable Expansion section	P3

---

Reviewed by Archon comprehensive-pr-review workflow — 3 agents (code-review, test-coverage, docs-impact)
Full artifacts: /home/laddy/.archon/workspaces/coleam00/Archon/artifacts/runs/542c3767a32d7c79a3beae7acf273715/review/

[kombiz]

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-test-issue-1612-smoke
Commit: 91832ed
Philosophy: Fix everything unless clearly a new concern

---

Fixes Applied (3 total)

Severity	Count
🔴 CRITICAL	0
🟠 HIGH	0
🟡 MEDIUM	1
🟢 LOW	2

View all fixes

• ✅ MEDIUM: Boundary test missing for ${VAR} in command/args (dag-executor.test.ts:2401) — Extended existing 'does not expand vars in command or args fields' test to include '${TEST_CMD_445}' in the args array and assert both bare and brace forms remain unexpanded
• ✅ LOW: varName === undefined guard is unreachable TypeScript narrowing workaround (provider.ts:226) — Added inline comment: // varName is always string — regex alternation guarantees one group matches
• ✅ LOW: No code example shows ${VAR} brace form (mcp-servers.md:131) — Updated DATABASE_URL value in the Environment Variable Expansion example from "$DATABASE_URL" to "${DATABASE_URL}", creating a mixed-syntax example that mirrors the mixed-syntax test

---

Tests Added

(none — existing boundary test extended, not a new file)

Docs Updated

• packages/docs-web/src/content/docs/guides/mcp-servers.md — DATABASE_URL example now shows ${VAR} brace form alongside $VAR_NAME

---

Skipped (1)

Finding	Reason
New tests use inline delete without try/finally	The code-review agent explicitly recommended leaving as-is (consistent with pre-existing pattern in the loadMcpConfig describe block). Full afterEach cleanup refactor is a separate P3 follow-up.

---

Suggested Follow-up Issues

1. "Harden loadMcpConfig tests: use afterEach for env var cleanup in loadMcpConfig describe block" — P3, affects all tests in the block for consistency

---

Validation

✅ Type check | ✅ Lint | ✅ Tests (210 passed)

---

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-test-issue-1612-smoke

[Wirasm]

Closing as superseded by #1728 (merged aa71520, fixes #1612). Thanks @kombiz for the earlier attempt — the surgical regex + tests + docs landed in #1728 covers the same case.