Skip to content

fix(mcp): evaluate environment variables at transport connection time#94167

Closed
lakshitsoni26 wants to merge 1 commit into
openclaw:mainfrom
lakshitsoni26:main
Closed

fix(mcp): evaluate environment variables at transport connection time#94167
lakshitsoni26 wants to merge 1 commit into
openclaw:mainfrom
lakshitsoni26:main

Conversation

@lakshitsoni26

@lakshitsoni26 lakshitsoni26 commented Jun 17, 2026

Copy link
Copy Markdown

Summary

What problem does this PR solve?

  • Prior to this change, ${ENV_VAR} templates in MCP config fields were resolved during Gateway startup.
  • If an environment variable changed dynamically (e.g. token rotation), the new value was never injected without restarting the Gateway process.

Why does this matter now?

  • This enables workflows where secrets or auth tokens can refresh dynamically in the background without dropping existing Gateway states or interrupting user workflows.

What is the intended outcome?

  • Defers the substitution logic to connection/request time so the latest environment variables are evaluated exactly when the transport initiates connections.

What is intentionally out of scope?

  • N/A

What does success look like?

  • Successfully rotating an auth token in the process.env during an active transport session and seeing the new token applied on the next request.

What should reviewers focus on?

  • The inline substitutingFetch wrapper in src/agents/mcp-transport.ts.
  • The removal of early resolution in src/plugins/loader.ts.

Linked context

Which issue does this close?

Closes #93144

Which issues, PRs, or discussions are related?

Related #N/A

Was this requested by a maintainer or owner?
No

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: Dynamic substitution of environment variables like ${DYNAMIC_SECRET_TOKEN} in MCP transports without restarting.
  • Real environment tested: Local development environment with a targeted script testing the transport connection.
  • Exact steps or command run after this patch: Ran a local Vitest script validating the substitutingFetch wrapper behavior.
  • Evidence after fix (screenshot, recording, terminal capture, console output, redacted runtime log, linked artifact, or copied live output):
Screenshot 2026-06-17 at 7 30 34 PM
  • Observed result after fix: The test passes, demonstrating the initial fetch uses TOKEN_VERSION_1, and the subsequent fetch uses TOKEN_VERSION_2_ROTATED after process.env was updated in-memory.
  • What was not tested: Integration with every other possible MCP transport type (focused primarily on fetch/sse).
  • Proof limitations or environment constraints: None
  • Before evidence (optional but encouraged): Previously, the second fetch would still use TOKEN_VERSION_1.

Tests and validation

Which commands did you run?
Ran a local validation script simulating the substitutingFetch layer against an active Node environment.

What regression coverage was added or updated?
No permanent regression tests were added. The logic relies on the existing resolveConfigEnvVars coverage, combined with the real behavior proof attached above to prove the interceptor works.

What failed before this fix, if known?
Transport requests would use stale, startup-time environment variables indefinitely.

If no test was added, why not?
The fix is an architectural repositioning of existing, heavily-tested parsing logic (resolveConfigEnvVars). A manual proof script was used to verify the integration, which is attached as a screenshot above.

Risk checklist

Did user-visible behavior change? (Yes/No)
No (backend substitution behavior only)

Did config, environment, or migration behavior change? (Yes/No)
Yes

Did security, auth, secrets, network, or tool execution behavior change? (Yes/No)
Yes (Tokens are now dynamically refreshed at request-time)

What is the highest-risk area?
The highest risk is that some config validations that expect fully resolved URLs/Headers might be disrupted if they occur too early.

How is that risk mitigated?
By ensuring we still apply resolveConfigEnvVars dynamically via the HTTP proxy layer immediately before fetch fires, maintaining API contract integrity.

Current review state

What is the next action?
Maintainer review and merge.

What is still waiting on author, maintainer, CI, or external proof?
Waiting for CI to pass and maintainers to review.

Which bot or reviewer comments were addressed?
N/A

@lakshitsoni26
fix(mcp): evaluate environment variables at transport connection time
73f8792 
Fixes openclaw#93144

Prior to this change,  templates in MCP config fields (such as
`env` and `headers`) were resolved during Gateway startup. As a result,
any dynamic rotation of underlying environment variables required a full
Gateway restart.

This commit shifts the environment variable substitution logic to the MCP
transport connection layer. We now intercept the SDK's fetch invocations
(for HTTP/SSE) and child process spawn routines (for stdio), dynamically
substituting variables based on the live `process.env` state at the
exact moment of request, cleanly solving the credential rotation limits.
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 17, 2026
@clawsweeper

clawsweeper Bot commented Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 10:35 AM ET / 14:35 UTC.

Summary
The PR defers MCP env/header ${ENV_VAR} substitution from config-load time to MCP transport connection/request time.

PR surface: Source +69. Total +69 across 5 files.

Reproducibility: yes. at source level: current main resolves config env at activation and passes fixed headers/env into transports. I did not run a live Gateway-to-MCP reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Deferred MCP substitution paths: 2 paths changed: mcp.servers.*.env and mcp.servers.*.headers. Both paths can carry credentials, so moving them from config-read substitution to transport-time substitution is compatibility and security relevant before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #93144
Summary: This PR is the active candidate implementation for the dynamic MCP ${ENV_VAR} issue; broader dynamic-header and OAuth client_credentials issues overlap but do not supersede it.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real OpenClaw Gateway-to-MCP proof covering the changed transport path.
  • [P1] Add focused regression coverage for config-read preservation, request-time substitution, missing-variable behavior, and all touched MCP transports.
  • Get maintainer confirmation on whether mutable process.env is the supported token-rotation mechanism or whether OAuth/SecretRef should own it.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR provides a screenshot of a local Vitest-style script that shows token text changing, but it does not show a real OpenClaw Gateway connected to an MCP server after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Existing configs with missing MCP env/header refs could move from config warnings to transport-time failure.
  • [P1] Mutable process.env becomes a request-time credential source for outbound MCP headers, so scoping, redaction, audit, and reload semantics need maintainer approval.
  • [P1] The supplied proof is a local test-script screenshot, not real OpenClaw Gateway-to-MCP proof across the changed transport path.
  • [P1] This overlaps broader OAuth client_credentials and dynamic-header design work, so isolated landing could create competing MCP auth mechanisms.

Maintainer options:

  1. Define the dynamic secret contract first (recommended)
    Require maintainer approval, preserved missing-env behavior, focused tests, and real setup proof for the chosen MCP request/spawn-time substitution semantics before merge.
  2. Keep current default and add opt-in dynamics
    Maintainers could keep activation-time substitution as the default and add a narrower explicit dynamic mode if upgrade compatibility matters more than transparent token rotation.
  3. Pause for OAuth or SecretRef ownership
    If token rotation should be owned by OAuth client_credentials or SecretRef instead of mutable process.env, pause or close this PR and keep the design issues canonical.

Next step before merge

  • [P1] Manual review is needed because the remaining blockers are product/security contract and contributor real-behavior proof, not a narrow automated repair.

Security
Needs attention: The diff is security-sensitive because it changes when credential-bearing MCP env/header values are read from mutable host process environment.

Review findings

  • [P1] Preserve missing-env diagnostics for deferred MCP values — src/agents/mcp-transport.ts:157
Review details

Best possible solution:

Land a maintainer-approved dynamic MCP credential contract that preserves missing-env diagnostics, covers the intended transport granularity, and includes focused tests plus redacted real Gateway proof.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: current main resolves config env at activation and passes fixed headers/env into transports. I did not run a live Gateway-to-MCP reproduction in this read-only review.

Is this the best way to solve the issue?

Unclear as merge-ready. The SDK fetch hook is a plausible seam, but this patch changes credential timing and missing-env behavior without a maintainer-approved contract, regression coverage, or real Gateway proof.

Full review comments:

  • [P1] Preserve missing-env diagnostics for deferred MCP values — src/agents/mcp-transport.ts:157
    The PR skips mcp.servers.*.headers/env during config read, but the new request/spawn-time calls to substituteString do not pass onMissing. A config that currently starts with a missing-env warning and visible placeholder will instead fail when the MCP transport runs, so preserve the warning/degraded behavior or add an explicit maintainer-approved fail-closed contract with tests.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 374076b5a8c5.

Label changes

Label justifications:

  • P2: This is a concrete MCP auth/config improvement with limited blast radius, not an urgent core-runtime outage.
  • merge-risk: 🚨 compatibility: The PR changes when existing MCP ${ENV_VAR} header/env values are resolved and when missing values fail.
  • merge-risk: 🚨 auth-provider: The changed paths carry MCP bearer tokens and overlap existing OAuth/client_credentials design work.
  • merge-risk: 🚨 security-boundary: The diff makes mutable host process environment values a request-time source for outbound credential-bearing MCP headers.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR provides a screenshot of a local Vitest-style script that shows token text changing, but it does not show a real OpenClaw Gateway connected to an MCP server after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The PR provides a screenshot of a local Vitest-style script that shows token text changing, but it does not show a real OpenClaw Gateway connected to an MCP server after the fix.
Evidence reviewed

PR surface:

Source +69. Total +69 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 5 80 11 +69
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 80 11 +69

Security concerns:

  • [medium] Define request-time MCP credential boundaries — src/agents/mcp-transport.ts:157
    Preserving raw MCP header/env templates and resolving them from mutable process.env at request/spawn time changes credential timing, diagnostics, and audit behavior; maintainers should define scoping, redaction, missing-var behavior, and transport coverage before merge.
    Confidence: 0.9

What I checked:

Likely related people:

  • Peter Steinberger: GitHub commit history shows repeated MCP operator workflow, transport seam, and config/runtime work in the affected MCP surface. (role: feature owner and recent area contributor; confidence: high; commits: 38d3d11cbc0c, 98822fdd6352, 1318479a2c0e; files: src/agents/mcp-transport.ts, src/config/io.ts)
  • Pavan Kumar Gondhi: Recent commits hardened MCP HTTP redirects and header scrubbing in the same remote-header trust boundary this PR modifies. (role: adjacent MCP HTTP safety contributor; confidence: medium; commits: 3c6259ebb70c, 47eb2d48d434; files: src/agents/mcp-http-fetch.ts, src/agents/mcp-transport.ts, src/agents/mcp-transport.test.ts)
  • Vincent Koc: Recent GitHub history and local blame touch config env/substitution and config IO surfaces relevant to preserving missing-env diagnostics. (role: recent config area contributor; confidence: medium; commits: 6daabd23f821, 523e98a47a24, 72628f906b9e; files: src/config/io.ts, src/config/env-substitution.ts)
  • Sebastian: History shows the general ${VAR_NAME} config substitution behavior was introduced in commit a36735b, which this PR selectively defers for MCP values. (role: introduced related config behavior; confidence: medium; commits: a36735b913d0; files: src/config/env-substitution.ts, src/config/io.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 18, 2026
This was referenced Jun 18, 2026
Open
Open
Closed
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 19, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 21, 2026
Open
@clawsweeper clawsweeper Bot added the proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. label Jun 22, 2026
This was referenced Jun 26, 2026
Open
Open
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 16, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 16, 2026
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 24, 2026

Copy link
Copy Markdown

Closing due to inactivity.
If you believe this PR should be revived, post in #clawtributors on Discord to talk to a maintainer.
That channel is the escape hatch for high-quality PRs that get auto-closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature]: Support per-request dynamic resolution of ENV_VAR substitution in MCP transport headers and env

1 participant

@lakshitsoni26