fix(sdk): decrypt encrypted fallback profiles - #4568
Draft
Sehlani042 wants to merge 1 commit into
Draft
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
📁 PR Artifacts Notice This PR contains a |
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
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:
AGENT:
Why
Encrypted LLM profiles are decrypted on the main conversation path, but fallback profiles are resolved lazily through a separate
LLMProfileStorethat did not receive the conversation cipher. As a result,FallbackStrategycould pass a Fernet ciphertext (gAAAAA...) to the provider as an API key. This also affected strategies that had cached a fallback before being attached to a conversation.Summary
LLMProfileStoreto retain an optional default cipher while preserving explicit per-load cipher precedence.FallbackStrategyand invalidate previously resolved fallback/profile-store caches when the cipher changes.Issue Number
Fixes #4558
How to Test
Reproduction and regression:
include_secrets=Trueand aCipher.FallbackStrategy; the old path exposes the encryptedgAAAAA...token.LocalConversation(..., cipher=cipher)with that strategy.Commands and results:
uv run pytest -q \ tests/sdk/llm/test_llm_fallback.py \ tests/sdk/llm/test_llm_profile_store.py \ tests/sdk/conversation/test_switch_model.py # 110 passeduv run pre-commit run --files \ openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py \ openhands-sdk/openhands/sdk/llm/fallback_strategy.py \ openhands-sdk/openhands/sdk/llm/llm_profile_store.py \ tests/sdk/llm/test_llm_fallback.py # Ruff format/lint, PEP8, Pyright, import rules, and Tool registration passeduv run pytest -q tests/sdk # 5948 passed, 9 skipped, 10 xfailed, 2 failedThe same two full-suite failures reproduce on untouched
origin/main: LiteLLM now reports support formoonshot/kimi-k3, and the local nonexistent MCP endpoint returns HTTP 503. No additional failure is introduced by this patch.git diff --checkalso passes.Video/Screenshots
Not applicable; this is an SDK behavior fix covered by a deterministic executable regression.
Design Doc
Not included; the change is limited to cipher propagation and cache invalidation across existing SDK objects.
Type
Notes
Codex assisted with issue triage, implementation, and test execution. The patch was constrained to the fallback-profile cipher path and checked for competing pull requests before publication.