[codex] fix ACP MCP auth forwarding#3667
Conversation
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Exercised the SDK ACPAgent session startup path with a local ACP-compatible stdio server and confirmed the PR forwards remote MCP auth as an ACP bearer header while preserving explicit Authorization headers.
Does this PR achieve its stated goal?
Yes. The PR set out to translate FastMCP-style remote MCP auth into ACP-compatible bearer headers and avoid overriding explicit Authorization headers. In the base branch, an ACP session created from an MCP server with auth: "token-y" received headers: []; on the PR branch, the same real Conversation.run() / ACPAgent path sent Authorization: Bearer token-y in session/new. When both headers.authorization and auth were configured, the PR branch forwarded only the explicit authorization: Bearer explicit header, confirming it does not overwrite user-provided Authorization.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Dependencies were available after repository setup (uv sync --dev completed earlier); SDK scripts ran through uv run successfully. |
| CI Status | 🟡 At review time: 22 checks passing, 6 Docker build/push checks still in progress, 1 skipped cleanup check. |
| Functional Verification | ✅ Base reproduced the missing ACP auth header; PR forwarded the bearer header and preserved explicit Authorization. |
Functional Verification
Test 1: Remote MCP string auth is forwarded to ACP as a bearer header
Step 1 — Reproduce / establish baseline (without the fix):
Checked out origin/main (4c66baf7) and ran a small SDK scenario that creates an ACPAgent, starts a real Conversation.run(), and points it at a local ACP-compatible stdio server that captures the session/new payload:
git checkout --quiet origin/main
rm -f /tmp/qa_acp_trace.jsonl /tmp/qa_acp_mcp_auth.jsonl
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_run_acp_mcp_auth.py authRelevant captured output:
{"method": "session/new", "params": {"cwd": "/home/runner/work/software-agent-sdk/software-agent-sdk/pr-repo", "mcpServers": [{"headers": [], "name": "remote", "type": "http", "url": "https://example.invalid/mcp"}]}}This confirms the bug: the SDK forwarded the remote MCP server to ACP, but the auth: "token-y" setting was dropped, leaving ACP with no Authorization header.
Step 2 — Apply the PR's changes:
Checked out the PR branch at 067e271583842f2512779af4afb7811b0f8d519a:
git checkout --quiet codex/acp-mcp-auth-forwardingStep 3 — Re-run with the fix in place:
Ran the same SDK scenario on the PR branch:
rm -f /tmp/qa_acp_trace.jsonl /tmp/qa_acp_mcp_auth.jsonl
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_run_acp_mcp_auth.py authRelevant captured output:
{"method": "session/new", "params": {"cwd": "/home/runner/work/software-agent-sdk/software-agent-sdk/pr-repo", "mcpServers": [{"headers": [{"name": "Authorization", "value": "Bearer token-y"}], "name": "remote", "type": "http", "url": "https://example.invalid/mcp"}]}}This shows the PR fixes the ACP forwarding boundary: the same user-facing SDK flow now sends the expected bearer Authorization header to the ACP provider.
Test 2: Explicit Authorization header is preserved when auth is also configured
Step 1 — Establish baseline:
On origin/main, I ran the same SDK scenario with both headers.authorization and auth configured:
git checkout --quiet origin/main
rm -f /tmp/qa_acp_mcp_auth.jsonl
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_run_acp_mcp_auth.py explicitRelevant captured output:
{"method": "session/new", "params": {"cwd": "/home/runner/work/software-agent-sdk/software-agent-sdk/pr-repo", "mcpServers": [{"headers": [{"name": "authorization", "value": "Bearer explicit"}], "name": "remote", "type": "http", "url": "https://example.invalid/mcp"}]}}The baseline already preserved the explicit header because it ignored auth entirely.
Step 2 — Apply the PR's changes:
Checked out the PR branch again:
git checkout --quiet codex/acp-mcp-auth-forwardingStep 3 — Re-run with the fix in place:
rm -f /tmp/qa_acp_trace.jsonl /tmp/qa_acp_mcp_auth.jsonl
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_run_acp_mcp_auth.py explicitRelevant captured output:
{"method": "session/new", "params": {"cwd": "/home/runner/work/software-agent-sdk/software-agent-sdk/pr-repo", "mcpServers": [{"headers": [{"name": "authorization", "value": "Bearer explicit"}], "name": "remote", "type": "http", "url": "https://example.invalid/mcp"}]}}This verifies the new auth-to-bearer behavior does not override an explicitly configured Authorization header or add a duplicate bearer token.
Issues Found
None.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
2 participants
Summary
authinto ACP-compatible bearer headersheadersandauthare configuredRoot cause
The normal SDK MCP client understands remote MCP
auth, but ACP session creation can only receive MCP headers. ACP forwarding passedheadersthrough and dropped stringauth, so authenticated remote MCP servers could be visible in settings but unavailable to ACP providers like Codex.Validation
uv run --project /home/gneubig/work/software-agent-sdk pytest tests/sdk/agent/test_acp_agent.py -quv run --project /home/gneubig/work/software-agent-sdk ruff check openhands-sdk/openhands/sdk/agent/acp_agent.py tests/sdk/agent/test_acp_agent.pyAgent 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:968cc8b-pythonRun
All tags pushed for this build
About Multi-Architecture Support
968cc8b-python) is a multi-arch manifest supporting both amd64 and arm64968cc8b-python-amd64) are also available if needed