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. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Greptile SummaryThis PR fixes a silent precedence bug in two SSH argument builders (
Confidence Score: 5/5Safe to merge; the change is a targeted, well-tested fix that eliminates silent option suppression and increases keepalive headroom on flaky links. Both changed builders apply the same conditional guard that already existed for StrictHostKeyChecking, the logic is straightforward and exercised by tight new tests, and no other SSH command builder in the codebase retains the old hardcoded CountMax=2. RemoteSessionCoordinator+SSHArguments.swift has identical logic to the tested batch builder but carries no new dedicated tests for the keepalive-override path; CI coverage is the only gate for that code path. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User sshOptions
participant Builder as batchSSHArguments / sshCommonArguments
participant SSH as ssh process
Note over Builder: For each supervision key
Builder->>User: hasSSHOptionKey(effectiveSSHOptions, key)
alt key NOT in user options
Builder-->>SSH: "-o Key=default (CountMax=6, Interval=20, ConnectTimeout=6)"
else key already set by user
Builder-->>SSH: default suppressed, user value wins
end
Builder->>User: hasSSHOptionKey(effectiveSSHOptions, StrictHostKeyChecking)
alt not set
Builder-->>SSH: "-o StrictHostKeyChecking=accept-new"
end
Builder-->>SSH: "-o BatchMode=yes, -o ControlMaster=no, -p port, -i identity"
Builder-->>SSH: -o each user option from effectiveSSHOptions
Reviews (5): Last reviewed commit: "fix: let user SSH options override daemo..." | Re-trigger Greptile |
|
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:
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 (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthrough
ChangesSSH Supervision Keepalive Defaults
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 passed)
✨ 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 |
|
Addressed the Greptile findings in bcc960f:
|
![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 `@Sources/WorkspaceRemoteSSHBatchCommandBuilder.swift`:
- Around line 81-99: The code repeatedly rescans effectiveSSHOptions with
hasSSHOptionKey; change the flow to compute a single Set of lowercased option
keys once (e.g. let optionKeys =
Set(backgroundSSHOptions(configuration.sshOptions).map { $0.lowercased() })) and
reuse it: modify sshSupervisionArguments to accept this Set (or add a helper
that returns default supervision args given the Set) and update batchArguments
to call that helper and to check StrictHostKeyChecking against the same Set
instead of calling hasSSHOptionKey multiple times; ensure keys compared use a
consistent lowercase form like "connecttimeout", "serveraliveinterval",
"serveralivecountmax", "stricthostkeychecking".
🪄 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: 84e5731f-7a56-4c37-8908-aa564d7fe761
📒 Files selected for processing (4)
Sources/Workspace.swiftSources/WorkspaceRemoteConfiguration.swiftSources/WorkspaceRemoteSSHBatchCommandBuilder.swiftcmuxTests/WorkspaceRemoteConnectionTests.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)
Sources/WorkspaceRemoteSSHBatchCommandBuilder.swift (1)
96-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuild the configured-keys Set once and reuse it for both supervision defaults and StrictHostKeyChecking.
batchArgumentsrescanseffectiveSSHOptionsviahasSSHOptionKeyon line 99 aftersshSupervisionArgumentsalready scanned the same list to build a Set on line 82. Build the Set once at the top ofbatchArgumentsand pass it to both checks.♻️ Proposed refactor
private static func batchArguments(configuration: WorkspaceRemoteConfiguration) -> [String] { let effectiveSSHOptions = backgroundSSHOptions(configuration.sshOptions) - var args: [String] = sshSupervisionArguments(effectiveSSHOptions: effectiveSSHOptions) - if !hasSSHOptionKey(effectiveSSHOptions, key: "StrictHostKeyChecking") { + let configuredKeys = Set(effectiveSSHOptions.compactMap(sshOptionKey)) + var args: [String] = sshSupervisionArguments(configuredKeys: configuredKeys) + if !configuredKeys.contains("stricthostkeychecking") { args += ["-o", "StrictHostKeyChecking=accept-new"] } args += ["-o", "BatchMode=yes"]Then update the helper signature:
-static func sshSupervisionArguments(effectiveSSHOptions: [String]) -> [String] { - let configuredKeys = Set(effectiveSSHOptions.compactMap(sshOptionKey)) +static func sshSupervisionArguments(configuredKeys: Set<String>) -> [String] { var args: [String] = []Or keep the existing signature and add an overload that accepts the Set, making the array-accepting version delegate to it.
Based on coding guidelines
.github/review-bot-rules/algorithmic-complexity.md: "Prefer one-pass parsing (scan once, store results in a Set/Map) rather than multiplecontains(where:)/filter/first(where:)over the same collection for each generated-ooption."🤖 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 `@Sources/WorkspaceRemoteSSHBatchCommandBuilder.swift` around lines 96 - 101, batchArguments currently calls backgroundSSHOptions then calls sshSupervisionArguments which scans effectiveSSHOptions to build a Set, and later calls hasSSHOptionKey which rescans the same list; to fix, compute the Set of configured keys once at the top of batchArguments (after effectiveSSHOptions = backgroundSSHOptions(...)) and pass that Set into sshSupervisionArguments (add an overload or new helper parameter) and use the same Set for the StrictHostKeyChecking check instead of calling hasSSHOptionKey again; update sshSupervisionArguments and/or provide an overload that accepts the precomputed Set so both places reuse the one-pass result.Source: Coding guidelines
🤖 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 `@Sources/WorkspaceRemoteSSHBatchCommandBuilder.swift`:
- Around line 96-101: batchArguments currently calls backgroundSSHOptions then
calls sshSupervisionArguments which scans effectiveSSHOptions to build a Set,
and later calls hasSSHOptionKey which rescans the same list; to fix, compute the
Set of configured keys once at the top of batchArguments (after
effectiveSSHOptions = backgroundSSHOptions(...)) and pass that Set into
sshSupervisionArguments (add an overload or new helper parameter) and use the
same Set for the StrictHostKeyChecking check instead of calling hasSSHOptionKey
again; update sshSupervisionArguments and/or provide an overload that accepts
the precomputed Set so both places reuse the one-pass result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c333d684-1cf2-457d-8219-b11118e443ae
📒 Files selected for processing (1)
Sources/WorkspaceRemoteSSHBatchCommandBuilder.swift
|
Addressed the outside-diff finding from the latest CodeRabbit review in cec94b5: added the Set-accepting |
Adds failing coverage for the daemon-transport batch builder: the keepalive defaults must be overridable by user SSH options (OpenSSH is first-value-wins, so emitting both the default and the user value silently ignores the user) and the default ServerAliveCountMax should be 6 rather than 2.
The daemon-transport batch builder and the remote-session coordinator emitted ConnectTimeout/ServerAliveInterval/ServerAliveCountMax unconditionally at the front of the ssh argv, before the user's own options. OpenSSH applies the first value seen for an option, so a user could never override these keepalives, and the 20s interval x CountMax=2 (40s) budget tore the supervision connection down on brief blips (the 'attempt N/20' reconnect flapping). Guard each default with hasSSHOptionKey so it is emitted only when the user has not set it, and raise the default ServerAliveCountMax 2 -> 6 (~120s budget).
cec94b5 to
71413cd
Compare
|
Force-pushed: rebased onto current |
|
@coderabbitai review |
|
@cubic-dev-ai review |
✅ Action performedReview finished.
|
@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:
|
|
Closing as stale - it conflicts with main after the package split. For reference, the underlying issue is still present on main: |
1 participant
Summary
What: The daemon-transport SSH command builders emitted
ConnectTimeout,ServerAliveInterval, andServerAliveCountMaxunconditionally at the front of thesshargv, before appending the configuration's own SSH options. OpenSSH applies the first value it sees for an option, so:ServerAliveInterval=20xServerAliveCountMax=2= ~40s) tore the supervision connection down on brief network blips, producing the "attempt N/20" reconnect flapping.Why / fix: Guard each default with
hasSSHOptionKeyso it is emitted only when the configuration has not already set it (matching the existingStrictHostKeyCheckingguard right below it), and raise the defaultServerAliveCountMaxfrom 2 to 6 (~120s budget) so a short outage no longer kills the connection.Applied to both daemon-transport builders:
CmuxCore-WorkspaceRemoteConfiguration.batchSSHArguments()(daemonTransportArguments/daemonSocketForwardArguments/ reverse-relay).CmuxRemoteSession-RemoteSessionCoordinator.sshCommonArguments(batchMode:).Testing
swift test --package-path Packages/CmuxCore --filter WorkspaceRemoteConfigurationSSHBatchCommandsTests- 9 passed.Two new regression tests (two-commit red/green):
supervision keepalive default tolerates longer outages (ServerAliveCountMax=6)user-supplied keepalive options override the supervision defaults- verifies a user'sServerAliveInterval/ServerAliveCountMax/ConnectTimeoutsurvive exactly once and no default token is emitted.Verified locally that both fail on the test-only commit (the argv contains both the hardcoded
=2and the user's value) and pass on the fix commit.CmuxRemoteSessionbuilds clean.Demo Video
Terminal-only argv change; before/after is the argument list asserted in the tests above. Before:
-o ServerAliveCountMax=2 ... -o ServerAliveCountMax=12(user value ignored, first-wins). After:-o ServerAliveCountMax=12only (user value honored), default=6when unset.Review Trigger
Override semantics for daemon-transport keepalives + default budget change.
Checklist
Summary by CodeRabbit
Bug Fixes
ServerAliveCountMax) when those options aren’t already set. User-supplied SSH keepalive settings continue to fully override the defaults.Tests