Skip to content

fix(providers): expand ${VAR_NAME} brace form in MCP config env vars#1725

Closed
coleam00 wants to merge 3 commits into
devfrom
archon/task-fix-issue-1612-dryrun
Closed

fix(providers): expand ${VAR_NAME} brace form in MCP config env vars#1725
coleam00 wants to merge 3 commits into
devfrom
archon/task-fix-issue-1612-dryrun

Conversation

@coleam00
Copy link
Copy Markdown
Owner

Summary

  • Problem: expandEnvVarsInRecord in packages/providers/src/mcp/config.ts only expanded $VAR_NAME (bare form). Users who wrote ${VAR_NAME} — following shell, Docker Compose, and most MCP server docs — had the literal string passed through unexpanded, silently breaking auth tokens, database URLs, and similar secrets.
  • Why it matters: MCP servers are often documented with the brace form (${VAR_NAME}). Every user who followed those docs had their env-var expansion silently fail.
  • What changed: The single-group regex in expandEnvVarsInRecord was replaced with an alternation regex capturing both ${VAR} and $VAR forms. Four tests added. Docs updated to document both forms.
  • What did not change: Bare $VAR_NAME expansion behavior is identical. No new files, no new imports, no schema changes.

UX Journey

Before

User writes in MCP config:
  env:
    TOKEN: "${MY_SECRET}"    ← follows shell/Docker Compose convention

Archon loads MCP config:
  expandEnvVarsInRecord: regex /\$([A-Z_][A-Z0-9_]*)/ does NOT match "${MY_SECRET}"
  → MCP server receives literal string "${MY_SECRET}" as TOKEN
  → Auth fails silently — no error, no warning

After

User writes in MCP config:
  env:
    TOKEN: "${MY_SECRET}"    ← follows shell/Docker Compose convention

Archon loads MCP config:
  expandEnvVarsInRecord: regex /\$(?:\{([A-Z_][A-Z0-9_]*)\}|([A-Z_][A-Z0-9_]*))/ matches both forms
  → varName = braced ?? bare ?? ''
  → MCP server receives expanded value from process.env.MY_SECRET
  → Auth works correctly

Architecture Diagram

Before

MCP config (mcp.json)
  └─ expandEnvVarsInRecord()   ← regex: /\$([A-Z_][A-Z0-9_]*)/g
       └─ matches $VAR only

After

MCP config (mcp.json)
  └─ expandEnvVarsInRecord()   ← regex: /\$(?:\{([A-Z_][A-Z0-9_]*)\}|([A-Z_][A-Z0-9_]*))/g  [~]
       └─ matches $VAR AND ${VAR}

Connection inventory:

From To Status Notes
expandEnvVarsInRecord regex modified Alternation added for brace form
dag-executor.test.ts loadMcpConfig new tests 4 new brace-form test cases
mcp-servers.md docs modified Both forms now documented

Label Snapshot

  • Risk: risk: low
  • Size: size: XS
  • Scope: providers, tests, docs
  • Module: providers:mcp

Change Metadata

  • Change type: bug
  • Primary scope: providers

Linked Issue

Validation Evidence (required)

bun run validate
Check Status
Bundled defaults PASS (36 commands, 20 workflows)
Bundled skill PASS (21 files)
Type check PASS (all 10 packages)
Lint PASS (0 warnings, 0 errors)
Format PASS
Tests PASS (0 failures, ~3,293 tests)
  • Evidence provided: Full bun run validate run — all six checks green
  • No commands skipped

Security Impact (required)

  • New permissions/capabilities? No
  • New external network calls? No
  • Secrets/tokens handling changed? Yes — env vars that were previously silently unexpanded now expand correctly; this is the fix, not a new risk
  • File system access scope changed? No
  • Mitigation: The fix only expands env vars that were already present in process.env; behavior for missing vars (empty string + missingVars list) is unchanged

Compatibility / Migration

  • Backward compatible? Yes — bare $VAR_NAME expansion is unchanged; brace form ${VAR_NAME} was previously a no-op (literal passthrough), now it expands correctly
  • Config/env changes? No
  • Database migration needed? No

Human Verification (required)

  • Verified scenarios: unit tests for env expansion, headers expansion, mixed bare+brace in same string, missing var → empty string + missingVars
  • Edge cases checked: ${FOO without closing } — regex does not match either branch, left as-is (safest behavior)
  • What was not verified: live end-to-end with a real MCP server (unit coverage is complete for the regex logic)

Side Effects / Blast Radius (required)

  • Affected subsystems/workflows: MCP config loading only (packages/providers/src/mcp/config.ts)
  • Potential unintended effects: None — the alternation regex cannot produce false positives; group 1 captures only ${UPPER}, group 2 captures only $UPPER
  • Guardrails: Existing and new unit tests; missingVars reporting unchanged

Rollback Plan (required)

  • Fast rollback: revert packages/providers/src/mcp/config.ts (single-function change)
  • Feature flags: None needed
  • Observable failure symptoms: MCP server receiving literal ${VAR} strings instead of expanded values

Risks and Mitigations

  • Risk: None — the change is additive, single-function, fully covered by tests, and does not touch any caller or interface.

AI-Layer Changes (chore commit)

Four process improvements committed in chore(ai-layer):

  1. CLAUDE.md — Added bun install note before bun dev & in the "Running the App in Worktrees" section so fresh-worktree agents don't hit module-resolution failures.
  2. piv-implement.md — Added bun install precondition check to Phase 1 LOAD.
  3. piv-code-review.md — Scoped diff command to PIV commits ($BASE_BRANCH..HEAD) and added carry-over file identification to Phase 1 and the review template Stats block.
  4. piv-plan.md — Added INSERT ANCHOR guidance to Task template requiring stable string anchors for code insertion points instead of fragile line numbers.

coleam00 and others added 3 commits May 19, 2026 08:03
…ixes #1612)

The regex in expandEnvVarsInRecord only matched $VAR_NAME (bare form),
silently passing through ${VAR_NAME} unexpanded. Add alternation to
handle both forms, with 4 new tests and updated docs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Hoist URL_FORGE constant to module scope in clone.ts, consistent with
FORGE_AUTH and SELF_HOSTED_FORGE which are already module-level.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Document `bun install` requirement for fresh worktrees in CLAUDE.md
Running the App in Worktrees section, before `bun dev &`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

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: e0d207e0-56d7-4f1c-91d2-2ae36bcf2e2a

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
  • Commit unit tests in branch archon/task-fix-issue-1612-dryrun

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

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

@coleam00 coleam00 closed this May 19, 2026
@coleam00 coleam00 deleted the archon/task-fix-issue-1612-dryrun branch May 19, 2026 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP env var expansion misses ${VAR} brace syntax

1 participant