feat: Add secrets support for MCP config variable expansion

[jpshackelford]

Summary

This PR fixes #2872 by letting per-conversation secrets participate in MCP config variable expansion for plugin .mcp.json files, and it also addresses two follow-up issues found during review:

1. ${VAR:-default} placeholders were being expanded too early during plugin loading, so later secret injection could not override the default.
2. Once MCP config placeholders were expanded to plaintext secrets, those values could leak through agent / conversation serialization.

Problem

Before this PR:

• expand_mcp_variables() only used the explicit variables dict (for example SKILL_ROOT) and os.environ.
• Per-conversation secrets injected through POST /api/conversations/{id}/secrets live in SecretRegistry, so they were invisible to MCP config expansion.
• Plugin loading eagerly applied defaults from ${VAR:-default} before conversation secrets were available.
• Expanded MCP config was stored on agent.mcp_config, and that field could be serialized into persisted state, WebSocket state updates, and API responses.

Solution

1. Use single-secret lookup instead of bulk export

   • SecretRegistry now exposes get_secret_value(name).
   • expand_mcp_variables() and load_mcp_config() accept get_secret: Callable[[str], str | None].
   • Resolution order is now:
     1. explicit variables (for example SKILL_ROOT)
     2. per-conversation secrets via callback
     3. environment variables
     4. default value from ${VAR:-default} when default expansion is enabled

2. Defer default expansion until conversation-time

   • Plugin loading now parses .mcp.json with expand_defaults=False.
   • SKILL_ROOT is still expanded immediately, but ${VAR} and ${VAR:-default} placeholders are preserved.
   • Final expansion happens after plugin configs are merged and conversation secrets are available, which fixes the double-expansion bug.

3. Expand merged MCP config at the right layer

   • load_plugins() accepts the secret lookup callback and expands the merged MCP config.
   • LocalConversation._ensure_plugins_loaded() expands the final merged MCP config with self._state.secret_registry.get_secret_value.

4. Prevent plaintext secret leakage after expansion

   • AgentBase.mcp_config is now excluded from serialization (exclude=True).
   • Expanded MCP config remains available in memory for MCP tool creation, but it is no longer written into persisted conversation state, WebSocket state updates, or API responses.

Behavior

Secret expansion now works

Given plugin MCP config:

{
  "mcpServers": {
    "my-server": {
      "url": "https://example.com/mcp",
      "headers": {
        "Authorization": "Bearer ${MCP_SECRET_TOKEN:-fallback-token}"
      }
    }
  }
}

And a per-conversation secret injected via:

POST /api/conversations/{id}/secrets
{"secrets": {"MCP_SECRET_TOKEN": "my-secret-value"}}

The runtime behavior is now:

• plugin load preserves Bearer ${MCP_SECRET_TOKEN:-fallback-token}
• conversation plugin initialization resolves it to Bearer my-secret-value
• if no secret is injected, it falls back to Bearer fallback-token

Security behavior

Expanded MCP config may contain plaintext secrets at runtime, so mcp_config is intentionally kept in memory only.

That means:

• MCP tools still receive the expanded config at runtime
• serialized Agent payloads no longer include mcp_config
• persisted conversation state no longer stores expanded MCP secrets
• WebSocket / API state payloads no longer expose expanded MCP secrets

Compatibility note

This PR intentionally changes serialization behavior: Agent.model_dump() / ConversationState.model_dump() no longer include mcp_config.

That is a security hardening change. Runtime MCP behavior is preserved, but any client that depended on serialized mcp_config being present will need to stop relying on that field.

Testing

Added or updated tests for:

• SecretRegistry.get_secret_value() callback behavior
• expand_mcp_variables() resolution order and defaults
• load_mcp_config() with secret lookup
• plugin loading preserving ${VAR} and ${VAR:-default} placeholders until conversation-time
• LocalConversation secret expansion, default fallback, and merged MCP behavior
• serialization leak prevention across agent dumps, persisted state, WebSocket state updates, and API-style state dumps

Evidence

• Targeted SDK verification run:

  uv run pytest tests/sdk/skills/test_mcp_config_expansion.py tests/sdk/plugin/test_plugin_loading.py tests/sdk/plugin/test_plugin_loader.py tests/sdk/conversation/test_local_conversation_plugins.py tests/sdk/conversation/test_mcp_secrets_serialization_leak.py tests/sdk/conversation/test_secrets_manager.py tests/sdk/agent/test_agent_serialization.py -k 'mcp or secret'

  Result: 46 passed, 78 deselected
• End-to-end staging validation was also run against the paired server-side change in OpenHands/OpenHands#14009, with example scripts here:
  • feat: add secrets field to AppConversationStartRequest for direct API secret passing OpenHands#14009
  • https://github.com/jpshackelford/oh-examples/tree/main/per-conversation-secrets

Changes

File	Change
secret_registry.py	Add get_secret_value(name) for callback-based lookup
skills/utils.py	Add get_secret callback and expand_defaults control to MCP expansion
plugin/plugin.py	Preserve unresolved placeholders during plugin load to avoid double-expansion
plugin/loader.py	Accept secret lookup callback and expand merged MCP config
local_conversation.py	Expand merged MCP config after conversation secrets are available
agent/base.py	Exclude mcp_config from serialization to prevent secret leakage
test_secrets_manager.py	Add tests for single-secret lookup callback behavior
test_mcp_config_expansion.py	Add MCP expansion tests covering secrets, defaults, and precedence
test_plugin_loading.py	Add regression tests for deferred placeholder expansion
test_local_conversation_plugins.py	Add conversation-level MCP secret expansion tests
test_mcp_secrets_serialization_leak.py	Add serialization leak regression tests
test_agent_serialization.py	Assert mcp_config is excluded from serialized agent payloads

Fixes #2872

---

This PR was created/updated by an AI assistant (OpenHands) on behalf of the user.

@jpshackelford can click here to continue refining the PR

---

Agent Server images for this PR

• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant	Architectures	Base Image	Docs / Tags
java	amd64, arm64	eclipse-temurin:17-jdk	Link
python	amd64, arm64	nikolaik/python-nodejs:python3.13-nodejs22-slim	Link
golang	amd64, arm64	golang:1.21-bookworm	Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:2ecddab-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-2ecddab-python \
  ghcr.io/openhands/agent-server:2ecddab-python

All tags pushed for this build

ghcr.io/openhands/agent-server:2ecddab-golang-amd64
ghcr.io/openhands/agent-server:2ecddab-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:2ecddab-golang-arm64
ghcr.io/openhands/agent-server:2ecddab-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:2ecddab-java-amd64
ghcr.io/openhands/agent-server:2ecddab-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:2ecddab-java-arm64
ghcr.io/openhands/agent-server:2ecddab-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:2ecddab-python-amd64
ghcr.io/openhands/agent-server:2ecddab-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-slim-amd64
ghcr.io/openhands/agent-server:2ecddab-python-arm64
ghcr.io/openhands/agent-server:2ecddab-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-slim-arm64
ghcr.io/openhands/agent-server:2ecddab-golang
ghcr.io/openhands/agent-server:2ecddab-java
ghcr.io/openhands/agent-server:2ecddab-python

About Multi-Architecture Support

• Each variant tag (e.g., 2ecddab-python) is a multi-arch manifest supporting both amd64 and arm64
• Docker automatically pulls the correct architecture for your platform
• Individual architecture tags (e.g., 2ecddab-python-amd64) are also available if needed

[github-actions]

Python API breakage checks — ✅ PASSED

Result: ✅ PASSED

Action log

[github-actions]

REST API breakage checks (OpenAPI) — ✅ PASSED

Result: ✅ PASSED

Action log

[github-actions]

Coverage Report •

File	Stmts	Miss	Cover	Missing
openhands-sdk/openhands/sdk/conversation
secret_registry.py	75	7	90%	109, 166, 170, 177, 179, 182, 191
openhands-sdk/openhands/sdk/conversation/impl
local_conversation.py	436	25	94%	295, 300, 429, 475, 512, 528, 593, 785–786, 789, 949, 957, 959, 963–964, 975, 977–979, 1004, 1199, 1203, 1273, 1280–1281
openhands-sdk/openhands/sdk/plugin
loader.py	29	2	93%	108, 111
plugin.py	210	16	92%	424–425, 432–433, 450–451, 454–456, 485–487, 503–504, 522–523
openhands-sdk/openhands/sdk/skills
utils.py	156	17	89%	48, 65, 162–163, 166, 183–184, 239, 252, 256, 284, 305, 355–357, 430–431
openhands-sdk/openhands/sdk/subagent
schema.py	119	7	94%	58, 71, 90, 105, 135, 231, 234
TOTAL	24160	5612	76%

[all-hands-bot]

Taste Rating: 🟢 Good taste - Well-implemented solution to a real problem

Code Quality: Clean implementation with comprehensive test coverage. The resolution order (variables > secrets > env > default) is clearly documented and correctly implemented.

[EVALUATION IMPACT NOTE]
This PR modifies how MCP configs are processed when per-conversation secrets are present. While backward compatible and well-tested, it could theoretically affect tool behavior in benchmarks. Recommend running lightweight evals to verify no unexpected impact before merging.

[RISK ASSESSMENT]
⚠️ Risk: 🟡 MEDIUM - Handles secrets (security-sensitive) and modifies MCP config processing. Changes are backward compatible and opt-in (only affects users who inject secrets via API). Risk mitigated by comprehensive tests and graceful error handling.

VERDICT: ✅ Worth merging - Core logic is sound, solves a real problem (#2872). One minor style improvement suggested inline.

KEY INSIGHT: The get_all_secrets() method with exception handling ensures one failing secret doesn't break the entire MCP config expansion - good defensive programming.

[all-hands-bot]

Taste Rating: 🟡 Acceptable - Solid fix with comprehensive tests, minor suggestions for robustness

The core solution is well-designed: using expand_defaults to control expansion phases prevents double-expansion, and the integration tests thoroughly cover critical scenarios. The resolution order (variables → secrets → env → defaults) is clearly documented.

A few suggestions for improvement around edge cases and consistency.

[all-hands-bot]

[RISK ASSESSMENT]

⚠️ Risk Level: 🟡 MEDIUM

Analysis: This PR modifies MCP config processing and variable expansion during agent initialization. While the implementation is backward compatible and well-tested, it changes how MCP tools are configured, which could theoretically affect benchmark/evaluation performance if benchmarks use MCP servers with variable expansion.

Risk Factors:

• ✅ Backward compatible (no breaking changes)
• ✅ Comprehensive test coverage (unit + integration)
• ⚠️ Modifies agent initialization behavior (MCP config expansion)
• ⚠️ Adds new expansion phase in conversation setup
• ⚠️ Could affect tool availability if MCP configs use secrets

Recommendation: Run lightweight evaluations before merging to ensure no unexpected benchmark impact. Consider testing with:

• Benchmarks that use MCP tools
• Scenarios with and without per-conversation secrets
• Verify tool initialization timing remains consistent

Per repository guidelines, PRs that modify agent behavior require eval verification before approval.

---

Overall: The implementation is solid and the fix addresses a real issue (#2872). Once evals confirm no negative impact, this is ready to merge. 👍

[all-hands-bot]

✅ QA Report: PASS

Per-conversation secrets now correctly expand MCP config variables, fixing both the missing expansion and the double-expansion bugs.

Does this PR achieve its stated goal?

Yes. The PR successfully enables per-conversation secrets (injected via REST API) to be used for MCP config variable expansion (${VAR} syntax). I verified:

1. Secrets are now expanded: ${MCP_SECRET_TOKEN} → "my-actual-secret" ✅
2. Double-expansion bug is fixed: ${SECRET_TOKEN:-fallback} uses the secret value, not the default ✅
3. Defaults work when no secret: ${VAR:-default} → "default" when secret not provided ✅

Phase	Result
Environment Setup	✅ uv environment ready, dependencies installed
CI & Tests	✅ All CI checks pass (sdk-tests, tools-tests, build, pre-commit)
Functional Verification	✅ Verified both bugs fixed with before/after testing

Functional Verification

Test 1: Basic Secrets Expansion (Issue #2872)

Step 1 — Reproduce the bug (main branch):

Created test plugin with MCP config:

{
  "mcpServers": {
    "test-server": {
      "url": "https://example.com/mcp",
      "headers": {
        "Authorization": "Bearer ${MCP_SECRET_TOKEN}"
      }
    }
  }
}

Injected secret via conversation.update_secrets({"MCP_SECRET_TOKEN": "my-actual-secret"})

Ran verification script:

Authorization header: Bearer ${MCP_SECRET_TOKEN}
❌ BUG: Secret was NOT expanded (placeholder remains)

This confirms the bug exists: secrets from SecretRegistry were not used during MCP config expansion.

Step 2 — Apply the PR's changes:

Checked out PR branch pr-2873 (commit 055e09f)

Step 3 — Re-run verification with the fix:

Authorization header: Bearer my-actual-secret
✅ SUCCESS: Secret was correctly expanded!

The placeholder is now correctly replaced with the secret value from SecretRegistry.

---

Test 2: Double-Expansion Bug (Variables with Defaults)

Step 1 — Reproduce the double-expansion bug (main branch):

Created test plugin with MCP config using default syntax:

{
  "mcpServers": {
    "test-server": {
      "headers": {
        "Authorization": "Bearer ${SECRET_TOKEN:-fallback-token}"
      }
    }
  }
}

Injected secret: {"SECRET_TOKEN": "my-actual-secret"}

Ran verification script:

Authorization header: Bearer fallback-token
❌ BUG: Default was used instead of secret (double-expansion bug)

This confirms the double-expansion bug: the default value was applied during plugin loading (first expansion), so the secret could not replace it later (second expansion).

Step 2 — Apply the PR's changes:

Checked out PR branch pr-2873

Step 3 — Re-run verification with the fix:

Authorization header: Bearer my-actual-secret
✅ SUCCESS: Secret was used (not default)!

The secret value is now correctly used instead of the default, confirming the double-expansion bug is fixed.

---

Test 3: Comprehensive Unit Tests

Ran all new unit tests added by the PR:

tests/sdk/conversation/test_secrets_manager.py::test_get_all_secrets_static_values PASSED
tests/sdk/conversation/test_secrets_manager.py::test_get_all_secrets_callable_values PASSED
tests/sdk/conversation/test_secrets_manager.py::test_get_all_secrets_handles_exceptions PASSED
tests/sdk/conversation/test_secrets_manager.py::test_get_all_secrets_empty_registry PASSED

tests/sdk/skills/test_mcp_config_expansion.py - 14 tests PASSED
  - test_expand_secrets
  - test_variable_resolution_order
  - test_secrets_take_precedence_over_env
  - test_default_not_used_when_secret_exists
  - etc.

tests/sdk/conversation/test_local_conversation_plugins.py::TestPluginMcpSecretsExpansion - 3 tests PASSED
  - test_plugin_mcp_secrets_without_defaults
  - test_plugin_mcp_secrets_with_defaults
  - test_plugin_mcp_secrets_fallback_to_default_when_no_secret

tests/sdk/plugin/test_plugin_loading.py::TestPluginMcpConfigLoading - 3 tests PASSED
  - test_plugin_mcp_config_preserves_unexpanded_variables
  - test_plugin_mcp_config_preserves_variables_with_defaults
  - test_plugin_mcp_skill_root_is_expanded

All tests pass, covering:

• SecretRegistry.get_all_secrets() method
• Variable expansion resolution order (variables → secrets → env → defaults)
• Secrets taking precedence over environment variables
• Plugin loading with expand_defaults=False to preserve placeholders
• End-to-end conversation-level secrets expansion

Issues Found

None.

[jpshackelford]

This is the SDK part of the solution. The enterprise-server part is in OpenHands/OpenHands#14009. See associated issues for both PRs for more context.

[jpshackelford]

I ran and end-to-end test with this change and OpenHands/OpenHands#14009 in staging posting results to the OpenHands/OpenHands#14009 PR. Example scripts for testing are here: https://github.com/jpshackelford/oh-examples/tree/main/per-conversation-secrets.

[neubig]

Looks like nice functionality to have! Just a few comments.

[jpshackelford]

@neubig Thanks very much for the quick review!

[jpshackelford]

@neubig As discussed in Slack, adding a method to expose all secrets in plaintext is a bad pattern even if it seems okay for the current implementation of the secrets store and our use of it now. At the same time, I'd like to keep coupling low for testability and avoid circular dependencies, so I implemented a callback pattern instead.

Changes:

• Replaced get_all_secrets() with get_secret_value(name) for single lookups
• expand_mcp_variables() now accepts get_secret: Callable[[str], str | None]
• Secrets are retrieved lazily, only when referenced in the config

Usage: expand_mcp_variables(config, vars, get_secret=registry.get_secret_value)
Testing: expand_mcp_variables(config, vars, get_secret={"KEY": "val"}.get)

Is this acceptable to you?

[desiorac]

The callback pattern (get_secret: Callable[[str], str | None]) over get_all_secrets() is the right call — bulk export would have made secret sprawl hard to audit.

One thing worth verifying: after expand_mcp_variables() resolves placeholders, the expanded config (now containing plaintext secrets) gets written into self.agent via model_copy() and then into self._state.agent. If ConversationState is serialized to disk or emitted in observation outputs, secrets end up in plaintext outside the registry. Concrete check: grep for any .model_dump() / .dict() / json() call on the agent or state objects downstream of _ensure_plugins_loaded() and verify mcp_config is excluded or redacted.

The expand_defaults=False approach for the double-expansion bug is clean — preserving placeholders through the plugin-load pass is the right abstraction boundary.

[tofarr]

The problem I see with this PR is that it redacts in all cases - so it would mean that these values are not saved when storing to disk - this would mean that the agent servers choke on resume. (Or at least that MCP operations do) What we actually want is the same sort of behavior we have in the StaticSecret class:

• encrypt values when storing to disk (In this case we have provided a cipher in the context)
• redact when writing externally (We have not provided any context / the context has no special parameters)
• Unless expose_secrets=true - in this case we write secrets in plaintext.

[tofarr]

I had an agent work on this same issue and it came up with this PR: #2900

[jpshackelford]

@tofarr I've incorporated the cipher-based encryption pattern you described into this PR.

Changes Made

The mcp_config field now follows the same serialization pattern as StaticSecret and other secrets in the SDK:

Serialization Context	mcp_config Behavior
No context	mcp_config: null (fully redacted)
{"expose_secrets": True}	mcp_config: {...} (plaintext)
{"cipher": cipher}	encrypted_mcp_config: "<encrypted>"

Implementation

1. @model_validator(mode="before") - Decrypts encrypted_mcp_config on load when cipher is in context
2. @field_serializer("mcp_config") - Returns None when redacting, plaintext when expose_secrets=True, or None (encryption handled by model_serializer) when cipher present
3. @model_serializer(mode="wrap") - Encrypts entire mcp_config as JSON blob when cipher is in context, stores as encrypted_mcp_config

This ensures:

• API/WebSocket responses: mcp_config is redacted (no secrets leak)
• Disk persistence (with cipher): mcp_config is encrypted and can be restored on conversation resume
• Runtime (in-memory): Full plaintext available for MCP tool use

Tests

Added/updated tests covering:

• Redaction by default (no context)
• Exposure with expose_secrets=True
• Encryption with cipher
• Full roundtrip (encrypt → decrypt)
• Backward compatibility with plaintext mcp_config
• Empty mcp_config handling

All tests pass:

uv run pytest tests/sdk/agent/test_agent_serialization.py tests/sdk/conversation/test_mcp_secrets_serialization_leak.py tests/sdk/conversation/local/test_state_serialization.py -v
# 61 passed

---

This comment was posted by an AI agent (OpenHands) on behalf of @jpshackelford.

[jpshackelford]

Note: The "Check OpenAPI Schema" and "REST API (OpenAPI)" CI failures are pre-existing issues unrelated to this PR. I verified by testing against the commit before my changes:

git stash  # stash my changes
uv run python -c "from openhands.agent_server.api import api; api.openapi()"
# KeyError: 'properties'

The issue is in DiscriminatedUnionMixin.__get_pydantic_json_schema__ where it assumes schema["properties"] exists, but this fails for certain nested model combinations. This is a separate bug.

---

This comment was posted by an AI agent (OpenHands) on behalf of @jpshackelford.

[tofarr]

🍰

[jpshackelford]

This PR enables per-conversation secrets to participate in MCP config variable
expansion for plugin `.mcp.json` files, and prevents expanded secrets from
leaking through serialization.

## Problem

Before this PR:
- `expand_mcp_variables()` only used explicit variables and `os.environ`
- Per-conversation secrets from `POST /api/conversations/{id}/secrets` were
  invisible to MCP config expansion
- Plugin loading eagerly applied defaults from `${VAR:-default}` before
  conversation secrets were available
- Expanded MCP config could leak through agent/conversation serialization

## Solution

1. **Callback-based secret lookup**
   - `SecretRegistry.get_secret_value(name)` for single-secret lookup
   - `expand_mcp_variables()` and `load_mcp_config()` accept `get_secret` callback
   - Resolution order: explicit variables → secrets → env vars → defaults

2. **Deferred default expansion**
   - Plugin loading parses `.mcp.json` with `expand_defaults=False`
   - `SKILL_ROOT` expanded immediately, `${VAR:-default}` preserved
   - Final expansion at conversation-time when secrets are available

3. **Conversation-level MCP expansion**
   - `LocalConversation._ensure_plugins_loaded()` expands merged MCP config
     with `self._state.secret_registry.get_secret_value`

4. **Prevent secret leakage via model_serializer**
   - Default: mcp_config is redacted (omitted from serialization)
   - With `cipher` context: mcp_config is encrypted for secure persistence
   - With `expose_secrets=True`: mcp_config exposed for debugging

Fixes #2872

Co-authored-by: enyst <engel.nyst@gmail.com>
Co-authored-by: openhands <openhands@all-hands.dev>