Expand ${VAR} in --mcp env and header values at MCP startup

[warp-agent-staging]

Description

MCP config that Warp reads from disk gets ${VAR} expansion before the servers start (file_mcp_watcher.rs), but MCP config delivered inline through the --mcp <json> CLI argument got none — a ${NAME} placeholder reached the MCP process literally.

That gap blocks the approved factory-files design: a factory file's typed secret reference ({ secret: NAME }) is canonicalized server-side into a ${NAME} placeholder inside the MCP env/headers values, and the secret itself is injected into the run's environment. The placeholder only resolves if the inline --mcp path expands it the same way.

What changed:

• Moved the file watcher's substitute_env_vars (and its regex) into ai::mcp::parsing so both paths share one implementation. Behavior of the watcher is unchanged: MissingEnvVarError's Display is the same string the old anyhow::anyhow! produced, and the single call site only formats it with {}.

• Added TemplatableMCPServerInstallation::expand_env_var_placeholders, applied in AgentDriver::installations_from_user_mcp_json. For an installation parsed from user MCP JSON the variable values are exactly the server's env and headers entries, so only those are rewritten — command, args and url stay literal (a deliberate difference from the on-disk path, which expands the whole document).

• A missing or empty variable is a hard failure: the error names the server, the key and the variable, and classifies to Failed / EnvironmentSetupFailed so it surfaces to the user and never becomes a Sentry issue. No value is ever logged or reported.

  Open question for the reviewer: MCPEnvVarExpansionFailed is not MCPStartupFailed, so it aborts the run even when strict_mcp_startup is false, taking the run's other MCP servers down with it. Note this is stricter than the on-disk path, which emits a FileMCPConfigDiagnostic and lets the run continue. Happy to degrade instead if that is preferred.

Placement note (worth a reviewer's eye): the expansion is applied where the driver turns inline JSON into a running MCP server, not in mcp_config::build_mcp_servers_from_specs. That function also feeds run-cloud, integration create|update and local task creation, all of which persist the config server-side — expanding there would upload resolved secret values to the server and would hard-fail on variables that only exist in the run environment. Doing it at startup covers both the worker's --task-id ... --mcp <json> invocation and local oz agent run --mcp, plus inline entries from an agent config file, and covers the Oz and third-party harnesses (both resolve through resolve_mcp_specs).

Linked Issue

No GitHub issue: this is the client half of the approved factory-files design ("Secrets — the one real fork"), tracked with the server-side work.

• The linked issue is labeled ready-to-spec or ready-to-implement.
• Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes).

Testing

Unit tests, in the packages' existing conventions:

• app/src/ai/mcp/parsing_tests.rs — the two substitute_env_vars tests moved verbatim from file_mcp_watcher_tests.rs along with their cleanup_env_vars helper, so no coverage was dropped.
• app/src/ai/agent_sdk/driver_tests.rs — expansion in an env value, expansion in a headers value, a literal value passing through untouched, ${VAR} in args staying literal, the missing-variable failure naming server + key + variable, the exact failure message, and deterministic key reporting when several placeholders are unresolvable.
• app/src/ai/agent_sdk/driver/error_classification_tests.rs — the new MCPEnvVarExpansionFailed arm, matching every sibling arm.

Commands run:

• ./script/format and ./script/format --check — clean.

• cargo clippy --workspace --exclude warp_completer --all-targets --tests -- -D warnings — clean.

• cargo clippy -p warp --all-targets --tests -- -D warnings — clean.

• cargo nextest run -p warp — 6196/6200 passed. The 4 failures are pre-existing/environmental and untouched by this diff: server::server_api::ai::tests::ambient_agent_headers_for_task_overrides_existing_cloud_agent_header, terminal::input::decorations::tests::test_decorations_with_multibyte_chars, terminal::input::tests::test_histignorespace_support_in_zsh (no zsh history in the sandbox), and ai::blocklist::action_model::execute::wait_for_events::tests::execute_invokes_parent_registration_and_honors_child_short_circuit, which passes in isolation 3/3. A clean master baseline run could not be produced — building the warp lib test from scratch was OOM-killed twice in this sandbox.

• I have manually tested my changes locally with ./script/run

No manual run: this path has no UI surface and only executes inside an agent run's MCP startup, which is covered by the unit tests above.

Agent Mode

• Warp Agent Mode - This PR was created via Warp's AI Agent Mode

Conversation: https://staging.warp.dev/conversation/887ede3f-ebf2-401c-afb0-415b3ba80675
Run: https://oz.staging.warp.dev/runs/019fd47e-2b11-7604-8462-51ad86cb5199

This PR was generated with Oz.

[warp-agent-staging]

Automated review: accept-with-findings. Secret safety is clean — the error carries only server, key and variable names, never a value, and the Failed classification does mechanically keep it out of Sentry. The placement reasoning holds: expanding here rather than in build_mcp_servers_from_specs avoids uploading resolved secrets to the server, and the worker's --task-id … --mcp <json> path, local agent run --mcp, agent-config-file entries and the third-party-harness path all funnel through the same function, so the paired server feature is fully covered. Expansion is genuinely limited to env and headers in the code, not just in the tests, and the watcher migration is byte-for-byte faithful.

Two findings need a human decision.

Fatal or degraded when a variable is unresolvable? — app/src/ai/agent_sdk/driver.rs:1350, consequences at :1598 and :1614

MCPEnvVarExpansionFailed is not MCPStartupFailed, so an unresolvable ${VAR} aborts the entire run even when strict_mcp_startup is false — taking the run's other MCP servers with it. Every other MCP startup failure merely degrades the run in that mode. Note the PR body's justification is inaccurate: the on-disk path emits a FileMCPConfigDiagnostic and the run continues, so the inline path is now strictly harsher than the path it claims to match. Failing loudly is defensible for a factory-file secret reference — a silently tool-less agent is arguably worse — and the sibling MCPMissingVariables at the same site is already fatal. Keep it fatal and correct the PR body, or fold it into the degradation list?

Unescaped substitution: fix now or track? — app/src/ai/mcp/templatable_installation.rs:190

Expanded values are spliced into the MCP JSON template unescaped (render_template does a raw push_str into a JSON document). A secret containing a quote, backslash or newline — a PEM key, a JSON service-account blob — produces invalid JSON and the run dies with a misleading parse error, and a crafted value could close the string and inject sibling keys such as command. This is inherited, not introduced: apply_secrets already splices managed-secret values through the same render, and the watcher substitutes into raw JSON text identically. But this feature's whole purpose is pushing secret values down this path, so it widens the exposure. Does this block enabling the server-side secret-reference feature, or is it a tracked follow-up? Ordinary alphanumeric tokens are unaffected either way.

Smaller items being handled separately: expand_env_var_placeholders should be pub(crate), two tests leak env vars on assertion failure, and one format-args nit.

One thing to know at merge time: every build, clippy and test job on this draft is SKIPPED, so build and test health is author-reported only.