Repository navigation
Conversation
Remove the duplicate workspaceRoot configuration from SandboxClientOptions and concrete state classes. The workspace root path is now exclusively stored in WorkspaceSpec.root, eliminating the inconsistency where two independent configuration paths could diverge. Changes: - Delete SandboxClientOptions.getWorkspaceRoot() base method - Remove workspaceRoot field, getter, and setter from all 5 concrete SandboxClientOptions subclasses (Docker, K8s, Daytona, AgentRun, E2B) - Remove workspaceRoot field, getter, and setter from all 5 concrete SandboxState subclasses; state.getWorkspaceRoot() now delegates to getWorkspaceSpec().getRoot() - Remove state.setWorkspaceRoot(...) from all SandboxClient.create() - Fix *FilesystemSpec.workspaceRoot() builder to write to WorkspaceSpec instead of options; set correct default root for Daytona, AgentRun, E2B - Add @JsonIgnoreProperties(ignoreUnknown = true) to SandboxState base class for backward compatibility with persisted state - Add getWorkspaceRoot() default method to Sandbox interface; make AbstractBaseSandbox.getWorkspaceRoot() public (was protected abstract) Issue: agentscope-ai#2171
Replace the hardcoded '/workspace' path in WorkspaceContextMiddleware with a dynamic sandbox.getWorkspaceRoot() call, so the agent prompt correctly reflects the actual workspace root for each sandbox type (e.g., /home/daytona/workspace for Daytona, /home/agentscope/workspace for AgentRun, /home/user/workspace for E2B). Changes: - Add getWorkspaceRoot() to AbstractSandboxFilesystem interface with default fallback '/workspace'; implement in SandboxBackedFilesystem, LocalFilesystemWithShell, OverlayFilesystem.ShellAwareOverlay, and ProjectAwareOverlay - Replace hardcoded sandbox root in WorkspaceContextMiddleware with sandbox.getWorkspaceRoot() - Migrate all concrete sandbox implementations to read workspace root from getWorkspaceSpec().getRoot() instead of state.getWorkspaceRoot() - Update HarnessAgent and integration test to use WorkspaceSpec path - Update sandbox state serialization tests for removed workspaceRoot field - Update workspace documentation Issue: agentscope-ai#2171
AgentScopeJavaBot
added
enhancement
Wrap HarnessAgent in try-with-resources in all 4 test methods so that agent state files under agents/parent/ are cleaned up before JUnit's @tempdir extension tries to delete the temp directory.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR correctly addresses the issue of hardcoded /workspace paths in WorkspaceContextMiddleware, replacing them with dynamic workspace roots read from the sandbox. The refactoring consolidates workspaceRoot into WorkspaceSpec and adds real OS/TMPDIR queries for sandbox environments. The 43-file change is symmetric and consistent across all sandbox backends.
Two areas for improvement: (1) The OS info + tempdir paragraph is duplicated across three branches in buildWorkspaceParagraph() — extracting to a private helper would reduce maintenance cost. (2) querySandbox() executes shell commands on every onSystemPrompt() call; caching results in SandboxBackedFilesystem or Sandbox.start() would avoid repeated exec overhead. (3) Double-brace initialization in AgentRunFilesystemSpec creates anonymous subclasses that implicitly capture the outer class — consider using a static factory method instead.
| } else { | ||
| sb.append( | ||
| "Relative paths resolve under the workspace and read-fall-back to" | ||
| + " the project (overlay copy-on-write).\n"); |
There was a problem hiding this comment.
[minor] The OS info + tempdir paragraph (os.name, os.version, java.io.tmpdir) is duplicated across the local, composite, and else branches. Consider extracting to a private appendHostPlatformInfo(StringBuilder) helper to DRY this up.
| return sb.toString(); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[minor] querySandbox() runs shell commands (cat /etc/os-release, uname -srm, echo $TMPDIR) on every onSystemPrompt() invocation. These values don't change during a sandbox's lifetime — consider caching in SandboxBackedFilesystem or at sandbox startup to avoid repeated exec overhead.
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR correctly addresses the issue of hardcoded /workspace paths in WorkspaceContextMiddleware, replacing them with dynamic workspace roots read from the sandbox. The refactoring consolidates workspaceRoot into WorkspaceSpec and adds real OS/TMPDIR queries for sandbox environments. The 43-file change is symmetric and consistent across all sandbox backends.
Two areas for improvement: (1) The OS info + tempdir paragraph is duplicated across three branches in buildWorkspaceParagraph() — extracting to a private helper would reduce maintenance cost. (2) querySandbox() executes shell commands on every onSystemPrompt() call; caching results in SandboxBackedFilesystem or Sandbox.start() would avoid repeated exec overhead. (3) Double-brace initialization in AgentRunFilesystemSpec creates anonymous subclasses that implicitly capture the outer class — consider using a static factory method instead.
| } else { | ||
| sb.append( | ||
| "Relative paths resolve under the workspace and read-fall-back to" | ||
| + " the project (overlay copy-on-write).\n"); |
There was a problem hiding this comment.
[minor] The OS info + tempdir paragraph (os.name, os.version, java.io.tmpdir) is duplicated across the local, composite, and else branches. Consider extracting to a private appendHostPlatformInfo(StringBuilder) helper to DRY this up.
| return sb.toString(); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[minor] querySandbox() runs shell commands (cat /etc/os-release, uname -srm, echo $TMPDIR) on every onSystemPrompt() invocation. These values don't change during a sandbox's lifetime — consider caching in SandboxBackedFilesystem or at sandbox startup to avoid repeated exec overhead.
- WorkspaceContextMiddlewareSandboxTest: 7 tests covering sandbox branch in buildWorkspaceParagraph, querySandbox success/failure/exception/ empty-output paths, and Session Context template - DockerSandboxTest: 2 tests covering getWorkspaceRoot() delegation to WorkspaceSpec.getRoot() - SandboxBackedFilesystemTest: 2 tests covering getWorkspaceRoot() null-sandbox fallback and sandbox delegation
…ampleTest Add @AfterEach cleanup to delete temp directory files before JUnit's @tempdir extension tries to clean up, preventing DirectoryNotEmptyException on sessionDir_isNamespaceAware and other flaky failures.
…dboxTest Add track() + @AfterEach closeOpenManagers() pattern to properly close WorkspaceManager after each test, releasing SQLite connections so that @tempdir cleanup succeeds on Windows.
The upstream/main branch had refactored the test to use a class-level HarnessAgent parent field (cleaned up in @AfterEach) instead of try-with-resources. The fix/sandbox branch still used the old try-with-resources pattern, causing 4 identical conflicts. Resolution: adopt the upstream approach by changing try-with-resources to direct field assignment and removing the extra closing braces.
- Extract appendHostPlatformInfo() helper to eliminate duplicated OS info + tmpdir block across 4 branches in buildWorkspaceParagraph() - Replace double-brace initialization with static factory method createDefaultWorkspaceSpec() in AgentRunFilesystemSpec, DaytonaFilesystemSpec, and E2bFilesystemSpec to avoid anonymous subclass overhead and implicit outer-class reference
…oring - Add SandboxTest for default getWorkspaceRoot() covering 3 paths: normal, state=null, workspaceSpec=null - Add WorkspaceContextMiddlewareBranchTest covering CompositeFilesystem and plain LocalFilesystem else branches - Add OS info assertions to existing overlay test
… factory methods - BaseSandboxFilesystemTest: verify AbstractSandboxFilesystem default getWorkspaceRoot() returns '/workspace' - ProjectAwareOverlayTest: verify getWorkspaceRoot() delegates to shell backend - OverlayFilesystemTest (new): verify ShellAwareOverlay.getWorkspaceRoot() - AgentRunFilesystemSpecTest (new): verify default workspace root - DaytonaFilesystemSpecTest (new): verify default workspace root - E2bFilesystemSpecTest (new): verify default workspace root
|
@chickenlj I am currently using agentscope-java in depth. Please help me review the code. I don’t want to separate it from the main version code, otherwise it will be difficult to maintain in the future. |
Conflicts resolved: - KubernetesSandboxClientOptions: accept upstream agent-sandbox fields, remove workspaceRoot (now from WorkspaceSpec) - KubernetesSandboxState: accept upstream agent-sandbox fields, remove workspaceRoot (now from WorkspaceSpec) - KubernetesSandbox: accept upstream CRD architecture, change getWorkspaceRoot() / getWorkspaceRoot() calls to getWorkspaceSpec().getRoot() - KubernetesSandboxClient: accept upstream CRD client, remove workspaceRoot references, remove old Pod fields from merge/copy - KubernetesSandboxStateSerdeTest: update for unified workspace root - SandboxBackedFilesystemTest: combine upstream tests with our getWorkspaceRoot tests - Delete Fabric8KubernetesPodRuntime.java/test (removed upstream)
Conflicts resolvedMerged upstream/main which had 8 conflicting files in the Kubernetes sandbox module due to upstream rewriting it to use the agent-sandbox CRD architecture (commit Resolution strategyAccept upstream's new Kubernetes architecture and re-apply workspace root unification changes on top.
Verification
|
- WorkspaceContextMiddleware: query container OS/tmpdir via querySandbox for the AgentStateStore template (local PR agentscope-ai#2194 semantics on the upstream template), instead of host System.getProperty values - MarketplaceStager GC: SKILL.md walk scan supporting multi-level namespaces (local PR agentscope-ai#2368 semantics) combined with the grace-window protection from upstream agentscope-ai#2840 - WorkspaceContextMiddlewareSandboxTest: restore command-dispatched execute stub plus sandbox query and fallback tests - MarketplaceStagerGcTest: restore multi-level namespace cases on the scoped layout with grace-window-aware assertions
|
This PR currently conflicts with git fetch origin
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved. Automated notification by github-manager-bot |
- KubernetesSandbox: keep upstream empty-command guard and newline wrap, read workspace root from getWorkspaceSpec().getRoot() - WorkspaceContextMiddleware: keep upstream AgentStateStore session template, effectiveWorkspace and artifact-delivery wording; sandbox branch uses dynamic sandbox.getWorkspaceRoot() plus sandbox OS/tmpdir query - Update SandboxBackedFilesystemTest, HarnessAgentIntegrationExampleTest, PathBoundsTest, BranchTest, SandboxPromptTest, WorkspaceContextMiddlewareSandboxTest - Adapt upstream Docker/AgentRun/E2B tests to WorkspaceSpec-based root; restore E2bFilesystemSpec.workspaceRoot() writing to WorkspaceSpec - Drop duplicated state.getWorkspaceRoot() calls in DockerSandbox
Second-round conflicts resolvedRebased onto latest Resolution
Also adapted new upstream tests to the Verification
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Unifying the workspace root behind WorkspaceSpec.root removes a real divergence hazard, and the middleware no longer hardcodes /workspace. The refactor is consistent across the five backends and the removed accessors are all internal to this repo's modules. Two things hold this back from an approval: the per-backend default root is now only preserved on the *FilesystemSpec path (persisted-state recovery and option-only configuration silently fall back to /workspace), and the middleware now issues three synchronous sandbox probes on every prompt build.
Findings
- [Warning]
E2bFilesystemSpec.java:121— backend-specific default root only survives via the FilesystemSpec path; restored/legacy persisted state loses it - [Warning]
WorkspaceContextMiddleware.java:388— 3 extra synchronousexecute()round trips per prompt build (hot ReAct loop) - [Warning]
AgentRunSandbox.java:209— public option/state accessors removed without a deprecation cycle (source/binary breakage for downstream extensions) - [Info]
WorkspaceContextMiddleware.java:445— swallowed exception with no logging; fallback values presented as fact
Suggestions
- Keep one authoritative per-backend default (constant referenced by both the spec and the state/client), or add an explicit migration for persisted
SandboxStatethat still carriesworkspaceRoot. - Cache the OS/TMPDIR probe result after the first successful query, or only render it on the first workspace section; log probe failures at
debug. - If the breaking API removal is intentional for 2.0, say so in the PR description so release notes and the docs for
*FilesystemSpec.workspaceRoot()cover it.
Test coverage for the new prompt content and the serialization backward-compat path looks appropriate; the 12/12, 7/7, 16/16 module results in the description are appreciated — a spotless:check + full mvn -DskipTests install note for the whole reactor would be the last missing piece.
Automated review by github-manager-bot
|
|
||
| private static WorkspaceSpec createDefaultWorkspaceSpec() { | ||
| WorkspaceSpec spec = new WorkspaceSpec(); | ||
| spec.setRoot("/home/user/workspace"); |
There was a problem hiding this comment.
[Warning] The backend-specific default root now only survives when the sandbox is configured through the *FilesystemSpec builder. E2bSandboxState/AgentRunSandboxState/DaytonaSandboxState lost their own workspaceRoot default (/home/user, /home/agentscope/workspace, ...), so any path that builds state from a plain WorkspaceSpec (default root /workspace) or restores a pre-refactor persisted state — where the removed field is now ignored by @JsonIgnoreProperties — silently relocates the workspace and breaks recovery of existing sessions.
Suggestion: keep the per-backend default in one place (e.g. override the root inside each concrete *SandboxState/*SandboxClient.create() when workspaceSpec.getRoot() equals the generic /workspace default), or add an explicit migration for persisted states. Also worth noting E2bFilesystemSpec hardcodes "/home/user/workspace" while AgentRun/Daytona reference a DEFAULT_WORKSPACE_ROOT constant — please align E2B with the constant style (and the old /home/user value or document the change).
| } | ||
| appendHostPlatformInfo( | ||
| sb, | ||
| querySandbox( |
There was a problem hiding this comment.
[Warning] This adds three synchronous sandbox execute() round trips (/etc/os-release, uname -srm, $TMPDIR) to every prompt build, i.e. once per ReAct iteration for a string that only matters the first time. For remote backends (AgentRun/K8s/E2B) that is measurable latency and extra requests against the sandbox in the hot loop.
Consider caching the computed value on the sandbox/state (or in the middleware instance) after the first successful query, or only collecting it when the workspace section is first rendered. Fallback text "Linux" / "/tmp" is also asserted as fact when the probe fails — a neutral value such as unknown (Linux assumed) would be more honest.
| * Executes a shell command in the sandbox and returns the stripped output, or the given | ||
| * fallback string if execution fails. | ||
| */ | ||
| private static String querySandbox( |
There was a problem hiding this comment.
[Info] catch (Exception e) { // fall through } with no logging will hide broken prompts (e.g. a sandbox that rejects execute, or a shell without cat/grep) — the prompt just silently degrades to the fallback. A debug/warn log with the command and exception would make field diagnosis much easier. Please also confirm the local E2bSandbox/Daytona PR description claim 16/16 tests passed covers this new code path; WorkspaceContextMiddlewareSandboxPromptTest looks like the right place for a fallback-case assertion.
| @Override | ||
| protected String getWorkspaceRoot() { | ||
| return arState.getWorkspaceRoot(); | ||
| public String getWorkspaceRoot() { |
There was a problem hiding this comment.
[Warning] Removing public getWorkspaceRoot()/setWorkspaceRoot(String) from SandboxClientOptions and all concrete option/state classes is a source- and binary-incompatible change for downstream users of the extension artifacts (custom sandbox impls, Spring Boot starters, persisted state). Since changes to agentscope-* interfaces cascade to harness, distribution and all extensions, could this be done as a deprecation cycle instead — keep the old accessors delegating to workspaceSpec.getRoot() marked @Deprecated(forRemoval = true), remove them in the next major — and add a migration note to the changelog/docs? If the breaking change is intentional for 2.0, please call it out explicitly in the PR description so release notes cover it.
- Add per-backend DEFAULT_WORKSPACE_ROOT constants (E2B) and WorkspaceSpec.withDefaultRoot(); all 5 SandboxClient.create() apply backend defaults when spec is null or root is blank - Migrate legacy persisted workspaceRoot into manifest via SandboxState.setLegacyWorkspaceRoot; add Jackson round-trip tests - Cache sandbox OS/TMPDIR probes per sandbox id in WorkspaceContextMiddleware; log probe failures at debug - Restore E2bFilesystemSpec.workspaceRoot() writing to WorkspaceSpec
|
All review findings addressed (pushed as
@oss-maintainer PTAL, thanks! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Good follow-up: the per-backend workspace roots are now applied through WorkspaceSpec.withDefaultRoot(...) at every create() site, the legacy persisted workspaceRoot is accepted again, the sandbox probes are cached and their failures logged instead of swallowed - all four of my previous points are handled, and the new H2/Jackson tests pin the migration behaviour. Two things keep me at COMMENT rather than APPROVE: the legacy-root hook is field-order sensitive and can silently drop the root, and the probe cache has no eviction bound.
Findings
- [Warning]
SandboxState.java:74-@JsonSetter("workspaceRoot")+ plainsetWorkspaceSpec(manifest)makes the migration depend on JSON field order; a manifest with a blank root followingworkspaceRootreplaces the adopted root. - [Warning]
WorkspaceContextMiddleware.java:152-sandboxProbeCacheis keyed by sandbox id and never evicted; unbounded over a long-lived multi-tenant process. - [Info] the Docker and Kubernetes
create()sites pass an inline"/workspace"string literal, whileE2bSandboxState/DaytonaSandboxState/AgentRunSandboxStatenow expose aDEFAULT_WORKSPACE_ROOTconstant. Same pattern everywhere would keep the "one place per backend" property the commit's own javadoc claims.
Suggestions
- Reconcile the legacy root after deserialization (code sketch in the inline comment) and add the
manifest-first blank-root test case. - Move the probe cache onto the sandbox (or bound it), so its lifetime matches the thing it describes.
Notes
- Two checks were still running on the current head at review time; nothing in the diff looked like it would change their outcome.
Automated review by github-manager-bot
| * @param workspaceRoot legacy standalone workspace root; ignored when blank or when the | ||
| * manifest already defines a root | ||
| */ | ||
| @JsonSetter("workspaceRoot") |
There was a problem hiding this comment.
[Warning] This migration hook is order-dependent, and one ordering silently drops the legacy root. Jackson invokes setters in the order the properties appear on the wire: for a payload that carries both workspaceRoot and a manifest whose root is null/blank, if workspaceRoot comes first this hook creates a spec with the legacy root and then setWorkspaceSpec(manifest) replaces the whole object, so the adopted root is lost and the resumed sandbox falls back to the generic /workspace - the exact failure this hook exists to prevent. The existing test only covers the good ordering (legacyWorkspaceRootDoesNotOverrideManifest has a non-blank manifest root).
Resolving after the full object is bound instead of inside a setter removes the dependency, e.g. stash the legacy value and reconcile once:
@JsonSetter("workspaceRoot")
void setLegacyWorkspaceRoot(String v) { this.legacyWorkspaceRoot = v; }
@JsonIgnore
private String legacyWorkspaceRoot;
/** Called after deserialization (or lazily from getWorkspaceSpec()). */
public WorkspaceSpec getWorkspaceSpec() {
if (workspaceSpec == null || workspaceSpec.getRoot() == null || workspaceSpec.getRoot().isBlank()) {
if (legacyWorkspaceRoot != null && !legacyWorkspaceRoot.isBlank()) {
WorkspaceSpec s = workspaceSpec != null ? workspaceSpec : new WorkspaceSpec();
s.setRoot(legacyWorkspaceRoot);
workspaceSpec = s;
}
}
return workspaceSpec;
}Alternatively a @JsonCreator taking both properties makes the precedence explicit and order-free. Worth one more test with {"manifest":{"root":null},"workspaceRoot":"/x"} field order to pin it either way.
| * lifetime is enough — this keeps the per-call prompt build down to zero extra execs | ||
| * after the first one. | ||
| */ | ||
| private final ConcurrentHashMap<String, String> sandboxProbeCache = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
[Warning] The caching is the right call - three synchronous execute() round trips per prompt build was the main thing I asked about. The unbounded lifetime of the map is the residual issue: entries are keyed by sandbox.id(), which is per-sandbox and monotonically increasing in a long-lived multi-tenant process, so nothing is ever evicted and the cache grows for the whole life of the middleware (or, in the distributed/composite setups, of the JVM). Each entry is small, but it is a slow leak on exactly the deployment shape this module targets.
Two cheap ways to bound it: cache on the sandbox object/state instead of in the middleware so the entry dies with the sandbox, or cap it (new LinkedHashMap<>(16, 0.75f, true) { removeEldestEntry ... } wrapped with Collections.synchronizedMap, or a Caffeine.maximumSize(...) if that dependency is already acceptable here). Given the probe result is static per sandbox, keying by the sandbox and evicting with it is the more natural ownership.
… uniform defaults - SandboxState.setWorkspaceSpec reconciles a previously adopted legacy root when the incoming manifest has a blank root (copy-on-write); add field-order matrix test - Bound sandboxProbeCache to a 1024-entry synchronized LRU - Add DEFAULT_WORKSPACE_ROOT to Docker/Kubernetes states; reference constants from all 5 SandboxClient.create() sites
|
All three follow-ups addressed (pushed as
Related suites green (62 tests incl. the 2 new ones). @oss-maintainer PTAL, thanks! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
This PR unifies the sandbox workspace root behind WorkspaceSpec.root and replaces the hardcoded /workspace in WorkspaceContextMiddleware with sandbox.getWorkspaceRoot(), backed by backend-specific defaults and a legacy-state migration hook. The refactor is consistent across the five backends and adds bounded caching for sandbox OS/TMPDIR probes. However, one critical correctness issue remains: AgentRun's NAS detection is evaluated against the pre-default WorkspaceSpec root, which can cause a NAS-backed workspace to be deleted on destroy when the caller passes a null or blank-root spec. Several *FilesystemSpec builders also mutate caller-provided WorkspaceSpec objects and can silently drop roots depending on call order, and HarnessAgent no longer falls back to the default /workspace prefix when the context's WorkspaceSpec has a blank root.
Findings
- [Critical]
AgentRunSandboxClient.java:86—isWorkspaceUnderMounts()uses the originalworkspaceSpec.getRoot()beforeWorkspaceSpec.withDefaultRoot()applies the backend default. With a null/blank spec the mount check sees a blank root, returns false, andAgentRunSandbox.doDestroyWorkspace()willrm -rfa NAS-backed workspace. Pass the defaulted spec's root to the mount check. - [Warning]
AgentRunFilesystemSpec.java:88—workspaceRoot(String)mutatesdefaultWorkspaceSpecdirectly. Combined withworkspaceSpec(WorkspaceSpec), it either mutates the caller's spec or silently drops a previously set root. Copy or preserve the root when replacing the spec. - [Warning]
DaytonaFilesystemSpec.java:78— same workspaceRoot/workspaceSpec ordering and caller-spec mutation issue as AgentRun. - [Warning]
E2bFilesystemSpec.java:104— same workspaceRoot/workspaceSpec ordering and caller-spec mutation issue as AgentRun. - [Warning]
KubernetesFilesystemSpec.java:54— same workspaceRoot/workspaceSpec ordering and caller-spec mutation issue as AgentRun. - [Warning]
DockerFilesystemSpec.java:48— same workspaceRoot/workspaceSpec ordering and caller-spec mutation issue as AgentRun. - [Warning]
HarnessAgent.java:2902— falls back toSANDBOX_WORKSPACE_PREFIXonly whengetWorkspaceSpec()is null, not when its root is blank. Add a blank-root guard. - [Info]
E2bSandboxState.java:26— default E2B root changed from/home/userto/home/user/workspace; confirm/document the intentional behavioral change.
Suggestions
- Add a unit test for
AgentRunSandboxClient.create()with a null/blankWorkspaceSpecand NAS/OSS mounts covering the default root, assertingworkspaceOnNasis true. - Make
*FilesystemSpec.workspaceSpec(WorkspaceSpec)copy the provided spec, and makeworkspaceRoot(String)set the root on the copy, so call order is commutative and caller objects are never mutated. - In
HarnessAgent, use a small helper that returns the default prefix when the context's root is null or blank.
Automated review by github-manager-bot
| state.setSandboxOwned(true); | ||
| state.setWorkspaceRootReady(false); | ||
| state.setWorkspaceOnNas(isWorkspaceUnderMounts(merged)); | ||
| state.setWorkspaceOnNas( |
There was a problem hiding this comment.
isWorkspaceUnderMounts() is passed workspaceSpec.getRoot() before the backend default is applied. When the caller passes a null or blank-root WorkspaceSpec (option-only configuration), the actual root becomes AgentRunSandboxState.DEFAULT_WORKSPACE_ROOT via WorkspaceSpec.withDefaultRoot(), but the NAS/OSS mount check sees a null/blank root and returns false. This causes AgentRunSandbox.doDestroyWorkspace() to rm -rf a NAS-backed workspace, leading to data loss. Use the defaulted WorkspaceSpec's root (returned by withDefaultRoot) for the mount check.
| @@ -86,7 +86,7 @@ public AgentRunFilesystemSpec addOssMount(AgentRunOssMountConfig mount) { | |||
| } | |||
|
|
|||
| public AgentRunFilesystemSpec workspaceRoot(String workspaceRoot) { | |||
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| @@ -76,7 +76,7 @@ public DaytonaFilesystemSpec disk(int diskGiB) { | |||
| } | |||
|
|
|||
| public DaytonaFilesystemSpec workspaceRoot(String workspaceRoot) { | |||
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| return this; | ||
| } | ||
|
|
||
| public E2bFilesystemSpec workspaceRoot(String workspaceRoot) { |
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| @@ -52,7 +52,7 @@ public KubernetesFilesystemSpec warmPoolName(String warmPoolName) { | |||
| } | |||
|
|
|||
| public KubernetesFilesystemSpec workspaceRoot(String workspaceRoot) { | |||
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| @@ -46,7 +46,7 @@ public DockerFilesystemSpec image(String image) { | |||
| } | |||
|
|
|||
| public DockerFilesystemSpec workspaceRoot(String workspaceRoot) { | |||
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| && defaultSandboxContext.getClientOptions() != null | ||
| ? defaultSandboxContext.getClientOptions().getWorkspaceRoot() | ||
| && defaultSandboxContext.getWorkspaceSpec() != null | ||
| ? defaultSandboxContext.getWorkspaceSpec().getRoot() |
There was a problem hiding this comment.
The shell workspace prefix falls back to SANDBOX_WORKSPACE_PREFIX only when defaultSandboxContext or its WorkspaceSpec is null. If the WorkspaceSpec exists but has a blank root (e.g., a caller-built SandboxContext), wsPrefix becomes blank and is passed to ShellPathPolicy.sandbox(String). Previously, getClientOptions().getWorkspaceRoot() always returned a non-null default. Add a blank-root check so the default prefix is used when getRoot() is null or blank.
| public class E2bSandboxState extends SandboxState { | ||
|
|
||
| /** Default workspace root inside E2B sandboxes. */ | ||
| public static final String DEFAULT_WORKSPACE_ROOT = "/home/user/workspace"; |
There was a problem hiding this comment.
E2B default workspace root changed from /home/user to /home/user/workspace. This is an intentional behavioral change for new sandboxes; confirm it matches the E2B runtime convention and document it clearly, since persisted state with the old root is migrated but new sandboxes will use the new path.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
This PR unifies the sandbox workspace root behind WorkspaceSpec.root and replaces the hardcoded /workspace in WorkspaceContextMiddleware with sandbox.getWorkspaceRoot(), backed by backend-specific defaults and a legacy-state migration hook. The refactor is consistent across the five backends and adds bounded caching for sandbox OS/TMPDIR probes. However, one critical correctness issue remains: AgentRun's NAS detection is evaluated against the pre-default WorkspaceSpec root, which can cause a NAS-backed workspace to be deleted on destroy when the caller passes a null or blank-root spec. Several *FilesystemSpec builders also mutate caller-provided WorkspaceSpec objects and can silently drop roots depending on call order, and HarnessAgent no longer falls back to the default /workspace prefix when the context's WorkspaceSpec has a blank root.
Findings
- [Critical]
AgentRunSandboxClient.java:86—isWorkspaceUnderMounts()uses the originalworkspaceSpec.getRoot()beforeWorkspaceSpec.withDefaultRoot()applies the backend default. With a null/blank spec the mount check sees a blank root, returns false, andAgentRunSandbox.doDestroyWorkspace()willrm -rfa NAS-backed workspace. Pass the defaulted spec's root to the mount check. - [Warning]
AgentRunFilesystemSpec.java:88—workspaceRoot(String)mutatesdefaultWorkspaceSpecdirectly. Combined withworkspaceSpec(WorkspaceSpec), it either mutates the caller's spec or silently drops a previously set root. Copy or preserve the root when replacing the spec. - [Warning]
DaytonaFilesystemSpec.java:78— same workspaceRoot/workspaceSpec ordering and caller-spec mutation issue as AgentRun. - [Warning]
E2bFilesystemSpec.java:104— same workspaceRoot/workspaceSpec ordering and caller-spec mutation issue as AgentRun. - [Warning]
KubernetesFilesystemSpec.java:54— same workspaceRoot/workspaceSpec ordering and caller-spec mutation issue as AgentRun. - [Warning]
DockerFilesystemSpec.java:48— same workspaceRoot/workspaceSpec ordering and caller-spec mutation issue as AgentRun. - [Warning]
HarnessAgent.java:2902— falls back toSANDBOX_WORKSPACE_PREFIXonly whengetWorkspaceSpec()is null, not when its root is blank. Add a blank-root guard. - [Info]
E2bSandboxState.java:26— default E2B root changed from/home/userto/home/user/workspace; confirm/document the intentional behavioral change.
Suggestions
- Add a unit test for
AgentRunSandboxClient.create()with a null/blankWorkspaceSpecand NAS/OSS mounts covering the default root, assertingworkspaceOnNasis true. - Make
*FilesystemSpec.workspaceSpec(WorkspaceSpec)copy the provided spec, and makeworkspaceRoot(String)set the root on the copy, so call order is commutative and caller objects are never mutated. - In
HarnessAgent, use a small helper that returns the default prefix when the context's root is null or blank.
Automated review by github-manager-bot
| state.setSandboxOwned(true); | ||
| state.setWorkspaceRootReady(false); | ||
| state.setWorkspaceOnNas(isWorkspaceUnderMounts(merged)); | ||
| state.setWorkspaceOnNas( |
There was a problem hiding this comment.
isWorkspaceUnderMounts() is passed workspaceSpec.getRoot() before the backend default is applied. When the caller passes a null or blank-root WorkspaceSpec (option-only configuration), the actual root becomes AgentRunSandboxState.DEFAULT_WORKSPACE_ROOT via WorkspaceSpec.withDefaultRoot(), but the NAS/OSS mount check sees a null/blank root and returns false. This causes AgentRunSandbox.doDestroyWorkspace() to rm -rf a NAS-backed workspace, leading to data loss. Use the defaulted WorkspaceSpec's root (returned by withDefaultRoot) for the mount check.
| @@ -86,7 +86,7 @@ public AgentRunFilesystemSpec addOssMount(AgentRunOssMountConfig mount) { | |||
| } | |||
|
|
|||
| public AgentRunFilesystemSpec workspaceRoot(String workspaceRoot) { | |||
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| @@ -76,7 +76,7 @@ public DaytonaFilesystemSpec disk(int diskGiB) { | |||
| } | |||
|
|
|||
| public DaytonaFilesystemSpec workspaceRoot(String workspaceRoot) { | |||
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| return this; | ||
| } | ||
|
|
||
| public E2bFilesystemSpec workspaceRoot(String workspaceRoot) { |
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| @@ -52,7 +52,7 @@ public KubernetesFilesystemSpec warmPoolName(String warmPoolName) { | |||
| } | |||
|
|
|||
| public KubernetesFilesystemSpec workspaceRoot(String workspaceRoot) { | |||
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| @@ -46,7 +46,7 @@ public DockerFilesystemSpec image(String image) { | |||
| } | |||
|
|
|||
| public DockerFilesystemSpec workspaceRoot(String workspaceRoot) { | |||
There was a problem hiding this comment.
workspaceRoot(String) mutates defaultWorkspaceSpec directly. If a caller first supplies a WorkspaceSpec via workspaceSpec(WorkspaceSpec) and then calls workspaceRoot(String), the caller's spec object is mutated. Conversely, if workspaceRoot(String) is called first and workspaceSpec(WorkspaceSpec) second, the root set by workspaceRoot is silently discarded. Preserve the configured root when replacing defaultWorkspaceSpec, or copy the spec before mutating it.
| && defaultSandboxContext.getClientOptions() != null | ||
| ? defaultSandboxContext.getClientOptions().getWorkspaceRoot() | ||
| && defaultSandboxContext.getWorkspaceSpec() != null | ||
| ? defaultSandboxContext.getWorkspaceSpec().getRoot() |
There was a problem hiding this comment.
The shell workspace prefix falls back to SANDBOX_WORKSPACE_PREFIX only when defaultSandboxContext or its WorkspaceSpec is null. If the WorkspaceSpec exists but has a blank root (e.g., a caller-built SandboxContext), wsPrefix becomes blank and is passed to ShellPathPolicy.sandbox(String). Previously, getClientOptions().getWorkspaceRoot() always returned a non-null default. Add a blank-root check so the default prefix is used when getRoot() is null or blank.
| public class E2bSandboxState extends SandboxState { | ||
|
|
||
| /** Default workspace root inside E2B sandboxes. */ | ||
| public static final String DEFAULT_WORKSPACE_ROOT = "/home/user/workspace"; |
There was a problem hiding this comment.
E2B default workspace root changed from /home/user to /home/user/workspace. This is an intentional behavioral change for new sandboxes; confirm it matches the E2B runtime convention and document it clearly, since persisted state with the old root is migrated but new sandboxes will use the new path.
…kspace root Address the oss-maintainer review on agentscope-ai#2194. * [Critical] AgentRunSandboxClient.create() passed the *raw* caller's root to isWorkspaceUnderMounts() while storing the backend-defaulted spec. With a null/blank spec the check saw a blank root, returned false and doDestroyWorkspace() would rm -rf a NAS-backed workspace. Resolve WorkspaceSpec.withDefaultRoot() once and use it for both the state and the mount check. * WorkspaceSpec.withDefaultRoot() never applied the backend default for a null spec: a fresh WorkspaceSpec already carries the generic "/workspace" root, so the blank check on the copy was never true. Decide from the input spec instead. This made every backend fall back to /workspace when configured by options alone. * The five *FilesystemSpec builders mutated the caller-provided WorkspaceSpec and workspaceRoot()/workspaceSpec() were order-dependent. Both now work on a copy: the caller's object is never mutated and either builder may be applied last. * HarnessAgent only fell back to the /workspace prefix when the context's WorkspaceSpec was null; a blank root produced an empty path prefix. Extract resolveSandboxWorkspacePrefix() to cover both cases.
|
Addressed the CHANGES_REQUESTED findings in [Critical] AgentRun mount check ran against the pre-default root
WorkspaceSpec resolvedSpec =
WorkspaceSpec.withDefaultRoot(
workspaceSpec, AgentRunSandboxState.DEFAULT_WORKSPACE_ROOT);
...
state.setWorkspaceSpec(resolvedSpec);
state.setWorkspaceOnNas(isWorkspaceUnderMounts(merged, resolvedSpec.getRoot()));A NAS mount at the AgentRun default root is now correctly detected, so Root cause behind it:
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of ba68ece3 following my CHANGES_REQUESTED on 4f797378. All blocking findings are resolved and I verified them in the code:
- [Critical] NAS/OSS mount check on the pre-default root —
AgentRunSandboxClient.create()now resolves the backend default once intoresolvedSpecand feeds that same root to bothstate.setWorkspaceSpec()andisWorkspaceUnderMounts(), so a null/blank spec can no longer mark a NAS-backed workspace as non-NAS and letdoDestroyWorkspace()delete it. - [Warning] caller-spec mutation / order-dependence —
workspaceRoot()andworkspaceSpec()are now copy-on-write in all five backends (AgentRun/Daytona/E2B/Kubernetes/Docker),WorkspaceSpec.copy()deep-copies theentries/environmentmaps, andworkspaceSpec(null)restores the default instead of installing a null spec. - [Warning]
HarnessAgentblank-root fallback — extracted intoresolveSandboxWorkspacePrefix(), which now falls back toShellPathPolicy.SANDBOX_WORKSPACE_PREFIXfor both a null spec and a blank root. - The
withDefaultRoot()fix is right: a freshWorkspaceSpecalready carries/workspace, so blankness has to be decided from the input spec rather than the copy — otherwise backend defaults would never apply. - CI on this head is green (ubuntu/windows builds, Check License, Check Module Sync, codecov/patch).
Approving. Two non-blocking notes are pinned inline: one on the strength of the new regression test, one on a small inconsistency between the Kubernetes/Docker and AgentRun/Daytona/E2B default-spec paths. Thanks for the thorough round-trip and for covering both setter orders in tests.
Automated review by github-manager-bot
| WorkspaceSpec.withDefaultRoot(null, AgentRunSandboxState.DEFAULT_WORKSPACE_ROOT); | ||
|
|
||
| assertEquals(nasMount, defaulted.getRoot()); | ||
| assertTrue(defaulted.getRoot().startsWith(nasMount)); |
There was a problem hiding this comment.
[Info] Minor nit: assertTrue(defaulted.getRoot().startsWith(nasMount)) is vacuous right after assertEquals(nasMount, defaulted.getRoot()) (a string always starts with itself). The real regression this PR fixed lives in AgentRunSandboxClient.create() — the mount check now runs against resolvedSpec.getRoot(). If you want the test to guard that path, assert isWorkspaceUnderMounts(merged, resolvedSpec.getRoot()) (or the resulting state.isWorkspaceOnNas()) for a NAS mount configured at the backend default root. Non-blocking, feel free to fold it into a follow-up.
| public KubernetesFilesystemSpec workspaceSpec(WorkspaceSpec workspaceSpec) { | ||
| this.defaultWorkspaceSpec = workspaceSpec; | ||
| this.defaultWorkspaceSpec = | ||
| workspaceSpec != null ? workspaceSpec.copy() : new WorkspaceSpec(); |
There was a problem hiding this comment.
[Info] Consistency note (also applies to DockerFilesystemSpec): the null branch here falls back to new WorkspaceSpec() (generic /workspace) while AgentRun/Daytona/E2B use createDefaultWorkspaceSpec() with the backend default. Harmless today because both backends' DEFAULT_WORKSPACE_ROOT is /workspace, but if those defaults ever diverge, resetting via workspaceSpec(null) would silently reintroduce the generic root. Using a backend default helper here would keep the five builders uniform.
Summary
SandboxClientOptions.workspaceRootandWorkspaceSpec.rootwere two independent configuration paths for the same value, which could diverge. This PR unifies them:WorkspaceSpec.rootis now the single source of truth/workspaceinWorkspaceContextMiddlewarewith a dynamicsandbox.getWorkspaceRoot()call, so the agent prompt correctly reflects the actual workspace root for each sandbox type (Docker, K8s, Daytona, AgentRun, E2B)Commit 1 — refactor(sandbox): unify workspace root to WorkspaceSpec.root
SandboxClientOptions.getWorkspaceRoot()base method and removeworkspaceRootfield/getter/setter from all 5 concreteSandboxClientOptionssubclassesworkspaceRootfield/getter/setter from all 5 concreteSandboxStatesubclasses;getWorkspaceRoot()now delegates togetWorkspaceSpec().getRoot()state.setWorkspaceRoot(...)from allSandboxClient.create()*FilesystemSpec.workspaceRoot()builder to write toWorkspaceSpec; set correct default root for Daytona/AgentRun/E2B@JsonIgnoreProperties(ignoreUnknown = true)toSandboxStatefor backward compatibility with persisted stategetWorkspaceRoot()default method toSandboxinterface;AbstractBaseSandbox.getWorkspaceRoot()becomespublicCommit 2 — fix(harness): use dynamic workspace root in middleware prompt
getWorkspaceRoot()toAbstractSandboxFilesysteminterface; implement inSandboxBackedFilesystem,LocalFilesystemWithShell,ShellAwareOverlay,ProjectAwareOverlayWorkspaceContextMiddlewarewithsandbox.getWorkspaceRoot()getWorkspaceSpec().getRoot()instead ofstate.getWorkspaceRoot()HarnessAgent, tests, and documentationTest plan
mvn spotless:apply— passedmvn compile— passedCloses #2171
Breaking-change note (intentional for 2.0)
The
workspaceRootfield/getter/setter on everySandboxClientOptionsandSandboxStatesubclass was removed without a deprecation cycle — downstream extensions referencing them
will need source changes. Migration path:
WorkspaceSpec.setRoot(...)(or the*FilesystemSpec.workspaceRoot()builder, which now writes to the
WorkspaceSpec); each backend keeps one authoritativedefault (
AgentRunSandboxState/DaytonaSandboxState/E2bSandboxState.DEFAULT_WORKSPACE_ROOT,/workspacefor Docker/K8s) applied byWorkspaceSpec.withDefaultRoot(...)in everySandboxClient.create()when the caller passes no spec or a blank root.SandboxStateignores unknownproperties and migrates a legacy standalone
workspaceRootvalue into the manifest whenthe manifest has no root (
setLegacyWorkspaceRoot, covered byHarnessSandboxJacksonModuleTest).Review follow-ups (oss-maintainer)
WorkspaceContextMiddlewareare now cached per sandbox(successful results only; failures fall back every time and log at
debug), so promptbuilds issue no repeated execs after the first one.
agentscope-harnesssuite (1076 tests, 0 failures) plus allsandbox backend modules green;
mvn spotless:checkand full-reactormvn -DskipTests installclean.Behavioral change: E2B default workspace root
The E2B backend's default workspace root moves from
/home/userto/home/user/workspaceso it matches the other sandboxes' "root" semantics(
/home/agentscope/workspace,/home/daytona/workspace,/workspace).Configure
E2bFilesystemSpec.workspaceRoot(...)to keep the previous value.