Fix/sandbox containerworkdir rw access#4226
Fix/sandbox containerworkdir rw access#4226ozgur-polat wants to merge 2 commits intoopenclaw:mainfrom
Conversation
| const agentWorkspaceMount = | ||
| sandbox.workspaceAccess === "rw" | ||
| ? sandbox.containerWorkdir | ||
| : sandbox.workspaceAccess === "ro" | ||
| ? "/agent" | ||
| : undefined; |
There was a problem hiding this comment.
[P1] agentWorkspaceMount can end up undefined for rw if sandbox.containerWorkdir is missing/empty.
The new branch returns sandbox.containerWorkdir directly for workspaceAccess === "rw". If containerWorkdir is ever unset (e.g., older/partial SandboxContext shapes, misconfig, or a future refactor), this silently produces no mount path even though access is rw, and the original bug can reappear. Consider either defaulting to sandbox.docker.workdir (which is already present on the context) or asserting containerWorkdir is present for rw to fail fast.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/sandbox-info.ts
Line: 14:19
Comment:
[P1] `agentWorkspaceMount` can end up `undefined` for `rw` if `sandbox.containerWorkdir` is missing/empty.
The new branch returns `sandbox.containerWorkdir` directly for `workspaceAccess === "rw"`. If `containerWorkdir` is ever unset (e.g., older/partial `SandboxContext` shapes, misconfig, or a future refactor), this silently produces no mount path even though access is `rw`, and the original bug can reappear. Consider either defaulting to `sandbox.docker.workdir` (which is already present on the context) or asserting `containerWorkdir` is present for `rw` to fail fast.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
Both added cases hardcode Prompt To Fix With AIThis is a comment left during a code review.
Path: src/agents/pi-embedded-runner.buildembeddedsandboxinfo.test.ts
Line: 146:181
Comment:
[P2] New tests don’t cover the real failing case (cron isolated agent showing host path).
Both added cases hardcode `containerWorkdir: "/workspace"` and only assert it’s copied into `agentWorkspaceMount`. The regression described in the PR sounds like the system prompt was using the *host* workspace path; it would be useful to add a test where `workspaceDir`/`agentWorkspaceDir` are host paths while `containerWorkdir` differs, and assert that `agentWorkspaceMount` uses the container path for `rw` (and `/agent` for `ro`). That would catch a future change that accidentally reverts to host paths.
How can I resolve this? If you propose a fix, please make it concise.
Double-check that the numbered “Page order (current)” section matches the later headings after renumbering (it looks aligned in this diff, but this file is easy to get out of sync if one is edited without the other). Prompt To Fix With AIThis is a comment left during a code review.
Path: docs/start/onboarding.md
Line: 24:26
Comment:
[P3] This doc tweak changes the heading numbering but doesn’t mention the new “security notice” page in the “Page order (current)” list above unless the list was also updated.
Double-check that the numbered “Page order (current)” section matches the later headings after renumbering (it looks aligned in this diff, but this file is easy to get out of sync if one is edited without the other).
How can I resolve this? If you propose a fix, please make it concise. |
bfc1ccb to
f92900f
Compare
This comment was marked as spam.
This comment was marked as spam.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Closing due to inactivity. |
Summary
Fixes #4171 - Cron isolated agent does not pass sandboxInfo to system prompt
Problem
When running a cron job with
sessionTarget: "isolated"and sandbox enabled withworkspaceAccess: "rw", the agent receives the host workspace path (e.g.,/home/node/loom-novia) in the system prompt instead of the Docker container mount path (/workspace).Root Cause
The buildEmbeddedSandboxInfo() function was only setting
agentWorkspaceMountfor read-only (ro) access, but not for read-write (rw) access.Changes
agentWorkspaceMounttocontainerWorkdir(typically/workspace) forrwaccessrwand ro workspace access modesTesting
AI-Assisted
Greptile Overview
Greptile Summary
This PR updates
buildEmbeddedSandboxInfoto populateagentWorkspaceMountforworkspaceAccess: "rw"using the sandbox’scontainerWorkdir, so system prompts reference the container-mounted workspace path (e.g./workspace) instead of host paths. It also adds unit coverage forrw/roaccess modes, and includes small doc/changelog updates.In the codebase,
EmbeddedSandboxInfois used to shape what the agent sees in the system prompt about its execution sandbox; this change ensures the prompt reflects the actual filesystem mount point when the agent has read/write workspace access inside the container.Confidence Score: 4/5
containerWorkdiris always set forrw, and that tests don’t fully assert the originally reported host-vs-container path mismatch scenario.(5/5) You can turn off certain types of comments like style here!