Repository navigation
Conversation
… cipher key After a cipher key change, undecryptable secrets loaded as None, so the stores' "different key" guards never fired and any settings PATCH or secrets PUT/DELETE answered 200 and saved null over the stored ciphertext. Read-modify-write paths in FileSettingsStore and FileSecretsStore now load with a strict_decrypt validation-context flag: validate_secret and validate_secret_dict raise SecretDecryptionError for a token the cipher cannot decrypt, the load fails, and the existing guards refuse the write (409 for settings, 500 for secrets), leaving both files untouched. Reads stay lenient. The F18.secret-key-rotation map bullet now passes, so its known-bug marker is removed. Fixes #5622 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
…hanged cipher key
After the cipher key changed (a new OH_SECRET_KEY, or a new first session
key when it is unset), a named LLM profile's stored key loaded as null.
POST /api/profiles/{name}/activate and PATCH /api/settings with
active_profile then answered 200 and applied an LLM with api_key: null over
the settings' key, and re-saving the keyless config read back wrote null
over the profile's recoverable ciphertext.
LLMProfileStore.load gains strict=True (the strict_decrypt context from the
settings/secrets fix), used by both activation paths, and save refuses to
overwrite a profile holding a token the cipher cannot decrypt. Both answer
409 "Profile '<name>' is encrypted with a different key" and write nothing.
Plain reads stay lenient. F20.secret-key-rotation loses its known-bug
marker and asserts the 409s.
Part of #5317
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
Contributor
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Contributor
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||
enyst
marked this pull request as ready for review
October 13, 2026 22:52
all-hands-bot
approved these changes
Oct 13, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Collaborator
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review of PR #5775
Scope check: this change belongs in this repository. It touches the SDK profile store (LLMProfileStore.load/save) and the agent-server REST paths that consume it (POST /api/profiles/{name}/activate, PATCH /api/settings with active_profile), which this repo owns. No cross-repository move is warranted. The author's three open questions (provider-connection half in #5338, strictness of the save refusal, no new read hint) are documented design decisions, not blockers.
What I verified on head f1572ab:
- The fix is wired through every affected path.
load(strict=True)is used by both activation entry points (profiles_router.activate_profile,settings_router._load_active_profile_llm), andsaverefuses an overwrite via_raise_if_undecryptable. Both raise the newSecretDecryptionError, which is deliberately not aValueError, sostore_errors()does not swallow it into a 400; the explicitexcept SecretDecryptionErrorarms map it to the shared 409profile_key_mismatch.loadre-raises it before the genericexcept Exceptionwrap that would have turned it into aValueError. - The guard is fail-closed.
_raise_if_undecryptableruns inside the same file lock as the write, and both refusals return before anything is written (activation raises beforesettings_store.update; save raises before the temp-file replace). The tests assert the profile file is byte-identical and that the original key still readssk-a. - The save guard covers every encrypted secret field. I confirmed empirically that
LLM.to_persistedemits all fourLLM_SECRET_FIELDS(api_key,aws_access_key_id,aws_secret_access_key,aws_session_token) as top-level Fernet-token strings, which is exactly what the guard walks. No nested secret-bearing dict exists in the serialized profile. - No secret leakage. The 409
detailand the logged message are generic; the stored token is never echoed, and the test assertsgAAAAAis absent from the response. - No compatibility break:
strictis keyword-only with aFalsedefault, so existing callers are unchanged, and the profile-store protocols are untouched. - Evidence quality: the new tests are real integration tests through
create_appwith a realLLMProfileStoreand realCipher(not mocked behavior), and they pass (70 passed). Related suites pass (325 passed: profiles/settings/agent-profiles/store-cipher/pydantic-secrets).ruff check/ruff format --checkare clean on the changed files, and all check runs onf1572abare green. The PR also supplies the runtime before/after evidence the guide asks for on a restart-with-rotated-key path: the F20 map bulletxfails on main andxpasses on this branch with the profile's own 409detail.
Notes (not findings): The provider-connection activation path can still answer 400 for an undecryptable connection key rather than 409; that half is explicitly deferred to #5338 and remains fail-closed (nothing written), so it does not affect this merge. The GET-by-name vs list api_key_set divergence after rotation is the author's deliberate, documented decision to avoid an API addition.
No material correctness, security, compatibility, or acceptance-criterion defect found on the current head.
✅ APPROVED
…keys Rotating OH_SECRET_KEY (or the session API key it falls back to) used to make every stored secret unreadable; #5622's guard only stopped writes from destroying them until the old key came back. Config gains previous_secret_keys (OH_PREVIOUS_SECRET_KEYS as a JSON list, or OH_PREVIOUS_SECRET_KEYS_0, _1, ...; also accepted by POST /api/init). Cipher encrypts with the current key and decrypts with the current key first, then each previous key (MultiFernet, same sha256 derivation per key), so values saved under an old key stay readable, and any save writes them back under the current key; a stale token carried through as is is rotated too. At startup the settings and secrets stores are re-encrypted under the current key when they hold a stale token, so the previous key can be dropped afterwards. Config.cipher is rebuilt when a model_copy changes the keys. The write refusal now applies only when no configured key can decrypt a value, and its message says to restore the key or add it to OH_PREVIOUS_SECRET_KEYS. The verify-agent-server map gains F18.secret-key-previous-keys and restart --keep-previous-secret-key. Fixes #5622 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
…-cipher-key-change-409' into claude/umbrella-bug-fixes-tgz9ai-cipher-profile-connections
…keys With the base branch's OH_PREVIOUS_SECRET_KEYS, profile reads, activation and re-saves go through Config.cipher, so a profile saved under a listed previous key reads, activates, and is re-encrypted under the current key when saved. The 409 for a profile no configured key can decrypt now says to restore the key or add it to OH_PREVIOUS_SECRET_KEYS. The map gains F20.secret-key-previous-keys; F20.secret-key-rotation covers the lost key. Part of #5317 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
…sal details by prefix The settings refusal detail now carries the OH_PREVIOUS_SECRET_KEYS guidance, so the F18, F20 and F22 corrupt-file bullets check it with contains instead of eq. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
…-cipher-key-change-409' into claude/umbrella-bug-fixes-tgz9ai-cipher-profile-connections
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
2 of 5 tasks
all-hands-bot
approved these changes
Oct 14, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Collaborator
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review of PR #5775 (head a22ce73)
Scope check: this change belongs in this repository. It touches the SDK cipher/profile store (openhands-sdk/openhands/sdk/utils/cipher.py, pydantic_secrets.py, llm/llm_profile_store.py) and the agent-server REST paths that consume them (profiles_router, settings_router, persistence/store.py, config.py, init_router), all owned here. No cross-repository move is warranted.
What I verified on this head:
- The named-profile fix is wired through both activation entry points (
profiles_router.activate_profile,settings_router._load_active_profile_llm) viaload(strict=True), andsaverefuses an overwrite through_raise_if_undecryptable. Both raiseSecretDecryptionError, which is deliberately not aValueError, sostore_errors()cannot swallow it into a 400; explicitexceptarms map it to the shared 409profile_key_mismatch, andloadre-raises it before the genericexcept Exceptionwrap. - The refusals are fail-closed: the save guard runs inside the same file lock and both refusals return before any write. Tests assert byte-identical files and that the original key still reads
sk-a. - Key rotation (
OH_PREVIOUS_SECRET_KEYS/OH_PREVIOUS_SECRET_KEYS_n, alsoprevious_secret_keysonPOST /api/init) is additive and backward compatible:Cipherstill encrypts with the current key only and decrypts current-then-previous viaMultiFernet; blank/duplicate/current-key entries are dropped;Config.cipherrebuilds whenmodel_copychanges the keys. I confirmed empirically that serializing a token only a previous key can read re-encrypts it under the current key, so the settings/secrets stores (and re-saved profiles) move to the new key on the next write. - The startup re-encryption (
reencrypt_stale_stores) only rewrites when the file actually holds a stale token, leaves undecryptable files untouched, and logs rather than raises on failure. It runs insideasyncio.to_threadin both the legacy lifespan and the deferred-init path. - No secret leakage: refusal details and logged messages are generic and never echo the token; tests assert
gAAAAAis absent from responses. - Evidence: the new tests are real integration tests through
create_appwith a realLLMProfileStore/Cipher(not mocked behavior). Focused runs pass here —test_profile_cipher_mismatch.py,test_cipher.py,test_config.py,test_init_router.py(81),test_store_cipher_mismatch.py,test_pydantic_secrets.py,test_llm_profile_store.py(127), andtest_profiles_router.py/test_settings_router.py(193).ruff checkis clean on the changed files, and all 53 check runs ona22ce73are green. The PR also supplies the restart-with-rotated-key before/after map evidence (F18/F20secret-key-rotationandsecret-key-previous-keys).
Notes (not findings): the provider-connection half of #5317 is explicitly deferred to #5338 and stays fail-closed (nothing written); the GET-by-name vs list api_key_set divergence after a lost-key rotation is the author's documented decision to avoid an API addition. The only OpenAPI delta is the additive optional previous_secret_keys on InitRequest.
No material correctness, security, compatibility, or acceptance-criterion defect found on the current head.
✅ APPROVED
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
approved these changes
Oct 23, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Collaborator
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: The change belongs here — it is agent-server/SDK profile persistence behavior, owned by this repository per AGENTS.md.
Review findings — no material issues on head 6d228a0c:
- Refusals write nothing.
activate_profileand_load_active_profile_llmload withstrict=Trueand mapSecretDecryptionErrorto 409 beforesettings_store.update(...)runs, so no keyless LLM is applied over the settings stored key.LLMProfileStore.saveruns_raise_if_undecryptableinside the file lock and before the tempfile write, leaving the profile byte-identical (covered by tests asserting file bytes are unchanged). - The save-side guard is complete for this store. Profile JSON is flat:
LLM_SECRET_FIELDS(api_key,aws_*) are top-levelSecretStrfields serialized flat viaserialize_secret, so scanning all top-level string values forFERNET_TOKEN_PREFIXcovers every encrypted secret a profile can hold. Corrupt-JSON and non-dict files intentionally pass through and remain left to the existing load/save error handling. - Secrets handling follows the established machinery. The strict flag rides the existing
pydantic_secretscontext mechanism — no parallel redaction sentinels. Error messages and logs (profile_key_mismatch) contain profile names and recovery hints only, never ciphertext or key material (tests assert nogAAAAAin responses). - Lenient/strict split is coherent. Ordinary
GETkeeps the lenient read (api_key_set: false), matching the agreed fail-closed-on-writes degrade-gracefully-on-reads behavior; strictness applies only to the two write-adjacent paths. - Scope matches the maintainer discussion on #5317. The strict refusal covers any overwrite, even one supplying a replacement key; deletes stay allowed, so the way forward under a new key is delete-and-recreate. That tradeoff, the missing GET broken-key indicator, lenient runtime-use loads, and the provider-connection half (#5338) are explicitly recorded as open decisions on the issue — consistent with Part-of-#5317, not Fixes.
- Compatibility. This PR own diff adds no REST schema change (the optional
InitRequest.previous_secret_keysin the contract summary comes from the stacked #5758 work already on main). The 200→409 change on the corrupt-key paths is the intended fix and is documented in route docstrings and the F20 feature map. - Evidence. The PR includes live before/after map runs on real restarted servers (base answered 200 activated-and-applied; head answers 409 with no ciphertext in logs), satisfying the live-evidence requirement for a persisted-state recovery fix. GitHub Actions is green on this head. I additionally ran the focused suites in a workspace checkout of this exact head:
test_profile_cipher_mismatch.py(4),tests/sdk/llm/test_llm_profile_store.py(67, incl. the 3 new tests),test_profiles_router.py(116),test_settings_router.py(89),test_store_cipher_mismatch.py(11),tests/sdk/utils/test_pydantic_secrets.py(49),test_agent_profiles_router.py(85) — all passing.
✅ APPROVED
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
📁 PR Artifacts Notice This PR contains a |
Co-authored-by: openhands <openhands@all-hands.dev>
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:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Why
An unreadable encrypted profile key could become
null, then activation or re-save could overwrite recoverable credentials with that empty value.Summary
Use strict profile loads for activation, guard overwrites, and share the profile-key-mismatch response. This branch includes the key-rotation foundation from #5758; profiles rotate when saved, not at startup.
Source: llm_profile_store.py · profiles_router.py · settings_router.py.
Issue Number
Part of #5317
How to Test
Previously recorded verification
New tests (fail on the base, pass here):
tests/agent_server/test_profile_cipher_mismatch.py(activate 409, PATCHactive_profile409, re-save 409 with the file byte-identical and the original key still readingsk-a, then delete and re-save under the new key works) and three tests intests/sdk/llm/test_llm_profile_store.py(lenient default load, strict load raises, save over an undecryptable profile raises).Related suites, run one file at a time (this machine was short on memory):
test_profiles_router.py109 passed,test_settings_router.py84,test_agent_profiles_router.py80,test_store_cipher_mismatch.py6,test_profile_cipher_mismatch.py3,tests/sdk/llm/test_llm_profile_store.py67,tests/sdk/utils/test_pydantic_secrets.py46.pre-commitis clean on the changed files.Live map proof (a real server restarted with a rotated secret key):
a430c77(map run --file F20 --only F20.secret-key-rotation --fresh --repeat 2 --checkout <main>):xfailin both attempts. Activation answers 200activated and applied.--checkout, marker still in place):xpassin both attempts. Activation answers 409Profile 'qa-rotate' is encrypted with a different key. The server log showsRefusing to use profile 'qa-rotate': A stored secret cannot be decrypted ...and contains nogAAAAA.map checkok, andmap run --file F20 --only F20.secret-key-rotation --freshgivespass.After merging the rotation work:
test_profile_cipher_mismatch.pygains a test where, with the old key listed, the profile reads its key, activates (and the settings get the key), and is re-saved under the new key; a server with only the new key then reads and activates it. The suitestests/agent_server tests/sdk/{llm,utils,profiles}give 4372 passed. Two tests failed under parallel load: the canvas-extension recovery test, which also fails on main here, and a websocket test that passes when re-run. Livemap run --file F20 --freshgives 32 pass and 5 xfail (the other known bugs), includingF20.secret-key-previous-keysandF20.secret-key-rotation.F18.secret-key-rotationandF18.secret-key-previous-keyspass, andmap checkis ok.Video/Screenshots
n/a
Type
Notes
Stacked on #5758. The refusal covers any overwrite, even one supplying a replacement key; alternatives and remaining choices are recorded on the issue. Recovery is to restore/list the old key, or explicitly delete and recreate the profile. Provider-connection failures and their 400/409 consistency are covered separately by #5338. GET adds no new broken-key indicator, and agent-profile launch / LocalConversation profile switching still load leniently. The issue is only partially addressed.
Visual explanation by Codex using HumanLayer show-me.
REST API contract changes
Compared with base OpenAPI
a430c778bd8afor public/api/**paths.🐳 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:b4452c7-pythonRun
All tags pushed for this build
About Multi-Architecture Support
b4452c7-python) is a multi-arch manifest supporting both amd64 and arm64b4452c7-python-amd64) are also available if neededJev-Fast-Audit
⚡ Jev fast audit · estimates · 0.36s · commit 6d228a0⚠️ reduced context — partial coverage; 26/28 hunks, 9/9 files (context budget: 26, hunk budget: 2).
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage:
All estimates and evidence