Repository navigation
feat(agent-server): add MCP settings CRUD endpoints - #4294
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Co-authored-by: openhands <openhands@all-hands.dev>
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
⚠️ QA Report: PASS WITH ISSUES
Live agent-server QA confirms the new MCP settings CRUD endpoints work end-to-end; the only issue observed is an existing failing PR-description CI check.
Does this PR achieve its stated goal?
Yes. The PR set out to add first-class POST/PATCH/DELETE /api/settings/mcp/{settings_key} operations with create-only, sparse update, delete, atomic key preconditions, redacted mutation responses, and OpenAPI discoverability. I verified those behaviors against a real agent-server: base returned 404 for the new endpoint and had no OpenAPI path, while the PR server created MCP entries, rejected duplicate create with 409, patched one server without losing a sibling credential, deleted only the targeted server, returned 404 for missing patch/delete, cleared auth with null, and exposed the three operations in /openapi.json.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully. |
| CI Status | Validate PR description is failing and qa-changes is in progress for this run. |
| Functional Verification | ✅ Real HTTP requests against live base and PR servers verified the changed behavior. |
Functional Verification
Test 1: Establish baseline without the new endpoint
Step 1 — Reproduce / establish baseline (without the fix):
Ran git checkout --detach origin/main && python /tmp/qa_mcp_settings_flow.py 18801 baseline baseline:
HEAD is now at 1f9f0b1a fix(llm): generalize model capability resolution (#4200)
SERVER_READY label=baseline mode=baseline url=http://127.0.0.1:18801
baseline_post_mcp_docs: {"detail": "Not Found", "status": 404}
baseline_get_settings_after_attempt: {"mcp_keys": [], "status": 200}
SERVER_STOPPED label=baseline returncode=143 log=/tmp/ohqa-baseline-yy9usdfb/agent-server.log
This confirms the old server did not have a first-class POST /api/settings/mcp/docs operation, and the failed request did not create an MCP server.
Also ran git checkout --detach origin/main && python /tmp/qa_openapi_path.py 18803 baseline-openapi:
{"label": "baseline-openapi", "mcp_path_present": false, "openapi_status": 200, "operations": {}}
This confirms the MCP CRUD path was not discoverable in base OpenAPI.
Step 2 — Apply the PR's changes:
Checked out 06e55b3eea555fecca8ca975b0e7bb64b9adbea2.
Step 3 — Re-run with the fix in place:
Ran python /tmp/qa_openapi_path.py 18804 pr-openapi:
{"label": "pr-openapi", "mcp_path_present": true, "openapi_status": 200, "operations": {"delete": "delete_mcp_server_api_settings_mcp__settings_key__delete", "patch": "patch_mcp_server_api_settings_mcp__settings_key__patch", "post": "create_mcp_server_api_settings_mcp__settings_key__post"}}
This shows the PR makes the new MCP CRUD operations discoverable through live OpenAPI output.
Test 2: Exercise MCP CRUD semantics as an API user
Step 1 — Baseline:
The baseline test above showed POST /api/settings/mcp/docs returned 404, so these first-class operations were not available before this PR.
Step 2 — Apply the PR's changes:
Checked out 06e55b3eea555fecca8ca975b0e7bb64b9adbea2.
Step 3 — Re-run with the fix in place:
Ran python /tmp/qa_mcp_settings_flow.py 18802 pr pr:
SERVER_READY label=pr mode=pr url=http://127.0.0.1:18802
create_github: {"mcp_keys": ["github"], "servers": {"github": {"auth": {"strategy": "bearer", "value": "**********"}, "transport": "http", "url": "https://github.example/mcp"}}, "status": 201}
duplicate_github: {"detail": "MCP server 'github' already exists", "status": 409}
create_docs: {"mcp_keys": ["docs", "github"], "servers": {"docs": {"transport": "http", "url": "https://docs.example/mcp"}, "github": {"auth": {"strategy": "bearer", "value": "**********"}, "transport": "http", "url": "https://github.example/mcp"}}, "status": 201}
patch_docs_description: {"mcp_keys": ["docs", "github"], "servers": {"docs": {"description": "Documentation", "transport": "http", "url": "https://docs.example/mcp"}, "github": {"auth": {"strategy": "bearer", "value": "**********"}, "transport": "http", "url": "https://github.example/mcp"}}, "status": 200}
plaintext_get_after_patch: {"mcp_keys": ["docs", "github"], "servers": {"docs": {"description": "Documentation", "transport": "http", "url": "https://docs.example/mcp"}, "github": {"auth": {"strategy": "bearer", "value": "github-secret"}, "transport": "http", "url": "https://github.example/mcp"}}, "status": 200}
delete_docs: {"mcp_keys": ["github"], "servers": {"github": {"auth": {"strategy": "bearer", "value": "**********"}, "transport": "http", "url": "https://github.example/mcp"}}, "status": 200}
patch_missing: {"detail": "MCP server 'missing' was not found", "status": 404}
delete_missing: {"detail": "MCP server 'missing' was not found", "status": 404}
clear_github_auth: {"mcp_keys": ["github"], "servers": {"github": {"transport": "http", "url": "https://github.example/mcp"}}, "status": 200}
SERVER_STOPPED label=pr returncode=143 log=/tmp/ohqa-pr-p8k_d_4a/agent-server.log
This confirms the PR delivers the promised API behavior: create returns 201, duplicate create returns 409, sparse patch preserves existing fields and sibling credentials, mutation responses redact credentials, plaintext GET can still retrieve the stored dummy credential, delete removes only the targeted server, missing patch/delete return 404, and auth: null clears auth.
Issues Found
- 🟡 Minor:
Validate PR descriptionis failing in CI. I did not edit the human-only PR template field; functional agent-server behavior verified successfully.
This review was created by an AI agent (OpenHands) on behalf of the user.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 OpenHands is reviewing this PR. Requested reviewer: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste
No material findings. The new endpoints reuse the canonical locked settings update path, keep create/update/delete preconditions atomic with persistence, preserve RFC 7386 sparse-update behavior, and return redacted settings. The unit, OpenAPI contract, live-server, and persistence coverage exercise the important success and failure paths.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This is an additive REST API change with focused scope. The main risks—concurrent mutation, accidental sibling/credential loss, secret exposure, persistence correctness, and contract drift—are handled by the existing store/update machinery and covered by tests and live QA evidence.
VERDICT:
✅ Worth merging: Core logic is sound and no important issues were found.
KEY INSIGHT:
Keeping the per-server operations as thin atomic adapters over the canonical settings merge path avoids a second source of persistence and secret-handling behavior.
This review was created by an AI agent (OpenHands) on behalf of the repository reviewer.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Verified the new Agent Server MCP settings CRUD API against a real local server; create, sparse update, delete, OpenAPI discoverability, redaction, and key preconditions worked as described.
Does this PR achieve its stated goal?
Yes. The PR set out to add first-class POST / PATCH / DELETE operations for /api/settings/mcp/{settings_key} with create-only collision handling, existing-key preconditions, sparse updates, and preserved/redacted credentials. Running the actual agent-server and making HTTP requests showed the path is absent on base, present in OpenAPI on the PR, and the new endpoints perform the claimed state transitions with 201, 200, 409, and 404 responses as expected.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv run created the project environment and a real agent-server started successfully with isolated HOME/XDG config directories. |
| CI Status | ✅ Current gh pr checks showed all non-skipped checks passing except this qa-changes run was still in progress at query time. |
| Functional Verification | ✅ Exercised the API via real HTTP requests against live base and PR servers. |
Functional Verification
Test 1: Establish base behavior without the new endpoints
Step 1 — Reproduce / establish baseline (without the fix):
Checked out origin/main (b3569aaf) and started the real server with:
env -i PATH="$PATH" HOME=/tmp/oh-qa-base2-home \
XDG_CONFIG_HOME=/tmp/oh-qa-base2-config \
XDG_RUNTIME_DIR=/tmp/oh-qa-base2-runtime \
OPENHANDS_SUPPRESS_BANNER=1 PYTHONUNBUFFERED=1 \
uv run agent-server --host 127.0.0.1 --port 18185Then queried OpenAPI and attempted the new endpoint shape:
BASE_HEAD=b3569aaf
openapi_has_mcp_settings_path False
post_qa_primary status 404 detail Not Found
patch_qa_primary status 404 detail Not Found
delete_qa_primary status 404 detail Not Found
This confirms the base server does not expose first-class MCP settings CRUD operations; clients would have to use the enclosing settings payload instead.
Step 2 — Apply the PR's changes:
Checked out PR commit 06e55b3eea555fecca8ca975b0e7bb64b9adbea2 and started the real server with the same isolated environment pattern on a new port.
Step 3 — Re-run with the fix in place:
Queried OpenAPI and exercised two user-created MCP keys (qa-primary, qa-docs) through real HTTP requests:
HEAD=06e55b3e
openapi_methods=delete,patch,post
initial_keys ['github']
create_primary status 201 response_auth {'strategy': 'bearer', 'value': '**********'}
create_docs status 201 created True primary_present True
patch_docs status 200 docs_description Documentation primary_auth_response {'strategy': 'bearer', 'value': '**********'}
plaintext_after_sibling_patch status 200 primary_auth_value primary-secret docs_description Documentation
duplicate_create status 409 detail MCP server 'qa-primary' already exists
missing_patch status 404 detail MCP server 'qa-missing' was not found
missing_delete status 404 detail MCP server 'qa-missing' was not found
replace_primary_auth status 200 response_auth {'strategy': 'bearer', 'value': '**********'}
plaintext_after_auth_replace status 200 primary_auth_value new-primary-secret
clear_primary_auth status 200 primary_has_auth False
delete_docs status 200 docs_present False primary_present True
final_plaintext status 200 keys_include_primary True docs_present False primary_has_auth False
This shows the PR adds the intended OpenAPI-discoverable operations and that the live API preserves sibling MCP entries and credentials during sparse updates, redacts secret values in mutation responses, allows plaintext retrieval through the existing explicit exposure header, enforces duplicate/missing preconditions, and deletes only the targeted MCP entry.
Issues Found
None.
This QA review was generated by an AI agent (OpenHands) on behalf of the user.
Co-authored-by: openhands <openhands@all-hands.dev>
3 participants
HUMAN:
As part of this stack of 3 PRs, I have verified that adding two consecutive MCPs works:
AGENT:
Why
Agent Server already owns canonical MCP settings, secrets, locking, and RFC 7386 merge behavior, but clients must currently construct the enclosing
PATCH /api/settingspayload themselves. First-class one-server operations make create, sparse update, and delete intent discoverable in OpenAPI and keep key preconditions atomic with persistence.Summary
POST /api/settings/mcp/{settings_key}for create-only semantics with409collision handling.PATCHandDELETEoperations on the same path for existing servers, returning404when the key is absent.REST API contract changes
Compared with base OpenAPI
1f9f0b1aa035for public/api/**paths.Issue Number
Fixes #4293
How to Test
uv run pytest tests/agent_server/test_settings_router.py tests/agent_server/test_openapi_contract.py -q— 74 passed.uv run pytest tests/cross/test_remote_conversation_live_server.py::test_settings_and_secrets_api_with_live_server -q— 1 passed against a real FastAPI/uvicorn server.make test-server-schema— deterministic export, OpenAPI type-quality check, and schema validation passed.uv run pre-commit run --files openhands-agent-server/openhands/agent_server/settings_router.py tests/agent_server/test_openapi_contract.py tests/agent_server/test_settings_router.py tests/cross/test_remote_conversation_live_server.py— all hooks passed.The stateful route test creates a bearer-authenticated GitHub MCP server, adds and edits a sibling, confirms the GitHub credential survives, deletes the sibling, replaces and clears auth, and verifies create/update/delete key preconditions.
Video/Screenshots
Not applicable: this is an Agent Server REST API change with no UI.
Type
Notes
The operations are additive. Mutation responses remain redacted, and all persistence still flows through the canonical settings update implementation under the existing store lock. The generated TypeScript contract and client wrappers will be updated after this Agent Server change lands in a release.
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:7dabdc1-pythonRun
All tags pushed for this build
About Multi-Architecture Support
7dabdc1-python) is a multi-arch manifest supporting both amd64 and arm647dabdc1-python-amd64) are also available if needed