Clear SSH auth marker after successful startup - #8410
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe SSH foreground authentication startup flow now centralizes successful cleanup commands, reuses one sharing-options instance for related paths and defaults, and exits explicitly after cleanup. Regression tests verify removal of the ChangesSSH authentication lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SSHStartupTest
participant SSHPTYAttachStartupCommandBuilder
participant AuthenticationShell
participant AuthInflightMarker
SSHStartupTest->>SSHPTYAttachStartupCommandBuilder: generate startup command
SSHPTYAttachStartupCommandBuilder->>AuthenticationShell: run foreground authentication
AuthenticationShell->>AuthInflightMarker: remove .inflight marker
AuthenticationShell-->>SSHStartupTest: return successful startup status
Possibly related PRs
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 fixes a marker lifecycle bug: the
Confidence Score: 5/5Safe to merge — the change is a targeted bug fix with no new state, no architectural risk, and two dedicated regression tests covering both affected paths. The fix is surgical: one new pure-factory method on an existing value type, two call sites updated to use it, and the shared three-step cleanup sequence (clear marker → release lock → disarm traps) is correctly ordered in both the CLI and the app-side builder. The cmux_ssh_clear_auth_inflight function is already defined in the script body before the new call site in both paths. The regression tests directly verify the inflight marker is absent after success and match the red/green proof described in the PR. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Shell as /bin/zsh -fc
participant Inflight as .inflight marker
participant Lock as advisory lock fd
participant Traps as EXIT/HUP/INT/TERM traps
Shell->>Inflight: write $$ (PID)
Shell->>Traps: trap cmux_ssh_clear_auth_inflight EXIT/HUP/INT/TERM
Shell->>Lock: zsystem flock -t 45 ... (acquire)
Shell->>Shell: ssh ... true (authenticate)
alt SSH succeeds
Shell->>Inflight: cmux_ssh_clear_auth_inflight (remove marker)
Shell->>Lock: zsystem flock -u (release)
Shell->>Traps: trap - EXIT HUP INT TERM (disarm)
Shell->>Shell: exit 0
else SSH fails / signal
Traps-->>Inflight: cmux_ssh_clear_auth_inflight (EXIT trap fires)
Shell->>Shell: exit non-zero
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Shell as /bin/zsh -fc
participant Inflight as .inflight marker
participant Lock as advisory lock fd
participant Traps as EXIT/HUP/INT/TERM traps
Shell->>Inflight: write $$ (PID)
Shell->>Traps: trap cmux_ssh_clear_auth_inflight EXIT/HUP/INT/TERM
Shell->>Lock: zsystem flock -t 45 ... (acquire)
Shell->>Shell: ssh ... true (authenticate)
alt SSH succeeds
Shell->>Inflight: cmux_ssh_clear_auth_inflight (remove marker)
Shell->>Lock: zsystem flock -u (release)
Shell->>Traps: trap - EXIT HUP INT TERM (disarm)
Shell->>Shell: exit 0
else SSH fails / signal
Traps-->>Inflight: cmux_ssh_clear_auth_inflight (EXIT trap fires)
Shell->>Shell: exit non-zero
end
Reviews (3): Last reviewed commit: "test: drain SSH auth test stderr safely" | Re-trigger Greptile |
![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/SSHForegroundAuthenticationMarkerCleanupTests.swift`:
- Around line 104-110: Update the process execution flow around process.run() so
stderrPipe.fileHandleForReading.readDataToEndOfFile() is performed before
process.waitUntilExit(). Preserve returning the termination status together with
the captured stderr after the pipe has been fully read.
🪄 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: 4926b29d-423e-4410-a210-0888ef05de88
📒 Files selected for processing (5)
CLI/cmux.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHConnectionSharingOptions.swiftSources/SSHPTYAttachStartupCommandBuilder.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SSHForegroundAuthenticationMarkerCleanupTests.swift
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
.inflightmarker after successful native SSH authenticationContext
Follow-up to #8308. That PR was squash-merged before its post-review marker-cleanup repair landed. Canonical review of this follow-up then found the same lifecycle bug in the app-side restored attach generator, so this PR fixes the duplicated behavior at its shared
SSHConnectionSharingOptionsowner.Red/green proof
CLI startup
Test-only commit
ea95d3e35bfailed as expected:Fix commit
52869a58depasses that path.Restored SSH PTY attach
Test-only commit
546f860fdefailed as expected:Shared fix commit
843336561fmakes both selected behavioral tests pass:Focused command:
Verification
scripts/check-pbxproj.shscripts/check-package-resolved-policy.pyscripts/check-workspace-package-groups.py --checkscripts/lint-pbxproj-test-wiring.sh(528 test files)git diff --checkorigin/main: cleanThe Swift file-length budget script and TSV are absent on this branch; the new test is 112 lines and no budget file changed.
No app reload or launch was performed. No user-facing strings or localization catalogs changed.
Summary by CodeRabbit