fix opencode terminal connection#353
Conversation
|
TimoWilhelm
force-pushed
the
tiwi/fix-opencode-terminal
branch
from
bde51ac to
89afb56
Compare
|
Hey @TimoWilhelm, Thanks for the PR, looks good & safe to merge if you can resolve the conflict? Thanks! |
|
/bonk fix the conflicts |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the PR @TimoWilhelm — the WebSocket proxy fix and CSP relaxation for Ghostty WASM are solid improvements. The Dockerfile HOME fix is a good catch too.
I have one correctness issue with the CSP response construction pattern and a few smaller observations.
Also noting that no tests were added for the two new code paths (WebSocket upgrade detection and CSP rewriting). These would be straightforward to add to the existing proxyToOpencode test suite.
| const csp = response.headers.get('Content-Security-Policy'); | ||
| if (csp) { | ||
| const newCsp = csp | ||
| .replace(/connect-src\s+'self'/, "connect-src 'self' data:") |
There was a problem hiding this comment.
Nit: These regexes only match connect-src 'self' and script-src 'self' exactly — if the upstream CSP ever includes additional sources (e.g., connect-src 'self' https://example.com), the replacement still works but only because it matches the prefix. However, if the directive were connect-src 'none' or used a different order, it would silently not match and the terminal would break without any visible error.
Might be worth a comment noting this is fragile / tied to the specific CSP that OpenCode emits today.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
@whoiskatrin Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>
Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>
|
New commits pushed -- |
There was a problem hiding this comment.
Thanks for the PR — the WebSocket proxy via sandbox.fetch(switchPort(...)), CSP relaxation for Ghostty WASM, and the Dockerfile HOME fix all look correct and well-motivated.
I see the previous review's suggestions (explicit status/statusText, .toLowerCase() on Upgrade header) are already incorporated in the code. A few remaining items below.
Merge conflict: The PR is currently in a conflicting state and needs a rebase.
Changeset: Since this changes the public SDK behavior (proxyToOpencode now handles WebSocket upgrades and rewrites CSP headers), a patch changeset for @cloudflare/sandbox should be added.
Test coverage: The two new code paths (WebSocket upgrade detection and CSP header rewriting) have no test coverage. The existing proxyToOpencode test suite would be a natural place to add these — a WebSocket request test asserting sandbox.fetch is called instead of containerFetch, and a test for CSP header modification. Note that createMockSandboxForProxy currently only provides containerFetch but the WebSocket path calls sandbox.fetch, so the mock will need updating too. The @cloudflare/containers module (for switchPort) will also need to be mocked in this test file.
| return sandbox.containerFetch(request, server.port); | ||
| const response = await sandbox.containerFetch(request, server.port); | ||
|
|
||
| // Modify CSP headers to allow Ghostty WASM loading (skip for WebSocket responses) |
There was a problem hiding this comment.
Nit: The parenthetical "(skip for WebSocket responses)" is misleading here. WebSocket requests already return early at line 434, so this code is never reached for WebSocket requests. The comment reads as if there's skip logic here, but there isn't.
| // Modify CSP headers to allow Ghostty WASM loading (skip for WebSocket responses) | |
| // Modify CSP headers to allow Ghostty WASM terminal loading |
| @@ -299,17 +298,17 @@ describe('proxyToOpencode', () => { | |||
| } as unknown as Sandbox; | |||
There was a problem hiding this comment.
This mock is now incomplete for the new code paths. The WebSocket branch calls sandbox.fetch(switchPort(...)), but this mock only provides containerFetch. Tests for the WebSocket path will need a fetch mock here.
Also, since the source file now imports switchPort from @cloudflare/containers, you'll need a vi.mock('@cloudflare/containers', ...) at the top of this test file (see tests/sandbox.test.ts for the pattern).
Suggested test cases to add:
- WebSocket upgrade request routes through
sandbox.fetch(notcontainerFetch) - Response with CSP header gets
connect-srcandscript-srcrewritten - Response without CSP header is returned unchanged
|
Review submitted successfully. Here's a summary of what I flagged: Review posted on PR #353 with 2 inline comments and a summary:
|
|
/bonk fix the conflicts |
There was a problem hiding this comment.
Thanks for the fix — the WebSocket proxy via sandbox.fetch(switchPort(...)), CSP relaxation for Ghostty WASM, and the Dockerfile HOME fix all make sense.
The previous review's core suggestions (explicit status/statusText, .toLowerCase() on Upgrade header) have been incorporated, which is great. A few things remain unresolved:
Merge conflict — The PR is in CONFLICTING state and needs a rebase onto main.
Missing changeset — This changes public SDK behavior (proxyToOpencode now handles WebSocket upgrades and rewrites CSP headers), so a patch changeset for @cloudflare/sandbox is needed per the project's contribution guidelines. Note: the changeset should only reference @cloudflare/sandbox, not @cloudflare/sandbox-opencode-example.
Missing test coverage — The two new code paths (WebSocket upgrade detection and CSP header rewriting) have no tests. The existing proxyToOpencode test suite is the natural place. Suggested test cases:
- WebSocket upgrade request routes through
sandbox.fetch(notcontainerFetch) - Response with CSP header gets
connect-srcandscript-srcrewritten correctly - Response without CSP header is returned unchanged
Note that createMockSandboxForProxy will need a fetch mock, and @cloudflare/containers will need to be mocked in this test file (see tests/sandbox.test.ts for the switchPort mock pattern).
I've left an inline comment on the remaining code-level issue.
| return sandbox.containerFetch(request, server.port); | ||
| const response = await sandbox.containerFetch(request, server.port); | ||
|
|
||
| // Modify CSP headers to allow Ghostty WASM loading (skip for WebSocket responses) |
There was a problem hiding this comment.
This parenthetical is misleading. WebSocket requests already return early at line 434, so this code is never reached for WebSocket responses. The comment reads as if there's skip logic here, but there isn't.
| // Modify CSP headers to allow Ghostty WASM loading (skip for WebSocket responses) | |
| // Modify CSP headers to allow Ghostty WASM terminal loading |
|
Review submitted successfully. Here's a summary of the feedback: Request changes on PR #353 with 1 inline comment:
|
|
/bonk resolve the conflicts |
There was a problem hiding this comment.
Thanks for the PR. The WebSocket proxy fix, CSP header rewriting, and Dockerfile HOME correction all look reasonable. A few items remain before this is mergeable.
Merge conflict: The PR is still in a conflicting state and needs a rebase onto main.
Missing changeset: proxyToOpencode is part of the public @cloudflare/sandbox API. Adding WebSocket handling and CSP rewriting is a behavioral change that needs a patch changeset. Per project guidelines, only reference @cloudflare/sandbox (not the example package).
Missing test coverage: The two new code paths have no tests. See inline comments for details.
Otherwise the implementation looks solid — the WebSocket path correctly mirrors the pattern in request-handler.ts, status/statusText are passed explicitly (avoiding the spread bug from the earlier review), and the .toLowerCase() on the Upgrade header is correct.
| return sandbox.containerFetch(request, server.port); | ||
| const response = await sandbox.containerFetch(request, server.port); | ||
|
|
||
| // Modify CSP headers to allow Ghostty WASM loading (skip for WebSocket responses) |
There was a problem hiding this comment.
Previous reviews already flagged this: the "(skip for WebSocket responses)" parenthetical is inaccurate. WebSocket requests return early at line 434, so this code is never reached for WebSocket requests. The comment reads as if there's skip logic here when there isn't.
| // Modify CSP headers to allow Ghostty WASM loading (skip for WebSocket responses) | |
| // Modify CSP headers to allow Ghostty WASM terminal loading |
| .replace(/connect-src\s+'self'/, "connect-src 'self' data:") | ||
| .replace(/script-src\s+'self'/, "script-src 'self' 'wasm-unsafe-eval'"); |
There was a problem hiding this comment.
These regexes are fragile — they only match the exact pattern connect-src 'self' / script-src 'self' (with flexible whitespace between the directive name and 'self'). If the upstream OpenCode CSP ever adds additional sources or changes ordering, the replacements silently won't match and the terminal will break with no visible error.
Not necessarily a blocker, but a short comment documenting this fragility and what CSP shape is expected would help future maintainers. Something like:
// These replacements assume OpenCode's current CSP format where connect-src and
// script-src contain only 'self'. If OpenCode's CSP changes, these may need updating.
| @@ -299,17 +298,17 @@ describe('proxyToOpencode', () => { | |||
| } as unknown as Sandbox; | |||
There was a problem hiding this comment.
This mock is now incomplete for the new code paths:
- The WebSocket branch calls
sandbox.fetch(switchPort(...)), but this mock only providescontainerFetch. Afetchmock is needed. - The source file now imports
switchPortfrom@cloudflare/containers, which needs to be mocked in this test file (seetests/sandbox.test.tslines 55-59 for the pattern).
Suggested test cases to add:
- WebSocket upgrade: Request with
Upgrade: websocketheader routes throughsandbox.fetch(notcontainerFetch), andswitchPortis called withserver.port. - CSP rewriting: Response with a
Content-Security-Policyheader getsconnect-srcandscript-srcrewritten with the expected additions (data:and'wasm-unsafe-eval'). - No CSP: Response without a CSP header is returned unchanged (body, status, headers all pass through).
|
Review posted successfully. Here's what I flagged on PR #353: Summary: The implementation is sound — WebSocket proxy mirrors the pattern in 3 items to address before merge:
2 inline code comments:
|
commit: |
📦 Preview BuildVersion: Install the SDK preview:
|
…minal # Conflicts: # .changeset/many-deers-dance.md
|
New commits pushed -- |
|
New commits pushed -- |
|
/bonk add a changeset, test coverage and update the openCode example in the repo from these changes |
There was a problem hiding this comment.
Thanks for the PR — the WebSocket proxy, CSP rewriting, and Dockerfile HOME fix all look correct. The core implementation is solid: WebSocket handling mirrors request-handler.ts, status/statusText are passed explicitly (avoiding the spread-doesn't-copy-getters issue), and the Upgrade header comparison is properly case-insensitive.
A few items remain from prior reviews that should be addressed before merge:
-
Misleading comment on line 482 — The
(skip for WebSocket responses)parenthetical is inaccurate since WebSocket requests return early at line 460. Previous reviews suggested this fix but it hasn't been applied yet. -
Missing changeset —
proxyToOpencodeis public API; adding WebSocket handling and CSP rewriting is a behavioral change that needs a patch changeset for@cloudflare/sandboxonly (not the example package). -
Missing test coverage — The two new code paths (WebSocket upgrade and CSP rewriting) have no tests. The mock at line 341 also needs updating: it lacks a
fetchmethod (needed by the WebSocket path), and@cloudflare/containersneeds to be mocked forswitchPort(seetests/sandbox.test.ts:10-16for the pattern). Three test cases would give good coverage:- WebSocket upgrade request routes through
sandbox.fetch+switchPort, notcontainerFetch - Response with CSP header gets
connect-srcandscript-srcrewritten - Response without CSP header passes through unchanged
- WebSocket upgrade request routes through
| return proxyToOpencodeServer(request, sandbox, server); | ||
| const response = await proxyToOpencodeServer(request, sandbox, server); | ||
|
|
||
| // Modify CSP headers to allow Ghostty WASM loading (skip for WebSocket responses) |
There was a problem hiding this comment.
This parenthetical is inaccurate — WebSocket requests already returned at line 460, so this code is never reached for WebSocket responses. There's no skip logic here.
| // Modify CSP headers to allow Ghostty WASM loading (skip for WebSocket responses) | |
| // Modify CSP headers to allow Ghostty WASM terminal loading |
| const newCsp = csp | ||
| .replace(/connect-src\s+'self'/, "connect-src 'self' data:") | ||
| .replace(/script-src\s+'self'/, "script-src 'self' 'wasm-unsafe-eval'"); |
There was a problem hiding this comment.
These regexes only match when connect-src / script-src contain exactly 'self' (possibly with flexible whitespace). If OpenCode ever adds additional sources to these directives or changes ordering, the replacements silently won't match and the terminal will break with no visible error.
A short comment documenting this assumption would help future maintainers:
| const newCsp = csp | |
| .replace(/connect-src\s+'self'/, "connect-src 'self' data:") | |
| .replace(/script-src\s+'self'/, "script-src 'self' 'wasm-unsafe-eval'"); | |
| // These replacements assume OpenCode's current CSP format where connect-src and | |
| // script-src directives start with only 'self'. If OpenCode's CSP changes, update these. | |
| const newCsp = csp | |
| .replace(/connect-src\s+'self'/, "connect-src 'self' data:") | |
| .replace(/script-src\s+'self'/, "script-src 'self' 'wasm-unsafe-eval'"); |
| @@ -342,17 +341,17 @@ describe('proxyToOpencode', () => { | |||
| } as unknown as Sandbox; | |||
There was a problem hiding this comment.
This mock is incomplete for the new code paths:
- The WebSocket branch calls
sandbox.fetch(switchPort(...)), but this mock only providescontainerFetch. Afetchmock is needed. - The source now imports
switchPortfrom@cloudflare/containers, which must be mocked in this test file. Seetests/sandbox.test.ts:10-16for the pattern.
Suggested test cases:
- WebSocket upgrade: Request with
Upgrade: websocketroutes throughsandbox.fetch(notcontainerFetch) - CSP rewriting: Response with
Content-Security-Policyheader getsconnect-srcandscript-srcrewritten - No CSP: Response without CSP header passes through unchanged (body, status, headers intact)
|
Review posted successfully on PR #353 with 3 inline comments and a summary. Here's what I flagged: Implementation assessment: The core changes are solid — WebSocket proxy mirrors 3 items to address before merge:
Additionally noted the need for a patch changeset for |
|
@TimoWilhelm bonk seems to be having some trouble, instead, could you please push a changeset, some test coverage and update the OpenCode example now this method is async? Everything else looks good to merge after those changes! Thanks for the PR :) |
3 participants
Fix WebSocket Proxy + CSP headers for terminal support in opencode web ui
Also fixes the home path to fix file listing