Repository navigation
Conversation
|
@kays0x is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create an environment for this repo. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSocketClient now caches a successful auth command and transparently replays it after detecting a peer-closed connection; send() guards reestablishment per request. PTY resize propagation errors are now caught and logged. A regression test verifies reconnect-and-resend behavior across multiple SIGWINCH signals. ChangesSocket Reconnection with Auth Replay and PTY Resize Error Logging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes a real, reproducible bug where
Confidence Score: 5/5Safe to merge — the reconnect logic is contained within SocketClient.send(), the isReplayingReconnectAuth sentinel removed by prior-review feedback is gone, and the regression test exercises the exact failure scenario end-to-end. The change is narrow and well-tested: it adds a poll-based dead-connection check and a clean reconnect+auth-replay path, validated by a purpose-built regression test that was red before the fix and green after. The only open gap (sendOneWay not calling reestablishConnectionIfPeerClosed) was flagged in a prior review and is not new. No new defects are introduced on the changed path. CLI/cmux.swift — specifically sendOneWay, which still bypasses the reconnect guard (pre-existing open comment). Important Files Changed
Sequence DiagramsequenceDiagram
participant SH as SIGWINCH Handler
participant SC as SocketClient.send()
participant RC as reestablishConnectionIfPeerClosed()
participant PS as performSend()
participant SS as Control Socket Server
SH->>SC: sendV2("workspace.remote.pty_resize")
SC->>RC: reestablishConnectionIfPeerClosed(responseTimeout)
RC->>RC: "connectionAppearsOpen() poll → POLLHUP set → false"
RC->>SC: close() then connect()
RC->>PS: performSend("auth password") via direct call
PS->>SS: "auth password newline on new connection"
SS-->>PS: "OK"
PS-->>RC: "OK"
RC-->>SC: return success
SC->>PS: performSend("workspace.remote.pty_resize params")
PS->>SS: "write RPC"
SS-->>PS: response
PS-->>SC: response string
SC-->>SH: success
Reviews (4): Last reviewed commit: "test: fail instead of crash when ssh-pty..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🤖 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 `@cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift`:
- Around line 3702-3760: The test currently stops after counting resize RPCs but
doesn't wait for the child Process to exit; after signalling closeBridge, call
process.waitUntilExit() and then assert the process exited cleanly by checking
process.terminationStatus == 0 (and optionally process.terminationReason ==
.exit) so the test verifies a successful end-to-end run; use the existing
process, closeBridge.signal(), resizeCount(), targetResizes and delivered
symbols to locate where to add the wait and assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2de1dbba-a11d-4bae-aea9-e8f519d05ffd
📒 Files selected for processing (2)
CLI/cmux.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swift
The cmux control socket serves a single request per connection (the server closes it after replying). `cmux ssh` forwards local SIGWINCH to the remote PTY via workspace.remote.pty_resize over one long-lived SocketClient, so after the first resize the cached connection is dead and every later resize is swallowed by `_ = try? client.sendV2(...)`: the remote PTY freezes at its attach-time width while the local PTY reflows. Add testSSHPTYAttachReconnectsResizeForEachSIGWINCH, which drives several SIGWINCH signals against a one-request-per-connection mock and asserts each resize is delivered as its own RPC. The count requires every resize to carry the full attach context (workspace/session/attachment ids + token), so a resize that drops it does not satisfy the assertion. Against the unfixed handler it fails with delivered=0, so this commit is intentionally red; the fix follows. Regressed by manaflow-ai#4323 (persistent SSH PTY sessions); reproduces on v0.64.10.
…n the CLI The control socket server applies a 30s receive timeout to each accepted connection and closes it once idle, so any long-lived cached SocketClient (the ssh-pty-attach SIGWINCH resize source, bridge EOF handling) is usually talking to a dead socket by the time it next sends. The resize handler swallowed the failure with `try?`, freezing the remote PTY at its previous size for every resize issued more than 30s after attach. Detect the peer-closed connection in SocketClient.send() before writing (a closed connection is the expected steady state for cached clients, not an error), re-establish it, and replay password auth when configured -- the server authenticates each connection independently. The resize handler now logs real failures instead of discarding them. Turns testSSHPTYAttachReconnectsResizeForEachSIGWINCH green.
Address review: route the auth replay through a performSend core that send() also uses, so reestablishConnectionIfPeerClosed never re-enters itself and the isReplayingReconnectAuth sentinel (a race window on a shared-mutable class) is gone entirely. Also tighten the regression test per review: after the bridge closes, wait for ssh-pty-attach to exit and assert a clean zero-status run with no stray output, so the EOF-cleanup path is covered end-to-end.
Reading terminationStatus on a still-running Process raises an ObjC exception, so a hung CLI would crash the test run rather than fail the assertion. Guard the exit wait and return on timeout.
ac45606 to
b6cc12a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@CLI/cmux.swift`:
- Around line 23217-23249: The resolveSurfaceAllowingFallbackDetailed method
calls claudeHookSurfaceIsListed multiple times, each performing a surface.list
RPC and linear scan, which violates algorithmic complexity guidelines for hot
paths. Refactor to fetch the surface list once at the start of
resolveSurfaceAllowingFallbackDetailed, build a Set containing both the id and
ref values from the returned surfaces for constant-time lookups, then replace
the two claudeHookSurfaceIsListed calls with direct Set membership checks
against the pre-built collection. This eliminates redundant RPC calls and linear
scans while maintaining the same verification logic.
- Around line 22201-22205: The code correctly computes
resolvedSurface.isAuthoritative and uses it to conditionally set surfaceId in
one location (lines 22201-22205), but the upsert/persist operations still write
surfaceId unconditionally into the session record. This allows a
non-authoritative fallback surface to be persisted as if it were authoritative,
causing incorrect reuse across panes. Apply the same isAuthoritative guard to
all upsert paths that persist surfaceId into the session: ensure that in lines
22201-22205 and the related upsert operations around lines 22450-22460, the
surfaceId is only written to the session when resolvedSurface.isAuthoritative is
true (otherwise pass nil or omit the field entirely to avoid persisting
borrowed/fallback surface identities).
- Around line 7283-7425: The runWindowDefaultDisplayCommand, runWindowNamespace,
runWindowDisplaysCommand, and runWindowDisplayCommand functions represent
substantial new window management functionality being added directly to the
monolithic cmux.swift file. Extract this newly added window command logic into a
dedicated CLI unit or package target rather than extending the already large
production Swift file. Create a separate module or component to encapsulate
window namespace handling, and have the main CLI file delegate to it, keeping
the primary CLI file focused and within size guidelines.
- Around line 7316-7334: The window default-display command currently silently
ignores unsupported flags (e.g., typos like --cleer), allowing them to fall
through to read mode and hiding user mistakes. Add validation to reject
unrecognized flags before processing the command. After filtering out positional
arguments, check if any remaining items in commandArgs start with a hyphen but
aren't the supported --clear flag, and throw a CLIError with an appropriate
message listing the supported options if such unsupported flags are found. This
validation should occur early in the command handling logic to catch user errors
immediately.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67e4f3d6-c6ee-4c0f-879c-ddb0a6ece403
📒 Files selected for processing (1)
CLI/cmux.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 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 `@CLI/cmux.swift`:
- Around line 23217-23249: The resolveSurfaceAllowingFallbackDetailed method
calls claudeHookSurfaceIsListed multiple times, each performing a surface.list
RPC and linear scan, which violates algorithmic complexity guidelines for hot
paths. Refactor to fetch the surface list once at the start of
resolveSurfaceAllowingFallbackDetailed, build a Set containing both the id and
ref values from the returned surfaces for constant-time lookups, then replace
the two claudeHookSurfaceIsListed calls with direct Set membership checks
against the pre-built collection. This eliminates redundant RPC calls and linear
scans while maintaining the same verification logic.
- Around line 22201-22205: The code correctly computes
resolvedSurface.isAuthoritative and uses it to conditionally set surfaceId in
one location (lines 22201-22205), but the upsert/persist operations still write
surfaceId unconditionally into the session record. This allows a
non-authoritative fallback surface to be persisted as if it were authoritative,
causing incorrect reuse across panes. Apply the same isAuthoritative guard to
all upsert paths that persist surfaceId into the session: ensure that in lines
22201-22205 and the related upsert operations around lines 22450-22460, the
surfaceId is only written to the session when resolvedSurface.isAuthoritative is
true (otherwise pass nil or omit the field entirely to avoid persisting
borrowed/fallback surface identities).
- Around line 7283-7425: The runWindowDefaultDisplayCommand, runWindowNamespace,
runWindowDisplaysCommand, and runWindowDisplayCommand functions represent
substantial new window management functionality being added directly to the
monolithic cmux.swift file. Extract this newly added window command logic into a
dedicated CLI unit or package target rather than extending the already large
production Swift file. Create a separate module or component to encapsulate
window namespace handling, and have the main CLI file delegate to it, keeping
the primary CLI file focused and within size guidelines.
- Around line 7316-7334: The window default-display command currently silently
ignores unsupported flags (e.g., typos like --cleer), allowing them to fall
through to read mode and hiding user mistakes. Add validation to reject
unrecognized flags before processing the command. After filtering out positional
arguments, check if any remaining items in commandArgs start with a hyphen but
aren't the supported --clear flag, and throw a CLIError with an appropriate
message listing the supported options if such unsupported flags are found. This
validation should occur early in the command handling logic to catch user errors
immediately.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67e4f3d6-c6ee-4c0f-879c-ddb0a6ece403
📒 Files selected for processing (1)
CLI/cmux.swift
🛑 Comments failed to post (4)
CLI/cmux.swift (4)
7283-7425: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Extract newly added command/hook logic from
CLI/cmux.swiftinstead of extending the monolith.This PR adds substantial new responsibilities (window/workspace namespace handling, browser routing, Claude-hook surface/state logic) to an already very large production Swift file. Please split these additions into dedicated CLI units/package targets before further growth.
As per coding guidelines, production Swift files over 800 lines and large additions to them should be flagged and decomposed.
Also applies to: 11938-12077, 22185-22620
🤖 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 `@CLI/cmux.swift` around lines 7283 - 7425, The runWindowDefaultDisplayCommand, runWindowNamespace, runWindowDisplaysCommand, and runWindowDisplayCommand functions represent substantial new window management functionality being added directly to the monolithic cmux.swift file. Extract this newly added window command logic into a dedicated CLI unit or package target rather than extending the already large production Swift file. Create a separate module or component to encapsulate window namespace handling, and have the main CLI file delegate to it, keeping the primary CLI file focused and within size guidelines.Source: Coding guidelines
7316-7334:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject unsupported flags in
window default-display.Unsupported flags are currently ignored and can silently fall through to read mode (e.g., typoed
--cleer), which hides user mistakes.Suggested change
+ let supportedFlags: Set<String> = ["--clear"] + if let stray = commandArgs.first(where: { $0.hasPrefix("-") && !supportedFlags.contains($0) }) { + throw CLIError(message: "window default-display does not support \(stray)") + } + if commandArgs.contains("--clear") {📝 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.let supportedFlags: Set<String> = ["--clear"] if let stray = commandArgs.first(where: { $0.hasPrefix("-") && !supportedFlags.contains($0) }) { throw CLIError(message: "window default-display does not support \(stray)") } if commandArgs.contains("--clear") { try runBlocking { try await store.reset(key) } if jsonOutput { print(jsonString(["default_display": NSNull()])) } else { print("Cleared dev window display default.") } return } let positional = commandArgs.filter { !$0.hasPrefix("-") } if let raw = positional.first { let name = raw.trimmingCharacters(in: .whitespacesAndNewlines) guard !name.isEmpty else { throw CLIError(message: "window default-display requires a display name, or --clear") } try runBlocking { try await store.set(name, for: key) } if jsonOutput { print(jsonString(["default_display": name])) } else { print("Dev builds will open on \"\(name)\" (DEBUG builds, applied at window creation).") } return }🤖 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 `@CLI/cmux.swift` around lines 7316 - 7334, The window default-display command currently silently ignores unsupported flags (e.g., typos like --cleer), allowing them to fall through to read mode and hiding user mistakes. Add validation to reject unrecognized flags before processing the command. After filtering out positional arguments, check if any remaining items in commandArgs start with a hyphen but aren't the supported --clear flag, and throw a CLIError with an appropriate message listing the supported options if such unsupported flags are found. This validation should occur early in the command handling logic to catch user errors immediately.
22201-22205:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not persist non-authoritative fallback surfaces into session ownership.
You correctly compute
resolvedSurface.isAuthoritative, but these upsert paths still persistsurfaceIdunconditionally. That lets a borrowed fallback surface be written into the session record and later reused as if authoritative, which can misclassify stale hooks across panes.Suggested change
- surfaceId: surfaceId, + surfaceId: resolvedSurface.isAuthoritative ? surfaceId : nil,As per coding guidelines, cache-substitution correctness requires cached/borrowed identity values to not silently become authoritative without explicit freshness/authority guarantees.
Also applies to: 22450-22460
🤖 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 `@CLI/cmux.swift` around lines 22201 - 22205, The code correctly computes resolvedSurface.isAuthoritative and uses it to conditionally set surfaceId in one location (lines 22201-22205), but the upsert/persist operations still write surfaceId unconditionally into the session record. This allows a non-authoritative fallback surface to be persisted as if it were authoritative, causing incorrect reuse across panes. Apply the same isAuthoritative guard to all upsert paths that persist surfaceId into the session: ensure that in lines 22201-22205 and the related upsert operations around lines 22450-22460, the surfaceId is only written to the session when resolvedSurface.isAuthoritative is true (otherwise pass nil or omit the field entirely to avoid persisting borrowed/fallback surface identities).Source: Coding guidelines
23217-23249:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid per-hook
surface.listrescans in surface resolution.
claudeHookSurfaceIsListeddoes asurface.listRPC + linear scan on each check, andresolveSurfaceAllowingFallbackDetailedcan invoke it multiple times per hook event. This introduces repeated scalable scans and extra round-trips in a hot path.Please fetch/list once per resolution (or once per hook handling path), build a
Setof ids/refs, and do constant-time membership checks.As per coding guidelines,
.github/review-bot-rules/algorithmic-complexity.mdfails per-event scalable-collection scans in hot paths.🤖 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 `@CLI/cmux.swift` around lines 23217 - 23249, The resolveSurfaceAllowingFallbackDetailed method calls claudeHookSurfaceIsListed multiple times, each performing a surface.list RPC and linear scan, which violates algorithmic complexity guidelines for hot paths. Refactor to fetch the surface list once at the start of resolveSurfaceAllowingFallbackDetailed, build a Set containing both the id and ref values from the returned surfaces for constant-time lookups, then replace the two claudeHookSurfaceIsListed calls with direct Set membership checks against the pre-built collection. This eliminates redundant RPC calls and linear scans while maintaining the same verification logic.Source: Coding guidelines
|
@coderabbitai review |
|
@cubic-dev-ai review |
@kays0x I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,260 of the 240,000 allowed lines of code this month. Reviews resume on 1 July 2026 (in 17 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
✅ Action performedReview finished.
|
|
@lawrencecchen would appreciate a look at this one. It still reproes on Two-commit red/green; bots green, the red checks are just the fork Vercel deploys. Happy to rebase if the CLI refactor moved things around. Related: #5809 is a separate PR for the keepalive override bug on the same ssh path. Thanks! |
|
Closing as superseded. As of v0.64.17 the user-visible This PR targeted a distinct root cause — the long-lived cached |
1 participant
Summary
SocketClient.send()now detects a peer-closed cached connection before writing (connectionAppearsOpen()), transparently re-establishes it, and replays password auth when configured. Thessh-pty-attachSIGWINCH resize handler logs real failures instead of discarding them withtry?.SocketTransport.clientReadTimeout, default 30) and closes it once idle —handleClient's read loop treats the timeout as EOF. Any long-lived cachedSocketClientis therefore usually talking to a dead socket by the time it next sends. The resize handler swallowed that failure, so everycmux sshwindow resize issued more than ~30s after attach silently never reached the remote PTY, freezing the remote at its previous size (local PTY reflows, remote doesn't — the TUI then renders garbled at the stale width).(none)inlsofwhile the data bridge stays healthy, and the daemon never receives the resize RPC.SocketClient.send()(rather than at the resize call site as Fix cmux ssh window resize not reaching the remote PTY #4960 did) also heals the other long-lived users of a cached client — e.g.handleSSHPTYBridgeEOF, which could misreport "bridge closed before remote PTY exit could be confirmed" (exit 255) on a stale connection. The relay-backed path already treats reconnect-per-send as steady state; this extends the same honesty to the unix-socket path. Per-connection password auth is replayed on reconnect (registered byauthenticateSocketClientIfNeeded), which a call-site-only fix would miss.Testing
testSSHPTYAttachReconnectsResizeForEachSIGWINCH(ported from Fix cmux ssh window resize not reaching the remote PTY #4960, kays0x authorship): drives several SIGWINCH signals against a one-request-per-connection control-socket mock and asserts each is delivered as its ownworkspace.remote.pty_resizeRPC.delivered=0(10.7s, exhausts the poll window).testSSHPTYAttachSerializesResizeBeforeEOFLocalCleanup,testSSHPTYAttachBridgeEOFWhileSessionRunsExitsWithoutSSHRetryStatus,testSSHPTYAttachBridgeEOFWhenSessionGoneClearsLocalState,testSSHPTYAttachWaitUsesCurrentTerminalSizeForBridgeHandshake.xcodebuild test -scheme cmux-unit ... -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTests/...→ TEST SUCCEEDED (5 tests, 0 failures).SIGWINCHno-op;lsofshowed the CLI's cached unix socket with peer gone while the bridge TCP connection stayed ESTABLISHED; daemon-sidepty_sessionsstill reported the attachment at 88x55 — resize RPCs were being silently dropped.Demo Video
TIOCGWINSZreads +lsoffd state + daemonpty_sessionsoutput).Checklist
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes
cmux sshresize freezes by auto-reconnecting stale control-socket connections and replaying auth safely. Resizes now reach the remote PTY after idle timeouts; real errors are logged and EOF cleanup exits cleanly.SocketClient.send()and reconnect before sending; replay per-connection password auth via a rememberedauthcommand, routed throughperformSendto avoid re-enteringsend().ssh-pty-attachresize handler now logs failures (with size and error) instead of swallowing them.SIGWINCHdelivers aworkspace.remote.pty_resizewith full attach context and that the process exits 0 after the bridge closes; exit wait is guarded to fail instead of crash if hung.Written for commit b6cc12a. Summary will update on new commits.
Summary by CodeRabbit
ssh-pty-attachreconnects and continues sending PTY resize requests across repeatedSIGWINCHsignals.