fix(sandbox): rebind RemoteSnapshotClient on state deserialize - #2423
Open
Blue-Berrys wants to merge 2 commits into
Open
fix(sandbox): rebind RemoteSnapshotClient on state deserialize#2423Blue-Berrys wants to merge 2 commits into
Blue-Berrys wants to merge 2 commits into
Conversation
Default deserializeState(json, snapshotSpec) ignored the snapshot spec, so non-Docker sandbox clients left RemoteSandboxSnapshot unbound after JSON round-trip. Move rebinding into the interface default and cover it with unit tests. Fixes agentscope-ai#2303
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
oss-maintainer
approved these changes
Jul 28, 2026
oss-maintainer
left a comment
Collaborator
There was a problem hiding this comment.
Code Review — Approved ✅
Overall: Good fix for the sandbox state deserialization issue. The PR correctly moves the rebindRemoteSnapshot logic from DockerSandboxClient to the SandboxClient interface as a default method, ensuring all sandbox client implementations properly rebind the RemoteSnapshotClient after deserialization.
Key Changes:
SandboxClient.deserializeState(json, snapshotSpec)now callsrebindRemoteSnapshotto re-inject the clientrebindRemoteSnapshotis a static helper that checks if the snapshot is aRemoteSandboxSnapshotand re-creates it with the client fromRemoteSnapshotSpec- Removed the duplicate implementation from
DockerSandboxClient
Code Quality:
- ✅ Clean refactoring — eliminates code duplication
- ✅ Proper null checks and type guards
- ✅ Good Javadoc explaining the rebinding rationale
- ✅
RemoteSandboxSnapshotonly persists itsidacross serialization, so the client must be re-injected
CI Status: ✅ All checks passed
Verdict: Well-structured fix that correctly addresses the deserialization issue.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
AgentScope-Java Version
2.0.1-SNAPSHOT
Description
Fixes #2303.
RemoteSandboxSnapshotonly serializes itsid. After JSON round-trip,RemoteSnapshotClientis null.SandboxClient.deserializeState(json, snapshotSpec)previously ignoredsnapshotSpec, so onlyDockerSandboxClient(which overrode the two-arg method) rebound the client. AgentRun / Daytona / E2B / Kubernetes clients were affected.Changes:
SandboxClientinterface defaultdeserializeState(String, SandboxSnapshotSpec).DockerSandboxClient(it now inherits the default).SandboxClientRemoteSnapshotRebindTestcovering unbound single-arg deserialize and two-arg rebind success.Validated with:
mvn -pl agentscope-harness -am \ -Dtest=SandboxClientRemoteSnapshotRebindTest,HarnessSandboxJacksonModuleTest \ testChecklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)