feat(agent-server): per-server CRUD for MCP servers#3235
Conversation
Adds a dedicated router for managing MCP servers as a true REST collection
rather than a sub-blob of the global settings PATCH:
GET /api/settings/mcp-servers
GET /api/settings/mcp-servers/{name}
PUT /api/settings/mcp-servers/{name}
PATCH /api/settings/mcp-servers/{name}
DELETE /api/settings/mcp-servers/{name}
Each server is treated as its own named resource:
* Per-server ETag, computed over a plaintext-canonical projection of
the server's config (stable across Fernet resaves of identical
content). Mutations honour If-Match / If-None-Match: * for
optimistic concurrency at the right granularity — concurrent edits
to different servers no longer collide at all.
* X-Expose-Secrets is honoured on GET /{name} just like the global
/api/settings, with the same redacted / encrypted / plaintext modes.
* Writes still go through the same file-locked store.update() as
/api/settings, so they remain atomic and serialise with each other
and with the legacy global PATCH.
* ACP variant has no mcp_config field: list returns empty (200), single-
item GET returns 404, mutations return 409 with a message pointing
the caller at the variant-switching settings endpoint.
Also exposes the new request/response shapes from the SDK
(MCPServerSummary, MCPServerListResponse, MCPServerResponse) so
clients can generate typed bindings.
Bonus fix in api.py: the shared HTTPException handler was building
the JSONResponse from status_code + detail but dropping exc.headers,
so the ETag header on a 412 (or WWW-Authenticate on a 401) was lost.
Propagate it.
Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Excellent design and implementation
[ARCHITECTURE WINS]
- Per-server ETag enables concurrent edits to different servers without conflicts - solves the fundamental "read-modify-write the whole collection" limitation of global settings PATCH
- Plaintext-canonical ETag computation avoids Fernet nonce issues (idempotent resaves)
- Clean separation of concerns: secret serialization via model context, redaction as response transform
- Comprehensive test coverage (11 scenarios including the critical "splice locally" property)
[RISK ASSESSMENT]
🟢 LOW RISK
- New API surface, zero changes to existing endpoints or persistence format
- All writes still go through file-locked
store.update()(serialized with global PATCH) - Properly tested: CRUD, ETag semantics, optimistic concurrency, secret exposure modes, ACP variant, name validation
- Exception handler change (propagating headers) is safe and necessary for ETag-on-412
VERDICT: ✅ Worth merging - Excellent module docstring explains motivation clearly; implementation is clean and well-tested.
Minor note: PR description says "placeholder" - consider adding a summary before merge.
|
@OpenHands read the PR linked in this PR's description too for context. And /codereview-roasted this PR. Respond directly on github. |
|
I'm on it! enyst can track my progress at all-hands.dev |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
All MCP server CRUD endpoints work correctly with per-server optimistic concurrency, avoiding the need to round-trip all servers' encrypted secrets.
Does this PR achieve its stated goal?
Yes. The PR successfully delivers fine-grained CRUD API endpoints for individual MCP servers with proper optimistic concurrency control. I verified by actually running the agent-server and making HTTP requests that: (1) all CRUD operations work end-to-end, (2) ETags are stable across identical resaves but change when content changes, (3) If-Match/If-None-Match preconditions enforce optimistic concurrency, (4) X-Expose-Secrets modulates secret exposure correctly, (5) ACP variant returns appropriate errors, and (6) editing one server preserves other servers' secrets without client round-tripping.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Built with make build, all dependencies installed |
| CI Status | ✅ agent-server-tests passed (1m10s), pre-commit passed, core checks green |
| Functional Verification | ✅ All CRUD endpoints verified with real HTTP requests |
Functional Verification
Test 1: Basic CRUD Operations
Setup: Started agent-server on port 8000 with persistence enabled.
Ran comprehensive CRUD test script:
./qa_mcp_crud.shResults:
- ✓ LIST empty collection:
GET /api/settings/mcp-servers→ 200 with{"servers":[]} - ✓ CREATE via PUT:
PUT /api/settings/mcp-servers/test-server→ 201 - ✓ READ with default redaction: secrets masked as
[REDACTED] - ✓ READ with X-Expose-Secrets: plaintext: raw secret values returned
- ✓ PATCH partial update: description updated, other fields preserved
- ✓ DELETE: 204 No Content, subsequent GET returns 404
- ✓ CREATE second server: concurrent additions don't conflict
- ✓ LIST all servers: both servers appear with correct summaries
This confirms all five CRUD endpoints (GET list, GET one, PUT, PATCH, DELETE) work correctly.
Test 2: ETag Stability (Plaintext-Canonical)
Goal: Verify ETags are computed from plaintext-canonical form, not on-disk ciphertext.
Step 1 — Create server and capture ETag:
curl -X PUT -H 'Content-Type: application/json' \
-d '{"command":"python","args":["server.py"],"env":{"API_KEY":"test-key-123"}}' \
http://127.0.0.1:8000/api/settings/mcp-servers/etag-test
curl -v http://127.0.0.1:8000/api/settings/mcp-servers/etag-test 2>&1 | grep etagOutput: ETag: "12dbfe5db8c6e78d9b1a848a0699b86c"
Step 2 — Re-PUT identical config (forces re-encryption with new Fernet nonce):
curl -X PUT -H 'Content-Type: application/json' \
-d '{"command":"python","args":["server.py"],"env":{"API_KEY":"test-key-123"}}' \
http://127.0.0.1:8000/api/settings/mcp-servers/etag-testStep 3 — Get ETag again:
Output: ETag: "12dbfe5db8c6e78d9b1a848a0699b86c" (identical)
This confirms ETag is stable despite on-disk ciphertext changing (different Fernet nonce).
Step 4 — Change content and verify ETag changes:
curl -X PUT -H 'Content-Type: application/json' \
-d '{"command":"python","args":["server.py"],"env":{"API_KEY":"different-key-456"}}' \
http://127.0.0.1:8000/api/settings/mcp-servers/etag-testOutput: ETag: "5906348764584012abf2d280128a3afd" (different)
This confirms ETag changes when content changes.
Test 3: Optimistic Concurrency (If-Match / If-None-Match)
Scenario A: Stale If-Match rejected with 412
curl -X PUT -H 'If-Match: "stale-etag-xyz"' \
-d '{"command":"python","args":["app.py"]}' \
http://127.0.0.1:8000/api/settings/mcp-servers/test-serverOutput: 412 Precondition Failed with current ETag in response headers: "edc3458be423398630349d9814f5bb2d"
This confirms clients get the current ETag in 412 responses so they can retry with correct value.
Scenario B: If-None-Match: * on existing server rejected
curl -X PUT -H 'If-None-Match: *' \
-d '{"command":"node","args":["app.js"]}' \
http://127.0.0.1:8000/api/settings/mcp-servers/test-serverOutput: 412 Precondition Failed with message: "If-None-Match: * requires the resource to not exist"
This confirms create-only semantics work.
Scenario C: Concurrent edits to different servers don't conflict
- Created
server-awithIf-None-Match: *→ 201 - Created
server-bwithIf-None-Match: *→ 201 (no conflict) - Both servers appear in
GET /api/settings/mcp-servers
This confirms per-server granularity eliminates false conflicts.
Test 4: X-Expose-Secrets Header Modes
Created server with secrets:
{"command":"curl","env":{"AUTH_TOKEN":"super-secret-token"},"headers":{"Authorization":"Bearer secret-bearer"}}Mode 1 — Default (no header):
curl http://127.0.0.1:8000/api/settings/mcp-servers/expose-testResult: "AUTH_TOKEN": "[REDACTED]" ✓
Mode 2 — X-Expose-Secrets: plaintext:
curl -H 'X-Expose-Secrets: plaintext' http://127.0.0.1:8000/api/settings/mcp-servers/expose-testResult: "AUTH_TOKEN": "super-secret-token" ✓
Mode 3 — X-Expose-Secrets: encrypted:
curl -H 'X-Expose-Secrets: encrypted' http://127.0.0.1:8000/api/settings/mcp-servers/expose-testResult: "AUTH_TOKEN": "gAAAAABqA5FOlf9HTswqxihPS4DfedJhcdGYffT9pkc7rCimVF..." (Fernet ciphertext) ✓
This confirms all three exposure modes work correctly.
Test 5: Server Isolation (Key Value Proposition)
Goal: Verify editing server A preserves server B's secrets without client round-tripping B's data.
Setup:
curl -X PUT -d '{"command":"echo","args":["alpha"],"env":{"SECRET_ALPHA":"alpha-secret-123"}}' \
http://127.0.0.1:8000/api/settings/mcp-servers/server-alpha
curl -X PUT -d '{"command":"echo","args":["beta"],"env":{"SECRET_BETA":"beta-secret-456"}}' \
http://127.0.0.1:8000/api/settings/mcp-servers/server-betaCaptured server-beta's secret:
curl -H 'X-Expose-Secrets: plaintext' http://127.0.0.1:8000/api/settings/mcp-servers/server-betaResult: "SECRET_BETA": "beta-secret-456"
Edited server-alpha (request does NOT include server-beta's data):
curl -X PATCH -d '{"description":"Updated alpha server"}' \
http://127.0.0.1:8000/api/settings/mcp-servers/server-alphaVerified server-beta's secret unchanged:
curl -H 'X-Expose-Secrets: plaintext' http://127.0.0.1:8000/api/settings/mcp-servers/server-betaResult: "SECRET_BETA": "beta-secret-456" (identical) ✓
This confirms the per-server CRUD avoids round-tripping other servers' secrets through the client.
Test 6: ACP Variant Handling
Switched to ACP variant:
curl -X PATCH -d '{"agent_settings_diff":{"agent_kind":"acp","acp_server":"claude-code","llm":{"model":"claude-sonnet-4-20250514"}}}' \
http://127.0.0.1:8000/api/settingsLIST on ACP variant:
curl http://127.0.0.1:8000/api/settings/mcp-serversResult: 200 OK with {"servers":[]} ✓ (permissive for polling clients)
GET on ACP variant:
curl http://127.0.0.1:8000/api/settings/mcp-servers/test-serverResult: 404 Not Found ✓
PUT on ACP variant:
curl -X PUT -d '{"command":"echo","args":["test"]}' \
http://127.0.0.1:8000/api/settings/mcp-servers/new-serverResult: 409 Conflict with message: "MCP servers are not supported by the current agent variant ('acp'). Switch to an OpenHands-variant agent via PATCH /api/settings before managing MCP servers." ✓
This confirms the router handles ACP variant correctly with appropriate status codes and helpful error messages.
Test 7: HTTP Exception Handler Propagates Headers
Verified 412 responses include ETag header (from api.py changes to propagate exc.headers):
curl -v -X PUT -H 'If-Match: "stale-etag-xyz"' \
-d '{"command":"python","args":["app.py"]}' \
http://127.0.0.1:8000/api/settings/mcp-servers/test-serverResponse headers include: etag: "edc3458be423398630349d9814f5bb2d" ✓
This confirms the HTTP exception handler changes work correctly — clients receive the current ETag in 412 responses so they can retry with the correct value.
Issues Found
None.
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Read #3234 for context. The per-server resource shape is the right direction, and the ETag/error-header plumbing is solid. But there’s one real data-loss bug here that keeps this off approval.
🔴 Needs improvement
[CRITICAL ISSUES]
-
openhands-agent-server/openhands/agent_server/mcp_servers_router.py:313Breaking Change / Data Loss:_validate_servers_dict()reconstructsmcp_configas just{"mcpServers": servers}, and the write paths then assign that back ontoagent_dump["mcp_config"].fastmcp.MCPConfigisadditionalProperties=True, so top-level keys besidesmcpServersare valid and round-trip today. This router silently deletes them on any PUT/PATCH/DELETE.I reproduced this locally by seeding settings through the legacy endpoint with:
{"mcp_config": {"mcpServers": {"a": {"command": "echo"}}, "extraTop": {"a": 1}}}then calling
PATCH /api/settings/mcp-servers/a, after whichextraTopdisappeared from persisted settings.That’s a bad boundary bug: the new fine-grained API is mutating more than the named resource. The fix should preserve the full existing
mcp_configpayload and splice onlymcpServers, then revalidate that full object.
[TESTING GAPS]
tests/agent_server/test_mcp_servers_router.pyMissing regression coverage: there’s no test that seedsmcp_configwith a sibling top-level key, performs a per-server PUT/PATCH/DELETE, and verifies that sibling data survives the persistence round-trip. Add one; this bug is easy to reintroduce.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
The API design is good and the concurrency/secret handling coverage is strong, but the current implementation can silently drop valid persisted MCPConfig data on ordinary writes. That’s user-visible settings loss.
VERDICT:
❌ Needs rework: preserve the full mcp_config object when mutating mcpServers.
KEY INSIGHT:
Once MCPConfig comes from FastMCP, the safe unit of mutation is “full mcp_config, replace only mcpServers” — not “rebuild mcp_config from scratch around mcpServers”.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.
|
OpenHands encountered an error: Request timeout after 30 seconds to https://fauiumbpeveovopc.prod-runtime.all-hands.dev/api/conversations/db269517-fa31-4bfc-af9c-0c9901ab8ef0/ask_agent See the conversation for more information. |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
4 participants
This PR was opened by an AI agent (OpenHands) on behalf of the user.
Follow-up to #3234 (typed settings update endpoints) — that PR called out the MCP-server-as-blob problem; this PR fixes it. The two are independent and don't share code; the small
api.pyexc.headerspropagation fix appears in both with identical content (trivial merge resolution either way).Why
mcp_configis the worst-fit case for the settings PATCHMCP servers live as a dict under
agent_settings.mcp_config.mcpServers. Editing one through the global settings PATCH means:mcpServersmapping — which forces the client to ship every other server's encryptedenv/headersciphertexts that they had no business round-tripping.mcp_configis documented in OpenAPI as one shape; adding/removing/editing a server is undocumented and indirect.env/headersare dicts of secrets. Every byte that doesn't need to leave the server shouldn't.What this PR adds
A dedicated router that treats each MCP server as its own named resource:
GET/api/settings/mcp-serversGET/api/settings/mcp-servers/{name}X-Expose-Secretslike/api/settings).PUT/api/settings/mcp-servers/{name}PATCH/api/settings/mcp-servers/{name}DELETE/api/settings/mcp-servers/{name}Body for PUT/PATCH is the raw
fastmcpserver config dict (the value ofMCPConfig.mcpServers[name]). fastmcp's own validators discriminate stdio vs remote vs the transforming variants based oncommand/urlpresence; this router passes through. Cross-server invariants fromMCPConfigre-fire on every write because each mutation re-validates the fullOpenHandsAgentSettingsafter splicing.Per-server optimistic concurrency
ETagon every read of/{name}. Computed over a plaintext-canonical JSON projection of just that server's config — not on-disk bytes, since Fernet nonces would make identical state hash differently every save and defeat idempotency.If-Match: <etag>on PUT/PATCH/DELETE → 412 (with currentETagechoed in the header) if stale.If-Match: *→ must exist (update-only PUT).If-None-Match: *→ must not exist (create-only PUT).If-None-Match: <etag>→ 412 if it matches current (symmetric toIf-Match).The headline win: concurrent edits to different servers don't conflict at all. Same-server conflicts get a clean 412 with the new state's ETag, so the client can rebase and retry.
X-Expose-Secretson GETSame three modes as
GET /api/settings:env/headersvalues are[REDACTED]encrypted→ Fernet ciphertextplaintext→ raw valuesLogged at WARNING level when plaintext is requested, just like the existing settings endpoint.
ACP variant handling
ACPAgentSettingshas nomcp_configfield. Choices for each verb:GET /api/settings/mcp-servers→200 {"servers": []}(list is non-modifying; permissive shape so polling clients stay simple)GET /api/settings/mcp-servers/{name}→404PUT/PATCH/DELETE→409 Conflictwith a message pointing the caller atPATCH /api/settingsto switch agent variant.Persistence
All writes go through the existing file-locked
store.update(), so they remain atomic and serialise with each other and with the globalPATCH /api/settings. No new persistence backend, no parallel lock. Each write rebuilds the fullOpenHandsAgentSettingspost-splice and re-validates, so encryption-on-save / decryption-on-load all run through the existing tested paths.Bonus fix in
api.pyThe shared
HTTPExceptionhandler was constructing theJSONResponsefromstatus_code+detailbut droppingexc.headers, so any handler-set response header on an error (e.g. our 412 ETag, orWWW-Authenticateon a 401) was silently lost. Three-line propagation fix; identical to the same fix in #3234 so either merge order is trivial to resolve.SDK additions
New public types in
openhands.sdk.settings:MCPServerSummary— list-view shape (no secrets).MCPServerListResponse— wraps the list.MCPServerResponse— single-server read/write response:{name, config: dict[str, Any]}.configisdict[str, Any](not a typed model) so the server controls secret serialization via context the same waySettingsResponse.agent_settingsdoes.Tests
tests/agent_server/test_mcp_servers_router.py— 32 tests covering:If-Matchmatching / stale,If-None-Match: *create-only,If-Match: *update-only, stale 412 returns current ETag.If-Match, doesn't disturb other servers.X-Expose-Secretshonoured: default redacted,plaintextraw,encryptedFernet token.[A-Za-z0-9_.@:-]{1,128}; slashes rejected (URL path segment).Compatibility
PATCH /api/settingscontinues to work for mcp_config exactly as today; clients can migrate incrementally.api.pyexception-handler fix is a tiny improvement that only adds headers fromHTTPException(headers=...)that route handlers explicitly set; it never alters the body, status code, or removes anything.Test summary
tests/agent_server/test_mcp_servers_router.py: 32 / 32 pass.tests/agent_server/test_settings_router.py: 28 / 28 pass (unchanged).tests/sdk/test_settings.py+tests/sdk/settings/*: 87 / 87 pass (unchanged).Out of scope (further follow-ups)
mcp_config.mcpServers[name].toolshas its own per-tool transformation config — if that becomes a hot edit target, a/mcp-servers/{name}/tools/{tool}sub-router would apply the same logic at the next level down.PATCH /api/settingsmutating something else, we could expose a compound ETag. Not needed for the per-server flow.Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:d75b1fe-pythonRun
All tags pushed for this build
About Multi-Architecture Support
d75b1fe-python) is a multi-arch manifest supporting both amd64 and arm64d75b1fe-python-amd64) are also available if needed