Sandbox: add actionable guidance when python3 is missing#56785
Sandbox: add actionable guidance when python3 is missing#56785tonga54 wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds an error-translation layer that converts opaque
Confidence Score: 5/5Safe to merge — no logic errors, no behavioral regressions, and the happy path is unaffected. All remaining findings are P2 (style/coverage suggestions). The core implementation is correct: error detection matches the real error structure, translation is properly scoped to mutation helpers only, and non-matching errors pass through unchanged. No P0 or P1 issues found. No files require special attention. Optional: consider adding a
|
| Filename | Overview |
|---|---|
| src/agents/sandbox/mutation-errors.ts | New helper module: detects python3: not found in error messages via regex and wraps with actionable guidance. Logic is correct — execDockerRaw sets error.message = stderr.toString() when stderr is non-empty, so the message-based check reliably catches real-world errors. |
| src/agents/sandbox/fs-bridge.ts | Adds try/catch around runCommand inside runCheckedCommand, guarded by plan.script.includes("python3 /dev/fd/3") to scope translation only to mutation helper invocations. Non-mutation and stat paths are unaffected. |
| src/agents/sandbox/remote-fs-bridge.ts | Wraps runRemoteScript inside the dedicated runMutation method with a try/catch calling toFriendlySandboxMutationError. No script-content guard needed since the catch is already inside the mutation-specific method. Non-matching errors pass through unchanged. |
| src/agents/sandbox/fs-bridge.shell.test.ts | Adds one new test that mocks execDockerRaw to throw an error whose .message is the python3: not found string, matching how execDockerRaw actually structures docker stderr failures, and asserts the actionable guidance is surfaced. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/sandbox/mutation-errors.ts
Line: 13-16
Comment:
**Consider preserving the original error as `cause`**
The new `Error` discards the original error's stack trace and `code`/`stderr` properties, which can make diagnosing edge-case failures harder for operators. Using the `cause` option keeps the chain intact while still surfacing the friendly message:
```suggestion
return new Error(
"Sandbox write/edit requires `python3` inside the sandbox container, but it is missing in the active image. Rebuild or update the sandbox image (or configure a custom sandbox image that includes `python3`). Original error: " +
rawMessage,
{ cause: error },
);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/sandbox/fs-bridge.shell.test.ts
Line: 144-171
Comment:
**No test coverage for `RemoteShellSandboxFsBridge` python3 detection**
The new test exercises the `SandboxFsBridgeImpl` path (`createSandboxFsBridge`) but not the `RemoteShellSandboxFsBridge` path (`createRemoteShellSandboxFsBridge`). Both paths were updated in this PR, and both have separate `try/catch` blocks calling `toFriendlySandboxMutationError`. A parallel test for the remote bridge would give symmetric coverage and ensure neither path regresses independently.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Sandbox: improve missing-python3 write/e..." | Re-trigger Greptile
| return new Error( | ||
| "Sandbox write/edit requires `python3` inside the sandbox container, but it is missing in the active image. Rebuild or update the sandbox image (or configure a custom sandbox image that includes `python3`). Original error: " + | ||
| rawMessage, | ||
| ); |
There was a problem hiding this comment.
cause
The new Error discards the original error's stack trace and code/stderr properties, which can make diagnosing edge-case failures harder for operators. Using the cause option keeps the chain intact while still surfacing the friendly message:
| return new Error( | |
| "Sandbox write/edit requires `python3` inside the sandbox container, but it is missing in the active image. Rebuild or update the sandbox image (or configure a custom sandbox image that includes `python3`). Original error: " + | |
| rawMessage, | |
| ); | |
| return new Error( | |
| "Sandbox write/edit requires `python3` inside the sandbox container, but it is missing in the active image. Rebuild or update the sandbox image (or configure a custom sandbox image that includes `python3`). Original error: " + | |
| rawMessage, | |
| { cause: error }, | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/sandbox/mutation-errors.ts
Line: 13-16
Comment:
**Consider preserving the original error as `cause`**
The new `Error` discards the original error's stack trace and `code`/`stderr` properties, which can make diagnosing edge-case failures harder for operators. Using the `cause` option keeps the chain intact while still surfacing the friendly message:
```suggestion
return new Error(
"Sandbox write/edit requires `python3` inside the sandbox container, but it is missing in the active image. Rebuild or update the sandbox image (or configure a custom sandbox image that includes `python3`). Original error: " +
rawMessage,
{ cause: error },
);
```
How can I resolve this? If you propose a fix, please make it concise.| it("surfaces actionable guidance when sandbox python3 is missing", async () => { | ||
| const bridge = createSandboxFsBridge({ sandbox: createSandbox() }); | ||
| const baseImpl = mockedExecDockerRaw.getMockImplementation(); | ||
| mockedExecDockerRaw.mockImplementation(async (...callArgs) => { | ||
| const args = callArgs[0] as string[]; | ||
| if (getDockerScript(args).includes("operation = sys.argv[1]")) { | ||
| const message = "moltbot-sandbox-fs: 2: python3: not found"; | ||
| const err = Object.assign(new Error(message), { | ||
| code: 127, | ||
| stdout: Buffer.alloc(0), | ||
| stderr: Buffer.from(message), | ||
| }); | ||
| throw err; | ||
| } | ||
| if (baseImpl) { | ||
| return await baseImpl(...callArgs); | ||
| } | ||
| return { | ||
| stdout: Buffer.alloc(0), | ||
| stderr: Buffer.alloc(0), | ||
| code: 0, | ||
| }; | ||
| }); | ||
|
|
||
| await expect(bridge.writeFile({ filePath: "b.txt", data: "hello" })).rejects.toThrow( | ||
| /requires `python3` inside the sandbox container/i, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
RemoteShellSandboxFsBridge python3 detection
The new test exercises the SandboxFsBridgeImpl path (createSandboxFsBridge) but not the RemoteShellSandboxFsBridge path (createRemoteShellSandboxFsBridge). Both paths were updated in this PR, and both have separate try/catch blocks calling toFriendlySandboxMutationError. A parallel test for the remote bridge would give symmetric coverage and ensure neither path regresses independently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/sandbox/fs-bridge.shell.test.ts
Line: 144-171
Comment:
**No test coverage for `RemoteShellSandboxFsBridge` python3 detection**
The new test exercises the `SandboxFsBridgeImpl` path (`createSandboxFsBridge`) but not the `RemoteShellSandboxFsBridge` path (`createRemoteShellSandboxFsBridge`). Both paths were updated in this PR, and both have separate `try/catch` blocks calling `toFriendlySandboxMutationError`. A parallel test for the remote bridge would give symmetric coverage and ensure neither path regresses independently.
How can I resolve this? If you propose a fix, please make it concise.|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. The local path can be reproduced by mocking a Docker write to throw the current Next step before merge Security Review findings
Review detailsBest possible solution: Land a fresh current-main patch that maps both the current Docker mutation helper failure and the remote-shell Do we have a high-confidence way to reproduce the issue? Yes. The local path can be reproduced by mocking a Docker write to throw the current Is this the best way to solve the issue? No. The PR's goal is right, but the local detector targets the old helper marker; using the current mutation helper contract and adding symmetric remote coverage is the narrower maintainable fix. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 929df0f5567f. |
Summary
Describe the problem and fix in 2–5 bullets:
python3: not found, users only see a raw shell error and cannot quickly recover.python3failures into actionable guidance that tells users to rebuild/update or configure a sandbox image with Python.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write
N/A. If the cause is unclear, writeUnknown.python3exists inside the sandbox container; when missing, raw stderr bubbles up without actionable remediation guidance.git blame, prior PR, issue, or refactor if known): issue [Bug]: Sandbox write/edit fails on openclaw-sandbox:bookworm-slim with "moltbot-sandbox-fs: 2: python3: not found" #45108 reports repeatedmoltbot-sandbox-fs: 2: python3: not foundfailures during write/edit.Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should have caught this. Otherwise write
N/A.src/agents/sandbox/fs-bridge.shell.test.tspython3: not found, writeFile rejects with actionable guidance instead of opaque stderr.User-visible / Behavior Changes
python3now include explicit recovery guidance about sandbox image requirements.Diagram (if applicable)
For UI changes or non-trivial logic flows, include a small ASCII diagram reviewers can scan quickly. Otherwise write
N/A.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
python3: not found.writeFilethrough fs bridge.Expected
Actual
Evidence
Attach at least one:
pnpm test -- src/agents/sandbox/fs-bridge.shell.test.tspasses (9 tests).pnpm checkpasses.Human Verification (required)
What you personally verified (not just CI), and how:
Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.python3: not found.Made with Cursor