Repository navigation
Fix SSH remote CLI wrapper and proxy follow-ups - #1596
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a streaming HTTP header rewriter (RemoteLoopbackHTTPRequestStreamRewriter) wired into the proxy tunnel to incrementally rewrite loopback HTTP request headers across chunks and handle EOF; introduces CLI-invocation routing helpers (shouldRunCLIForInvocation/isDaemonEntryCommand) and changes proxy EOF event payload behavior to avoid duplicating payloads. Changes
Sequence Diagram(s)sequenceDiagram
participant Upstream as Upstream Stream
participant ProxySession as ProxySession
participant Rewriter as RemoteLoopbackHTTPRequest<br/>StreamRewriter
participant Remote as Remote Host
rect rgba(100, 150, 200, 0.5)
Note over Upstream,Remote: Streaming Header Rewrite Flow
end
Upstream->>ProxySession: Data Chunk 1 (partial HTTP request)
ProxySession->>Rewriter: rewriteNextChunk(chunk1, eof: false)
Rewriter->>Rewriter: Buffer partial headers
Rewriter-->>ProxySession: No output (headers incomplete)
Upstream->>ProxySession: Data Chunk 2 (header continuation + body)
ProxySession->>Rewriter: rewriteNextChunk(chunk2, eof: false)
Rewriter->>Rewriter: Combine buffer, rewrite headers
Rewriter-->>ProxySession: Rewritten data (Host/Origin/Referer updated)
ProxySession->>Remote: Forward rewritten data (eof: false)
Upstream->>ProxySession: EOF
ProxySession->>Rewriter: rewriteNextChunk(empty, eof: true)
Rewriter-->>ProxySession: Flush buffered state / trailers
ProxySession->>Remote: Forward EOF (with any trailing data)
sequenceDiagram
participant Wrapper as Wrapper Binary
participant Main as main.go
participant Decision as shouldRunCLIForInvocation
participant CLI as CLI Handler
participant Server as Server Handler
rect rgba(150, 150, 100, 0.5)
Note over Wrapper,Server: CLI Dispatch Decision Flow
end
Wrapper->>Main: Invocation (argv0, args)
Main->>Decision: Check argv0 and args
alt Dispatch to CLI
Decision-->>Main: true
Main->>CLI: Run CLI handler
CLI-->>Wrapper: CLI output
else Run as server
Decision-->>Main: false
Main->>Server: Start server path
Server-->>Wrapper: Server starts normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
daemon/remote/cmd/cmuxd-remote/main.go (1)
1031-1036:⚠️ Potential issue | 🟠 MajorEmpty EOF payload can drop buffered tail bytes in downstream rewriters.
At Line 1035, forcing
DataBase64to empty may break consumers that flush pending buffered bytes on EOF payload. This introduces a data-loss edge case for partial-header streams.Suggested fix (no duplication, preserves EOF tail compatibility)
- if len(data) > 0 { - _ = s.frameWriter.writeEvent(rpcEvent{ - Event: "proxy.stream.data", - StreamID: streamID, - DataBase64: base64.StdEncoding.EncodeToString(data), - }) - } + if len(data) > 0 && readErr == nil { + _ = s.frameWriter.writeEvent(rpcEvent{ + Event: "proxy.stream.data", + StreamID: streamID, + DataBase64: base64.StdEncoding.EncodeToString(data), + }) + } if readErr == io.EOF { + eofPayload := "" + if len(data) > 0 { + eofPayload = base64.StdEncoding.EncodeToString(data) + } _ = s.frameWriter.writeEvent(rpcEvent{ Event: "proxy.stream.eof", StreamID: streamID, - DataBase64: "", + DataBase64: eofPayload, }) } else if !errors.Is(readErr, net.ErrClosed) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@daemon/remote/cmd/cmuxd-remote/main.go` around lines 1031 - 1036, The EOF handling in the read loop sets rpcEvent.DataBase64 to an empty string which can drop buffered tail bytes; instead, when readErr == io.EOF use the actual buffered payload (the same variable used for prior chunks) and include it (base64-encoded) in the rpcEvent passed to s.frameWriter.writeEvent so downstream rewriters receive any trailing bytes on EOF; modify the EOF branch in the read loop that calls s.frameWriter.writeEvent(rpcEvent{...}) to populate DataBase64 from the existing payload buffer rather than forcing "" while preserving StreamID and Event "proxy.stream.eof".
🧹 Nitpick comments (1)
cmuxTests/GhosttyConfigTests.swift (1)
870-902: Add an EOF-flush regression case for incomplete headers.This test only exercises the “headers complete before EOF” path (
eof: falseon both chunks). Please add a case where EOF arrives before\r\n\r\nand assert buffered bytes are flushed exactly once on EOF.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 870 - 902, Add a regression test that covers EOF arriving before the header terminator: create a new test (e.g., testBuffersFlushOnEOFWhenHeadersIncomplete) that uses RemoteLoopbackHTTPRequestStreamRewriter and feeds it two chunks where the second chunk does NOT contain the full "\r\n\r\n" sequence and call rewriteNextChunk(firstChunk, eof:false) then rewriteNextChunk(secondChunk, eof:true); assert the first call returns empty, the second call returns the buffered bytes exactly once (contains the rewritten Host/Origin/Referer replacements you expect), that the output includes the remaining body data as a suffix, and that no additional bytes are emitted on subsequent calls — this ensures rewriteNextChunk(...) flushes buffered bytes once when EOF is true even if headers were incomplete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 1402-1435: The code buffers incoming bytes in
RemoteLoopbackHTTPRequestStreamRewriter.rewriteNextChunk into pendingHeaderBytes
until it sees "\r\n\r\n", which can grow unbounded for malformed input; add a
max header size constant (e.g., maxHeaderBytes = 64 * 1024) and after appending
data check if pendingHeaderBytes.count > maxHeaderBytes, and if so set
hasForwardedHeaders = true, take payload = pendingHeaderBytes, clear
pendingHeaderBytes, and return
RemoteLoopbackHTTPRequestRewriter.rewriteIfNeeded(data: payload, aliasHost:
aliasHost) to flush and stop further buffering; keep references to
pendingHeaderBytes, rewriteNextChunk, hasForwardedHeaders, and
RemoteLoopbackHTTPRequestRewriter in the change.
---
Outside diff comments:
In `@daemon/remote/cmd/cmuxd-remote/main.go`:
- Around line 1031-1036: The EOF handling in the read loop sets
rpcEvent.DataBase64 to an empty string which can drop buffered tail bytes;
instead, when readErr == io.EOF use the actual buffered payload (the same
variable used for prior chunks) and include it (base64-encoded) in the rpcEvent
passed to s.frameWriter.writeEvent so downstream rewriters receive any trailing
bytes on EOF; modify the EOF branch in the read loop that calls
s.frameWriter.writeEvent(rpcEvent{...}) to populate DataBase64 from the existing
payload buffer rather than forcing "" while preserving StreamID and Event
"proxy.stream.eof".
---
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 870-902: Add a regression test that covers EOF arriving before the
header terminator: create a new test (e.g.,
testBuffersFlushOnEOFWhenHeadersIncomplete) that uses
RemoteLoopbackHTTPRequestStreamRewriter and feeds it two chunks where the second
chunk does NOT contain the full "\r\n\r\n" sequence and call
rewriteNextChunk(firstChunk, eof:false) then rewriteNextChunk(secondChunk,
eof:true); assert the first call returns empty, the second call returns the
buffered bytes exactly once (contains the rewritten Host/Origin/Referer
replacements you expect), that the output includes the remaining body data as a
suffix, and that no additional bytes are emitted on subsequent calls — this
ensures rewriteNextChunk(...) flushes buffered bytes once when EOF is true even
if headers were incomplete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96a7944d-9600-44f1-8096-50626ebc5537
📒 Files selected for processing (4)
Sources/Workspace.swiftcmuxTests/GhosttyConfigTests.swiftdaemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.go
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:1414">
P1: Add a size limit for `pendingHeaderBytes` before continuing to buffer. If the header delimiter never arrives, this currently accumulates data without bound and can exhaust memory while blocking upstream forwarding.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Author
|
Addressed the follow-up review notes in d8a968c.
Local verification:
|
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…p-review-fixes Fix SSH remote CLI wrapper and proxy follow-ups
2 of 4 tasks
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by cubic
Fixes CLI wrapper dispatch for the SSH remote binary and makes loopback proxy header rewriting robust for streamed requests. Prevents header corruption and removes duplicate EOF payloads in proxy streams.
cmuxd-remotenow runs in CLI mode when invoked viacmuxor without a daemon entry subcommand (serve,version,cli).proxy.stream.dataandproxy.stream.eof(EOF now sends emptydata_base64).Written for commit d8a968c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests