Preserve encrypted MCP credentials for agent conversations#1426
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
✅ Mock-LLM E2E Tests60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
neubig
changed the title
Contributor
✅ Mock-LLM E2E Tests60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
neubig
force-pushed
the
codex/fix-mcp-acp-secret-preservation
branch
from
eae12fd to
b048161
Compare
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. This comment was generated by an AI agent (OpenHands) on behalf of the user. |
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
🟡 Acceptable - The main credential preservation path is reasonable, but one redacted stdio edit path can still lose credentials.
[CRITICAL ISSUES]
- [src/api/mcp-service/mcp-redacted-credentials.ts, Line 53] Credential Preservation: Renaming a stdio MCP server while leaving a secret env value unchanged still writes literal
<redacted>over the stored encrypted secret. See inline comment.
[TESTING GAPS]
- Add a regression test for renaming a stdio server with unchanged redacted env values; it should assert the saved SDK config contains the encrypted value, not
<redacted>.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This touches credential handling and conversation-start payload encryption for MCP/ACP. The happy paths are covered, but the remaining stdio rename case can destroy saved credentials.
VERDICT:
❌ Needs rework: Please preserve stdio credentials across rename/edit before this lands.
KEY INSIGHT:
The data structure needs a stable server identity for secret restoration; the edited display name is not stable enough.
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. 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 is merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Contributor
✅ Mock-LLM E2E Tests60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Resolve stored stdio MCP credentials by the editor-assigned positional id (`stdio-<i>`) instead of the display name, so renaming a stdio server no longer loses its stored encrypted env. Previously the rename changed the dict-key lookup, missed the stored entry, and saved the literal `<redacted>` placeholder over the encrypted secret. Add regression tests for the rename case (direct unit test + save-flow test asserting the saved SDK config contains the encrypted value, not the placeholder).
Member
Author
|
Addressed the stdio rename credential-loss feedback in 0737ad4. Stdio servers are now resolved by their positional editor id ( |
Contributor
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
approved these changes
Jun 19, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Contributor
There was a problem hiding this comment.
🟢 Good taste — The previous critical issue (stdio server rename silently overwriting stored encrypted credentials with <redacted>) has been comprehensively fixed. The implementation is clean and the test coverage is solid.
What Changed Since the Last Review
The prior review raised one CRITICAL issue: substituteRedactedMcpCredentials was looking up stdio servers by display name, so a rename would miss the stored entry and write the literal placeholder over the encrypted secret. That is now fixed:
- Positional-id lookup (
stdio-<i>) is used instead of display-name matching. The id encodes the server's position in theparseMcpConfigstdio array, which is stable across renames becauseObject.entriesorder is deterministic for both the redacted and encrypted reads of the same stored dict. - The fallback (name match when no
idis available) is preserved for configs that pre-date the editor-assigned id. - A "rename onto an existing name" test ensures the positional path returns the original server's credentials, not the coincidentally-named one.
Code Quality Observations
mcp-redacted-credentials.ts: Clean extraction. The decision to separate credential-substitution frommcp-service.api.tsanduse-update-mcp-server.tsis the right call — the logic is now testable in isolation and shared between the two callers.hasEncryptedMcpSecretsinagent-server-adapter.ts: Correct scope — ACP needssecrets_encryptedonly when it actually carries Fernet tokens inmcp_config.env/mcp_config.headers. The Fernet prefix heuristic (gAAAAA) is stable enough for this purpose.getRemoteCredentialFieldsinmcp-config.ts: Handles the transition from api_key → headers cleanly, including the guard against writing the redacted placeholder into the SDK config.- Type additions (
headersonMCPSSEServer,MCPSHTTPServer,MCPServerConfig): Correctly propagated throughflattenMcpConfigand both mutation paths.
Tests
The four new test files cover the material scenarios:
| Scenario | File |
|---|---|
| stdio rename: redacted env → encrypted value | mcp-redacted-credentials.test.ts + use-update-mcp-server.test.ts |
| stdio rename onto existing name: positional id wins | mcp-redacted-credentials.test.ts |
| Non-redacted values pass through unchanged | mcp-redacted-credentials.test.ts |
Remote shttp: redacted api_key → encrypted Authorization header |
mcp-service.api.test.ts |
ACP with encrypted MCP headers sets secrets_encrypted |
agent-server-adapter.test.ts |
The tests run against real code paths and assert on outputs, not just mock invocations — exactly the right approach.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Credential preservation is the sensitive path here, but the change is additive (new helpers + positional lookup) and all regression scenarios are covered by dedicated tests. The human confirmation in the PR description further reduces risk. CI is in progress at review time.
VERDICT:
✅ Worth merging — Previous critical issue fully addressed, test coverage solid, implementation clean.
KEY INSIGHT:
Stable server identity (positional id, not mutable display name) is the right key for credential lookup; this PR gets that design exactly right.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Contributor
✅ Mock-LLM E2E Tests60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Contributor
✅ Mock-LLM Docker E2E Test Results60/60 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
Member
Author
|
All review feedback addressed and CI is green (16/16 successful, 0 failing). The PR is rebased onto main, mergeable with no conflicts, and approved. Requesting approval for merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HUMAN:
I confirmed that this failed before and worked after.
AGENT:
Why
ACP conversation start payloads that include MCP config need to preserve encrypted MCP env/header credentials. Redacted edit and test flows should restore stored encrypted credentials instead of overwriting them with placeholders.
Summary
How to Test
npm run make-i18n >/tmp/make-i18n.log && npx vitest run __tests__/utils/mcp-config.test.ts src/api/agent-server-adapter.test.ts src/api/mcp-service/mcp-service.api.test.tsnpm testnpm run typechecknpm run buildFixes #1427
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.28.1-pythonopenhands-automation==1.0.0a100737ad458cda53d05ffc10ff974d4cf25eaee234Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-0737ad4Run
All tags pushed for this build
About Multi-Architecture Support
sha-0737ad4) is a multi-arch manifest supporting both amd64 and arm64sha-0737ad4-amd64) are also available if neededFixes #1427