fix(harness): use dynamic workspace root in middleware prompt#2194
fix(harness): use dynamic workspace root in middleware prompt#2194chcodex wants to merge 9 commits into
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.
| @@ -232,15 +231,40 @@ private static String buildWorkspaceParagraph(Path workspace, AbstractFilesystem | |||
| + " 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.
| @@ -232,15 +231,40 @@ private static String buildWorkspaceParagraph(Path workspace, AbstractFilesystem | |||
| + " 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.
2 participants
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