Repository navigation
Conversation
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
c127cf2 to
df5b985
Compare
df5b985 to
92ef949
Compare
AgentScopeJavaBot
added
enhancement
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR introduces a new OpenSandbox sandbox provider for the AgentScope Java SDK, enabling users to create and manage sandboxes through OpenSandbox Cloud's REST API. The implementation is well-structured with comprehensive test coverage, clean separation of concerns, and good documentation. The provider supports core sandbox lifecycle operations (create, start, stop, delete) and file operations, with proper error handling and timeout management.
Key strengths:
- Complete implementation following existing provider patterns (similar to E2B, Daytona)
- Comprehensive unit and integration tests with realistic mock scenarios
- Clean separation between API client, provider, state, and filesystem classes
- Good Javadoc documentation with usage examples
Areas for improvement:
- The
OpenSandboxApiExceptionclass is defined but never actually used in the codebase (dead code) - The retry logic in
OpenSandboxApi.createSandbox()has a subtle bug where the retry loop condition is inverted - The integration test
OpenSandboxProviderITmakes real HTTP calls to localhost which will fail in CI environments - The
OpenSandboxSandboxStatedoesn't implementgetWorkspaceRoot()which is needed by the harness runtime
(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)
|
有人审核下这个PR吗，想用这个功能 |
可以本地打包帮试用下有无问题，暂时抽不出人力来审 |
覆盖空沙箱标识、幂等关闭、临时归档清理、挂载排除、参数校验及官方 SDK 空输出等边界场景。
|
Addressed the review by checking each item against the current implementation and updating the branch to the latest upstream
I also added coverage for lifecycle, configuration, archive, and SDK edge cases. The previous Ubuntu CI failure was in Local verification after the update:
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
New agentscope-extensions-sandbox-opensandbox module: an OpenSandbox provider for the harness sandbox layer (client + options + state + filesystem spec + Jackson module), with 47 unit tests and a documented integration test, wired into docs/v2 and the BOM/distribution poms. The structure follows the existing E2B/Kubernetes providers closely and the exec/file paths are consistently shell-quoted, so I did not find an injection issue. Two things deserve a decision before merge: the deterministic /tmp archive name used by persist/hydrate, and the unconditional field overwrite in OpenSandboxClientOptions.merge(). Not approving — a new sandbox backend at this size wants maintainer eyes on the lifecycle/ownership semantics as well as the items below.
Findings
- [Warning]
OpenSandbox.java:246— temp persist/hydrate archive name derives fromsession.hashCode()only: collisions across sessions, plus/tmpis world-readable on a shared sandbox host - [Warning]
OpenSandboxClient.java:129—merge()copiesendpoint(and every other field) from the call options unconditionally, whileapiKeyis null-guarded; a call options object built from the defaulthttp://localhost:8080silently overrides a production default - [Info]
OpenSandbox.java:141/:64/OfficialOpenSandboxSdk.java:125/OpenSandboxEndpoint.java:30— unuseddoDestroyWorkspace()on non-owned sandboxes, SDK handle closed instop()rather thanshutdown(), hardcodedtruncated=false, and uncaughtIllegalArgumentExceptionfromURI.create
Suggestions
For the temp path, mixing a random nonce into the name is enough (/tmp/agentscope-<op>-<sessionId>-<ThreadLocalRandom hex>.tar), and it also removes the cross-session collision class entirely. For merge(), either treat blank/default values as unset or document that call-level options must be complete; a null-guard like the one already used for apiKey would make the two fields behave alike.
Existing @AgentScopeJavaBot review on this PR lists findings (an unused OpenSandboxApiException, an inverted retry loop, an integration test that hits live HTTP) that do not match the current file set or hunk line numbers — I could not reproduce any of them against head 40547011, so I have not repeated them here.
Automated review by github-manager-bot
| return "/tmp/agentscope-" | ||
| + operation | ||
| + "-" | ||
| + Integer.toHexString(session.hashCode()) |
There was a problem hiding this comment.
Predictable temp archive path risks cross-session collisions
The tempArchive() method uses Integer.toHexString(session.hashCode()) to generate temp file names. String hashCode is deterministic and collision-prone—two concurrent sessions with the same hashCode will write to the same temp file path (/tmp/agentscope-persist-<hash>.tar), causing data corruption during persist/hydrate operations.
Impact: In multi-tenant or high-concurrency scenarios, one session could read another session's workspace archive, leading to data leakage or corruption.
Suggested fix: Use a random UUID or combine sessionId with a random nonce:
private String tempArchive(String operation) {
String session = state.getSessionId() == null ? "unknown" : state.getSessionId();
String random = Integer.toHexString(System.nanoTime() ^ session.hashCode());
return "/tmp/agentscope-" + operation + "-" + random + ".tar";
}There was a problem hiding this comment.
Fixed in efef9e9. tempArchive(...) now uses UUID.randomUUID() for every operation. persistUsesFreshTemporaryArchivePath verifies that two persist operations do not reuse the same path, removing the predictable-name collision class.
| if (call == null) { | ||
| return merged; | ||
| } | ||
| merged.setEndpoint(call.getEndpoint()); |
There was a problem hiding this comment.
merge() unconditionally overwrites endpoint, losing configured defaults
The merge() method always calls merged.setEndpoint(call.getEndpoint()) without checking if the caller explicitly set it. Since OpenSandboxClientOptions initializes endpoint to "http://localhost:8080" by default, a caller who only sets apiKey will inadvertently reset the endpoint to localhost, even if the client was configured with a production endpoint.
Impact: Callers must set all fields explicitly or risk silent misconfiguration. This is especially dangerous for endpoint and apiKey—the two most critical connection parameters.
Suggested fix: Only override fields that were explicitly set by the caller. Consider using a builder pattern or tracking which fields were set:
if (call.isEndpointExplicitlySet()) {
merged.setEndpoint(call.getEndpoint());
}Alternatively, document that callers must provide complete options or pass null to use defaults.
There was a problem hiding this comment.
Fixed in efef9e9. OpenSandboxClientOptions now tracks explicitly set fields, and merge() copies only those fields. A call-level options object built from class defaults no longer overwrites the client configured endpoint, image, timeouts, or proxy flag. callOptionsOverrideOnlyExplicitValues covers this behavior.
| } | ||
|
|
||
| @Override | ||
| protected void doDestroyWorkspace() throws Exception { |
There was a problem hiding this comment.
doDestroyWorkspace() is defined but never called
The doDestroyWorkspace() method (line 141-143) executes rm -rf <workspaceRoot>, but it's never invoked from shutdown() or any other lifecycle method. For owned sandboxes, sdk.kill() destroys the entire container, making explicit cleanup redundant. For non-owned sandboxes (where isSandboxOwned() is false), the workspace directory persists after shutdown.
Impact: Minor—this matches the E2B pattern where doDestroyWorkspace() is also not called from shutdown(). However, if the intent is to clean up non-owned sandboxes, consider calling doDestroyWorkspace() before closeHandle() in shutdown() when !sandboxOwned.
Suggested action: Clarify the contract in a comment or remove the method if it's intentionally unused.
There was a problem hiding this comment.
No code change. doDestroyWorkspace() is required by AbstractBaseSandbox and is intentionally not called from shutdown(), because shutdown() must not delete an externally managed workspace. Docker, Kubernetes, and E2B follow the same ownership boundary. Owned OpenSandbox instances are destroyed through sdk.kill(), while the protected method remains available for explicit cleanup and testing.
There was a problem hiding this comment.
Follow-up in 370a9b2: the ownership boundary is now documented on doDestroyWorkspace().
| execution == null || execution.getExitCode() == null | ||
| ? -1 | ||
| : execution.getExitCode(); | ||
| return new ExecResult(exitCode, stdout, stderr, false); |
There was a problem hiding this comment.
truncated flag is always false, even when commands are killed by timeout
The exec() method hardcodes truncated=false when constructing ExecResult. If the SDK kills a command due to the timeout (e.g., exit code 124 for timeout command, or 137 for SIGKILL), the result doesn't indicate that output may be incomplete.
Impact: Callers cannot distinguish between a command that exited cleanly with exit code 0 and one that was killed mid-execution but happened to return a non-zero exit code. This could lead to incorrect assumptions about command completion.
Suggested fix: Detect timeout-killed commands and set truncated=true:
boolean truncated = exitCode == 124 || exitCode == 137;
return new ExecResult(exitCode, stdout, stderr, truncated);There was a problem hiding this comment.
No code change. ExecResult.truncated means the captured output exceeded the capture limit, not that the process timed out; that is its record contract. Exit codes 124 and 137 are not reliable timeout signals because user commands can return them directly. This adapter does not swallow SDK timeout failures, and setting truncated for those exit codes would conflate two different states.
| try { | ||
| super.stop(); | ||
| } finally { | ||
| closeHandle(); |
There was a problem hiding this comment.
stop() closes the SDK handle, forcing reconnect on next start()
The stop() method calls closeHandle() (line 64), which closes the SDK connection. A subsequent start() call will trigger ensureSandbox(), which reconnects to the existing sandbox. This works correctly but incurs an extra reconnect cycle.
Impact: Performance overhead in stop-then-start scenarios (e.g., snapshot persistence). The E2B implementation keeps the handle open during stop() and only closes it during shutdown().
Suggested action: Consider whether closing the handle in stop() is necessary. If the intent is to release resources, document this behavior. Otherwise, keep the handle open and only close it in shutdown().
There was a problem hiding this comment.
No code change. Closing the SDK handle in stop() is deliberate. stop() is the lifecycle boundary used for call-scoped and external sandboxes; retaining the handle would keep SDK connection resources alive after the call. It does not destroy the sandbox, and start() reconnects to the existing sandbox. The reconnect cost is accepted for deterministic connection cleanup.
There was a problem hiding this comment.
Follow-up in 370a9b2: the reconnect/resource-release intent is now documented on stop().
| if (raw.isEmpty()) { | ||
| throw new IllegalArgumentException("endpoint must not be blank"); | ||
| } | ||
| URI uri = URI.create(raw.contains("://") ? raw : "http://" + raw); |
There was a problem hiding this comment.
URI.create() can throw uncaught IllegalArgumentException on malformed input
The parse() method calls URI.create(raw.contains("://") ? raw : "http://" + raw) without catching IllegalArgumentException. If the input contains non-ASCII characters or invalid URI syntax, URI.create() throws an exception with a generic message like "Illegal character in path", bypassing the user-friendly error at line 37.
Impact: Minor—callers still get an IllegalArgumentException, but the error message is less helpful. For example, parse("http://example.com:8080/path with spaces") throws "Illegal character in path at index 28" instead of "Invalid OpenSandbox endpoint: ...".
Suggested fix: Wrap URI.create() in a try-catch:
URI uri;
try {
uri = URI.create(raw.contains("://") ? raw : "http://" + raw);
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException("Invalid OpenSandbox endpoint: " + value, e);
}There was a problem hiding this comment.
Fixed in efef9e9. OpenSandboxEndpoint.parse() now catches the IllegalArgumentException from URI.create() and rethrows Invalid OpenSandbox endpoint: <value> with the original exception as cause. malformedEndpointUsesFriendlyValidationMessage covers malformed path input.
为持久化临时归档加入随机 UUID，避免会话哈希碰撞。按显式设置字段合并调用级配置，防止默认 endpoint 覆盖客户端配置。统一包装 URI 解析异常并补充回归测试。
|
Addressed the review in efef9e9. Fixed:
Kept intentionally:
Verification:
Inline replies follow on each thread. |
说明 stop 关闭的是 SDK 连接而非沙箱本身，并说明 doDestroyWorkspace 仅供显式清理，shutdown 遵守托管沙箱所有权边界。
|
@oss-maintainer Follow-up: CI is now fully green on All six review findings have been either fixed or explicitly resolved in the inline threads. No further code changes are pending. |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Delta re-review of 40547011 → 370a9b2c (2 commits, 7 files). All four points from the previous pass are genuinely addressed, and CI is green on this head (build (ubuntu-latest), build (windows-latest), validate, Check License, Check Module Sync, codecov/patch, license/cla):
- Collision-prone archive name —
persistWorkspace()now appends a random UUID to the temporary archive path, and the newpersistUsesFreshTemporaryArchivePathtest drives the productionpersistWorkspace()entry point rather than a helper, so it actually guards the regression. - Default endpoint overriding client config —
merge()now gates every one of the nine fields behind an explicitisXxxSet()flag instead of copying unconditionally;OpenSandboxClientOptionsTestcovers the explicit-set path. Correct shape for the bug that was reported. URI.create()throwing rawIllegalArgumentException— wrapped at the parse boundary and the cause is preserved, with a regression test reachingOpenSandboxEndpoint.parse().- Lifecycle comments — the
stop()comment ("closes the SDK connection without destroying the sandbox") and thedoDestroyWorkspace()comment ("explicit cleanup only") both match what the method bodies do; no contradiction between the docs and the ownership boundary.
Two residual nits below, neither blocking.
Findings
- [Warning]
OpenSandboxClientOptions.java:73—setApiKey(null)marks the field as explicitly set, so a null call-level key now overrides the client default instead of being skipped. - [Info]
OpenSandboxClientOptions.java:37—copyOfgoes through the public setters, so a copy reports every field as explicitly set; harmless at the moment, but the flags are not a faithful "user set this" carrier once copied.
Suggestions
For the first item, apiKeySet = apiKey != null; keeps the "null means unset" contract that the rest of the class implicitly assumes — apiKey is currently the only setter that accepts null, so it is the only one that can push a null over a configured default.
Automated review by github-manager-bot
|
|
||
| public void setApiKey(String apiKey) { | ||
| this.apiKey = apiKey; | ||
| apiKeySet = true; |
There was a problem hiding this comment.
[Warning] setApiKey(null) now marks the field as explicitly set, so a call-level options object whose apiKey is null overwrites the client default with null in merge() — the previous if (call.getApiKey() != null) guard prevented exactly that. Every other setter here rejects null/blank input, so apiKey is the only one that can carry an explicit null through this path, and the result is a request sent without authorization rather than with the configured key. If "null means unset" is the intended contract, use apiKeySet = apiKey != null; here (and keep the isApiKeySet() gate in merge()); if null is meant to clear the key, worth saying so in the javadoc so the two behaviours are not confused.
There was a problem hiding this comment.
Fixed in af5f331. setApiKey(null) now records the field as unset, so a call-level null no longer overrides a configured default key during merge(). nullApiKeyDoesNotOverrideConfiguredDefault exercises create(...) and start() and verifies that the configured key is preserved.
| private int readyTimeoutSeconds = 30; | ||
| private int requestTimeoutSeconds = 30; | ||
| private boolean useServerProxy; | ||
| private boolean endpointSet; |
There was a problem hiding this comment.
[Info] The new *Set flags are not preserved by copyOf — it assigns through the public setters, so every copied field ends up flagged as explicitly set. Harmless today: merge() only reads the flags on the caller-supplied call argument, and every internal copy (OpenSandboxClient constructor, resume, OpenSandbox) is on the default side. But it means a copyOf result is no longer a faithful "did the user set this" carrier, and passing one as call in the future would re-introduce the default-overrides-client-config bug these commits just fixed. Consider copying the values by field assignment in copyOf, or copying the nine flags over explicitly, so the invariant is local to this class.
There was a problem hiding this comment.
Fixed in af5f331. copyOf(...) now copies the explicit-set flags together with the values, so copying a default options object no longer marks every field as explicitly set. copyOfPreservesExplicitFieldFlags covers both set and unset fields.
|
Addressed both residual delta-review items in af5f331:
Verification after the final edit:
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Delta re-review of 370a9b2c..af5f331c — the remaining items from my previous pass are closed, and the fix is at the right layer: copyOf now copies fields and explicit-set flags directly instead of routing through setters (so a copy no longer fabricates "explicitly set" state), and setApiKey(null) is treated as unset. Both directions are pinned by tests (copyOfPreservesExplicitFieldFlags, nullApiKeyDoesNotOverrideConfiguredDefault), and CI is green on this head with the branch mergeable.
One follow-up worth deciding before merge, plus two nits:
- [Warning] the "
nullmeans unset" convention now applies only toapiKey; the other reference-typed setters still mark themselves explicitly set onnull, so the same override-with-null bug remains reachable for endpoint/image/entrypoint/resourceLimits. - [Info]
copyOfis now a hand-maintained 18-field assignment list with no completeness guard.
Findings
3 inline comments — see the diff. Nothing here reopens a previous thread.
Notes
Approving the resolution of both items; the setter-convention question is a one-line decision (fix or document), so @dargoner please reply either way and I will not need another full pass. This comment is also the reply to your @oss-maintainer ping — thanks for the verification run output, it made the delta trivial to follow.
Automated review by github-manager-bot
| */ | ||
| public void setApiKey(String apiKey) { | ||
| this.apiKey = apiKey; | ||
| apiKeySet = apiKey != null; |
There was a problem hiding this comment.
[Warning] The new semantics are right for apiKey, but they make the setters inconsistent about null: setEndpoint(null) (line 63), setImage(null) (line 88), setEntrypoint(null) and setResourceLimits(null) still set their explicit flag, so a call-level options object carrying an explicit null endpoint/image will still override a configured client default — the exact failure mode this commit closes for the API key. For a secret this matters most, but a call-level setEndpoint(null) silently pointing the client at the default endpoint (or an image=null overriding a configured one) is the same class of bug.
Either apply the same "null means unset" convention to the other reference-typed setters, or — if null is meant to be a meaningful explicit value for them — say so in the Javadoc of each setter, because the current asymmetry is not discoverable from the API.
There was a problem hiding this comment.
Checked against the current implementation: setEndpoint(null), setImage(null), setEntrypoint(null), and setResourceLimits(null) all throw before assigning the field or setting the explicit flag. OpenSandboxEndpoint.parse() rejects the null endpoint first, and the other setters validate at method entry, so there is no call-level null path that can override a configured default for these fields.
I added rejectedNullValuesDoNotBecomeExplicitOptions to pin that behavior, and the class Javadoc now states that only apiKey accepts null as unset.
| return useServerProxySet; | ||
| } | ||
|
|
||
| static OpenSandboxClientOptions copyOf(OpenSandboxClientOptions source) { |
There was a problem hiding this comment.
[Info] Switching copyOf to direct field assignment is the correct fix for the flag-preservation bug, but it turns the method into a hand-maintained list of 18 assignments: a field added to the class later can be forgotten here and will be silently dropped on copy, and the existing copyIsIndependentAndPreservesAllValues test only asserts the handful of fields its author remembered.
If there is no way to assert completeness in a unit test here, a // When adding a field: also copy it here and add an assertion in OpenSandboxClientOptionsTest#copyOfPreservesExplicitFieldFlags comment above the method is the cheap mitigation. Alternatively a BeanUtils/reflection-free toBuilder()-style copy that keeps field lists in one place.
There was a problem hiding this comment.
Documented in 99fa2fd. copyOf(...) now has an inline guard comment requiring future option fields to copy both the value and explicit-set flag, and to extend copyOfPreservesExplicitFieldFlags.
| Map<String, String> nullValue = new LinkedHashMap<>(); | ||
| nullValue.put("cpu", null); | ||
| assertThrows(IllegalArgumentException.class, () -> options.setResourceLimits(nullValue)); | ||
| options.setApiKey("secret"); |
There was a problem hiding this comment.
[Info] Small test-placement nit: assertFalse(options.isApiKeySet()) after setApiKey(null) encodes a semantic contract ("null means unset"), not rejection of an invalid value, and this test is named/asserted around IllegalArgumentException paths. Moving the three lines into the same test that covers nullApiKeyDoesNotOverrideConfiguredDefault (or its own one-assertion test) would make the contract easier to find when someone later wonders why one setter behaves differently from the rest.
There was a problem hiding this comment.
Moved in 99fa2fd. The API-key semantics assertion now lives in a dedicated nullApiKeyMeansUnset test; invalidValuesAreRejected is limited to rejection paths again.
|
Follow-up in 99fa2fd:
Verification:
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Delta re-review af5f331c → 99fa2fdd. The follow-up is test-only plus documentation: the two new cases pin exactly the boundary my previous COMMENTED review asked to be made explicit — rejectedNullValuesDoNotBecomeExplicitOptions asserts that a rejected null throws and leaves isEndpointSet()/isImageSet()/isEntrypointSet()/isResourceLimitsSet() false, so a rejected setter cannot silently upgrade into an "explicit" option that overrides client defaults; nullApiKeyMeansUnset pins the one setter where null is meaningful. The class javadoc now states that exception for setApiKey in one place instead of leaving readers to infer it from the merge logic, and the note above copyOf records the value+flag pairing rule that the copy helper is easy to break when a field is added.
No production behaviour changes in this delta — copyOf itself is untouched apart from the comment — so the earlier analysis stands. CLA signed (license/cla=success), CI SUCCESS on 99fa2fdd, mergeable/clean. Approving; the blocked merge state is branch-protection (still REVIEW_REQUIRED/needs a maintainer), not something on the author's side.
Automated review by github-manager-bot
保留 OpenSandbox 实现并合入官方最新依赖、核心与服务代码。
This branch has not been deployed
6 participants
AgentScope-Java Version
2.0.3-SNAPSHOT
Description
Add OpenSandbox as a standalone Harness sandbox extension.
WorkspaceSpeccontract and expose it throughOpenSandbox#getWorkspaceRoot().OPEN_SANDBOX_ENDPOINTandOPEN_SANDBOX_API_KEYare configured.The branch has been synchronized with the current upstream
main. Additional regression coverage exercises blank sandbox IDs, lifecycle idempotency and failure composition, bind-mount archive exclusions, temporary archive cleanup, option validation, SDK output normalization, and lazy official-client construction.Testing
mvn -B -T1 -pl agentscope-extensions/agentscope-extensions-sandbox/agentscope-extensions-sandbox-opensandbox -am verify -DskipTests: 6/6 reactor modules successful, including Spotless, compilation, Javadoc, and packaging.Checklist
mvn spotless:check