feat: restore command sandbox preview and improve unread attention - #286
Conversation
WalkthroughThis PR introduces a macOS-only Command Sandbox Preview with authenticated policies, restricted environments, process cleanup, private-network approval, HTTP/SOCKS5 proxying, OpenCode shell integration, and end-to-end verification. Permission modes now synchronize with sandbox execution. Runtime capabilities and settings expose sandbox availability, while full-access confirmation UI is removed. Attention state now aggregates unread teams and displays indicators in workspace navigation. Documentation and packaging are updated accordingly. Sequence Diagram(s)sequenceDiagram
participant User
participant ChatService
participant AgentManager
participant OpenCode
participant CommandSandbox
User->>ChatService: select permission mode or send message
ChatService->>AgentManager: update sandbox policy
AgentManager->>OpenCode: configure shell and plugin
OpenCode->>CommandSandbox: execute shell command
CommandSandbox->>AgentManager: request private-network approval
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
electron/chat/node.ts (1)
950-956: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDetaching the method reference risks binding to a swapped agent.
updatePolicyis captured fromthis.agent, but the laterupdatePolicy.call(this.agent, …)re-readsthis.agent; ifsetAgent()runs in between (sign-out/runtime restart), the old method is invoked against the new manager instance. A direct optional call keeps the receiver and the guard consistent:♻️ Proposed simplification
- const updatePolicy = this.agent?.updateCommandSandboxPolicy - if (!updatePolicy) return + const agent = this.agent + if (!agent?.updateCommandSandboxPolicy) return @@ - await updatePolicy.call(this.agent, { + await agent.updateCommandSandboxPolicy({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/chat/node.ts` around lines 950 - 956, Update the policy update flow around updateCommandSandboxPolicy to avoid storing the method separately from its receiver. Guard and invoke this.agent?.updateCommandSandboxPolicy directly so the method and bound agent instance remain consistent if setAgent() changes the agent before the call.scripts/verify-command-sandbox.ts (1)
31-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFail fast on unsupported platforms and missing probe prerequisites.
The script hard-codes
/bin/zshand depends oncurlandnc(lines 47-48); on Linux/Windows the runtime throws "Command Sandbox (Preview) is currently available only on macOS" deep inside a spawned child, surfacing as an opaque non-zero exit. An explicitprocess.platform !== "darwin"guard plus an existence check for the built wrapper (dist-electron/wanta-command-shell.js) would make the failure self-explanatory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify-command-sandbox.ts` around lines 31 - 61, Update the verification script before creating or running the sandbox probe to exit clearly when process.platform is not "darwin", and validate that the built wrapper at dist-electron/wanta-command-shell.js exists before use. Report actionable messages for either unsupported platforms or missing wrapper prerequisites, while preserving the existing macOS probe flow and dependency checks.electron/agent/manager.ts (1)
1302-1305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePer-session policy, grant, and home directories are never reclaimed.
disposecloses the broker but leavescommand-sandbox/{policies,network-grants,home}entries behind, andcommandSandboxPolicyInputsgrows for the process lifetime. Deleted sessions in particular keep a signed policy plus a managed home dir on disk indefinitely. A cleanup hook alongside the existing session-forget path would keep this bounded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/agent/manager.ts` around lines 1302 - 1305, Extend the session disposal flow around the async cleanup that closes commandSandboxBroker to also reclaim that session’s command-sandbox policy, network-grant, and home directories, and remove its entry from commandSandboxPolicyInputs. Reuse the existing session-forget cleanup path or helper if available, ensuring cleanup runs for deleted sessions and preserves broker shutdown behavior.package.json (1)
48-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope
@vscode/sandbox-runtimeto macOS builds only. It’s only used behind thedarwingate, so shipping it in Windows/Linux artifacts is unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 48 - 49, Update the `@vscode/sandbox-runtime` dependency declaration in package.json to be included only for macOS (darwin) builds, using the package manager’s platform-specific dependency configuration. Keep the dependency version unchanged and leave `@pondwader/socks5-server` unaffected.electron/agent/command-sandbox/network-proxy.ts (2)
49-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCONNECT and SOCKS5 tunnel handling duplicate the same pipe/cleanup logic.
handleConnectand the SOCKS5setConnectionHandlercallback both establish an upstreamDuplex, pipe it bidirectionally, and wire the sameclose/errorteardown. A future fix (e.g., the timeout above) has to be applied twice, and it's easy for the two paths to drift.♻️ Suggested extraction
+function pipeTunnel(client: Duplex, upstream: Duplex): void { + upstream.pipe(client) + client.pipe(upstream) + client.once("close", () => upstream.destroy()) + client.once("error", () => upstream.destroy()) + upstream.once("error", () => client.destroy()) +}Call
pipeTunnel(client, upstream)from bothhandleConnectand the SOCKS5 connection handler (usingconnection.socketasclient).Also applies to: 87-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/agent/command-sandbox/network-proxy.ts` around lines 49 - 69, Extract the duplicated bidirectional piping and teardown logic into a shared pipeTunnel(client, upstream) helper. Replace the inline piping and close/error cleanup in both handleConnect and the SOCKS5 setConnectionHandler success path with calls to pipeTunnel, passing connection.socket for the SOCKS5 client while preserving the existing status and failure handling.
376-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid depending on
createSocksServer()’s internal.serverfield.listenSocksServerreaches into a non-public wrapper to read the bound port, so this helper is brittle across@pondwader/socks5-serverupgrades. Keep the cast isolated here or switch to a documented accessor if one is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/agent/command-sandbox/network-proxy.ts` around lines 376 - 390, Update listenSocksServer so its dependency on createSocksServer’s non-public server field is isolated to this helper, preferably through a single localized typed adapter/accessor; use a documented accessor instead if the library provides one. Keep the existing error handling and bound-port resolution behavior unchanged.electron/agent/command-sandbox/plugin.ts (1)
6-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo error handling around the session-walk in
shell.env.If
client.session.get(Line 13) rejects or throws for any reason (transient IPC hiccup, etc.), the whole"shell.env"hook throws with no fallback, and downstreamruntime.tswill fail the command outright (itsrequireEnvironmenttreats a missingWANTA_COMMAND_SANDBOX_SESSION_IDas "shell path is not supported"). Consider wrapping the walk in try/catch and falling back toinput.sessionIDon failure.How does OpenCode's plugin system handle an exception thrown from a "shell.env" hook — does it fail the shell command, or ignore the hook and continue?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/agent/command-sandbox/plugin.ts` around lines 6 - 20, Update the "shell.env" hook in the default exported plugin to catch failures from the session-walk, including client.session.get, and fall back to input.sessionID as the policy session ID before setting WANTA_COMMAND_SANDBOX_SESSION_ID. Preserve the existing parent traversal and call ID assignment when lookups succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/agent/command-sandbox/network-address.ts`:
- Around line 123-127: Extract and export a shared grant-matching helper from
electron/agent/command-sandbox/network-address.ts, replacing the local
hasPrivateGrant predicate at lines 123-127 while preserving address
normalization, wildcard ports, and exact port matching. In
electron/agent/command-sandbox/broker.ts lines 171-176, replace hasGrant with an
import/use of that shared helper and remove the duplicate local implementation.
In `@electron/agent/command-sandbox/network-proxy.ts`:
- Around line 202-286: Add explicit timeouts to all outbound connection and
proxy negotiation paths: the direct socket created in connectTarget, both socket
variants in connectProxy, and the response wait managed by
waitForConnectResponse. Reuse the existing timeout pattern or shared timeout
configuration, and ensure expiration cleans up listeners/sockets and rejects
with an appropriate timeout error instead of leaving the sandboxed command
hanging.
- Around line 34-48: Require authentication on both listeners created in the
network proxy setup: configure createSocksServer to validate the existing
session secret through SOCKS5 auth, and update the HTTP request and CONNECT
handling around handleHttpRequest and handleConnect to require a valid
Proxy-Authorization header using that same secret. Reject unauthenticated
connections before invoking resolve or forwarding requests.
In `@electron/agent/command-sandbox/policy.test.ts`:
- Around line 43-75: Update both environment-mode tests around
buildCommandSandboxEnvironment and buildDirectCommandEnvironment to include
OPENCODE_CONFIG_CONTENT, OPENCODE_SERVER_PASSWORD, OO_API_KEY, and
WANTA_BROWSER_CONTROL_TOKEN as credential sentinels in each input. Assert every
listed credential is undefined in both resulting environments, while preserving
the existing non-credential environment assertions and secret-persistence check.
In `@electron/agent/command-sandbox/runtime.ts`:
- Around line 217-243: Update spawnCommand to avoid installing per-command
SIGINT and SIGTERM listeners on the shared process object; route signals through
a centralized, concurrency-safe mechanism that tracks active child processes and
forwards each signal only to its intended command. Ensure command cleanup
removes its tracking state on exit or error without affecting other concurrent
sandboxed commands.
In `@electron/agent/command-sandbox/shell-bin.ts`:
- Around line 14-30: Update ensureCommandSandboxShellBin in
electron/agent/command-sandbox/shell-bin.ts:14-30 to write the generated shell
script to a temporary file within binDir, apply chmod, then atomically rename it
over commandPath. Apply the same temp-file, chmod, and rename pattern to
pluginPath in electron/agent/command-sandbox/plugin.ts:24-30; do not write
either fixed path directly.
In `@electron/agent/manager.ts`:
- Around line 455-477: The updateCommandSandboxPolicy method must serialize the
complete per-session read, merge, broker synchronization, and write through a
per-session promise chain, following the existing queueTeamUpdate pattern so
overlapping callers cannot lose fields or persist stale policies. Also update
the broker setSession condition to register or refresh the session when
private-network grants require it even if next.userMessage is absent, while
preserving the existing reviewer context behavior when a user message exists.
- Around line 602-634: Ensure commandSandboxRuntimeReadPaths is initialized
before any updateCommandSandboxPolicy call, including startup and runtime
recovery, using rootDir, cliPath, process.execPath, and resolved PATH
directories; otherwise defer policy writes until populated. In
prepareCommandSandbox, preserve or refresh the same paths consistently. Validate
that process.env.SHELL is an absolute executable before selecting it as
delegateShell, falling back to /bin/zsh when invalid.
In `@patches/`@vscode__sandbox-runtime@0.0.1.patch:
- Around line 18-20: The allowLoopbackOutbound rule in generateSandboxProfile
currently permits access to every localhost port; restrict it to
localhost:httpProxyPort and localhost:socksProxyPort only. Preserve the intended
proxy egress while removing blanket loopback access, using the existing in-scope
proxy port values when constructing the network-outbound rules.
In `@scripts/verify-command-sandbox.ts`:
- Around line 85-107: Update probeNestedControlDirectory so store.write receives
an allowedRoot that does not contain or overlap the policy control directory,
preventing the expected overlap error from short-circuiting the probe. Preserve
the existing spawned read-denial check and ensure the function reaches run to
verify sandbox enforcement rather than merely validating store.write rejection.
- Around line 62-71: Replace the fixed 1.4-second delay before constructing
assertions in the verification flow with bounded polling for detachedMarker.
Ensure the probe waits until the marker appears or the timeout expires, then
tears down the process group and verifies detachedMarker never appears
afterward; preserve detachedReaped as a failure if the marker was written before
teardown.
---
Nitpick comments:
In `@electron/agent/command-sandbox/network-proxy.ts`:
- Around line 49-69: Extract the duplicated bidirectional piping and teardown
logic into a shared pipeTunnel(client, upstream) helper. Replace the inline
piping and close/error cleanup in both handleConnect and the SOCKS5
setConnectionHandler success path with calls to pipeTunnel, passing
connection.socket for the SOCKS5 client while preserving the existing status and
failure handling.
- Around line 376-390: Update listenSocksServer so its dependency on
createSocksServer’s non-public server field is isolated to this helper,
preferably through a single localized typed adapter/accessor; use a documented
accessor instead if the library provides one. Keep the existing error handling
and bound-port resolution behavior unchanged.
In `@electron/agent/command-sandbox/plugin.ts`:
- Around line 6-20: Update the "shell.env" hook in the default exported plugin
to catch failures from the session-walk, including client.session.get, and fall
back to input.sessionID as the policy session ID before setting
WANTA_COMMAND_SANDBOX_SESSION_ID. Preserve the existing parent traversal and
call ID assignment when lookups succeed.
In `@electron/agent/manager.ts`:
- Around line 1302-1305: Extend the session disposal flow around the async
cleanup that closes commandSandboxBroker to also reclaim that session’s
command-sandbox policy, network-grant, and home directories, and remove its
entry from commandSandboxPolicyInputs. Reuse the existing session-forget cleanup
path or helper if available, ensuring cleanup runs for deleted sessions and
preserves broker shutdown behavior.
In `@electron/chat/node.ts`:
- Around line 950-956: Update the policy update flow around
updateCommandSandboxPolicy to avoid storing the method separately from its
receiver. Guard and invoke this.agent?.updateCommandSandboxPolicy directly so
the method and bound agent instance remain consistent if setAgent() changes the
agent before the call.
In `@package.json`:
- Around line 48-49: Update the `@vscode/sandbox-runtime` dependency declaration
in package.json to be included only for macOS (darwin) builds, using the package
manager’s platform-specific dependency configuration. Keep the dependency
version unchanged and leave `@pondwader/socks5-server` unaffected.
In `@scripts/verify-command-sandbox.ts`:
- Around line 31-61: Update the verification script before creating or running
the sandbox probe to exit clearly when process.platform is not "darwin", and
validate that the built wrapper at dist-electron/wanta-command-shell.js exists
before use. Report actionable messages for either unsupported platforms or
missing wrapper prerequisites, while preserving the existing macOS probe flow
and dependency checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55d3662a-fa80-431a-834b-444f64082e05
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (50)
AGENTS.mddocs/ai/README.mddocs/ai/command-sandbox-implementation.mddocs/architecture.mddocs/conventions.mddocs/key-decisions.mdelectron-builder.tselectron/agent/command-sandbox-shell.tselectron/agent/command-sandbox/broker.test.tselectron/agent/command-sandbox/broker.tselectron/agent/command-sandbox/network-address.test.tselectron/agent/command-sandbox/network-address.tselectron/agent/command-sandbox/network-proxy.test.tselectron/agent/command-sandbox/network-proxy.tselectron/agent/command-sandbox/plugin.tselectron/agent/command-sandbox/policy-reviewer.test.tselectron/agent/command-sandbox/policy-reviewer.tselectron/agent/command-sandbox/policy.test.tselectron/agent/command-sandbox/policy.tselectron/agent/command-sandbox/runtime.tselectron/agent/command-sandbox/shell-bin.test.tselectron/agent/command-sandbox/shell-bin.tselectron/agent/config.tselectron/agent/manager.tselectron/attention/common.tselectron/attention/node.tselectron/attention/policy.test.tselectron/attention/policy.tselectron/chat/node.test.tselectron/chat/node.tselectron/chat/trusted-local-access.test.tselectron/chat/trusted-local-access.tselectron/main.tselectron/runtime/common.test.tselectron/runtime/common.tspackage.jsonpatches/@vscode__sandbox-runtime@0.0.1.patchscripts/verify-command-sandbox.tssrc/components/app-shell/AppShell.tsxsrc/components/app-shell/AppShellNavigationSidebar.tsxsrc/components/app-shell/SidebarAccountControls.tsxsrc/hooks/useAttention.tssrc/i18n/app-messages.en.tssrc/i18n/app-messages.zh.tssrc/i18n/i18n.test.tssrc/routes/Chat/FullAccessConfirmDialog.tsxsrc/routes/Chat/PermissionModePicker.tsxsrc/routes/Chat/index.tsxsrc/routes/Settings/index.tsxvite.config.ts
💤 Files with no reviewable changes (1)
- src/routes/Chat/FullAccessConfirmDialog.tsx
| function hasPrivateGrant(grants: readonly PrivateNetworkGrant[], address: string, port: number): boolean { | ||
| return grants.some( | ||
| (grant) => normalizeAddress(grant.address) === address && (grant.port === undefined || grant.port === port), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Extract a shared private-network grant-matching helper. Both files independently implement the identical "does an existing grant cover this address+port" predicate. Since one enforces actual connections and the other gates whether the broker re-runs LLM review, letting these diverge could desync the broker's cached "already granted" state from what connections actually enforce.
electron/agent/command-sandbox/network-address.ts#L123-L127: replacehasPrivateGrantwith a call to a shared, exported grant-matching helper.electron/agent/command-sandbox/broker.ts#L171-L176: replacehasGrantwith the same shared helper (e.g. re-export/import fromnetwork-address.ts) instead of a local reimplementation.
📍 Affects 2 files
electron/agent/command-sandbox/network-address.ts#L123-L127(this comment)electron/agent/command-sandbox/broker.ts#L171-L176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/agent/command-sandbox/network-address.ts` around lines 123 - 127,
Extract and export a shared grant-matching helper from
electron/agent/command-sandbox/network-address.ts, replacing the local
hasPrivateGrant predicate at lines 123-127 while preserving address
normalization, wildcard ports, and exact port matching. In
electron/agent/command-sandbox/broker.ts lines 171-176, replace hasGrant with an
import/use of that shared helper and remove the duplicate local implementation.
| const httpServer = createServer() | ||
| httpServer.on("connect", (request, client, head) => { | ||
| void handleConnect(request.url, client, head, resolve, options.upstreamProxy) | ||
| }) | ||
| httpServer.on("request", (request, response) => { | ||
| void handleHttpRequest(request, response, resolve, options.upstreamProxy) | ||
| }) | ||
| const httpPort = await listenHttpServer(httpServer) | ||
|
|
||
| const socksServer = createSocksServer() | ||
| socksServer.setRulesetValidator(async (connection) => { | ||
| const target = await resolve(connection.destAddress, connection.destPort) | ||
| connection.metadata = { target } | ||
| return target !== null | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does @pondwader/socks5-serverenforce theauth option before setRulesetValidator runs, or can rulesets be bypassed without authenticating?
💡 Result:
Based on the provided documentation for @pondwader/socks5-server, the library handles the SOCKS5 protocol and provides mechanisms for both authentication and connection validation (rulesets) [1][2]. The authentication option (passed via the auth object in createServer or via setAuthHandler) is a fundamental part of the SOCKS5 handshake [1][2]. In a standard SOCKS5 implementation, the client must successfully complete the authentication phase before it can send a connection request (which includes the destination address and port) [3]. The setRulesetValidator function is specifically designed to validate these connection requests (e.g., checking destAddress and destPort) [1][2]. Because the SOCKS5 protocol requires the authentication handshake to precede the connection request, authentication is enforced before the server processes the connection request, and therefore before any ruleset validation logic is executed [1][2][3]. There is no evidence in the provided documentation that rulesets can be bypassed without authenticating. If authentication fails, the connection is rejected by the server during the handshake phase, preventing the client from reaching the state where a connection request—and thus ruleset validation—would occur [1][2][3]. Users have granular control, but they must implement the handlers correctly to maintain security [1].
Citations:
- 1: https://www.npmjs.com/package/@pondwader/socks5-server
- 2: https://npmx.dev/package/@pondwader/socks5-server
- 3: https://seyare.org/en/proxy/socks5-proxy/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first
ast-grep outline electron/agent/command-sandbox/network-proxy.ts --view expanded || true
echo
echo "---- relevant file slice ----"
sed -n '1,160p' electron/agent/command-sandbox/network-proxy.ts
echo
echo "---- auth references ----"
rg -n "WANTA_COMMAND_SANDBOX_AUTH|Proxy-Authorization|createSocksServer|auth:" electron/agent/command-sandbox/network-proxy.ts electron/agent/command-sandbox -SRepository: oomol-lab/wanta
Length of output: 9022
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- usages of network proxies ----"
rg -n "startCommandSandboxNetworkProxies|httpPort|socksPort|WANTA_COMMAND_SANDBOX_AUTH|Proxy-Authorization" electron/agent/command-sandbox -S
echo
echo "---- runtime slice around auth/env propagation ----"
sed -n '1,140p' electron/agent/command-sandbox/runtime.ts
echo
echo "---- any proxy auth helper in this file ----"
sed -n '200,260p' electron/agent/command-sandbox/network-proxy.tsRepository: oomol-lab/wanta
Length of output: 10024
Require auth on both local proxy listeners.
Any local process can connect to these ports and reuse this sandbox’s approved network access. Gate SOCKS5 with auth and require Proxy-Authorization on the HTTP/CONNECT path using the existing session secret.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/agent/command-sandbox/network-proxy.ts` around lines 34 - 48,
Require authentication on both listeners created in the network proxy setup:
configure createSocksServer to validate the existing session secret through
SOCKS5 auth, and update the HTTP request and CONNECT handling around
handleHttpRequest and handleConnect to require a valid Proxy-Authorization
header using that same secret. Reject unauthenticated connections before
invoking resolve or forwarding requests.
| async function connectTarget( | ||
| target: ApprovedNetworkTarget, | ||
| configuration: UpstreamProxyConfiguration | undefined, | ||
| preferHttps: boolean, | ||
| ): Promise<Duplex> { | ||
| const parentProxy = selectUpstreamProxy(configuration, target, preferHttps) | ||
| if (!parentProxy) { | ||
| return new Promise((resolve, reject) => { | ||
| const socket = connect({ host: target.address, port: target.port, family: target.family }) | ||
| socket.once("connect", () => resolve(socket)) | ||
| socket.once("error", reject) | ||
| }) | ||
| } | ||
| const proxySocket = await connectProxy(parentProxy) | ||
| const destination = `${formatHost(target.address)}:${target.port}` | ||
| const authorization = proxyAuthorizationHeader(parentProxy) | ||
| proxySocket.write( | ||
| [ | ||
| `CONNECT ${destination} HTTP/1.1`, | ||
| `Host: ${destination}`, | ||
| ...(authorization ? [`Proxy-Authorization: ${authorization}`] : []), | ||
| "Connection: keep-alive", | ||
| "", | ||
| "", | ||
| ].join("\r\n"), | ||
| ) | ||
| return await waitForConnectResponse(proxySocket) | ||
| } | ||
|
|
||
| function connectProxy(proxy: URL): Promise<Duplex> { | ||
| return new Promise((resolve, reject) => { | ||
| if (proxy.protocol === "https:") { | ||
| const socket = connectTls({ host: proxy.hostname, port: proxyPort(proxy), servername: proxy.hostname }) | ||
| socket.once("secureConnect", () => resolve(socket)) | ||
| socket.once("error", reject) | ||
| return | ||
| } | ||
| const socket = connect({ host: proxy.hostname, port: proxyPort(proxy) }) | ||
| socket.once("connect", () => resolve(socket)) | ||
| socket.once("error", reject) | ||
| }) | ||
| } | ||
|
|
||
| function waitForConnectResponse(socket: Duplex): Promise<Duplex> { | ||
| return new Promise((resolve, reject) => { | ||
| let response = Buffer.alloc(0) | ||
| const onData = (chunk: Buffer) => { | ||
| response = Buffer.concat([response, chunk]) | ||
| if (response.length > 16_384) { | ||
| cleanup() | ||
| socket.destroy() | ||
| reject(new Error("Upstream proxy response is too large.")) | ||
| return | ||
| } | ||
| const headerEnd = response.indexOf("\r\n\r\n") | ||
| if (headerEnd === -1) return | ||
| cleanup() | ||
| const statusLine = response.subarray(0, response.indexOf("\r\n")).toString("ascii") | ||
| if (!/^HTTP\/1\.[01] 2\d\d(?: |$)/u.test(statusLine)) { | ||
| socket.destroy() | ||
| reject(new Error("Upstream proxy rejected the connection.")) | ||
| return | ||
| } | ||
| const remaining = response.subarray(headerEnd + 4) | ||
| if (remaining.length > 0) socket.unshift(remaining) | ||
| resolve(socket) | ||
| } | ||
| const onError = (error: Error) => { | ||
| cleanup() | ||
| reject(error) | ||
| } | ||
| const onClose = () => { | ||
| cleanup() | ||
| reject(new Error("Upstream proxy closed before establishing the tunnel.")) | ||
| } | ||
| const cleanup = () => { | ||
| socket.off("data", onData) | ||
| socket.off("error", onError) | ||
| socket.off("close", onClose) | ||
| } | ||
| socket.on("data", onData) | ||
| socket.once("error", onError) | ||
| socket.once("close", onClose) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeouts on outbound connect / upstream CONNECT negotiation.
connect() in connectTarget (Line 210), connectTls/connect in connectProxy (234-241), and the data/error/close wait in waitForConnectResponse (245-286) all rely solely on the OS/library default timeout (which for TCP connect() can be very long). A slow or unresponsive target or upstream proxy will hang the sandboxed command's network call indefinitely, blocking the whole command.
🕒 Suggested fix
function connectProxy(proxy: URL): Promise<Duplex> {
return new Promise((resolve, reject) => {
if (proxy.protocol === "https:") {
const socket = connectTls({ host: proxy.hostname, port: proxyPort(proxy), servername: proxy.hostname })
+ socket.setTimeout(10_000, () => socket.destroy(new Error("Timed out connecting to upstream proxy.")))
socket.once("secureConnect", () => resolve(socket))
socket.once("error", reject)
return
}
const socket = connect({ host: proxy.hostname, port: proxyPort(proxy) })
+ socket.setTimeout(10_000, () => socket.destroy(new Error("Timed out connecting to upstream proxy.")))
socket.once("connect", () => resolve(socket))
socket.once("error", reject)
})
}Apply the same pattern to the direct-connect branch in connectTarget and to the response wait in waitForConnectResponse.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/agent/command-sandbox/network-proxy.ts` around lines 202 - 286, Add
explicit timeouts to all outbound connection and proxy negotiation paths: the
direct socket created in connectTarget, both socket variants in connectProxy,
and the response wait managed by waitForConnectResponse. Reuse the existing
timeout pattern or shared timeout configuration, and ensure expiration cleans up
listeners/sockets and rejects with an appropriate timeout error instead of
leaving the sandboxed command hanging.
| const environment = await buildCommandSandboxEnvironment(policy, { | ||
| LANG: "en_US.UTF-8", | ||
| PATH: "/usr/bin:/bin", | ||
| OO_API_KEY: "secret", | ||
| WANTA_BROWSER_CONTROL_TOKEN: "secret", | ||
| WANTA_NODE_BIN: "/Applications/Wanta.app/Contents/MacOS/Wanta", | ||
| }) | ||
|
|
||
| expect(environment.HOME).toBe(policy.homeDir) | ||
| expect(environment.PATH).toBe("/usr/bin:/bin") | ||
| expect(environment.LANG).toBe("en_US.UTF-8") | ||
| expect(environment.WANTA_NODE_BIN).toContain("Wanta") | ||
| expect(environment.OO_API_KEY).toBeUndefined() | ||
| expect(environment.WANTA_BROWSER_CONTROL_TOKEN).toBeUndefined() | ||
| expect(await readFile(path.join(store.pathForSession("session-1")), "utf8")).not.toContain("secret") | ||
| }) | ||
|
|
||
| it("builds a direct environment with the real home without sidecar credentials", () => { | ||
| const environment = buildDirectCommandEnvironment({ | ||
| HOME: "/managed/home", | ||
| HTTP_PROXY: "http://127.0.0.1:7890", | ||
| OO_API_KEY: "secret", | ||
| OPENCODE_SERVER_PASSWORD: "secret", | ||
| PATH: "/usr/bin:/bin", | ||
| SSH_AUTH_SOCK: "/tmp/ssh-agent", | ||
| }) | ||
|
|
||
| expect(environment.HOME).toBe(os.homedir()) | ||
| expect(environment.PATH).toBe("/usr/bin:/bin") | ||
| expect(environment.HTTP_PROXY).toBe("http://127.0.0.1:7890") | ||
| expect(environment.SSH_AUTH_SOCK).toBe("/tmp/ssh-agent") | ||
| expect(environment.OO_API_KEY).toBeUndefined() | ||
| expect(environment.OPENCODE_SERVER_PASSWORD).toBeUndefined() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Cover every sidecar credential carrier in both environment modes.
OPENCODE_CONFIG_CONTENT carries injected provider credentials, but neither test asserts it is stripped. The sandbox test also omits OPENCODE_SERVER_PASSWORD; the direct test omits WANTA_BROWSER_CONTROL_TOKEN. Add all sentinels to both inputs and assert each is absent.
Proposed test additions
const environment = await buildCommandSandboxEnvironment(policy, {
LANG: "en_US.UTF-8",
PATH: "/usr/bin:/bin",
OO_API_KEY: "secret",
WANTA_BROWSER_CONTROL_TOKEN: "secret",
+ OPENCODE_CONFIG_CONTENT: '{"apiKey":"secret"}',
+ OPENCODE_SERVER_PASSWORD: "secret",
WANTA_NODE_BIN: "/Applications/Wanta.app/Contents/MacOS/Wanta",
})
@@
expect(environment.OO_API_KEY).toBeUndefined()
expect(environment.WANTA_BROWSER_CONTROL_TOKEN).toBeUndefined()
+ expect(environment.OPENCODE_CONFIG_CONTENT).toBeUndefined()
+ expect(environment.OPENCODE_SERVER_PASSWORD).toBeUndefined()
@@
OO_API_KEY: "secret",
+ WANTA_BROWSER_CONTROL_TOKEN: "secret",
+ OPENCODE_CONFIG_CONTENT: '{"apiKey":"secret"}',
OPENCODE_SERVER_PASSWORD: "secret",
@@
expect(environment.OO_API_KEY).toBeUndefined()
+ expect(environment.WANTA_BROWSER_CONTROL_TOKEN).toBeUndefined()
+ expect(environment.OPENCODE_CONFIG_CONTENT).toBeUndefined()
expect(environment.OPENCODE_SERVER_PASSWORD).toBeUndefined()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const environment = await buildCommandSandboxEnvironment(policy, { | |
| LANG: "en_US.UTF-8", | |
| PATH: "/usr/bin:/bin", | |
| OO_API_KEY: "secret", | |
| WANTA_BROWSER_CONTROL_TOKEN: "secret", | |
| WANTA_NODE_BIN: "/Applications/Wanta.app/Contents/MacOS/Wanta", | |
| }) | |
| expect(environment.HOME).toBe(policy.homeDir) | |
| expect(environment.PATH).toBe("/usr/bin:/bin") | |
| expect(environment.LANG).toBe("en_US.UTF-8") | |
| expect(environment.WANTA_NODE_BIN).toContain("Wanta") | |
| expect(environment.OO_API_KEY).toBeUndefined() | |
| expect(environment.WANTA_BROWSER_CONTROL_TOKEN).toBeUndefined() | |
| expect(await readFile(path.join(store.pathForSession("session-1")), "utf8")).not.toContain("secret") | |
| }) | |
| it("builds a direct environment with the real home without sidecar credentials", () => { | |
| const environment = buildDirectCommandEnvironment({ | |
| HOME: "/managed/home", | |
| HTTP_PROXY: "http://127.0.0.1:7890", | |
| OO_API_KEY: "secret", | |
| OPENCODE_SERVER_PASSWORD: "secret", | |
| PATH: "/usr/bin:/bin", | |
| SSH_AUTH_SOCK: "/tmp/ssh-agent", | |
| }) | |
| expect(environment.HOME).toBe(os.homedir()) | |
| expect(environment.PATH).toBe("/usr/bin:/bin") | |
| expect(environment.HTTP_PROXY).toBe("http://127.0.0.1:7890") | |
| expect(environment.SSH_AUTH_SOCK).toBe("/tmp/ssh-agent") | |
| expect(environment.OO_API_KEY).toBeUndefined() | |
| expect(environment.OPENCODE_SERVER_PASSWORD).toBeUndefined() | |
| const environment = await buildCommandSandboxEnvironment(policy, { | |
| LANG: "en_US.UTF-8", | |
| PATH: "/usr/bin:/bin", | |
| OO_API_KEY: "secret", | |
| WANTA_BROWSER_CONTROL_TOKEN: "secret", | |
| OPENCODE_CONFIG_CONTENT: '{"apiKey":"secret"}', | |
| OPENCODE_SERVER_PASSWORD: "secret", | |
| WANTA_NODE_BIN: "/Applications/Wanta.app/Contents/MacOS/Wanta", | |
| }) | |
| expect(environment.HOME).toBe(policy.homeDir) | |
| expect(environment.PATH).toBe("/usr/bin:/bin") | |
| expect(environment.LANG).toBe("en_US.UTF-8") | |
| expect(environment.WANTA_NODE_BIN).toContain("Wanta") | |
| expect(environment.OO_API_KEY).toBeUndefined() | |
| expect(environment.WANTA_BROWSER_CONTROL_TOKEN).toBeUndefined() | |
| expect(environment.OPENCODE_CONFIG_CONTENT).toBeUndefined() | |
| expect(environment.OPENCODE_SERVER_PASSWORD).toBeUndefined() | |
| expect(await readFile(path.join(store.pathForSession("session-1")), "utf8")).not.toContain("secret") | |
| }) | |
| it("builds a direct environment with the real home without sidecar credentials", () => { | |
| const environment = buildDirectCommandEnvironment({ | |
| HOME: "/managed/home", | |
| HTTP_PROXY: "http://127.0.0.1:7890", | |
| OO_API_KEY: "secret", | |
| WANTA_BROWSER_CONTROL_TOKEN: "secret", | |
| OPENCODE_CONFIG_CONTENT: '{"apiKey":"secret"}', | |
| OPENCODE_SERVER_PASSWORD: "secret", | |
| PATH: "/usr/bin:/bin", | |
| SSH_AUTH_SOCK: "/tmp/ssh-agent", | |
| }) | |
| expect(environment.HOME).toBe(os.homedir()) | |
| expect(environment.PATH).toBe("/usr/bin:/bin") | |
| expect(environment.HTTP_PROXY).toBe("http://127.0.0.1:7890") | |
| expect(environment.SSH_AUTH_SOCK).toBe("/tmp/ssh-agent") | |
| expect(environment.OO_API_KEY).toBeUndefined() | |
| expect(environment.WANTA_BROWSER_CONTROL_TOKEN).toBeUndefined() | |
| expect(environment.OPENCODE_CONFIG_CONTENT).toBeUndefined() | |
| expect(environment.OPENCODE_SERVER_PASSWORD).toBeUndefined() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/agent/command-sandbox/policy.test.ts` around lines 43 - 75, Update
both environment-mode tests around buildCommandSandboxEnvironment and
buildDirectCommandEnvironment to include OPENCODE_CONFIG_CONTENT,
OPENCODE_SERVER_PASSWORD, OO_API_KEY, and WANTA_BROWSER_CONTROL_TOKEN as
credential sentinels in each input. Assert every listed credential is undefined
in both resulting environments, while preserving the existing non-credential
environment assertions and secret-persistence check.
| function spawnCommand(command: string, delegateShell: string, environment: NodeJS.ProcessEnv): Promise<number> { | ||
| return new Promise((resolve, reject) => { | ||
| const child = spawn(delegateShell, ["-c", command], { | ||
| cwd: process.cwd(), | ||
| detached: true, | ||
| env: environment, | ||
| stdio: "inherit", | ||
| }) | ||
| const relay = (signal: NodeJS.Signals) => signalProcessGroup(child.pid, signal) | ||
| const onInterrupt = () => relay("SIGINT") | ||
| const onTerminate = () => relay("SIGTERM") | ||
| process.on("SIGINT", onInterrupt) | ||
| process.on("SIGTERM", onTerminate) | ||
| child.once("error", reject) | ||
| child.once("exit", (code, signal) => { | ||
| process.off("SIGINT", onInterrupt) | ||
| process.off("SIGTERM", onTerminate) | ||
| void terminateProcessGroup(child.pid).finally(() => { | ||
| if (signal) { | ||
| resolve(128 + (os.constants.signals[signal] ?? 0)) | ||
| } else { | ||
| resolve(code ?? 1) | ||
| } | ||
| }) | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Global process signal listeners accumulate across concurrent sandboxed commands.
Each spawnCommand call adds its own SIGINT/SIGTERM listeners to the shared process object and removes them on exit, but with several sessions running sandboxed commands concurrently, listener count grows with concurrency (risking MaxListenersExceededWarning) and every signal handler runs on every process signal regardless of which command it targets. This is worth hardening given concurrent-agent sessions are a first-class scenario for this app.
Based on learnings: "Treat worktree isolation and concurrent agents as first-class concerns."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/agent/command-sandbox/runtime.ts` around lines 217 - 243, Update
spawnCommand to avoid installing per-command SIGINT and SIGTERM listeners on the
shared process object; route signals through a centralized, concurrency-safe
mechanism that tracks active child processes and forwards each signal only to
its intended command. Ensure command cleanup removes its tracking state on exit
or error without affecting other concurrent sandboxed commands.
Source: Learnings
| public async updateCommandSandboxPolicy(input: CommandSandboxSessionPolicyInput): Promise<void> { | ||
| if (!this.commandSandboxPolicyStore) return | ||
| const previous = this.commandSandboxPolicyInputs.get(input.sessionId) | ||
| const persistedGrants = previous ? [] : await this.commandSandboxPolicyStore.readNetworkGrants(input.sessionId) | ||
| const next = { | ||
| ...previous, | ||
| ...input, | ||
| privateNetworkGrants: input.privateNetworkGrants ?? previous?.privateNetworkGrants ?? persistedGrants, | ||
| } | ||
| this.commandSandboxPolicyInputs.set(input.sessionId, next) | ||
| if (next.userMessage && this.commandSandboxBroker) { | ||
| this.commandSandboxBroker.setSession( | ||
| input.sessionId, | ||
| { | ||
| modelTarget: this.resolvePolicyReviewerTarget(next.model), | ||
| origin: "main", | ||
| userMessage: next.userMessage, | ||
| }, | ||
| next.privateNetworkGrants, | ||
| ) | ||
| } | ||
| await this.writeCommandSandboxPolicy(next) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize per-session policy updates; concurrent callers can lose fields and write stale policies.
updateCommandSandboxPolicy does an unguarded read-modify-write across an await (readNetworkGrants), and ChatServiceImpl invokes it from several independent paths (sendMessage, answerPermission, answerLocalAccessPermission, setPermissionMode) that can overlap for the same session. Two in-flight calls both read commandSandboxPolicyInputs.get(sessionId) before either stores, so the later set drops the earlier caller's readWritePaths/executionMode, and the two writeCommandSandboxPolicy calls can land out of order — leaving the signed on-disk policy narrower (turn dirs missing → sandbox denies writes) or wider (direct after a downgrade to sandbox) than intended.
A per-session promise chain (same pattern as queueTeamUpdate) around the merge + write closes both windows.
Separately, setSession is gated on next.userMessage: a policy sync triggered by a permission answer before the session's first prompt registers no broker context, so private-network review for that session silently denies until a message is sent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/agent/manager.ts` around lines 455 - 477, The
updateCommandSandboxPolicy method must serialize the complete per-session read,
merge, broker synchronization, and write through a per-session promise chain,
following the existing queueTeamUpdate pattern so overlapping callers cannot
lose fields or persist stale policies. Also update the broker setSession
condition to register or refresh the session when private-network grants require
it even if next.userMessage is absent, while preserving the existing reviewer
context behavior when a user message exists.
| private async prepareCommandSandbox({ | ||
| baseCommandPath, | ||
| cliPath, | ||
| workspaceDir, | ||
| }: { | ||
| baseCommandPath: string | ||
| cliPath: string | ||
| workspaceDir: string | ||
| }): Promise<{ delegateShell: string; pluginUrl: string; shellPath: string }> { | ||
| if (!this.commandSandboxPolicyStore) { | ||
| throw new Error("Command Sandbox (Preview) is not initialized.") | ||
| } | ||
| await this.commandSandboxPolicyStore.initialize() | ||
| const runtimeDir = path.join(this.options.rootDir, "command-sandbox", "runtime") | ||
| const shellPath = await ensureCommandSandboxShellBin({ | ||
| binDir: path.join(this.options.rootDir, "bin"), | ||
| cliPath, | ||
| nodeBin: process.execPath, | ||
| }) | ||
| const pluginUrl = await ensureCommandSandboxPlugin(runtimeDir) | ||
| const delegateShell = | ||
| process.env.SHELL && path.isAbsolute(process.env.SHELL) && process.env.SHELL !== shellPath | ||
| ? process.env.SHELL | ||
| : "/bin/zsh" | ||
| this.commandSandboxRuntimeReadPaths = [ | ||
| workspaceDir, | ||
| path.dirname(cliPath), | ||
| path.dirname(process.execPath), | ||
| path.dirname(shellPath), | ||
| ...baseCommandPath.split(path.delimiter), | ||
| ] | ||
| return { delegateShell, pluginUrl, shellPath } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
commandSandboxRuntimeReadPaths is only populated during startSidecar.
Any updateCommandSandboxPolicy that lands before prepareCommandSandbox (or while a runtime recovery is mid-restart) writes a signed policy with an empty runtime read set, so the sandboxed shell loses read access to node/CLI/PATH directories and commands fail until the next policy write. Consider computing these paths in the constructor (they only depend on rootDir, cliPath, execPath, plus the resolved PATH) or refusing to write a policy until they are set.
Also worth noting: process.env.SHELL is accepted as the delegate shell purely on path.isAbsolute, with no check that it is executable — a user with an exotic or stale SHELL gets a hard failure rather than the /bin/zsh fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/agent/manager.ts` around lines 602 - 634, Ensure
commandSandboxRuntimeReadPaths is initialized before any
updateCommandSandboxPolicy call, including startup and runtime recovery, using
rootDir, cliPath, process.execPath, and resolved PATH directories; otherwise
defer policy writes until populated. In prepareCommandSandbox, preserve or
refresh the same paths consistently. Validate that process.env.SHELL is an
absolute executable before selecting it as delegateShell, falling back to
/bin/zsh when invalid.
| + if (allowLoopbackOutbound) { | ||
| + profile.push('(allow network-outbound (remote ip "localhost:*"))'); | ||
| + } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
allowLoopbackOutbound opens every loopback port, not just the sandbox proxies.
(allow network-outbound (remote ip "localhost:*")) lets a sandboxed command connect to any service bound on 127.0.0.1 — the command-sandbox broker, the OpenCode sidecar's HTTP API, the app's own dev server, and any unrelated local daemon (databases, SSH forwards, other agents). That bypasses the HTTP/SOCKS5 policy enforcement the rest of this feature builds, and the private-network classifier explicitly treats loopback as out-of-band.
Since httpProxyPort/socksProxyPort are already in scope in generateSandboxProfile, scoping the rule to those two ports (localhost:<httpProxyPort> / localhost:<socksProxyPort>) preserves the intended egress path without granting blanket local access.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@patches/`@vscode__sandbox-runtime@0.0.1.patch around lines 18 - 20, The
allowLoopbackOutbound rule in generateSandboxProfile currently permits access to
every localhost port; restrict it to localhost:httpProxyPort and
localhost:socksProxyPort only. Preserve the intended proxy egress while removing
blanket loopback access, using the existing in-scope proxy port values when
constructing the network-outbound rules.
| await new Promise((resolve) => setTimeout(resolve, 1_400)) | ||
| const assertions = { | ||
| attachmentRead: result.code === 0, | ||
| attachmentWriteBlocked: (await readFile(attachment, "utf8")) === "attachment", | ||
| controlDirProtected: await probeNestedControlDirectory(store, root), | ||
| detachedReaped: !(await exists(detachedMarker)), | ||
| outsideReadBlocked: !(await exists(outsideReadMarker)), | ||
| outsideWriteBlocked: (await readFile(outside, "utf8")) === "outside-secret", | ||
| projectWrite: (await readFile(path.join(project, "created.txt"), "utf8")) === "project-ok", | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fixed 1.4 s sleep makes detachedReaped a flaky assertion.
The detached child sleeps 1 s before touching the marker; a loaded machine can push that past the 1.4 s budget, so the probe reports "reaped" for a process that simply hadn't written yet — a false pass on the most security-relevant assertion here. Polling for the marker for a bounded window (and asserting it never appears after the process group is torn down) removes the timing dependency.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/verify-command-sandbox.ts` around lines 62 - 71, Replace the fixed
1.4-second delay before constructing assertions in the verification flow with
bounded polling for detachedMarker. Ensure the probe waits until the marker
appears or the timeout expires, then tears down the process group and verifies
detachedMarker never appears afterward; preserve detachedReaped as a failure if
the marker was written before teardown.
| async function probeNestedControlDirectory(store: CommandSandboxPolicyStore, allowedRoot: string): Promise<boolean> { | ||
| try { | ||
| await store.write({ | ||
| executionMode: "sandbox", | ||
| sessionId, | ||
| readWritePaths: [allowedRoot], | ||
| runtimeReadPaths: [path.resolve("."), path.dirname(process.execPath)], | ||
| }) | ||
| } catch { | ||
| return true | ||
| } | ||
| const result = await run(process.execPath, [wrapper, "-c", `! cat ${quote(store.pathForSession(sessionId))}`], { | ||
| ...process.env, | ||
| PATH: process.env.PATH, | ||
| WANTA_COMMAND_SANDBOX_AUTH: authKey, | ||
| WANTA_COMMAND_SANDBOX_BROKER_URL: "http://127.0.0.1:9", | ||
| WANTA_COMMAND_SANDBOX_CALL_ID: "probe-control-dir", | ||
| WANTA_COMMAND_SANDBOX_DELEGATE_SHELL: "/bin/zsh", | ||
| WANTA_COMMAND_SANDBOX_POLICY_DIR: store.policyDir, | ||
| WANTA_COMMAND_SANDBOX_SESSION_ID: sessionId, | ||
| }) | ||
| return result.code === 0 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
probeNestedControlDirectory returns true without ever exercising the sandbox.
allowedRoot is root, which contains policyRoot, so store.write reliably throws the control-directory overlap error and the catch short-circuits to true. The spawned read-denial check below it is dead code, and the assertion no longer proves the sandbox blocks policy-file reads — only that the store rejects the overlapping input (already unit-tested). Point the write at a root that does not enclose the control dir so the runtime path is actually reached.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/verify-command-sandbox.ts` around lines 85 - 107, Update
probeNestedControlDirectory so store.write receives an allowedRoot that does not
contain or overlap the policy control directory, preventing the expected overlap
error from short-circuiting the probe. Preserve the existing spawned read-denial
check and ensure the function reaches run to verify sandbox enforcement rather
than merely validating store.write rejection.
2 participants
Summary
mainContext
PR #281 introduced the Command Sandbox preview and was reverted by PR #285 after the rollout appeared to make the product unusable. This branch restores the sandbox implementation on top of the current main branch for continued validation, together with the unread-attention fixes found during manual testing.
The unread issue had two related causes. The Dock badge counted unread tasks globally, while the sidebar only rendered sessions from the active team, leaving no visible route to an unread task in another team. Separately, selecting a conversation did not directly acknowledge it as viewed; clearing depended on a React visibility effect and matching renderer/main-process focus state, so reselecting an already-active conversation could leave its blue dot stuck.
Changes
Command Sandbox
Unread attention
Verification
corepack pnpm run lintcorepack pnpm run formatcorepack pnpm run ts-checkcorepack pnpm test(287 files, 2144 tests)corepack pnpm run buildgit diff --checkThe Electron development build was also launched from this branch. The renderer, main process, preload, command-shell adapter, and OpenCode sidecar started successfully.
Known external observation
Manual testing still observed TLS connection resets when reaching
llm.oomol.comandconnector.oomol.com. Those network failures are not hidden by this PR and remain separately diagnosable from the command-sandbox and unread-attention changes.