Repository navigation
Conversation
LocalConversation.fork() built the fork without the source's cipher, so the fork's base_state.json stored the LLM key redacted. The agent-server resumes every fork from that file, so the fork's first turn failed with LLMAuthenticationError. Forward the cipher, and copy the source's conversation secrets into the fork's registry so they persist too. The verify-agent-server map bullets F10.fork-keeps-credentials and F10.fork-run-memory now pass; their known-bug markers are removed. Fixes #5465 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
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).
Review of PR #5765
Scope: In-repo and in-scope. Fork behavior for LocalConversation is owned by this repository (Python SDK / Agent Server), per AGENTS.md cross-repository boundaries and the review guide's ownership section. No product/architecture relocation is needed.
Freshness: Head is 5383a6f38494598db44d3d4f798c9dc3d19d6192 (matches), PR open, no labels, no prior reviews, no review threads. Linked issue #5465 is open with bug, priority:low, ready-for-dev. Head CI is green (sdk-tests, build, build-binary-and-test on ubuntu/macos/windows, Check OpenAPI Schema, image builds all success).
What the change does
LocalConversation.fork()now passescipher=self._cipherinto the fork's constructor. This is the actual root cause of #5465: without the source cipher, the fork'sbase_state.jsonserialized the LLMapi_keyas the redacted sentinel, and the agent-server resumes every fork from that file (EventService.startwithagent=None), so the first turn failed withLLMAuthenticationError. The fix is minimal and directly traceable to the reported symptom; the cipher-less path still redacts (unchanged behavior, covered bytest_fork_without_cipher_still_redacts_secrets).- The fork also copies the source's
secret_registry.secret_sourcesby assigning the field (which triggersConversationState.__setattr__autosave), so conversation-level secrets (secrets=/update_secrets) persist encrypted under the same cipher. On the agent-server this is consistent with the existing resume path (EventServicere-appliesmeta.jsonsecrets) and withfork_conversation, which copies the source's stored metadata onto the fork. I verified the server's_cipher_for()resolves to the same configured cipher thatfork()now forwards, and thatforkis 501 in Docker runtime mode (where a per-conversation cipher resolver exists), so the forwarded cipher matches wherever a fork can be created.
Verification performed
uv run pytest tests/sdk/conversation/local/test_fork.py-> 20 passed, including the new cipher and secret assertions (base_state.jsonkey and secret decrypt to the source values; aConversationState.model_validate(..., context={"cipher": cipher})resume returns the key).uv run pytest tests/sdk/conversation -k "fork or secret or cipher"-> 151 passed.uv run pytest tests/agent_server -k "fork or secret or cipher"-> 130 passed, 2 failed. Both failures (test_local_storage.py::test_a_fork_keeps_the_shared_worktree_until_it_is_deleted,test_credential_binding.py::test_direct_start_strips_reserved_conversation_secret) reproduce identically on the base commitdfd8fe8in a separate worktree, i.e. they are pre-existing environment failures (test-server auth / git-fixture setup), not regressions from this diff. Neither touches the fork cipher/secret path under review.
Acceptance
The linked issue's acceptance criterion is met: a cipher-backed fork persists its credentials encrypted and can resume from base_state.json. The PR additionally supplies authentic before/after live evidence on the agent-server (map bullets F10.fork-keeps-credentials and F10.fork-run-memory xfail on main, xpass on this head), which satisfies the review guide's live-evidence requirement for a production-facing bug fix. No material correctness, security, compatibility, or acceptance defect found.
Non-blocking note (not a finding)
This PR overlaps the independently written open PR #5473, which targets the same issue. As the author flags in the description, a maintainer must pick one and close the other. That is a merge-selection decision between two valid implementations, not a defect in this head, so it does not change the verdict.
✅ APPROVED
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
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).
Review of PR #5765 (head 0308e031)
Scope: In-repo and in-scope. LocalConversation.fork() is SDK conversation behavior this repository owns, and it directly addresses #5465 (bug, ready-for-dev). The agent-server regression test and the F10 verify-agent-server feature-map update belong here as well.
Current state: Head is unchanged at 0308e031d1c3286d1d2368d2762127c7036fe2d7; PR is open and non-draft; all GitHub Actions checks pass on this head; no unresolved review threads.
Analysis:
- Passing the source's
cipherinto the fork constructor makes the fork'sbase_state.jsonencrypt secrets with the same key as the source instead of redacting them, and the cipherless path still redacts with a warning (covered bytest_fork_without_cipher_still_redacts_secrets). - Deep-copying source secret sources into the fork's registry via field assignment (rather than mutating the registry) routes persistence through the normal autosave, now encrypted with the inherited cipher. The merge gives source secrets precedence and the resume-time fill-if-absent seeding cannot downgrade them. The fork is not yet visible to other threads when this runs, so the registry swap is race-free, and the server's
fork_conversationcleans up the fork directory if service start fails. - Tests exercise real paths, no mocks: persisted-state decrypt assertions,
ConversationState.model_validate(..., context={"cipher": ...})resume, and a fullConversationServicefork → server restart → resume cycle asserting both the LLM key and a runtime-added secret survive. - Independently verified in the workspace:
tests/sdk/conversation/local/test_fork.py(22 passed) and the fork tests intests/agent_server/test_conversation_service.py(11 passed) on this head, and the new regression test fails on the merge-base (24a3a511) with the fork's persisted key redacted — confirming both the bug and the fix. - The PR's live agent-server map evidence (fork of a live DeepSeek conversation:
xfailon main →xpasson this branch, plus a passingmap checkafter marker removal) satisfies the repository's live-evidence requirement for a production-facing lifecycle fix.
No material correctness, security, compatibility, or acceptance-criterion defects found. The linked issue's acceptance criteria are met on the current head.
✅ APPROVED
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Why
A fork could lose its source’s encryption context and runtime secrets, leaving persisted credentials redacted and the fork unable to resume model calls.
Summary
Carry the source cipher into the fork and deep-copy its secret sources into the fork’s registry. The registry assignment participates in normal state persistence.
Source: local_conversation.py.
Issue Number
Fixes #5465
How to Test
Added kyletser's
test_cipher_backed_fork_resumes_with_credentialsfrom #5473, preserving Xiaoping Liao as commit author in 0fb50d610. It exercises a realConversationServicefork, service restart, and persisted resume, asserting that both the LLM key and a runtime-added secret survive. The existing registry merge and deep-copy implementation is retained.Validation after incorporation:
uv run pytest tests/agent_server/test_conversation_service.py tests/sdk/conversation/local/test_fork.py— 152 passed, with an isolatedOH_PERSISTENCE_DIR. Per-file pre-commit passed.Previously recorded verification
uv run pytest tests/sdk/conversation/local/test_fork.py: 20 passed. The newtest_fork_keeps_source_cipher_for_persisted_secretsfails on main (fork key redacted, secret missing) and passes here; it checks the fork'sbase_state.jsonkey and secret decrypt to the source's values and that a resume from that file (ConversationState.model_validate(..., context={"cipher": ...})) gets the key back.test_fork_without_cipher_still_redacts_secretscovers the cipher-less path.uv run pytest tests/sdk/conversation tests/agent_server -k "fork or secret or cipher": 283 passed.control-agent-server map run --file F10 --only F10.fork-keeps-credentials,F10.fork-run-memory --fresh --repeat 2 --checkout <main checkout>reportsxfailfor both bullets in both runs.xpassfor both bullets in both runs; the fork's run finishes and recalls the codename only the source was told.map checkok;map run --file F10 --fresh22 pass, 0 fail, 4 xfail (other, unrelated known bugs).uv run pre-commit run --files <changed files>: all passed.Video/Screenshots
n/a
Type
Notes
Cipherless conversations still serialize secrets as redacted. Copying the registry is deliberate, beyond a cipher-only fix; server creation secrets can still be reapplied through metadata. Docker fork remains unsupported (501). The useful server-restart regression from #5473 is incorporated; its overlapping production change and duplicate SDK assertions are not needed; verification umbrella #5653.
Visual explanation by Codex using HumanLayer show-me.
🐳 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:0308e03-pythonRun
All tags pushed for this build
About Multi-Architecture Support
0308e03-python) is a multi-arch manifest supporting both amd64 and arm640308e03-python-amd64) are also available if needed