fix(subagent): decrypt encrypted model profiles

[Shimada666]

HUMAN:

This fixes sub-agent authentication failures when named LLM profiles are encrypted at rest.

---

AGENT:

Why

When OH_SECRET_KEY is configured, saved LLM profile secrets are encrypted at rest. File-based sub-agents loaded named profiles without the conversation cipher, so the encrypted API key reached the LLM provider and authentication failed.

Summary

• Forward the conversation cipher when registering file-based, plugin, and remote sub-agent definitions.
• Decrypt named LLM profiles before creating a sub-agent.
• Add a regression test for encrypted profile API keys.

Issue Number

Related to #4288.

How to Test

1. Run uv run pytest -q tests/sdk/subagent/test_subagent_registry.py tests/sdk/conversation/test_local_conversation_plugins.py — 81 passed.
2. Run uv run pytest -q tests/agent_server/test_conversation_service.py tests/agent_server/test_conversation_service_plugin.py — 109 passed.
3. Run pre-commit on all changed files — Ruff, Pyright, import rules, and tool registration passed.
4. Save a Luna LLM profile with include_secrets=True and a Cipher, load an evidence-coder agent from an explicit plugin through LocalConversation, and instantiate the registered factory. The smoke test returned model openai/gpt-5.6-luna and confirmed the API key matched the original plaintext.

Video/Screenshots

Not applicable; this is an SDK credential-loading fix. The deterministic regression test and plugin smoke-test results are described above.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

Documentation: OpenHands/docs#696

[all-hands-bot]

🚦 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 @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[enyst]

Thank you for the PR! 🤔 TBH seeing how we need to keep treading it around only for the profile store (which is the only place who needs it) makes me think that maybe we could have set it in the profile store in the first place...

This is not a suggestion for change right now, though, because we also have a better secrets design issue, and those changes will clean up this... I think. @OpenHands can you find and link that issue? Then tell us WDYT.

[enyst]

I am OpenHands (OpenHands-Deepseek-Flash), an AI agent acting on behalf of @enyst. (Note: this is posted through enyst's GitHub token, so it appears under their name — the words here are from the AI agent, not enyst.)

Regarding the "better secrets design issue" you asked about — it's #4288 (Design: reference-only credentials and safe runtime delivery): #4288

That's the consolidated design doc enyst contributed to (stdio-MCP, egress-proxy, OS-keyring, iron-proxy), and it's the one whose §2/§9 covers exactly this coupling: the cipher/expose_secrets serialization context is threaded through ~30 modules including llm/llm_profile_store.py, profiles/agent_profile_store.py, and the profile refs — and §9.4 explicitly calls out that removing secret-aware serialization from durable models is cut for now in favor of Phase 0 (canary gate) + Phase 1 (fail-soft loading) + Phase 2 (SecretRef).

On the WDYT: I partly agree the threading is a smell — cipher is only consumed by the profile store on this path, so it looks heavy to thread it through register_*_agents / agent_definition_to_factory. But it's worth noting this isn't a new pattern the PR invents: passing cipher into LLMProfileStore.load(...) is already the codebase convention at ~7 call sites (profiles/resolver.py, LocalConversation on resume/switch, settings_router, profiles_router, agent_profiles_router). The cipher is a process-global value derived deterministically from OH_SECRET_KEY, so "setting it in the profile store" in the constructor (rather than per-call) is feasible and would shrink a diff like this one — but it's a broader API change to the store, and per enyst's own note it gets superseded by #4288 Phase 2 (SecretRef, where the store stops holding/ciphering values at all). For this bugfix, keeping the existing load(name, cipher=...) convention is the minimal, consistent fix; the real cleanup is tracked in #4288. I'd suggest a small follow-up (e.g. cache the cipher on the @lru_cached store) rather than changing this PR's shape.

So: agree it's temporary, agree it's the profile store's job to own at-rest handling, and #4288 is the right place to track killing the whole pattern — link added above. WDYT, good to proceed with this as-is?

[Shimada666]

Thanks for the review! I agree that threading the cipher through solely for the profile store is not ideal. Given #4288 and the bot’s recommendation, I’m happy to keep this PR focused as-is. Please let me know if you’d like me to make any changes before approval.

[all-hands-bot]

🚦 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 @all-hands-bot as a reviewer to have it reviewed regardless of CI status.)

This is an automated check - no AI was used to generate this comment.

[CarlosLanderas]

Hello, just found this bug as I'm also experiencing it when using subagents:

subagent AuthenticationError

Is the fix getting merged? Thanks!

[Shimada666]

Thanks @CarlosLanderas for confirming this in real use.

@enyst I rebased the PR onto the latest main; all checks are green on d87a6e8d. The current main still loads named sub-agent profiles without the conversation cipher, and we now have another user hitting the same subagent AuthenticationError. Would you mind taking another look when you have a chance?

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: d87a6e8de7890a83e0f7712bddf41ac841b0284c
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/07f9778e-472d-4ae7-9e2f-e9f21339cc54

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Overall verdict: No material findings. Approve from a review standpoint.

The change correctly threads the conversation cipher through the sub-agent registration path so that encrypted LLM profiles are decrypted before reaching the LLM provider. All four modified production call sites are covered:

• agent_definition_to_factory now accepts cipher and forwards it to store.load(profile_name, cipher=cipher) — the only place where the cipher is actually needed.
• register_file_agents / register_plugin_agents forward cipher to the factory.
• LocalConversation passes self._cipher at all three registration call sites (2 plugin-agent, 1 file-agent).
• ConversationService._register_agent_definitions receives and forwards self.cipher at both the new-conversation and resume-conversation call sites.

Design note (not blocking): The per-load cipher approach (passing it to store.load rather than setting it on the LLMProfileStore instance) is the right call given that _get_profile_store is a module-level lru_cache singleton shared across conversations. If the cipher were stored on the shared store instance, concurrent conversations with different secret keys but the same profile_store_dir would race on the cipher. The per-load parameter avoids that cleanly. This aligns with enyst's comment that a broader secrets redesign may simplify this later, but the current approach is correct for the existing architecture.

Coverage: register_builtins_agents in preset/default.py still calls agent_definition_to_factory(agent_def) without cipher. This is fine — all built-in agents use model: inherit, so the store.load path where cipher is consumed is never reached for builtins. No change needed.

Test: The regression test (test_agent_definition_to_factory_decrypts_model_profile) exercises the real encrypt→decrypt round trip (no mocks) and verifies the decrypted API key matches the original plaintext. I confirmed it passes.

Risk: LOW. Backward compatible (cipher defaults to None), focused scope, and the fix improves credential handling rather than introducing new surface area.