Repository navigation
Conversation
Return current linked credentials to runtime clients without changing stored profiles or default settings. Keep editor reads unresolved. Cover encrypted storage, rotation, and real HTTP profile selection with RemoteWorkspace. Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
|
@OpenHands /codereview this pr please and post your feedback as a comment. look at the linked pr too. do we really need to add a plaintext secrets? other options? why does it seemingly needed? |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
🟢 Good taste I looked through this PR and the linked OpenHands/extensions#547 flow. I do not think this PR adds a new plaintext-secret capability: Why it is needed in the current design: a linked profile deliberately stores only There are alternatives, but none is a smaller fix:
The relevant trust boundary remains unchanged: The tests cover encrypted-at-rest provider credentials, key rotation, unchanged stored profiles/default settings, and a real HTTP path through [RISK ASSESSMENT]
This touches credential material, so the impact of a mistake is meaningful. The implementation itself is narrow, authenticated under the existing trust model, preserves redacted/encrypted editor reads, and adds no new secret-exposure mode. The two current failing checks appear unrelated: a timing-sensitive session-socket test and an expired deprecation elsewhere. VERDICT: KEY INSIGHT: This review comment was generated by an AI agent (OpenHands) on behalf of @enyst. |
|
Reviewed PR #4952 and linked OpenHands/extensions#547, then posted feedback here: Conclusion: the PR does not introduce plaintext exposure; it makes provider-linked profiles consistent with the existing authenticated Validation:
GitHub formal approval was not possible because the available token belongs to the PR author, so I posted the requested review as a PR comment. |
Coverage Report •
|
||||||||||||||||||||
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope
This change belongs in this repository. openhands-agent-server/openhands/agent_server/profiles_router.py is the canonical Agent Server REST surface, which this repo owns; the extensions/automation follow-ups only consume it. No file move or maintainer architecture decision is needed.
What the change does
GET /api/profiles/{name} now passes resolve_provider=expose_mode == "plaintext" to LLMProfileStore.load. Previously the route always used resolve_provider=False. I traced the surrounding code:
- The plaintext branch (the only one that resolves) then dumps with
build_expose_context("plaintext", cipher), so the profile's own secret is exposed as before and now the linked provider'sapi_key/base_urlare added. - The absent-header and
encryptedbranches keepresolve_provider=False, so editor/frontend reads still return the stored reference with a nulledapi_key. This matches the PR's stated intent. loaddoes not write anything, so resolving a provider does not copy credentials into the stored profile. The new parametrized test asserts exactly that (store.load(..., resolve_provider=False)still hasapi_key is None).
The change is minimal, has no side effects on unrelated state, and is consistent with the existing plaintext exposure already available through GET /api/settings (which exposes the resolved provider key on the active profile). The routes are behind check_session_api_key, so plaintext reads are authenticated backend-only traffic. I found no new authorization or disclosure escalation.
One behavior nuance worth making explicit for reviewers (not a defect): for a plaintext read of a profile whose provider_connection_id dangles, load raises ProviderConnectionNotFound, which store_errors() maps to 422 — previously the plaintext read returned 200 with a nulled key. That is the right call (a dangling link cannot yield a runnable LLM config, and it matches /{name}/activate), and ordinary/encrypted reads keep the old unresolved behavior as the description says. The docstring enumerates the modes but does not mention this 422; a one-line note would be nice-to-have, not blocking.
Verification
- Local:
uv run pytest tests/agent_server/test_profiles_router.py -q→ 106 passed. - CI: the exact head
edc90a4passed all required checks, and the cross-tests job log shows both new live-server tests running and passing (test_workspace_named_llm_resolves_current_provider_credentials PASSED,test_workspace_default_llm_resolves_active_profile_despite_settings_drift PASSED). - I could not run the two live-server tests locally: they abort at import in
env_parser.merge(IndexError: list assignment index out of range) because this sandbox injects anOH_*env var that collides with config-file list merging. I reproduced the identical failure on a checkout oforigin/mainfor a pre-existing test, so it is an environment artifact, not a regression from this PR.
Failing check on this head
The Validate PR description check is failing on edc90a4. The failure is not about code: the ## Issue Number section references OpenHands/extensions#548 and OpenHands/automation#430. The validator regex cannot match a repo#N reference, so it finds no linked issue and errors with "Link an issue in the ## Issue Number section". Both referenced issues are healthy — extensions#548 and automation#430 are open and carry ready-for-dev — and the same-numbered issues in this repo are unrelated, closed, and pre-rollout. Because the actual work is owned downstream, the PR structurally cannot satisfy this repo's in-repo linked-issue gate. This is a process/merge-readiness gap rather than a code defect, but it is red on the current head, so I am leaving this for a human maintainer to confirm the cross-repo linkage is acceptable (or to add an in-repo tracking issue) before merging. No version bumps or dependency changes are present.
🔄 CHANGES REQUESTED
Co-authored-by: openhands <openhands@all-hands.dev>
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope
Belongs here. openhands-agent-server/openhands/agent_server/profiles_router.py is this repo's canonical Agent Server REST surface; the extensions/automation follow-ups only consume it. No file move or architecture decision needed.
Change
GET /api/profiles/{name} now passes resolve_provider=expose_mode == "plaintext" to LLMProfileStore.load instead of the hard-coded resolve_provider=False.
I traced both branches against the base behavior:
- Plaintext (the only mode that resolves):
loadapplies the linked connection'sapi_key/base_url, and the subsequentbuild_expose_context("plaintext", cipher)dump exposes them alongside the profile's own secret. This is exactly the credential setRemoteWorkspace.get_llm(profile_name=...)needs, and it matches the plaintext exposure already available on the active profile viaGET /api/settings. - Absent-header and
encrypted: stillresolve_provider=False, so editor/frontend reads keep the stored reference and a nulledapi_key. Matches the stated intent. loadperforms no writes, so resolving does not copy provider credentials into the stored profile; the new parametrized test asserts precisely that with aresolve_provider=Falsere-read.
No new authorization or disclosure escalation: the routes sit behind check_session_api_key, so plaintext reads are authenticated backend traffic, and the same resolved key was already reachable through the settings endpoint. No version bumps or dependency changes.
One non-blocking nuance: a plaintext read of a profile whose provider_connection_id dangles now surfaces ProviderConnectionNotFound -> 422 via store_errors(), where it previously returned 200 with a nulled key. That is the correct trade (a dangling link cannot yield a runnable config, and it mirrors /{name}/activate); the mode docstring just doesn't call it out. Optional to note, not worth blocking.
Verification
- Local:
uv run pytest tests/agent_server/test_profiles_router.py -q-> 109 passed. - CI on exact head
da27a9ec: every check green, includingValidate PR description(the previously failing cross-repo linked-issue gate is resolved by the newFixes #5514, which is open and carriesbug,priority:medium,ready-for-dev). The cross-tests log confirms both new live-server tests ran and passed. - The two live-server tests cannot run in this sandbox: they abort at import in
env_parser.merge(IndexError) because an injectedOH_*var collides with config-file list merging. I reproduced the identical failure on a cleanorigin/maincheckout for a pre-existing test, so it is an environment artifact, not a regression from this PR.
✅ APPROVED
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope
Belongs here. openhands-agent-server/openhands/agent_server/profiles_router.py is this repo's canonical Agent Server REST surface; the extensions/automation follow-ups only consume it.
What changed since the last reviewed head
The previous head da27a9ec was approved. This head 6b41f7ad differs only by commit "chore: Remove PR-only artifacts [automated]", which deletes .pr/stop-bash-evidence.md and .pr/stop-bash-evidence.py — scratch evidence files left over from the unrelated BashCommand PR #5348, absent from both main and this head. The router diff and both test files are byte-identical to the approved head (git diff da27a9ec 6b41f7ad -- .../profiles_router.py is empty), so the substantive change is unchanged.
Change
GET /api/profiles/{name} now passes resolve_provider=expose_mode == "plaintext" to LLMProfileStore.load instead of the hard-coded resolve_provider=False.
- Plaintext (the only resolving mode):
loadapplies the linked connection'sapi_key/base_url, and thebuild_expose_context("plaintext", cipher)dump exposes them alongside the profile's own secret — the credential setRemoteWorkspace.get_llm(profile_name=...)needs. - Absent-header and
encrypted: stillresolve_provider=False, so editor/frontend reads keep the stored reference and a nulledapi_key. loadperforms no writes, so resolution does not copy provider credentials into the stored profile; the parametrized test asserts that with aresolve_provider=Falsere-read.
No new authorization or disclosure escalation: routes sit behind check_session_api_key, and the same resolved key was already reachable via GET /api/settings plaintext. No version bumps or dependency changes in this diff.
One non-blocking nuance carried over: a plaintext read of a profile with a dangling provider_connection_id now surfaces ProviderConnectionNotFound -> 422 via store_errors() rather than 200 with a nulled key. That is the correct trade and mirrors /{name}/activate.
Verification
- Local:
uv run pytest tests/agent_server/test_profiles_router.py -q-> 109 passed. - CI on exact head
6b41f7ad: all checks green, includingValidate PR description, and the cross-tests log shows both new live-server tests passing. Linked issue #5514 is open withbug,priority:medium,ready-for-dev. - The two live-server tests cannot run in this sandbox: they abort at import in
env_parser.merge(IndexError) because an injectedOH_*var collides with config-file list merging. Reproduced identically on a cleanorigin/maincheckout for a pre-existing test, so it is an environment artifact, not a regression.
✅ APPROVED
2 participants
HUMAN:
This PR proposes an addition to assist PR 547 on extensions/ repo to show the LLM profile of an Automation.
AGENT:
I am an AI agent (smolpaws) acting on behalf of Engel.
Why
OpenHands/extensions#547 selects a named LLM profile and passes its configuration to a new conversation, but plaintext profile reads currently omit a linked provider's API key and base URL.
Summary
GET /api/profiles/{name}withX-Expose-Secrets: plaintext.RemoteWorkspace.get_llm(profile_name=...).REST API contract changes
Compared with base OpenAPI
b66c72436157for public/api/**paths.Issue Number
Fixes #5514
Supports OpenHands/extensions#548 and OpenHands/automation#430.
How to Test
uv run pytest tests/agent_server/test_profiles_router.py -q uv run pytest tests/cross/test_remote_conversation_live_server.py -k 'workspace_named_llm_resolves_current_provider_credentials or workspace_default_llm_resolves_active_profile_despite_settings_drift' -q uv run pre-commit run --files openhands-agent-server/openhands/agent_server/profiles_router.py tests/agent_server/test_profiles_router.py tests/cross/test_remote_conversation_live_server.pyResults: 106 profile tests passed; both live-server tests passed; all applicable pre-commit hooks passed.
The live test starts Uvicorn, creates a provider and profile through HTTP, selects the profile through the SDK, rotates the provider key, and confirms the next selection uses that key while the active default is unchanged.
The existing shared-provider test failed on the original route because the plaintext API key was
None, then passed with this fix.Type
Notes
This dependency must reach the deployed Agent Server before the extensions#547 follow-up can run provider-linked profiles. No external LLM calls are used by these tests.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang: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:6b41f7a-pythonRun
All tags pushed for this build
About Multi-Architecture Support
6b41f7a-python) is a multi-arch manifest supporting both amd64 and arm646b41f7a-python-amd64) are also available if neededJev-Fast-Audit
⚡ Jev fast audit · estimates · 0.74s · commit edc90a4
Strongest signal: Sensitive data disclosure · 25% estimated likelihood.
Evidence: F001H003 · openhands-agent-server/openhands/agent_server/profiles_router.py:181–191.
Coverage: complete supplied coverage; 8/8 hunks, 3/3 files.
All estimates and evidence