test/changelog: follow up sandbox workspace mount fixes by steipete · Pull Request #32274 · openclaw/openclaw

steipete · 2026-03-02T22:59:02Z

Follow-up to #32227 and #32217.

Scope:

Add regression tests for workspace mount mode behavior in both sandbox create paths:
- src/agents/sandbox/browser.create.test.ts
- src/agents/sandbox/docker.config-hash-recreate.test.ts
Add missing changelog entries for:
- fix(sandbox): make /workspace bind mount read-only when workspaceAcce… #32227
- fix(signal): fall back to toolContext.currentMessageId for reactions #32217

Why:

fix(sandbox): make /workspace bind mount read-only when workspaceAcce… #32227 merged before maintainer-side fixups could be pushed.
This keeps the fix fully regression-tested and changelog-complete.

)

cursor · 2026-03-02T22:59:08Z

PR Summary

Low Risk
Low risk: changes are limited to tests and changelog updates, with no production behavior modifications.

Overview
Adds regression coverage to verify the primary /workspace bind mount is read-only when workspaceAccess is ro or none, and remains writable for rw, across both ensureSandboxContainer and ensureSandboxBrowser create flows.

Updates CHANGELOG.md to include the missing fix entries for sandbox workspace mount permissions and Signal react message-id fallback.

^{Written by Cursor Bugbot for commit 4f9ffea. This will update automatically on new commits. Configure here.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f9ffea9cb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-02T23:00:58Z

src/agents/sandbox/docker.config-hash-recreate.test.ts

+          bindArgs.push(args[i + 1]);
+        }
+      }
+      expect(bindArgs).toContain(expectedMainMount);


Strengthen mount-permission assertion in workspaceAccess table test

This assertion only checks that the expected mount string is present, but this test case builds cfg with createSandboxConfig([], undefined, workspaceAccess), which keeps the default custom bind "/tmp/workspace:/workspace:rw"; since custom binds are appended after the main mount in container creation, a ro/none case can still end up effectively writable while this test passes. As written, the regression test can miss exactly the permission regression it is meant to guard against.

Useful? React with 👍 / 👎.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

cursor · 2026-03-02T23:02:59Z

src/agents/sandbox/docker.config-hash-recreate.test.ts

+    "uses expected main mount permissions when workspaceAccess=$workspaceAccess",
+    async ({ workspaceAccess, expectedMainMount }) => {
+      const workspaceDir = "/tmp/workspace";
+      const cfg = createSandboxConfig([], undefined, workspaceAccess);


Test fixture adds conflicting rw bind overriding ro mount

Medium Severity

The new parameterized test calls createSandboxConfig([], undefined, workspaceAccess), where undefined for binds falls through to the default ["/tmp/workspace:/workspace:rw"]. The production code in createSandboxContainer first adds the main mount with the correct :ro suffix, then appendCustomBinds appends this default bind as -v /tmp/workspace:/workspace:rw for the same target. For the ro and none cases, the Docker args end up with both a :ro and a :rw bind for /workspace, with the :rw coming last. The toContain assertion passes because the :ro entry exists, but the regression test doesn't catch that a conflicting :rw mount undermines the read-only protection. Passing [] instead of undefined for binds would make this a proper regression test.

Additional Locations (1)

src/agents/sandbox/docker.config-hash-recreate.test.ts#L106-L107

greptile-apps · 2026-03-02T23:03:58Z

Greptile Summary

This PR adds regression tests for the workspace mount permissions fix from #32227 across both sandbox create paths (browser.create.test.ts and docker.config-hash-recreate.test.ts), and adds the two missing changelog entries for #32227 and #32217.

docker.config-hash-recreate.test.ts: a new createSandboxConfig optional parameter (workspaceAccess, defaulting to "rw") and a parameterised it.each covering all three valid access modes (rw, ro, none) with correct mount-suffix expectations.
browser.create.test.ts: two individual tests added for none and rw modes; the ro mode is not explicitly tested (minor coverage gap — both ro and none share the same implementation branch in browser.ts, so it is implicitly covered).
The redundant cfg.workspaceAccess = "none" assignment in the first new browser test is a no-op since buildConfig already returns that value.
CHANGELOG.md entries are correctly formatted and placed.

Confidence Score: 4/5

Safe to merge — tests and changelog only; no production code changed.
The PR only touches tests and changelog. The core logic being tested was already verified against the implementation. The only gaps are a missing ro case in the browser tests and a trivially redundant assignment, both non-blocking.
Minor attention to src/agents/sandbox/browser.create.test.ts for the missing ro coverage case.

_{Last reviewed commit: 4f9ffea}

greptile-apps

_{3 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-03-02T23:04:02Z

src/agents/sandbox/browser.create.test.ts

+  it("mounts the main workspace read-only when workspaceAccess is none", async () => {
+    const cfg = buildConfig(false);
+    cfg.workspaceAccess = "none";


Redundant workspaceAccess assignment

buildConfig(false) already returns a config with workspaceAccess: "none" (line 51 of this file), so the explicit reassignment on line 190 is a no-op. It creates a false impression that the test is changing something meaningful. The line can be removed without affecting behaviour.

Suggested change

it("mounts the main workspace read-only when workspaceAccess is none", async () => {

const cfg = buildConfig(false);

cfg.workspaceAccess = "none";

const cfg = buildConfig(false);

// workspaceAccess is already "none" from buildConfig

Prompt To Fix With AI

This is a comment left during a code review. Path: src/agents/sandbox/browser.create.test.ts Line: 188-190 Comment: **Redundant `workspaceAccess` assignment** `buildConfig(false)` already returns a config with `workspaceAccess: "none"` (line 51 of this file), so the explicit reassignment on line 190 is a no-op. It creates a false impression that the test is changing something meaningful. The line can be removed without affecting behaviour. ```suggestion const cfg = buildConfig(false); // workspaceAccess is already "none" from buildConfig ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-03-02T23:04:03Z

src/agents/sandbox/browser.create.test.ts

+  it("mounts the main workspace read-only when workspaceAccess is none", async () => {
+    const cfg = buildConfig(false);
+    cfg.workspaceAccess = "none";
+
+    await ensureSandboxBrowser({
+      scopeKey: "session:test",
+      workspaceDir: "/tmp/workspace",
+      agentWorkspaceDir: "/tmp/workspace",
+      cfg,
+    });
+
+    const createArgs = dockerMocks.execDocker.mock.calls.find(
+      (call: unknown[]) => Array.isArray(call[0]) && call[0][0] === "create",
+    )?.[0] as string[] | undefined;
+
+    expect(createArgs).toBeDefined();
+    expect(createArgs).toContain("/tmp/workspace:/workspace:ro");
+  });
+
+  it("keeps the main workspace writable when workspaceAccess is rw", async () => {
+    const cfg = buildConfig(false);
+    cfg.workspaceAccess = "rw";
+
+    await ensureSandboxBrowser({
+      scopeKey: "session:test",
+      workspaceDir: "/tmp/workspace",
+      agentWorkspaceDir: "/tmp/workspace",
+      cfg,
+    });
+
+    const createArgs = dockerMocks.execDocker.mock.calls.find(
+      (call: unknown[]) => Array.isArray(call[0]) && call[0][0] === "create",
+    )?.[0] as string[] | undefined;
+
+    expect(createArgs).toBeDefined();
+    expect(createArgs).toContain("/tmp/workspace:/workspace");
+    expect(createArgs).not.toContain("/tmp/workspace:/workspace:ro");
+  });


Missing ro workspace access test case

The docker path in docker.config-hash-recreate.test.ts uses it.each to cover all three valid SandboxWorkspaceAccess values (rw, ro, none), but this file only adds two tests — none and rw — leaving the ro case untested for the browser create path.

In browser.ts, both ro and none follow the same code branch (both map to :ro), so the none test provides implicit coverage. However, to stay consistent with the docker test and to guard against any future divergence in that branch, adding an explicit ro test here (or converting the two tests into an it.each over all three modes) would be a worthwhile follow-up.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/agents/sandbox/browser.create.test.ts Line: 188-225 Comment: **Missing `ro` workspace access test case** The docker path in `docker.config-hash-recreate.test.ts` uses `it.each` to cover all three valid `SandboxWorkspaceAccess` values (`rw`, `ro`, `none`), but this file only adds two tests — `none` and `rw` — leaving the `ro` case untested for the browser create path. In `browser.ts`, both `ro` and `none` follow the same code branch (both map to `:ro`), so the `none` test provides implicit coverage. However, to stay consistent with the docker test and to guard against any future divergence in that branch, adding an explicit `ro` test here (or converting the two tests into an `it.each` over all three modes) would be a worthwhile follow-up. How can I resolve this? If you propose a fix, please make it concise.

fix: enforce sandbox workspace mount mode (#32227) (thanks @guanyu-zhang

4f9ffea

)

steipete merged commit 02eeb08 into main Mar 2, 2026
11 of 12 checks passed

steipete deleted the fix/32227-followup-tests-changelog branch March 2, 2026 22:59

openclaw-barnacle bot added agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels Mar 2, 2026

This was referenced Mar 2, 2026

fix(sandbox): make /workspace bind mount read-only when workspaceAcce… #32227

Merged

fix(signal): fall back to toolContext.currentMessageId for reactions #32217

Merged

chatgpt-codex-connector bot reviewed Mar 2, 2026

View reviewed changes

cursor bot reviewed Mar 2, 2026

View reviewed changes

greptile-apps bot reviewed Mar 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test/changelog: follow up sandbox workspace mount fixes#32274

test/changelog: follow up sandbox workspace mount fixes#32274
steipete merged 1 commit intomainfrom
fix/32227-followup-tests-changelog

steipete commented Mar 2, 2026

Uh oh!

cursor bot commented Mar 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 2, 2026

Uh oh!

cursor bot left a comment

Uh oh!

cursor bot Mar 2, 2026

Uh oh!

greptile-apps bot commented Mar 2, 2026

Uh oh!

greptile-apps bot left a comment

Uh oh!

greptile-apps bot Mar 2, 2026

Uh oh!

greptile-apps bot Mar 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

steipete commented Mar 2, 2026

Uh oh!

cursor bot commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor bot Mar 2, 2026

Choose a reason for hiding this comment

Test fixture adds conflicting rw bind overriding ro mount

Uh oh!

greptile-apps bot commented Mar 2, 2026

Greptile Summary

Confidence Score: 4/5

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cursor bot commented Mar 2, 2026 •

edited

Loading