Fix remote PTY restore probe reply leak#6070
Open
austinywang wants to merge 26 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces ChangesSSH PTY Reconnect Input Filter
Sequence DiagramsequenceDiagram
participant CLI as ssh-pty-attach (CLI)
participant Filter as SSHPTYAttachReconnectInputFilter
participant MockRPC as Mock JSON-RPC Server
participant Bridge as SSH Bridge (TCP)
CLI->>MockRPC: workspace.remote.pty_bridge
MockRPC-->>CLI: bridge address
CLI->>MockRPC: workspace.remote.pty_sessions
MockRPC-->>CLI: session list
CLI->>Bridge: connect
Bridge-->>CLI: JSON ready handshake
Note over CLI,Filter: stdin contains probe replies + forwardedInput
CLI->>Filter: startStdinPump(fd: bridgeFD, filterEnabled: true)
Filter->>Filter: filter(probeReplyBytes) → stripped
Filter->>Filter: filter(forwardedInputBytes) → passthrough
Filter->>Bridge: writeAll(forwardedInputBytes)
CLI->>MockRPC: workspace.remote.pty_attach_end
Filter->>Bridge: shutdown(SHUT_WR) on EOF
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
Contributor
Greptile SummaryThis PR adds a reconnect-only stdin filter for
Confidence Score: 5/5The change is safe to merge: the filter is off-by-default for all non-TTY and command-based paths, the drain-to-pump state handoff uses an immutable value snapshot with no shared mutable state, and the stop-signal lifecycle is correctly anchored to the first remote PTY output. All three filter execution paths (drain phase, continued pump filtering, and stop-signal teardown) were traced end-to-end and produce correct results. The OSC split-reply, bare-ESC buffering, max-pending cutoff, and piped-stdin bypass cases are covered by focused unit tests and an integration regression. No data races, resource leaks, or correctness gaps were found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as cmux.swift
participant Filter as SSHPTYAttachReconnectInputFilter
participant Pump as pumpStdin
participant Bridge as Remote PTY Bridge
Caller->>Filter: startStdinPump(filterEnabled)
alt filterEnabled
Filter->>Filter: drainQueuedProbeReplies sync
Filter-->>Bridge: non-probe bytes
Filter-->>Caller: SSHPTYAttachReconnectInputFilterControl
else not enabled
Filter-->>Caller: nil
end
Filter->>Pump: async pumpStdin(state, stopSignalFD)
loop stdin pump
Pump->>Pump: poll with 25ms timeout if pending
alt stop requested
Pump->>Pump: flush pending
Pump-->>Bridge: flushed bytes
end
Pump->>Pump: read stdin
Pump->>Pump: filter chunk
Pump-->>Bridge: forwarded bytes
end
Caller->>Caller: first bridge output
Caller->>Filter: stopFiltering
Filter-->>Pump: stop signal byte
Reviews (19): Last reviewed commit: "fix: signal reconnect filter stop withou..." | 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 4301-4314: The test needs to be scoped to reconnect mode by adding
the --require-existing flag to the ssh-pty-attach command invocation. At the
sibling location (lines 4353-4358 in the file
cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift), modify the
ssh-pty-attach command call to include the --require-existing flag so that the
test explicitly exercises the reconnect-only filtering contract and avoids
masking unintended filtering behavior on first attach.
🪄 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: 2239f4ec-19cc-4f03-8780-2080d39b8c6e
📒 Files selected for processing (3)
CLI/cmux.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/CLINotifyProcessTestSupport.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/SSHPTYAttachReconnectInputFilter.swift (1)
115-176: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider documenting the conservative early-passthrough design choice.
Lines 119-121 and 149-151 return
.passThroughfor partial sequences (e.g., loneESC, orESC ] 10without semicolon) rather than buffering as.incomplete. This is a reasonable conservative choice—escape sequences typically arrive atomically, and false negatives are safer than eating user input. A brief comment explaining this tradeoff would help future maintainers understand the design.🤖 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/SSHPTYAttachReconnectInputFilter.swift` around lines 115 - 176, Add explanatory comments documenting the conservative early-passthrough design choice in two locations: First, add a comment above the guard statement checking "start + 1 < bytes.count" in the reconnectProbeReplySequence function explaining that partial sequences (like a lone ESC byte) are passed through rather than buffered as incomplete, because escape sequences typically arrive atomically and false negatives are safer than consuming user input prematurely. Second, add a similar comment above the guard statement checking "cursor < bytes.count" in the oscColorReplySequence function after the command parsing loop, explaining the same rationale for why incomplete color reply sequences are passed through rather than buffered. These comments should briefly clarify the deliberate tradeoff being made in the escape sequence handling logic.
🤖 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.
Outside diff comments:
In `@CLI/SSHPTYAttachReconnectInputFilter.swift`:
- Around line 115-176: Add explanatory comments documenting the conservative
early-passthrough design choice in two locations: First, add a comment above the
guard statement checking "start + 1 < bytes.count" in the
reconnectProbeReplySequence function explaining that partial sequences (like a
lone ESC byte) are passed through rather than buffered as incomplete, because
escape sequences typically arrive atomically and false negatives are safer than
consuming user input prematurely. Second, add a similar comment above the guard
statement checking "cursor < bytes.count" in the oscColorReplySequence function
after the command parsing loop, explaining the same rationale for why incomplete
color reply sequences are passed through rather than buffered. These comments
should briefly clarify the deliberate tradeoff being made in the escape sequence
handling logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10f9b88b-29d4-464b-a1fa-7c8d41232096
📒 Files selected for processing (3)
CLI/SSHPTYAttachReconnectInputFilter.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SSHPTYAttachReconnectInputFilterTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/SSHPTYAttachReconnectInputFilter.swift (1)
118-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuffer lone
ESCas incomplete during reconnect filtering.At Line 118, returning
.passThroughfor a single-byteESCcausesfilter(_:)to disable filtering at Line 96. If a probe reply is split across reads (ESCthen]11;.../[?1;2c), queued probe bytes are forwarded instead of stripped.💡 Suggested fix
- guard start + 1 < bytes.count else { - return .passThrough - } + guard start + 1 < bytes.count else { + return .incomplete + }🤖 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/SSHPTYAttachReconnectInputFilter.swift` around lines 118 - 120, The guard statement at line 118-120 returns .passThrough when encountering a lone ESC byte, which disables filtering in the filter(_:) method at line 96. When a probe reply spans multiple reads (ESC followed by ]11;... or [?1;2c), this causes queued probe bytes to leak through instead of being stripped. Instead of returning .passThrough for incomplete escape sequences, buffer the lone ESC byte as an incomplete sequence (using an appropriate return value that keeps filtering enabled) so that when subsequent bytes arrive in the next read, the complete probe reply can be properly stripped by the filter.
🤖 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.
Outside diff comments:
In `@CLI/SSHPTYAttachReconnectInputFilter.swift`:
- Around line 118-120: The guard statement at line 118-120 returns .passThrough
when encountering a lone ESC byte, which disables filtering in the filter(_:)
method at line 96. When a probe reply spans multiple reads (ESC followed by
]11;... or [?1;2c), this causes queued probe bytes to leak through instead of
being stripped. Instead of returning .passThrough for incomplete escape
sequences, buffer the lone ESC byte as an incomplete sequence (using an
appropriate return value that keeps filtering enabled) so that when subsequent
bytes arrive in the next read, the complete probe reply can be properly stripped
by the filter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04076f47-3d46-4847-bf20-4105c75ad3bc
📒 Files selected for processing (2)
CLI/SSHPTYAttachReconnectInputFilter.swiftcmuxTests/SSHPTYAttachReconnectInputFilterTests.swift
…tore-pty-leak # Conflicts: # .github/swift-file-length-budget.tsv
…tore-pty-leak # Conflicts: # .github/swift-file-length-budget.tsv
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
ssh-pty-attach --require-existingthat runs only when stdin is a TTY and no command is being injected.Reproduction
ssh-pty-attach --require-existingbridge reattach stdin pump and added focused coverage for queued probe replies, split replies, bounded drain cutoff, bare Escape handling, and piped stdin preservation.Testing
swiftc -typecheck CLI/SSHPTYAttachReconnectInputFilterSequenceMatch.swift CLI/SSHPTYAttachReconnectInputFilter.swift/Users/austinwang/manaflow/cmuxterm-hq/skills/autoreview/scripts/cmux-policy-check --mode branch --base origin/mainpython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv./tests/test_ci_pbxproj_test_wiring.sh./scripts/check-pbxproj.shgit diff --checkxcodebuild,reload.sh, or the dev app launch; this branch explicitly requires CI first and user approval before the dev build is launched.Demo Video
N/A. This is a CLI/PTY reconnect path fix with no UI changes, and the full Fedora 44 remote restore setup is not available locally.
Checklist
mainand includesFixes #6061.Fixes #6061