Fix remote daemon upload without SFTP - #8434
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThe remote daemon installer now uses SSH exec with a local file streamed over stdin instead of SCP/SFTP. Process requests support file-backed stdin, coordinator execution helpers are reorganized, and tests cover successful uploads and detailed remote execution failures. ChangesRemote daemon upload
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RemoteSessionCoordinator
participant RemoteSessionProcessRunner
participant SSH
RemoteSessionCoordinator->>RemoteSessionProcessRunner: run SSH mkdir command
RemoteSessionProcessRunner->>SSH: Execute remote directory creation
RemoteSessionCoordinator->>RemoteSessionProcessRunner: run SSH cat > with stdinFile
RemoteSessionProcessRunner->>SSH: Stream daemon binary
RemoteSessionCoordinator->>RemoteSessionProcessRunner: run SSH chmod and mv
RemoteSessionProcessRunner->>SSH: Finalize daemon installation
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 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 SummaryReplaces the SCP/SFTP-based daemon upload with a pure SSH exec channel approach: the local
Confidence Score: 5/5Safe to merge — the upload path is self-contained, all failure branches clean up the temp file, and the StdinNull fix is well-guarded and tested. The change replaces a well-scoped upload routine with a structurally identical one that uses a different transport. Every failure branch after temp-file creation calls cleanupUploadedRemotePaths, the FileHandle lifecycle is correctly bounded by waitUntilExit, the StdinNull override is positionally enforced and regression-tested, and all six new localised strings cover both supported locales. No actor isolation, blocking primitives, ambient globals, or test seams in production source are introduced. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant C as RemoteSessionCoordinator
participant PE as ProcessExecution
participant PR as RemoteSessionProcessRunner
participant SSH as /usr/bin/ssh
participant R as Remote Shell
C->>PE: uploadRemoteDaemonBinaryLocked(localBinary:, location:)
PE->>SSH: ssh ... "sh -c 'mkdir -p dir'"
SSH->>R: mkdir -p dir
R-->>SSH: exit 0
SSH-->>PE: RemoteCommandResult(status:0)
PE->>PE: sshExec(arguments:, stdinFile: localBinary)
Note over PE: Prepends -o StdinNull=no
PE->>PR: run(RemoteProcessRequest(stdinFile: localBinary))
PR->>PR: FileHandle(forReadingFrom: localBinary)
PR->>SSH: "ssh -o StdinNull=no ... "sh -c 'cat > tmpPath'""
Note over PR,SSH: streams binary bytes via stdin
SSH->>R: "cat > tmpPath"
R-->>SSH: exit 0
SSH-->>PR: exit 0
PR-->>PE: RemoteCommandResult(status:0)
PE->>SSH: ssh ... "sh -c 'chmod 755 tmpPath and mv tmpPath dstPath'"
SSH->>R: chmod + mv
R-->>SSH: exit 0
SSH-->>PE: RemoteCommandResult(status:0)
PE-->>C: success
Note over C,R: On any failure after temp upload: cleanupUploadedRemotePaths([tmpPath])
%%{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 C as RemoteSessionCoordinator
participant PE as ProcessExecution
participant PR as RemoteSessionProcessRunner
participant SSH as /usr/bin/ssh
participant R as Remote Shell
C->>PE: uploadRemoteDaemonBinaryLocked(localBinary:, location:)
PE->>SSH: ssh ... "sh -c 'mkdir -p dir'"
SSH->>R: mkdir -p dir
R-->>SSH: exit 0
SSH-->>PE: RemoteCommandResult(status:0)
PE->>PE: sshExec(arguments:, stdinFile: localBinary)
Note over PE: Prepends -o StdinNull=no
PE->>PR: run(RemoteProcessRequest(stdinFile: localBinary))
PR->>PR: FileHandle(forReadingFrom: localBinary)
PR->>SSH: "ssh -o StdinNull=no ... "sh -c 'cat > tmpPath'""
Note over PR,SSH: streams binary bytes via stdin
SSH->>R: "cat > tmpPath"
R-->>SSH: exit 0
SSH-->>PR: exit 0
PR-->>PE: RemoteCommandResult(status:0)
PE->>SSH: ssh ... "sh -c 'chmod 755 tmpPath and mv tmpPath dstPath'"
SSH->>R: chmod + mv
R-->>SSH: exit 0
SSH-->>PE: RemoteCommandResult(status:0)
PE-->>C: success
Note over C,R: On any failure after temp upload: cleanupUploadedRemotePaths([tmpPath])
Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteDaemonUploadTests.swift (1)
23-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the full upload transaction in these tests.
The runner returns success for every non-SCP request, so the tests would still pass if directory creation,
chmod && mvfinalization, or failure cleanup were removed. Return per-step responses and assert the expected command sequence—including the cleanup SSH command after upload failure.Also applies to: 47-55, 68-78, 86-101
🤖 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 `@Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteDaemonUploadTests.swift` around lines 23 - 32, Update the test runner setup in the RemoteDaemonUploadTests cases to return distinct responses for directory creation, SCP upload, finalization, and cleanup commands instead of succeeding for every non-SCP request. Assert the complete expected command sequence for each transaction, including the cleanup SSH command after upload failure, so removing any upload step causes the tests to fail.
🤖 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
`@Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteDaemonUploadTests.swift`:
- Around line 23-32: Update the test runner setup in the RemoteDaemonUploadTests
cases to return distinct responses for directory creation, SCP upload,
finalization, and cleanup commands instead of succeeding for every non-SCP
request. Assert the complete expected command sequence for each transaction,
including the cleanup SSH command after upload failure, so removing any upload
step causes the tests to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa0a2c53-0e68-4ec2-89f8-a937db5f4f63
📒 Files selected for processing (2)
Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteDaemonUploadTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionSSHRemoteCommandOverrideTests.swift
added 6 commits
July 19, 2026 02:39
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator`+DaemonUpload.swift:
- Around line 69-72: Sanitize the installation failure in
`RemoteSessionCoordinator` by replacing the raw `error.localizedDescription`
interpolation with a safe, user-friendly `String(localized:defaultValue:)`
message. Update
`Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+DaemonUpload.swift`
lines 69-72 and apply the same localization/sanitization to the adjacent
unchanged error at line 77 if it exposes raw details; update
`Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteDaemonUploadTests.swift`
lines 186-190 to assert the sanitized localized string or its `defaultValue`.
🪄 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: fbd6fa4e-7458-4838-ab63-ff36285c600e
📒 Files selected for processing (4)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+DaemonUpload.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ProcessExecution.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteDaemonUploadTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionSSHRemoteCommandOverrideTests.swift
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 24, 2026
…ed scp Two tests waited on an scp invocation that no longer happens. manaflow-ai#8434 moved the daemon upload off scp and onto the ssh exec channel, streaming the binary into `cat >`, and did not touch these tests. Their stubs only fulfilled inside an `executable == "/usr/bin/scp"` branch, so the expectation could never fire, the wait spent its whole budget, and the unwrap on the next line reported nil. Both now capture the upload from the ssh branch. The property each one is about is unchanged: the daemon still has to land on an absolute path under the remote HOME, that path just travels inside the remote command instead of an scp destination, so the assertion moved with it. The scp branch is kept and fails loudly. If the upload ever returns to scp, that should be a sentence in the failure output rather than a silent timeout, which is precisely how these two broke. The reinstall test also now records how many capability hellos preceded the upload and requires at least one. Retargeting alone would have let it pass on a first install, which is not the missing-pty-capability path it is named for. Renamed the first test off "ScpDestination" since it no longer describes what is asserted.
This was referenced Jul 24, 2026
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 25, 2026
…ed scp Two tests waited on an scp invocation that no longer happens. manaflow-ai#8434 moved the daemon upload off scp and onto the ssh exec channel, streaming the binary into `cat >`, and did not touch these tests. Their stubs only fulfilled inside an `executable == "/usr/bin/scp"` branch, so the expectation could never fire, the wait spent its whole budget, and the unwrap on the next line reported nil. Both now capture the upload from the ssh branch. The property each one is about is unchanged: the daemon still has to land on an absolute path under the remote HOME, that path just travels inside the remote command instead of an scp destination, so the assertion moved with it. The scp branch is kept and fails loudly. If the upload ever returns to scp, that should be a sentence in the failure output rather than a silent timeout, which is precisely how these two broke. The reinstall test also now records how many capability hellos preceded the upload and requires at least one. Retargeting alone would have let it pass on a first install, which is not the missing-pty-capability path it is named for. Renamed the first test off "ScpDestination" since it no longer describes what is asserted.
ejc3
added a commit
to ejc3/cmux
that referenced
this pull request
Jul 31, 2026
…ed scp Two tests waited on an scp invocation that no longer happens. manaflow-ai#8434 moved the daemon upload off scp and onto the ssh exec channel, streaming the binary into `cat >`, and did not touch these tests. Their stubs only fulfilled inside an `executable == "/usr/bin/scp"` branch, so the expectation could never fire, the wait spent its whole budget, and the unwrap on the next line reported nil. Both now capture the upload from the ssh branch. The property each one is about is unchanged: the daemon still has to land on an absolute path under the remote HOME, that path just travels inside the remote command instead of an scp destination, so the assertion moved with it. The scp branch is kept and fails loudly. If the upload ever returns to scp, that should be a sentence in the failure output rather than a silent timeout, which is precisely how these two broke. The reinstall test also now records how many capability hellos preceded the upload and requires at least one. Retargeting alone would have let it pass on a first install, which is not the missing-pty-capability path it is named for. Renamed the first test off "ScpDestination" since it no longer describes what is asserted.
austinywang
added a commit
that referenced
this pull request
Aug 4, 2026
…test host (#9572) * cmuxTests: derive the theme reload target from a dash-free socket suffix The CLI derives a theme reload target from the socket file name, collapsing every run of non-alphanumerics in the slug to a dot. #6452 made this fixture's socket path unique with a raw UUID to stop two runs colliding in /tmp, which put the UUID's dashes into the derived identifier as dots, so the expected literal could no longer match and the test waited out its five seconds. The stdout assertion kept passing because the derived id still has the expected value as a prefix, which is why this read as a timeout rather than a string mismatch. Keeps the unique suffix hex-only so the expected identifier stays a plain template instead of a call into the CLI's own helper, which would agree by construction. * cmuxTests: drop two palette assertions for a gate that no longer exists #8173 replaced the fork-probe reuse gate: `!cachedResultHadFallback` became `cachedResultIsFresh`, and the fallback case is now re-verified against SharedLiveAgentIndex at the call site instead of being refused outright. The parameter stayed in both signatures, so these two assertions still compiled while asserting the opposite of what the product does, and WorkspaceForkConversationContextMenuTests asserts the new contract in both directions a few files away. Removes the two assertions whose only purpose was the removed term, and renames the clear-side test to say what it still covers. * cmuxTests: stop the remote-connection suite killing its own test host Three separate problems, in order of blast radius. Two assertions indexed `operations` right after asserting its count. A count assertion does not stop execution, so on failure the next line trapped with Index out of range and took the shared test host down, and every remaining test in the shard never ran. Measured twice in one run. Four @mainactor tests waited on a DispatchSemaphore. configureRemoteConnection enqueues its session transition as a main-actor Task, so blocking the main actor stopped the very work being waited on from ever being scheduled. They now use expectations, which pump the run loop. Fifteen fixtures passed an unresolved %C control template. The broker deliberately refuses to own a path it cannot resolve, so no lease was ever taken and cleanup could not run; six inverted expectations were passing vacuously as a result. They now use the resolved form ssh -G produces, and a new test pins the unowned-template policy so the fixtures cannot quietly regress to it. Two more read activeRemoteSessionControllerID straight after configureRemoteConnection and now await the transition instead. * cmuxTests: point the daemon-upload tests at the transport that replaced scp Two tests waited on an scp invocation that no longer happens. #8434 moved the daemon upload off scp and onto the ssh exec channel, streaming the binary into `cat >`, and did not touch these tests. Their stubs only fulfilled inside an `executable == "/usr/bin/scp"` branch, so the expectation could never fire, the wait spent its whole budget, and the unwrap on the next line reported nil. Both now capture the upload from the ssh branch. The property each one is about is unchanged: the daemon still has to land on an absolute path under the remote HOME, that path just travels inside the remote command instead of an scp destination, so the assertion moved with it. The scp branch is kept and fails loudly. If the upload ever returns to scp, that should be a sentence in the failure output rather than a silent timeout, which is precisely how these two broke. The reinstall test also now records how many capability hellos preceded the upload and requires at least one. Retargeting alone would have let it pass on a first install, which is not the missing-pty-capability path it is named for. Renamed the first test off "ScpDestination" since it no longer describes what is asserted. * cmuxTests: fix three CLI tests that could not pass, and stop one hiding why Three separate causes, all in the fixtures rather than the product. Two socket-selection tests replied to the CLI with a bareword. SocketClient only treats OK, OK …, PONG, ERROR: … or JSON as a complete single-line reply, so a bareword sends it into the multiline drain pass, where reconfiguring the receive timeout on a socket whose peer already hung up fails with EINVAL — and the CLI reports "Invalid argument" instead of the reply it already had buffered. The replies are now OK-framed. These were the only two barewords in the suite, which is why eleven near-identical siblings pass. Both now also assert which responder received the request. That is the property they exist for — the tagged socket is chosen and the stable one is not — and unlike the stdout comparison it cannot be made vacuous by a future change to the reply. A fork-diagnostics fixture passed agent "project-agent", which is not in the CLI's catalog, so the command exited before emitting any JSON. The test has never passed; it went in already red alongside the pi-family gate it is meant to cover. It now uses grok, a catalog agent that is neither pi-family nor one of the transcript-walking agents, so the basename gate is still what is under test. The shared helper turned all of that into a JSON decoding failure, because it only expected a zero exit before parsing. It now requires the exit status and a completed run, so the next fixture mistake reports the CLI's own error text instead of a parse error. * cmuxTests: pair the pi-basename fixture with an agent that can actually fork The pi-family basename test asked for fork_command_available, fork_supported and fork_startup_input_available, but its fixture stored the record under a grok launcher pair. A captured launch command is only used when its launcher describes the requested agent, so the grok/omo pair was dropped as untrusted, no fork argv was built for any agent, and all four assertions failed on agent_has_no_fork_command without ever reaching the rule under test. Store the record under opencode instead, whose wrapper launcher is omo. The capture is now trusted, the fork argv resolves through the omo launcher, and the executable basename stays /tmp/pi so the disagreement between the structured identity and the basename is still what the test measures. The omo launcher also answers fork support before the opencode executable probe, so the result does not depend on a /tmp/pi existing on the machine running the test. * cmuxTests: assert the stderr-closed CLI does not crash, instead of a CLI that no longer exists This test asserted exit 1 and a "Usage:" banner on stdout. Neither has been true since #f48922aa94: an unknown command exits 2 with a single line and no usage dump, and that line goes to stderr — which the test closes with 2>&-. So it could not pass, and the crash it was written for was not what it checked. The regression is still worth guarding. cc4a610 replaced FileHandle.standardError.write, which raises and aborts when stderr is closed, with a raw Darwin.write that returns -1 on EBADF. The oracle is therefore that the CLI exited on its own terms rather than dying from a signal, so ProcessRunResult now carries terminationReason and both runners set it. Without that, a signalled process is indistinguishable from an ordinary non-zero exit, because its terminationStatus is just the signal number. The command now runs under exec, so the process being waited on is the CLI rather than the shell. A shell reports a signalled child as a normal exit with status 128+signal, which would have hidden exactly the crash being tested. It also pins CMUX_SOCKET_PATH and the home directory. Socket resolution otherwise consults a machine-global marker file, and a spawn with a pristine temp home was measured reaching a real running app — which would make the exit code depend on what is running on the machine. With the socket pinned the unknown-command path is a single branch, so the test asserts exit 2 exactly rather than settling for non-zero. * cmuxTests: isolate the CLI regression suite from the machine's own cmux A CLI spawned from this suite with a pristine temp home and a scrubbed environment still reached a real running app. CFFIXED_USER_HOME moves the socket directory but not socket discovery: the CLI also reads the machine-wide /tmp/cmux-last-socket-path marker, and for an untagged debug build it scans /tmp for cmux-debug-*.sock and connects to what it finds. Resolution runs before the command dispatches, so even `claude-teams --help` did this. Every spawn site that is not itself testing resolution now pins CMUX_SOCKET_PATH to a per-run path, the three stable-variant tests write the marker inside their own temp home, and runShell takes an explicit environment instead of handing the child everything the test host was launched with. Two tests bound a responder on /tmp/cmux.sock, the release app's socket path, and UnixSocketResponder unlinks before it binds, so a run could take the control socket away from a release app in use. The early returns meant to prevent that raced the app, disagreed about whether a dangling symlink counts as present, and turned the tests into silent passes. The symlink fallback case moves to the user-scoped stable path inside its temp home. The legacy case keeps the part that needs the real path, that /tmp/cmux.sock is classified as a stable implicit default, and no longer creates, binds, or removes it. Three more guards tested paths inside a freshly created temp home and could never fire, so they are gone. stderr was pointed at the stdout pipe while about thirty tests parse stdout as JSON or compare it to an exact reply, so one diagnostic line from the runtime broke a content check instead of naming itself. stderr now has its own pipe, failure messages carry both streams, and the negative checks that meant "the CLI never said this anywhere" read both rather than silently narrowing to stdout. Readers for both pipes start before the wait, because reading after waitUntilExit deadlocks once a child fills a pipe buffer and that looks like a hang inside the CLI. A launch failure is reported on stdout as well as stderr, since five sibling suites share this runner and print only stdout. Runs that assert nothing about latency no longer carry a 5s cap and take a 60s guard instead, which still fails a stuck CLI rather than passing slowly. The two browser-download tests keep their 3s and 16s caps, where the deadline is the assertion. The two theme tests with fixed bundle identifiers now scope them per run, since the reload notification goes out machine-wide; for the nightly one that means scoping the socket file name too, because the identifier is derived from it. * cmuxTests: assert the exit code this fixture actually produces The stderr-closed test asserted exit 2, the unknown-command code. Measured, it exits 1: the pinned socket has no listener, so the CLI fails at connect and the top-level handler returns before the unknown-command arm runs. That ordering makes the fixture a better exercise of what the test guards, not a worse one, because the connect error is written to the stderr the test has closed. The run confirmed the guard itself holds — termination reason was a normal exit, not a signal. * cmuxTests: report stderr in sessions helper failures * cmuxTests: preserve restore assertions after stream split * cmuxTests: close review gaps in process and upload fixtures * cmuxTests: align remote fixtures with streamed input and scoped identity * cmuxTests: yield main actor while awaiting daemon upload * cmuxTests: repair CLI regression fixtures and child lifetimes * cmuxTests: isolate daemon bootstrap fixtures from ControlMaster * cmuxTests: keep theme notification state nonisolated * cmuxTests: detach live argv fixture from test host * cmuxTests: own Go discovery in daemon reinstall fixture * cmuxTests: make subprocess and bootstrap fixtures deterministic * cmuxTests: remove detached fixture wall clock * cmuxTests: use async-safe scoped locking * cmuxTests: make off-host process work concurrent * cmuxTests: keep blocking process wait off cooperative executor --------- Co-authored-by: ejc3 <ejc3@users.noreply.github.com>
This was referenced Sep 25, 2026
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
cmuxd-remotefile into remotecatover stdin.chmod 755, atomicmv, and cleanup on every post-transfer failure.StdinNull=noat the file-backed SSH seam so caller/host configuration cannot silently discard the helper while remotecatexits successfully.Regression history
The bug coverage and fixes remain separately visible in the commit history:
e254283025: failing test proving helper bootstrap breaks when SCP/SFTP is unavailable34bbdf7d0b: SSH-exec/file-stdin uploader implementation8a15caec82: full upload/finalize/cleanup transaction assertions0ff4f29088: failingStdinNull=yesregression2514438599:StdinNull=noupload override87f68d6866/c32014da6b: finalization-failure cleanup coverage and fixb8dd1b8494: failing coverage for arbitrary local process-error disclosure8e79c8fe27: sanitized/localized upload process errors with English and Japanese catalog entriesFinal head:
4c6bfb0c0cb5cf71a3d847b96621c46408152037Merged base:
98a701ffd95a9a9f237ca882aed8bbacf92ec167Exact-head build and tests
./scripts/reload.sh --tag issue-7851-synology-scp-sftp --launchcom.cmuxterm.app.debug.issue.7851.synology.scp.sftp, version0.64.20 (100)4c6bfb0c05793722afe1a21338756097c91488b8e932e2d8a92143188d0c9a4248da236deCmuxRemoteSessionSwiftPM suite: 111 tests in 17 suites passedgit diff --check, package lockfile policy, workspace package grouping, pbxproj test wiring, cmux/Aziz policy, and warning budget all passedmain, so the final audit also checks the last enforced budgets.Live SFTP-disabled verification
A deterministic loopback fixture uses OpenSSH 10.2 with SSH exec enabled and no SFTP subsystem. The SCP probe writes outside the app's cold remote home.
scpfails withsubsystem request failed(exit 255)scp -Osucceeds byte-identicallytransport=ssh-stdinupload installs an executable 7,432,928-byte helper byte-identical to the local binarychmodand atomicmv; no temporary residue remainsPermission deniederror, makes only the bounded 4/8/16-second retries, and publishes neither a final helper nor a partial fileStdinNull=yes, the uploader still transfers one complete byte-identical helper with zero residue; this is intentionally upload-only because that caller option disables the later long-lived stdin protocolThe retained nonvisual acceptance packet is bound to the exact head with a per-state SHA manifest. All 93 retained artifact hashes verify; its sensitive-data scan passed across all 94 files with zero secret/private-key matches and no zero-byte artifacts.
CI and review
Exact-head workflow: https://github.com/manaflow-ai/cmux/actions/runs/29784495752
The change-relevant jobs passed, including
remote-daemon-tests,diff-sidecar-check, web tests/typecheck/migrations, React checks, agent-session resources, and change detection. The repository-wide workflow stops at two strict determinism findings inPackages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohRelayCredentialCoordinatorTests+Refresh.swift(lines 43 and 46). That file is absent from this PR diff and has the identical blob0a953b3b51b37b7c0cc6d23b5b3d338f79d5e054in the PR head and merged base; downstream preflight/test/aggregate failures are consequences of that guard.GitHub has no configured required checks for this branch. All PR-attached checks pass (CodeRabbit, Greptile, Socket, Cubic; Vercel is neutral). Canonical structured autoreview reports zero actionable findings, cmux policy is clean, Greptile reports 5/5 safe to merge, all actionable review threads are resolved, and GitHub reports
MERGEABLE / CLEAN.Coverage limit
No physical Synology DSM appliance was available. The live fixture deterministically covers the reported bug class—OpenSSH 10.2 with working SSH exec and a broken/unavailable SFTP subsystem—and exercises the real tagged app, remote filesystem transaction, daemon, proxy, terminal, retry, and restore paths. CUA/video was excluded from final acceptance at the requester's direction.
Closes #7851