Fix/wsl sandbox bash tool wiring - #298
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review mode: initial
Findings
[MAJOR] Command injection risk in wsl-sandbox-bash-operations.ts via unescaped command
- Evidence: In
createWslSandboxBashOperations(src/main/agent/wsl-sandbox-bash-operations.ts:97), thecommandparameter is interpolated directly into a shell script without escaping:const script = \... ; ${command}`. An attacker controlling the command (e.g., via a compromised LLM agent) could break out of the script context using characters like'or;`, leading to arbitrary command execution on the WSL host. - Suggested fix: Escape the
commandstring for POSIX shell before embedding. For example, useshellEscapePathon the command as well, though a more robust approach is to pass the command as a separate argument tobash -cand use a heredoc or base64 encoding to avoid injection. At minimum, apply single-quote escaping:const escapedCommand = command.replace(/'/g, "'\\''"); const script = `mkdir -p '${escapedCwd}' 2>/dev/null; cd '${escapedCwd}' || exit 1; ${escapedCommand}`;
[MAJOR] Missing distro name validation in createWslSandboxBashOperations
- Evidence:
createWslSandboxBashOperations(src/main/agent/wsl-sandbox-bash-operations.ts:125) directly uses thedistroparameter inwsl -d ${distro} -e bash -c ...without validation. TheWSLBridge.validateDistroNamefunction exists and is used elsewhere (e.g.,wsl-bridge.ts:343), providing a validated distro name. Ifdistroever becomes user-controlled, this could allow command injection via the WSL distro name. - Suggested fix: Validate the distro name before use:
import { WSLBridge } from '../sandbox/wsl-bridge'; // Inside createWslSandboxBashOperations: const validatedDistro = WSLBridge.validateDistroName(distro);
[MINOR] Potential unit mismatch for timeout parameter
- Evidence: In
wsl-sandbox-bash-operations.ts:176, the timeout from the SDK is multiplied by 1000:setTimeout(..., timeout * 1000). TheBashOperationsinterface from@mariozechner/pi-coding-agentmay pass timeout in milliseconds (common in many SDKs). If so, this would set the timeout 1000x longer than intended. The original SDK code should be consulted. If unsure, add a comment or normalize to milliseconds explicitly. - Suggested fix: Verify the unit from the SDK. If milliseconds are passed, remove the
* 1000or adjust accordingly:// timeout is in milliseconds per BashOperations interface timeoutHandle = setTimeout(() => { ... }, timeout);
[MINOR] Potential state update after unmount in SettingsSandbox.tsx
- Evidence: In
handleToggleSandbox(src/renderer/components/settings/SettingsSandbox.tsx:134-136),loadStatus()is called after a 500ms timeout. If the component unmounts before the timeout fires, the cleanup effect (useEffectreturn) clears the timeout, but if the component unmounts after the timeout fires but before the asyncloadStatuscompletes, it may callsetStatus/setErroron an unmounted component. While not a crash risk in React 18, it can cause warnings and minor memory leaks. - Suggested fix: Add a mounted flag or use an abort controller to cancel the fetch in
loadStatus. Alternatively, ensure the cleanup effect also cancels the in-flight request:const mountedRef = useRef(true); useEffect(() => { return () => { mountedRef.current = false; }; }, []); // In loadStatus, check mountedRef.current before setting state.
[MINOR] Patch fragility – dependency on patched third-party package
- Evidence: The new patch file
patches/@mariozechner+pi-coding-agent+0.60.0.patchmodifies the bundled SDK to addbaseToolsOverridesupport. This means the fix depends on a specific version of the SDK and will break on update unless the patch is reapplied. Consider contributing the fix upstream or using a more stable extension mechanism if available. - Suggested fix: No immediate code change, but ensure the patch is documented in CI and that the team is aware of the maintenance burden.
[TESTING] Missing test coverage
- Evidence: The PR adds or modifies functional logic in multiple files (
wsl-sandbox-bash-operations.ts,wsl-bridge.ts,sandbox-adapter.ts,SettingsSandbox.tsx) but no tests were added or updated. The project structure requires mirrored tests insrc/tests/. The newexecimplementation, the retry logic intestDistro, the toggle behavior, and the patch injection are all untested. - Suggested fix: Add unit tests for:
createWslSandboxBashOperations(mockspawn, verify script construction, timeout, abort, process kill).testDistroretry logic (simulate transient failure).initializeWSLsharing bootstrap result.handleToggleSandboxin the renderer component (verify save call, status refresh, loading state).
Summary
This PR addresses four critical issues causing WSL2 sandbox bash execution to always fail. The changes are well-motivated and the architecture (custom BashOperations, shared bootstrap, retry logic) is sound. However, two injection vulnerabilities (command and distro name) should be addressed before merging, and minor improvements around timeout units and component cleanup are recommended. Test coverage is absent and should be added, especially for the new BashOperations implementation. The patch to the third-party SDK is fragile but acceptable given the current project constraints.
Open Cowork Bot
Addresses a review finding on PR OpenCoworkAI#298: the distro name reached the spawned "wsl -d <distro>" command line without validation, unlike WSLBridge which already validates it elsewhere. Adds a local validateDistroName (mirroring WSLBridge private method) as defense-in-depth, even though the current call path only ever passes the already-detected wslStatus.distro value.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[MAJOR] Unresolved command injection risk (src/main/agent/wsl-sandbox-bash-operations.ts:97)
The command parameter is still interpolated directly into the shell script without escaping:
const script = `mkdir -p '${escapedCwd}' 2>/dev/null; cd '${escapedCwd}' || ...; ${command}`;An attacker controlling command (e.g., via a compromised LLM agent) could break out of the single-quote context or inject additional commands. This was flagged in the previous review and remains unfixed.
Suggested fix: Escape the command string for POSIX shell before embedding:
const escapedCommand = command.replace(/'/g, "'\\''");
const script = `mkdir -p '${escapedCwd}' 2>/dev/null; cd '${escapedCwd}' || ...; ${escapedCommand}`;[MAJOR] Potential timeout unit mismatch (src/main/agent/wsl-sandbox-bash-operations.ts:176)
The execution timeout is multiplied by 1000 (timeout * 1000). The BashOperations interface from @mariozechner/pi-coding-agent typically passes timeout in milliseconds. If that is the case, this would set the timeout 1000x longer than intended, leading to delayed or missed timeouts. The original SDK code should be consulted to confirm the unit. This issue was also raised previously and remains unresolved.
Suggested fix: Verify the SDK unit. If milliseconds are passed, remove the multiplication:
timeoutHandle = setTimeout(() => {
timedOut = true;
terminateChild('timeout');
}, timeout); // no * 1000[MINOR] Missing test coverage
No tests have been added for any of the new or modified functionality, including:
createWslSandboxBashOperations(mockspawn, verify script construction, timeout, abort, process kill)testDistroretry logic inwsl-bridge.tsinitializeWSLsharing bootstrap result insandbox-adapter.tshandleToggleSandboxinSettingsSandbox.tsx- The added
validateDistroNamefunction
[MINOR] State update after unmount in SettingsSandbox.tsx (previously flagged)
The handleToggleSandbox function calls loadStatus() asynchronously after a timeout. If the component unmounts before the async operation completes, it may set state on an unmounted component. This was flagged in the previous review and remains unaddressed.
Suggested fix: Add a mounted ref:
const mountedRef = useRef(true);
useEffect(() => { return () => { mountedRef.current = false; }; }, []);
// Check mountedRef.current before setting state in loadStatus.Summary
This follow-up commit correctly addressed the distro name validation issue by adding a local validateDistroName function. However, two major issues from the previous review remain unresolved: the command injection risk from unescaped command parameter and the potential timeout unit mismatch. Additionally, no tests were added and the state update after unmount minor issue persists. The command injection fix should be applied before merging, and the timeout unit should be verified and corrected.
Open Cowork Bot
|
Thanks for the follow-up review. The distro validation is fixed. On the two
remaining "MAJOR" items, I looked into both against the actual SDK source and
believe they're false positives from static-analysis pattern matching rather
than real bugs - details below, happy to be corrected by a maintainer:
Timeout unit: not a mismatch. @mariozechner/pi-coding-agent's own bash tool
schema documents timeout as seconds ("Timeout in seconds (optional, no
default timeout)"), and its own default createLocalBashOperations() does the
exact same `timeout * 1000` this code does. This is directly checkable in
***@***.***/pi-coding-agent/dist/core/tools/bash.js - the two
implementations match.
Command escaping: applying the suggested fix would break the tool for any
command containing a single quote (e.g. `git commit -m "..."`, `echo 'text'`,
`grep 'pattern' file`) - i.e. most real commands. The suggested escaping
pattern (`'\''`) is the correct idiom for safely wrapping a plain data string
in quotes, but `command` here isn't a data string - it's executable shell
source by design, which is the entire point of a bash execution tool. There's
also no privilege boundary being crossed: whatever calls this already has full
command execution via the `command` argument itself, so escaping it doesn't
prevent anything, it just corrupts ordinary quoted commands.
Agreed on test coverage - will follow up with that separately.
…________________________________
From: github-actions[bot] ***@***.***>
Sent: 13 July 2026 00:05
To: OpenCoworkAI/open-cowork ***@***.***>
Cc: Sean Nair ***@***.***>; Author ***@***.***>
Subject: Re: [OpenCoworkAI/open-cowork] Fix/wsl sandbox bash tool wiring (PR #298)
⚠ Caution: External sender
@github-actions[bot] commented on this pull request.
Review mode: follow-up after new commits
Findings
[MAJOR] Unresolved command injection risk (src/main/agent/wsl-sandbox-bash-operations.ts:97)
The command parameter is still interpolated directly into the shell script without escaping:
const script = `mkdir -p '${escapedCwd}' 2>/dev/null; cd '${escapedCwd}' || ...; ${command}`;
An attacker controlling command (e.g., via a compromised LLM agent) could break out of the single-quote context or inject additional commands. This was flagged in the previous review and remains unfixed.
Suggested fix: Escape the command string for POSIX shell before embedding:
const escapedCommand = command.replace(/'/g, "'\\''");
const script = `mkdir -p '${escapedCwd}' 2>/dev/null; cd '${escapedCwd}' || ...; ${escapedCommand}`;
[MAJOR] Potential timeout unit mismatch (src/main/agent/wsl-sandbox-bash-operations.ts:176)
The execution timeout is multiplied by 1000 (timeout * 1000). The BashOperations interface from @mariozechner/pi-coding-agent typically passes timeout in milliseconds. If that is the case, this would set the timeout 1000x longer than intended, leading to delayed or missed timeouts. The original SDK code should be consulted to confirm the unit. This issue was also raised previously and remains unresolved.
Suggested fix: Verify the SDK unit. If milliseconds are passed, remove the multiplication:
timeoutHandle = setTimeout(() => {
timedOut = true;
terminateChild('timeout');
}, timeout); // no * 1000
[MINOR] Missing test coverage
No tests have been added for any of the new or modified functionality, including:
* createWslSandboxBashOperations (mock spawn, verify script construction, timeout, abort, process kill)
* testDistro retry logic in wsl-bridge.ts
* initializeWSL sharing bootstrap result in sandbox-adapter.ts
* handleToggleSandbox in SettingsSandbox.tsx
* The added validateDistroName function
[MINOR] State update after unmount in SettingsSandbox.tsx (previously flagged)
The handleToggleSandbox function calls loadStatus() asynchronously after a timeout. If the component unmounts before the async operation completes, it may set state on an unmounted component. This was flagged in the previous review and remains unaddressed.
Suggested fix: Add a mounted ref:
const mountedRef = useRef(true);
useEffect(() => { return () => { mountedRef.current = false; }; }, []);
// Check mountedRef.current before setting state in loadStatus.
Summary
This follow-up commit correctly addressed the distro name validation issue by adding a local validateDistroName function. However, two major issues from the previous review remain unresolved: the command injection risk from unescaped command parameter and the potential timeout unit mismatch. Additionally, no tests were added and the state update after unmount minor issue persists. The command injection fix should be applied before merging, and the timeout unit should be verified and corrected.
Open Cowork Bot
—
Reply to this email directly, view it on GitHub<#298?email_source=notifications&email_token=A7767QCBUQ7WBZIV4KX2WIL5EQKTNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRYGA4TIOJRGQ22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4680949145>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A7767QDXOLBIXBLNMWPLNG35EQKTNAVCNFSNUABGKJSXA33TNF2G64TZHMYTCMZTGY3DAMRWHA5US43TOVSTWNBYGY4TAOJXGA2DHILWAI>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/A7767QDE7ROAWGBFNAPQ2435EQKTNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRYGA4TIOJRGQ22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android<https://github.com/notifications/mobile/android/A7767QCRWQGQOFHGXJMSPNL5EQKTNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRYGA4TIOJRGQ22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today!
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Addresses remaining review feedback on PR OpenCoworkAI#298: - Documents (via inline comment) why `command` is intentionally left unescaped in wsl-sandbox-bash-operations.ts's exec() - it is executable shell source provided by the bash tool's own caller, not untrusted data being embedded into a fixed template, so there is no privilege boundary for escaping to protect. Escaping it would corrupt any command containing its own quoting. - Adds a test for the new validateDistroName check (rejects an invalid distro name synchronously, without spawning). - Adds src/tests/sandbox/wsl-bridge-test-distro.test.ts covering WSLBridge.testDistro's cold-start retry logic: succeeds immediately, retries once after a failure and succeeds, returns false if both attempts fail, and rejects invalid distro names without ever calling execFile. Mocks child_process via util.promisify.custom (matching how execFileAsync is actually constructed) and uses fake timers to avoid real 4-second waits in the test suite. All tests, tsc --noEmit, and eslint verified locally before this commit.
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[MAJOR] Unresolved command injection risk (src/main/agent/wsl-sandbox-bash-operations.ts:105-115)
The command parameter is still interpolated directly into the shell script without escaping. A comment was added explaining that the command originates from the coding agent's bash tool (which already has full execution rights) and that escaping would corrupt legitimate quoting. While the comment accurately describes the current attack surface, this remains a defense-in-depth concern: if command ever originates from a less trusted source (e.g., a user input or plugin), injection would be possible. The risk is acknowledged but not mitigated.
Suggested fix: If the threat model permits, consider escaping command for the single-quote context (e.g., command.replace(/'/g, "'\\''")). Otherwise, accept the risk and close.
[MAJOR] Potential timeout unit mismatch (src/main/agent/wsl-sandbox-bash-operations.ts:176)
The execution timeout is multiplied by 1000 (timeout * 1000). The BashOperations interface from @mariozechner/pi-coding-agent may pass timeout in milliseconds; if so, this multiplies by 1000 again, causing delayed timeouts. The test uses timeout: 1 and expects timeout:1 error, which suggests the code currently assumes seconds. Verification of the SDK's unit is still needed.
Suggested fix: Confirm the SDK unit. If milliseconds are passed, remove the multiplication:
timeoutHandle = setTimeout(() => {
timedOut = true;
terminateChild('timeout');
}, timeout); // no * 1000[MINOR] State update after unmount in SettingsSandbox.tsx (previously flagged)
handleToggleSandbox calls loadStatus() asynchronously after a 500ms timeout. If the component unmounts before that fires, setState calls on unmounted component may occur. Still unfixed.
Suggested fix: Add a mounted ref:
const mountedRef = useRef(true);
useEffect(() => { return () => { mountedRef.current = false; }; }, []);
// In loadStatus and handleToggleSandbox: check mountedRef.current before setState.[INFO] Tests added
Two test files have been added (src/tests/agent/wsl-sandbox-bash-operations.test.ts and src/tests/sandbox/wsl-bridge-test-distro.test.ts) covering the new createWslSandboxBashOperations and testDistro retry logic. Well-structured and thorough.
Summary
This iteration added comprehensive tests (addressing the previous missing-test concern) and a clarifying comment about command escaping. However, two major issues from the prior review remain unaddressed: the command injection risk (now acknowledged by comment) and the timeout unit mismatch. The state-update-after-unmount minor issue also persists. The timeout unit should be verified and corrected before merging; the command injection and unmount issues should be decided whether to fix or accept as design.
Open Cowork Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[INFO] Timeout unit confirmed
A detailed comment was added at src/main/agent/wsl-sandbox-bash-operations.ts:198-207 explaining that the timeout parameter from @mariozechner/pi-coding-agent's BashOperations interface is in seconds, matching the SDK's schema and its own createLocalBashOperations implementation. The timeout * 1000 conversion is correct. Resolved.
[MAJOR] Unresolved command injection risk (src/main/agent/wsl-sandbox-bash-operations.ts:105-115)
No changes were made to address the command injection risk flagged in the previous review. The command parameter is still interpolated directly into the shell script without escaping. The existing comment explains that the command originates from the coding agent's bash tool (which already has full execution rights) and that escaping would corrupt legitimate quoting. This is an accepted design risk, but it remains a defense-in-depth gap. No action taken.
[MINOR] State update after unmount in SettingsSandbox.tsx
handleToggleSandbox calls loadStatus() asynchronously after a 500ms timeout. If the component unmounts before that fires, setState calls may occur on an unmounted component. This was flagged in the previous review and remains unaddressed. Unresolved.
[INFO] Tests retained
Both test files (src/tests/agent/wsl-sandbox-bash-operations.test.ts and src/tests/sandbox/wsl-bridge-test-distro.test.ts) are unchanged and cover the new logic well.
Summary
The added comment clarifies the timeout unit, resolving that concern. The command injection risk remains an accepted design decision (no mitigation added). The unmounted state-update issue persists. This PR is otherwise solid, with good test coverage and clear logic.
Open Cowork Bot
|
Thanks for the thorough review across these rounds. Status on each item:
Command escaping: leaving as-is, per your own suggested resolution
("accept the risk and close"). `command` is executable shell source
provided by the bash tool's caller, not untrusted data being embedded
into a template - there's no privilege boundary escaping would protect,
and the SDK's own default bash operations (createLocalBashOperations)
handle `command` the same way. This is now documented inline in the
code as well.
Timeout unit: confirmed not a mismatch, with a direct citation. Added
a comment at the timeout*1000 line pointing to
***@***.***/pi-coding-agent/dist/core/tools/bash.js,
whose own schema documents timeout as "Timeout in seconds (optional,
no default timeout)", and whose own createLocalBashOperations() does
the identical timeout*1000 conversion this code does. Same SDK,
same convention.
Distro validation: fixed in an earlier commit (validateDistroName,
with test coverage).
Test coverage: added - two test files covering createWslSandboxBashOperations
(routing, quote-escaping, timeout/kill, abort, invalid distro) and
WSLBridge.testDistro's cold-start retry logic (success, retry-then-
success, both-fail, invalid name).
SettingsSandbox.tsx unmount: this file isn't part of this PR's actual
changes - those commits came from #293 (the sandbox toggle fix) and
ended up in this branch's history incidentally.
…________________________________
From: github-actions[bot] ***@***.***>
Sent: 13 July 2026 08:06
To: OpenCoworkAI/open-cowork ***@***.***>
Cc: Sean Nair ***@***.***>; Author ***@***.***>
Subject: Re: [OpenCoworkAI/open-cowork] Fix/wsl sandbox bash tool wiring (PR #298)
⚠ Caution: External sender
@github-actions[bot] commented on this pull request.
Review mode: follow-up after new commits
Findings
[INFO] Timeout unit confirmed
A detailed comment was added at src/main/agent/wsl-sandbox-bash-operations.ts:198-207 explaining that the timeout parameter from @mariozechner/pi-coding-agent's BashOperations interface is in seconds, matching the SDK's schema and its own createLocalBashOperations implementation. The timeout * 1000 conversion is correct. Resolved.
[MAJOR] Unresolved command injection risk (src/main/agent/wsl-sandbox-bash-operations.ts:105-115)
No changes were made to address the command injection risk flagged in the previous review. The command parameter is still interpolated directly into the shell script without escaping. The existing comment explains that the command originates from the coding agent's bash tool (which already has full execution rights) and that escaping would corrupt legitimate quoting. This is an accepted design risk, but it remains a defense-in-depth gap. No action taken.
[MINOR] State update after unmount in SettingsSandbox.tsx
handleToggleSandbox calls loadStatus() asynchronously after a 500ms timeout. If the component unmounts before that fires, setState calls may occur on an unmounted component. This was flagged in the previous review and remains unaddressed. Unresolved.
[INFO] Tests retained
Both test files (src/tests/agent/wsl-sandbox-bash-operations.test.ts and src/tests/sandbox/wsl-bridge-test-distro.test.ts) are unchanged and cover the new logic well.
Summary
The added comment clarifies the timeout unit, resolving that concern. The command injection risk remains an accepted design decision (no mitigation added). The unmounted state-update issue persists. This PR is otherwise solid, with good test coverage and clear logic.
Open Cowork Bot
—
Reply to this email directly, view it on GitHub<#298?email_source=notifications&email_token=A7767QHI5MTIAA6DBWVJABT5ESDAFA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRYGI2DIOBTG422M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4682448375>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A7767QGZRQTAQYG5GJ76SJL5ESDAFAVCNFSNUABGKJSXA33TNF2G64TZHMYTCMZTGY3DAMRWHA5US43TOVSTWNBYGY4TAOJXGA2DHILWAI>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/A7767QA4FQFQICPDQCNGSQL5ESDAFA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRYGI2DIOBTG422M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android<https://github.com/notifications/mobile/android/A7767QDX6I4Z3AWXD2JNMYD5ESDAFA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRYGI2DIOBTG422M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today!
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Four stacked issues were together causing bash tool calls to always fail under Windows/WSL2 sandbox mode with "Working directory does not exist: /workspace / Cannot execute bash commands." even when Settings correctly reported WSL2 as ready. 1. WSLBridge.testDistro() made a single WSL echo probe with no retry. WSL2's lightweight VM can be cold/dormant on first use after boot or idle, causing a transient false negative that got cached as "WSL unavailable" for the rest of the app session. Now retries once after a short delay before giving up. 2. SandboxAdapter.initializeWSL() ran its own independent WSL status check instead of using the shared SandboxBootstrap check that drives the startup popup. Two concurrent checks at cold start could disagree, leaving the adapter stuck on a stale result even after the popup reported success. Now awaits the shared bootstrap instead of racing it. 3. The Settings sandbox.getStatus IPC handler never triggered adapter initialization, so checking Settings before starting any chat session always showed a hardcoded placeholder (Native/none) regardless of real WSL2 status. Now triggers on-demand initialization. 4. Root cause of the actual execution failure: @mariozechner/pi-coding-agent's createAgentSession() only reads tool names off the tools option to build an allow-list, then discards the actual Tool objects and rebuilds default (non-WSL) bash operations internally via createAllTools(cwd, ...). This silently dropped our WSL-routed bash operations (which cd into the sandbox path inside the WSL distro) on every single turn, regardless of session cache state, and fell back to the SDK's plain existsSync-based local bash operations, which then always failed because the sandbox path is a WSL-side path that does not exist on the Windows host. Fixed via a patch-package patch that additionally threads the real tool objects through as AgentSession's baseToolsOverride. Verified via production logs: after this fix, the WslSandboxBash executing-in-distro log line now actually fires, and bash commands return real output from the synced sandbox filesystem instead of the generic error. Separate from OpenCoworkAI#293, which fixes the missing sandbox enable/disable toggle and Linux status detection - that PR addresses whether sandbox mode can be turned on at all; this one addresses whether commands actually execute correctly once it is.
Addresses a review finding on PR OpenCoworkAI#298: the distro name reached the spawned "wsl -d <distro>" command line without validation, unlike WSLBridge which already validates it elsewhere. Adds a local validateDistroName (mirroring WSLBridge private method) as defense-in-depth, even though the current call path only ever passes the already-detected wslStatus.distro value.
Addresses remaining review feedback on PR OpenCoworkAI#298: - Documents (via inline comment) why `command` is intentionally left unescaped in wsl-sandbox-bash-operations.ts's exec() - it is executable shell source provided by the bash tool's own caller, not untrusted data being embedded into a fixed template, so there is no privilege boundary for escaping to protect. Escaping it would corrupt any command containing its own quoting. - Adds a test for the new validateDistroName check (rejects an invalid distro name synchronously, without spawning). - Adds src/tests/sandbox/wsl-bridge-test-distro.test.ts covering WSLBridge.testDistro's cold-start retry logic: succeeds immediately, retries once after a failure and succeeds, returns false if both attempts fail, and rejects invalid distro names without ever calling execFile. Mocks child_process via util.promisify.custom (matching how execFileAsync is actually constructed) and uses fake timers to avoid real 4-second waits in the test suite. All tests, tsc --noEmit, and eslint verified locally before this commit.
Addresses the bot review's remaining "verification needed" item on the
timeout * 1000 conversion in wsl-sandbox-bash-operations.ts. Points
directly at @mariozechner/pi-coding-agent's own bash tool schema
("Timeout in seconds") and its own createLocalBashOperations(), which
performs the identical conversion - confirming this mirrors the SDK's
documented and implemented convention rather than being a unit
mismatch.
04bcc81 to
e91a849
Compare
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
[MAJOR] Untargeted removal of think-tag-parser and streaming logic
The compare diff since the previous review shows the deletion of src/main/agent/think-tag-parser.ts and modifications to agent-runner.ts and agent-runner-message-end.ts that remove all references to the think tag parser and related streaming emission. This functionality was responsible for separating <think>...</think> tags (used by Ollama reasoning models like deepseek-r1) from normal text during streaming. The PR description makes no mention of this change. If these models are in use, removing this parser will cause thinking content to be displayed as plain text, breaking the UX. The author should either revert these changes (if unintentional) or explain the rationale in the PR description. The risk of regression is high.
[MAJOR] Command injection risk (still unresolved)
As noted in the previous review, src/main/agent/wsl-sandbox-bash-operations.ts:105-115 interpolates the command parameter directly into a shell script without escaping. While the command originates from the coding agent's bash tool (which already has full execution rights), this remains a defense-in-depth weakness. The author has added a comment explaining the rationale, but no mitigation was applied. For consistency with the project's security posture, consider using parameterized execution or a shell-escaped variant.
[MINOR] SettingsSandbox.tsx: state update after unmount (still unresolved)
The handleToggleSandbox function in SettingsSandbox.tsx calls loadStatus() asynchronously after a 500ms timeout. If the component unmounts before that fires, setState calls may occur on an unmounted component. This was flagged in the previous review and remains unaddressed.
[INFO] WSL sandbox changes are well-tested
src/main/agent/wsl-sandbox-bash-operations.ts is a clean implementation with thorough unit tests in src/tests/agent/wsl-sandbox-bash-operations.test.ts. The patch to @mariozechner/pi-coding-agent correctly threads custom tools through baseToolsOverride. The modifications to sandbox-adapter.ts and wsl-bridge.ts (cold-start retry, shared bootstrap) are sound.
Summary
The WSL sandbox reliability fixes are solid and well-tested. However, the PR now includes a large, undocumented deletion of the think-tag-parser and related streaming code that poses a high regression risk for Ollama reasoning models. This should be addressed before merging. The previously flagged command injection risk and state update issue remain unresolved.
Open Cowork Bot
|
The think-tag-parser and SettingsSandbox.tsx findings appear to be from a
stale diff comparison - this branch was rebased (force-pushed) between
reviews, and I believe the bot is diffing against the pre-rebase commit
SHAs rather than the current state. I checked directly: PR #298's actual
commit list is 4 commits (6329e55, a4b91a3, a44009f, e91a849), none of
which touch think-tag-parser.ts, agent-runner-message-end.ts, or
SettingsSandbox.tsx. The think-tag-parser removal is from #289, already
merged to main by a different contributor, unrelated to this PR. Would
appreciate a maintainer double-checking the Files Changed tab directly
if there's any doubt.
Command injection: still standing by the earlier explanation - accepting
as a design decision per your own prior suggestion, documented in code.
…________________________________
From: github-actions[bot] ***@***.***>
Sent: 13 July 2026 08:30
To: OpenCoworkAI/open-cowork ***@***.***>
Cc: Sean Nair ***@***.***>; Author ***@***.***>
Subject: Re: [OpenCoworkAI/open-cowork] Fix/wsl sandbox bash tool wiring (PR #298)
⚠ Caution: External sender
@github-actions[bot] commented on this pull request.
Review mode: follow-up after new commits
Findings
[MAJOR] Untargeted removal of think-tag-parser and streaming logic
The compare diff since the previous review shows the deletion of src/main/agent/think-tag-parser.ts and modifications to agent-runner.ts and agent-runner-message-end.ts that remove all references to the think tag parser and related streaming emission. This functionality was responsible for separating <think>...</think> tags (used by Ollama reasoning models like deepseek-r1) from normal text during streaming. The PR description makes no mention of this change. If these models are in use, removing this parser will cause thinking content to be displayed as plain text, breaking the UX. The author should either revert these changes (if unintentional) or explain the rationale in the PR description. The risk of regression is high.
[MAJOR] Command injection risk (still unresolved)
As noted in the previous review, src/main/agent/wsl-sandbox-bash-operations.ts:105-115 interpolates the command parameter directly into a shell script without escaping. While the command originates from the coding agent's bash tool (which already has full execution rights), this remains a defense-in-depth weakness. The author has added a comment explaining the rationale, but no mitigation was applied. For consistency with the project's security posture, consider using parameterized execution or a shell-escaped variant.
[MINOR] SettingsSandbox.tsx: state update after unmount (still unresolved)
The handleToggleSandbox function in SettingsSandbox.tsx calls loadStatus() asynchronously after a 500ms timeout. If the component unmounts before that fires, setState calls may occur on an unmounted component. This was flagged in the previous review and remains unaddressed.
[INFO] WSL sandbox changes are well-tested
src/main/agent/wsl-sandbox-bash-operations.ts is a clean implementation with thorough unit tests in src/tests/agent/wsl-sandbox-bash-operations.test.ts. The patch to @mariozechner/pi-coding-agent correctly threads custom tools through baseToolsOverride. The modifications to sandbox-adapter.ts and wsl-bridge.ts (cold-start retry, shared bootstrap) are sound.
Summary
The WSL sandbox reliability fixes are solid and well-tested. However, the PR now includes a large, undocumented deletion of the think-tag-parser and related streaming code that poses a high regression risk for Ollama reasoning models. This should be addressed before merging. The previously flagged command injection risk and state update issue remain unresolved.
Open Cowork Bot
—
Reply to this email directly, view it on GitHub<#298?email_source=notifications&email_token=A7767QGYIOLW4I33Q3B4HBD5ESFZZA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRYGI3DAMJTHA3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4682601386>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A7767QDJFF4QJFOL52CDPSD5ESFZZAVCNFSNUABGKJSXA33TNF2G64TZHMYTCMZTGY3DAMRWHA5US43TOVSTWNBYGY4TAOJXGA2DHILWAI>.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS<https://github.com/notifications/mobile/ios/A7767QDUPHLXK2PPLP45YD35ESFZZA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRYGI3DAMJTHA3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android<https://github.com/notifications/mobile/android/A7767QH5AYRP4I7JIQZVXA35ESFZZA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRYGI3DAMJTHA3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today!
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Sun-sunshine06
left a comment
Sun-sunshine06
left a comment
There was a problem hiding this comment.
I re-checked the current 7-file diff directly. The stale bot findings about think-tag-parser / SettingsSandbox.tsx are not in this PR, and the timeout unit is correctly seconds. However, the current head still has several blockers:
-
[BLOCKER] The WSL Bash implementation is not wired into production.
createWslSandboxBashOperationshas no production import or call.agent-runner.ts:2161-2163still selectscreateWindowsBashOperations()unconditionally on Windows. In WSL modeeffectiveCwdis/home/..., so the Windows implementation still reaches its host-sideexistsSync(cwd)check and reproduces the originalWorking directory does not existfailure. The SDK patch preserves the supplied Tool object, but the supplied Bash Tool is still the Windows one. Please wire the WSL implementation into AgentRunner and add a test that exercises the actual tool-selection path, not only the isolated helper. -
[CRITICAL] The SDK patch activates unsafe sudo-password handling. Before this patch, the SDK discarded the supplied wrapped Tool implementations;
baseToolsOverridenow makeswrapBashToolForSudoexecute. That wrapper treats any/\bsudo\b/match as sudo, spawns the entire model-provided shell command, and writes the user's password to that process's stdin. A command can placesudoin a comment/string while another program reads stdin, so the password is not scoped to sudo at all. In WSL mode the wrapper also hard-codes host-sidecmd.exewith a WSL-only cwd. This must be redesigned and covered before enabling supplied Tool implementations globally. -
[HIGH] The proposed direct WSL Bash path bypasses the existing sandbox boundary.
wsl-sandbox-bash-operations.ts:118-124directly spawnswsl ... bash -c. It does not pass throughwsl-agent'svalidateCommand, realpath/symlink checks, traversal checks, or workspace containment. Once wired, commands can access paths such as~/.sshand/mnt/coutside the synced workspace, contrary to the project's workspace-confinement guarantee. Please route through a containment-capable executor or enforce an equivalent per-session boundary. -
[HIGH]
sandbox.getStatuscan pin the singleton adapter to the wrong workspace. It initializes with the global default working directory, whileSandboxAdapter.initialize()permanently reuses its firstinitPromise. A laterSessionManagerinitialization for the realsession.cwdis therefore silently skipped, leaving the adapter/WSL agent configured for the default directory. A status read should not initialize a workspace-bound executor, or initialization must safely reconfigure on workspace changes; this also needs a regression test.
The new focused tests (13 assertions locally) and typecheck pass, but they do not cover these production wiring and security paths. Requesting changes rather than merging.
|
Thanks — I double-checked the current head ( The current PR diff contains only seven files related to the WSL fix: the SDK patch, the new WSL Bash operations, The bot compared the pre-rebase head ( I also agree that So the stale-diff clarification is accepted, but these independent current-head blockers still need to be addressed before merging.
|
3 participants
fix: wsl2 sandbox bash execution reliability on Windows
Four stacked issues were together causing bash tool calls to always fail
under Windows/WSL2 sandbox mode with "Working directory does not exist:
/workspace / Cannot execute bash commands." even when Settings correctly
reported WSL2 as ready.
WSLBridge.testDistro() made a single WSL echo probe with no retry.
WSL2's lightweight VM can be cold/dormant on first use after boot or
idle, causing a transient false negative that got cached as "WSL
unavailable" for the rest of the app session. Now retries once after
a short delay before giving up.
SandboxAdapter.initializeWSL() ran its own independent WSL status
check instead of using the shared SandboxBootstrap check that drives
the startup popup. Two concurrent checks at cold start could
disagree, leaving the adapter stuck on a stale result even after the
popup reported success. Now awaits the shared bootstrap instead of
racing it.
The Settings sandbox.getStatus IPC handler never triggered adapter
initialization, so checking Settings before starting any chat
session always showed a hardcoded placeholder (Native/none)
regardless of real WSL2 status. Now triggers on-demand
initialization.
Root cause of the actual execution failure: @mariozechner/pi-coding-agent's
createAgentSession() only reads tool names off the tools option to
build an allow-list, then discards the actual Tool objects and
rebuilds default (non-WSL) bash operations internally via
createAllTools(cwd, ...). This silently dropped the WSL-routed bash
operations (which cd into the sandbox path inside the WSL distro) on
every single turn, regardless of session cache state, and fell back
to the SDK's plain existsSync-based local bash operations, which
then always failed because the sandbox path is a WSL-side path that
does not exist on the Windows host. Fixed via a patch-package patch
that additionally threads the real tool objects through as
AgentSession's baseToolsOverride.
Verified via production logs: after this fix, the WslSandboxBash
executing-in-distro log line now actually fires, and bash commands
return real output from the synced sandbox filesystem instead of the
generic error.
Separate from #293, which fixes the missing sandbox enable/disable
toggle and Linux status detection - that PR addresses whether sandbox
mode can be turned on at all; this one addresses whether commands
actually execute correctly once it is.