Bridge sidecar proxy and align sandbox runtime paths - #288
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds runtime layout resolution for Electron-managed command-sandbox paths, including packaged and development environments. Passes the resolved layout into Sequence Diagram(s)sequenceDiagram
participant ElectronMain
participant AgentManager
participant CommandSandbox
participant OpencodeSidecar
participant SystemProxy
ElectronMain->>AgentManager: provide runtime layout
AgentManager->>CommandSandbox: write session policy with conditional runtime paths
OpencodeSidecar->>SystemProxy: query host proxy configuration
SystemProxy-->>OpencodeSidecar: return proxy environment and summary
OpencodeSidecar->>OpencodeSidecar: merge missing proxy variables
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches✨ Simplify code
Comment |
5aae064 to
0686b21
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
electron/agent/sidecar.test.ts (2)
145-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a SOCKS-only macOS case.
The fixture enables all three proxies, so the
httpsProxy ?? httpProxy ?? socksProxyfallback inparseMacSystemProxyis never exercised for the SOCKS branch (socks5://URL). A case with onlySOCKSEnable : 1would lock that behavior in.🤖 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/sidecar.test.ts` around lines 145 - 217, Add a SOCKS-only test case in the “macOS system proxy environment” suite using a fixture with only SOCKSEnable enabled and the SOCKS host/port configured. Assert that parseMacSystemProxy returns the expected socks5:// proxy environment, including the SOCKS-derived ALL_PROXY value, so the socksProxy fallback path is covered.
219-275: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider a credentialed
ProxyServercase.Adding a fixture like
user:secret@127.0.0.1:7897would cover the redaction path in thesummary, which is where the leak flagged inelectron/agent/sidecar.ts(Line 109) surfaces.🤖 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/sidecar.test.ts` around lines 219 - 275, Add a credentialed ProxyServer fixture to the Windows system proxy tests around parseWindowsSystemProxy, using a user/password proxy URL and asserting that summary.ProxyServer redacts the credentials while the generated environment retains the usable proxy value. Ensure the test covers the redaction path in parseWindowsSystemProxy without changing unrelated proxy behavior.electron/agent/sidecar.ts (2)
194-201: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueKey lookups are case-sensitive while the line match is not.
The regex matches value names case-insensitively but stores
match[1]verbatim, sovalues.get("ProxyEnable")misses any output whose casing differs. Normalize the key when storing.♻️ Suggested tweak
- if (match?.[1] && match[2]) { - values.set(match[1], match[2]) - } + if (match?.[1] && match[2]) { + const key = ["ProxyEnable", "ProxyServer", "ProxyOverride", "AutoConfigURL"].find( + (name) => name.toLowerCase() === match[1].toLowerCase(), + ) + if (key) values.set(key, match[2]) + }🤖 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/sidecar.ts` around lines 194 - 201, Normalize the captured registry value name before storing it in the values map within parseWindowsSystemProxy, so case-variant matches resolve through the existing canonical lookups such as values.get("ProxyEnable"). Preserve the current parsing and value handling.
344-365: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFailure path silently yields no proxy diagnostics distinction.
systemProxy()returning{ env: {}, summary: { error } }is fine formergeSystemProxyEnvironment(no-op), and the 1s timeout bounds startup. Consider logging atwarnlevel rather than only embedding the message in the trace-level network summary, so proxy-detection failures are visible when users report connectivity issues.🤖 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/sidecar.ts` around lines 344 - 365, Update the catch path in systemProxy to emit a warn-level log containing the proxy-detection error and relevant context before returning the existing { env: {}, summary: { error } } fallback. Preserve the current fallback shape and timeout behavior.
🤖 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/event-translator.ts`:
- Around line 408-417: Update the opencode session error logging call in the
event translator to remove the raw p.error object from the diagnostic payload.
Retain only the necessary structured fields, such as the sanitized error message
and sessionId, while preserving the existing error severity and diagnostic event
context.
In `@electron/agent/manager.ts`:
- Line 71: Update the sandbox-enabled manager configuration around runtimeLayout
and commandSandboxCliPath so callers cannot lose access to the command CLI and
process.execPath roots when runtimeLayout is absent. Either require
runtimeLayout whenever sandboxing is enabled, or preserve the prior runtime
roots as an explicit fallback, and add a regression test covering a
sandbox-enabled manager without runtimeLayout.
In `@electron/agent/sidecar.ts`:
- Around line 109-118: Update redactProxyValue so the parsed-URL path only
returns the URL when url.host is non-empty; otherwise use the existing regex
fallback to redact credentials in scheme-less proxy values. Preserve the current
behavior for valid URLs with a host and malformed values.
---
Nitpick comments:
In `@electron/agent/sidecar.test.ts`:
- Around line 145-217: Add a SOCKS-only test case in the “macOS system proxy
environment” suite using a fixture with only SOCKSEnable enabled and the SOCKS
host/port configured. Assert that parseMacSystemProxy returns the expected
socks5:// proxy environment, including the SOCKS-derived ALL_PROXY value, so the
socksProxy fallback path is covered.
- Around line 219-275: Add a credentialed ProxyServer fixture to the Windows
system proxy tests around parseWindowsSystemProxy, using a user/password proxy
URL and asserting that summary.ProxyServer redacts the credentials while the
generated environment retains the usable proxy value. Ensure the test covers the
redaction path in parseWindowsSystemProxy without changing unrelated proxy
behavior.
In `@electron/agent/sidecar.ts`:
- Around line 194-201: Normalize the captured registry value name before storing
it in the values map within parseWindowsSystemProxy, so case-variant matches
resolve through the existing canonical lookups such as
values.get("ProxyEnable"). Preserve the current parsing and value handling.
- Around line 344-365: Update the catch path in systemProxy to emit a warn-level
log containing the proxy-detection error and relevant context before returning
the existing { env: {}, summary: { error } } fallback. Preserve the current
fallback shape and timeout behavior.
🪄 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: 96a424f3-befc-45d8-a090-422aaf259609
📒 Files selected for processing (8)
electron/agent/event-translator.tselectron/agent/manager.test.tselectron/agent/manager.tselectron/agent/runtime-layout.test.tselectron/agent/runtime-layout.tselectron/agent/sidecar.test.tselectron/agent/sidecar.tselectron/main.ts
| logDiagnostic( | ||
| "opencode-event-stream", | ||
| "opencode session error", | ||
| { | ||
| error: p.error, | ||
| message: errorMessage(p.error), | ||
| sessionId: p.sessionID, | ||
| }, | ||
| "error", | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'diagnostics-log.ts' -x cat -n {}Repository: oomol-lab/wanta
Length of output: 8862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== event-translator excerpt =="
sed -n '380,435p' electron/agent/event-translator.ts
echo
echo "== diagnostics-log serializer excerpt =="
sed -n '1,260p' electron/agent/diagnostics-log.tsRepository: oomol-lab/wanta
Length of output: 1750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== diagnostics-related files =="
fd -t f 'diagnostics-log.ts' -t f 'diagnostics*.ts' -t f 'log*.ts' electron
echo
echo "== errorMessage references =="
rg -n "function errorMessage|const errorMessage|errorMessage\(" electron/agent electron -g '!**/dist/**' -g '!**/build/**'
echo
echo "== opencode session error context =="
rg -n "session.error|opencode session error|agentError" electron/agent -g '!**/dist/**' -g '!**/build/**'Repository: oomol-lab/wanta
Length of output: 3589
Avoid logging the raw OpenCode error object in electron/agent/event-translator.ts. The diagnostics logger recursively serializes object fields (including nested Error.stack/cause) and only truncates strings, so upstream response contents can still leak; log only the structured fields you need.
🤖 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/event-translator.ts` around lines 408 - 417, Update the
opencode session error logging call in the event translator to remove the raw
p.error object from the diagnostic payload. Retain only the necessary structured
fields, such as the sanitized error message and sessionId, while preserving the
existing error severity and diagnostic event context.
| bundledToolRuntimePath?: string | ||
| /** App 私有根目录（userData 下）：workspace / oo-store / isolation 都在其下。 */ | ||
| rootDir: string | ||
| runtimeLayout?: WantaRuntimeLayout |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve runtime read roots when the optional layout is absent.
A caller with commandSandboxCliPath but no runtimeLayout now grants only the generated shell directory and PATH entries; it no longer grants the command CLI or process.execPath roots. The sandbox wrapper can therefore be denied access to its own managed runtime. Require runtimeLayout for sandbox-enabled managers, or retain the previous roots as a fallback and add a regression test.
Proposed fallback
this.commandSandboxRuntimeReadPaths = [
workspaceDir,
path.dirname(shellPath),
- ...(this.options.runtimeLayout?.managedRuntimeReadPaths ?? []),
+ ...(this.options.runtimeLayout?.managedRuntimeReadPaths ?? [
+ path.dirname(cliPath),
+ path.dirname(process.execPath),
+ ]),
...baseCommandPath.split(path.delimiter),
]Also applies to: 629-635
🤖 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` at line 71, Update the sandbox-enabled manager
configuration around runtimeLayout and commandSandboxCliPath so callers cannot
lose access to the command CLI and process.execPath roots when runtimeLayout is
absent. Either require runtimeLayout whenever sandboxing is enabled, or preserve
the prior runtime roots as an explicit fallback, and add a regression test
covering a sandbox-enabled manager without runtimeLayout.
| function redactProxyValue(value: string): string { | ||
| try { | ||
| const url = new URL(value) | ||
| url.username = url.username ? "[redacted]" : "" | ||
| url.password = url.password ? "[redacted]" : "" | ||
| return url.toString() | ||
| } catch { | ||
| return value.replace(/\/\/[^/@\s]+@/u, "//[redacted]@") | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node -e '
const v = "user:pass@proxy.example:8080";
const u = new URL(v);
u.username = "[redacted]"; u.password = "[redacted]";
console.log("parsed:", u.protocol, JSON.stringify(u.host), "->", u.toString());
'Repository: oomol-lab/wanta
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sidecar.ts outline =="
ast-grep outline electron/agent/sidecar.ts --view expanded || true
echo
echo "== relevant section =="
sed -n '90,140p' electron/agent/sidecar.ts
echo
echo "== search for proxy value sources/usages =="
rg -n "ProxyServer|redactProxyValue|proxy" electron/agent -g '!**/dist/**' -g '!**/build/**' || trueRepository: oomol-lab/wanta
Length of output: 11016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '194,310p' electron/agent/sidecar.ts
echo
echo "== tests around Windows proxy redaction =="
sed -n '219,320p' electron/agent/sidecar.test.ts
echo
echo "== focused search for redactProxyValue test coverage =="
rg -n "redactProxyValue|redactWindowsProxyServer|AutoConfigURL|ProxyServer" electron/agent/sidecar.test.tsRepository: oomol-lab/wanta
Length of output: 7730
Redact scheme-less proxy credentials when the parsed URL has no host.
new URL("user:pass@proxy.example:8080") stays opaque (host === ""), so the username/password assignments do nothing and the credentials reach the summary unchanged. Fall back to the regex redaction whenever url.host is empty.
🤖 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/sidecar.ts` around lines 109 - 118, Update redactProxyValue so
the parsed-URL path only returns the URL when url.host is non-empty; otherwise
use the existing regex fallback to redact credentials in scheme-less proxy
values. Preserve the current behavior for valid URLs with a host and malformed
values.
1 participant
Summary
Verification