Fix SSH image transfer followups (#1904)

* Add regressions for SSH image transfer followups

* Fix SSH image transfer followup regressions

---------

Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>

Sources/TerminalImageTransfer.swift

@@ -166,11 +166,11 @@ enum TerminalImageTransferPlanner {
 
         switch target {
         case .local:
-            let text = fileURLs
+            return .insertText(insertedText(for: fileURLs))
-                .map { escapeForShell($0.path) }
-                .joined(separator: " ")
-            return .insertText(text)
         case .remote(let remoteTarget):
+            guard fileURLs.allSatisfy(isRemoteUploadableFileURL) else {
+                return .insertText(insertedText(for: fileURLs))
+            }
             return .uploadFiles(fileURLs, remoteTarget)
         }
     }
@@ -233,6 +233,22 @@ enum TerminalImageTransferPlanner {
         GhosttyPasteboardHelper.escapeForShell(value)
     }
 
+    private static func insertedText(for fileURLs: [URL]) -> String {
+        fileURLs
+            .map { escapeForShell($0.path) }
+            .joined(separator: " ")
+    }
+
+    private static func isRemoteUploadableFileURL(_ fileURL: URL) -> Bool {
+        let normalizedFileURL = fileURL.standardizedFileURL
+        guard normalizedFileURL.isFileURL,
+              let resourceValues = try? normalizedFileURL.resourceValues(forKeys: [.isRegularFileKey]),
+              resourceValues.isRegularFile == true else {
+            return false
+        }
+        return true
+    }
+
     private static func preparePaste(
         pasteboard: NSPasteboard
     ) -> TerminalImageTransferPreparedContent {

Sources/TerminalSSHSessionDetector.swift

@@ -21,13 +21,24 @@ struct DetectedSSHSession: Equatable {
     ) {
         let session = self
         DispatchQueue.global(qos: .userInitiated).async {
-            let result = Result {
+            let result: Result<[String], Error>
+            do {
                 let remotePaths = try session.uploadDroppedFilesSync(fileURLs, operation: operation)
-                try operation.throwIfCancelled()
+                do {
-                return remotePaths
+                    try operation.throwIfCancelled()
+                    result = .success(remotePaths)
+                } catch {
+                    session.cleanupUploadedRemotePathsAsync(remotePaths)
+                    result = .failure(error)
+                }
+            } catch {
+                result = .failure(error)
             }
             DispatchQueue.main.async {
                 if operation.isCancelled {
+                    if case .success(let remotePaths) = result {
+                        session.cleanupUploadedRemotePathsAsync(remotePaths)
+                    }
                     completion(.failure(TerminalImageTransferExecutionError.cancelled))
                 } else {
                     completion(result)
@@ -47,37 +58,67 @@ struct DetectedSSHSession: Equatable {
         )
     }
 
+#if DEBUG
+    typealias ProcessOverrideResultForTesting = (
+        status: Int32,
+        stdout: String,
+        stderr: String
+    )
+
+    static var runProcessOverrideForTesting: ((
+        String,
+        [String],
+        TimeInterval,
+        TerminalImageTransferOperation?
+    ) throws -> ProcessOverrideResultForTesting)?
+
+    func uploadDroppedFilesSyncForTesting(
+        _ fileURLs: [URL],
+        operation: TerminalImageTransferOperation = TerminalImageTransferOperation()
+    ) throws -> [String] {
+        try uploadDroppedFilesSync(fileURLs, operation: operation)
+    }
+#endif
+
     private func uploadDroppedFilesSync(
         _ fileURLs: [URL],
         operation: TerminalImageTransferOperation
     ) throws -> [String] {
         guard !fileURLs.isEmpty else { return [] }
 
-        return try fileURLs.map { localURL in
+        var uploadedRemotePaths: [String] = []
-            try operation.throwIfCancelled()
+        do {
-            let normalizedLocalURL = localURL.standardizedFileURL
+            for localURL in fileURLs {
-            guard normalizedLocalURL.isFileURL else {
+                try operation.throwIfCancelled()
-                throw NSError(domain: "cmux.detected-ssh.drop", code: 1, userInfo: [
+                let normalizedLocalURL = localURL.standardizedFileURL
-                    NSLocalizedDescriptionKey: "dropped item is not a file URL",
+                guard normalizedLocalURL.isFileURL else {
-                ])
+                    throw NSError(domain: "cmux.detected-ssh.drop", code: 1, userInfo: [
+                        NSLocalizedDescriptionKey: "dropped item is not a file URL",
+                    ])
+                }
+
+                let remotePath = WorkspaceRemoteSessionController.remoteDropPath(for: normalizedLocalURL)
+                let result = try Self.runProcess(
+                    executable: "/usr/bin/scp",
+                    arguments: scpArguments(localPath: normalizedLocalURL.path, remotePath: remotePath),
+                    timeout: 45,
+                    operation: operation
+                )
+                guard result.status == 0 else {
+                    let detail = Self.bestErrorLine(stderr: result.stderr, stdout: result.stdout) ??
+                        "scp exited \(result.status)"
+                    throw NSError(domain: "cmux.detected-ssh.drop", code: 2, userInfo: [
+                        NSLocalizedDescriptionKey: "failed to upload dropped file: \(detail)",
+                    ])
+                }
+
+                uploadedRemotePaths.append(remotePath)
             }
 
-            let remotePath = WorkspaceRemoteSessionController.remoteDropPath(for: normalizedLocalURL)
+            return uploadedRemotePaths
-            let result = try Self.runProcess(
+        } catch {
-                executable: "/usr/bin/scp",
+            cleanupUploadedRemotePaths(uploadedRemotePaths)
-                arguments: scpArguments(localPath: normalizedLocalURL.path, remotePath: remotePath),
+            throw error
-                timeout: 45,
-                operation: operation
-            )
-            guard result.status == 0 else {
-                let detail = Self.bestErrorLine(stderr: result.stderr, stdout: result.stdout) ??
-                    "scp exited \(result.status)"
-                throw NSError(domain: "cmux.detected-ssh.drop", code: 2, userInfo: [
-                    NSLocalizedDescriptionKey: "failed to upload dropped file: \(detail)",
-                ])
-            }
-
-            return remotePath
         }
     }
 
@@ -130,6 +171,74 @@ struct DetectedSSHSession: Equatable {
         return args
     }
 
+    private func sshArguments(command: String) -> [String] {
+        var args: [String] = [
+            "-T",
+            "-o", "ConnectTimeout=6",
+            "-o", "ServerAliveInterval=20",
+            "-o", "ServerAliveCountMax=2",
+            "-o", "BatchMode=yes",
+            "-o", "ControlMaster=no",
+        ]
+
+        if useIPv4 {
+            args.append("-4")
+        } else if useIPv6 {
+            args.append("-6")
+        }
+        if forwardAgent {
+            args.append("-A")
+        }
+        if compressionEnabled {
+            args.append("-C")
+        }
+        if let configFile, !configFile.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
+            args += ["-F", configFile]
+        }
+        if let jumpHost, !jumpHost.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
+            args += ["-J", jumpHost]
+        }
+        if let port {
+            args += ["-p", String(port)]
+        }
+        if let identityFile, !identityFile.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
+            args += ["-i", identityFile]
+        }
+        if let controlPath,
+           !controlPath.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty,
+           !Self.hasSSHOptionKey(sshOptions, key: "ControlPath") {
+            args += ["-o", "ControlPath=\(controlPath)"]
+        }
+        if !Self.hasSSHOptionKey(sshOptions, key: "StrictHostKeyChecking") {
+            args += ["-o", "StrictHostKeyChecking=accept-new"]
+        }
+        for option in sshOptions {
+            args += ["-o", option]
+        }
+
+        args += [destination, command]
+        return args
+    }
+
+    private func cleanupUploadedRemotePaths(_ remotePaths: [String]) {
+        guard !remotePaths.isEmpty else { return }
+        let cleanupScript = "rm -f -- " + remotePaths.map(Self.shellSingleQuoted).joined(separator: " ")
+        let cleanupCommand = "sh -c \(Self.shellSingleQuoted(cleanupScript))"
+        _ = try? Self.runProcess(
+            executable: "/usr/bin/ssh",
+            arguments: sshArguments(command: cleanupCommand),
+            timeout: 8
+        )
+    }
+
+    private func cleanupUploadedRemotePathsAsync(_ remotePaths: [String]) {
+        guard !remotePaths.isEmpty else { return }
+        let session = self
+        DispatchQueue.global(qos: .utility).async {
+            session.cleanupUploadedRemotePaths(remotePaths)
+        }
+    }
+
     private struct CommandResult {
         let status: Int32
         let stdout: String
@@ -142,6 +251,13 @@ struct DetectedSSHSession: Equatable {
         timeout: TimeInterval,
         operation: TerminalImageTransferOperation? = nil
     ) throws -> CommandResult {
+#if DEBUG
+        if let runProcessOverrideForTesting {
+            let result = try runProcessOverrideForTesting(executable, arguments, timeout, operation)
+            return CommandResult(status: result.status, stdout: result.stdout, stderr: result.stderr)
+        }
+#endif
+
         let process = Process()
         let stdoutPipe = Pipe()
         let stderrPipe = Pipe()
@@ -230,6 +346,10 @@ struct DetectedSSHSession: Equatable {
             .lowercased()
     }
 
+    private static func shellSingleQuoted(_ value: String) -> String {
+        "'" + value.replacingOccurrences(of: "'", with: "'\"'\"'") + "'"
+    }
+
 #if DEBUG
     func scpArgumentsForTesting(localPath: String, remotePath: String) -> [String] {
         scpArguments(localPath: localPath, remotePath: remotePath)

Sources/Workspace.swift

@@ -5576,6 +5576,7 @@ final class Workspace: Identifiable, ObservableObject {
         let isLoading: Bool
         let isPinned: Bool
         let directory: String?
+        let ttyName: String?
         let cachedTitle: String?
         let customTitle: String?
         let manuallyUnread: Bool
@@ -7878,6 +7879,11 @@ final class Workspace: Identifiable, ObservableObject {
         if let directory = detached.directory {
             panelDirectories[detached.panelId] = directory
         }
+        if let ttyName = detached.ttyName?.trimmingCharacters(in: .whitespacesAndNewlines), !ttyName.isEmpty {
+            surfaceTTYNames[detached.panelId] = ttyName
+        } else {
+            surfaceTTYNames.removeValue(forKey: detached.panelId)
+        }
         if let cachedTitle = detached.cachedTitle {
             panelTitles[detached.panelId] = cachedTitle
         }
@@ -7910,6 +7916,7 @@ final class Workspace: Identifiable, ObservableObject {
         ) else {
             panels.removeValue(forKey: detached.panelId)
             panelDirectories.removeValue(forKey: detached.panelId)
+            surfaceTTYNames.removeValue(forKey: detached.panelId)
             panelTitles.removeValue(forKey: detached.panelId)
             panelCustomTitles.removeValue(forKey: detached.panelId)
             pinnedPanelIds.remove(detached.panelId)
@@ -9734,6 +9741,7 @@ extension Workspace: BonsplitDelegate {
                 isLoading: browserPanel?.isLoading ?? false,
                 isPinned: pinnedPanelIds.contains(panelId),
                 directory: panelDirectories[panelId],
+                ttyName: surfaceTTYNames[panelId],
                 cachedTitle: cachedTitle,
                 customTitle: panelCustomTitles[panelId],
                 manuallyUnread: manualUnreadPanelIds.contains(panelId),

cmuxTests/TerminalAndGhosttyTests.swift

@@ -275,6 +275,31 @@ final class GhosttyPasteboardHelperTests: XCTestCase {
         XCTAssertEqual(urls, [fileURL])
     }
 
+    func testRemoteDirectoryPastePlanFallsBackToEscapedPathInsertion() throws {
+        let directoryURL = FileManager.default.temporaryDirectory.appendingPathComponent(
+            "clipboard-folder-\(UUID().uuidString)",
+            isDirectory: true
+        )
+        try FileManager.default.createDirectory(at: directoryURL, withIntermediateDirectories: true)
+        defer { try? FileManager.default.removeItem(at: directoryURL) }
+
+        let pasteboard = NSPasteboard(name: .init("cmux-test-remote-directory-paste-\(UUID().uuidString)"))
+        pasteboard.clearContents()
+        XCTAssertTrue(pasteboard.writeObjects([directoryURL as NSURL]))
+
+        let plan = TerminalImageTransferPlanner.plan(
+            pasteboard: pasteboard,
+            mode: .paste,
+            target: .remote(.workspaceRemote)
+        )
+
+        guard case .insertText(let text) = plan else {
+            return XCTFail("expected directory path insertion, got \(plan)")
+        }
+
+        XCTAssertEqual(text, TerminalImageTransferPlanner.escapeForShell(directoryURL.path))
+    }
+
     func testLazyPastePlanSkipsTargetResolutionForPlainText() {
         let pasteboard = NSPasteboard(name: .init("cmux-test-lazy-text-paste-\(UUID().uuidString)"))
         pasteboard.clearContents()

cmuxTests/WorkspaceRemoteConnectionTests.swift

@@ -297,6 +297,96 @@ final class WorkspaceRemoteConnectionTests: XCTestCase {
         XCTAssertTrue(workspace.isRemoteTerminalSurface(movedPanel.id))
     }
 
+    @MainActor
+    func testDetachAttachPreservesSurfaceTTYMetadata() throws {
+        let source = Workspace()
+        let destination = Workspace()
+
+        let panelID = try XCTUnwrap(source.focusedTerminalPanel?.id)
+        let sourcePaneID = try XCTUnwrap(source.paneId(forPanelId: panelID))
+        let destinationPaneID = try XCTUnwrap(destination.bonsplitController.allPaneIds.first)
+        source.surfaceTTYNames[panelID] = "/dev/ttys004"
+
+        let detached = try XCTUnwrap(source.detachSurface(panelId: panelID))
+        XCTAssertEqual(source.surfaceTTYNames[panelID], nil)
+
+        let restoredPanelID = destination.attachDetachedSurface(
+            detached,
+            inPane: destinationPaneID,
+            focus: false
+        )
+
+        XCTAssertEqual(restoredPanelID, panelID)
+        XCTAssertEqual(destination.surfaceTTYNames[panelID], "/dev/ttys004")
+        XCTAssertEqual(source.bonsplitController.tabs(inPane: sourcePaneID).count, 0)
+    }
+
+    func testDetectedSSHUploadFailureCleansUpEarlierRemoteUploads() throws {
+        let fileManager = FileManager.default
+        let directoryURL = fileManager.temporaryDirectory.appendingPathComponent(
+            "cmux-detected-ssh-upload-\(UUID().uuidString)",
+            isDirectory: true
+        )
+        try fileManager.createDirectory(at: directoryURL, withIntermediateDirectories: true)
+        defer { try? fileManager.removeItem(at: directoryURL) }
+
+        let firstFileURL = directoryURL.appendingPathComponent("first.png")
+        let secondFileURL = directoryURL.appendingPathComponent("second.png")
+        try Data("first".utf8).write(to: firstFileURL)
+        try Data("second".utf8).write(to: secondFileURL)
+
+        let session = DetectedSSHSession(
+            destination: "lawrence@example.com",
+            port: 2200,
+            identityFile: "/Users/test/.ssh/id_ed25519",
+            configFile: nil,
+            jumpHost: nil,
+            controlPath: nil,
+            useIPv4: false,
+            useIPv6: false,
+            forwardAgent: false,
+            compressionEnabled: false,
+            sshOptions: []
+        )
+
+        var invocations: [(executable: String, arguments: [String])] = []
+        var scpInvocationCount = 0
+        DetectedSSHSession.runProcessOverrideForTesting = { executable, arguments, _, _ in
+            invocations.append((executable, arguments))
+            if executable == "/usr/bin/scp" {
+                scpInvocationCount += 1
+                if scpInvocationCount == 1 {
+                    return (status: 0, stdout: "", stderr: "")
+                }
+                return (status: 1, stdout: "", stderr: "copy failed")
+            }
+            if executable == "/usr/bin/ssh" {
+                return (status: 0, stdout: "", stderr: "")
+            }
+            XCTFail("unexpected executable \(executable)")
+            return (status: 1, stdout: "", stderr: "unexpected executable")
+        }
+        defer { DetectedSSHSession.runProcessOverrideForTesting = nil }
+
+        XCTAssertThrowsError(
+            try session.uploadDroppedFilesSyncForTesting([firstFileURL, secondFileURL])
+        )
+
+        let firstSCPDestination = try XCTUnwrap(
+            invocations
+                .first(where: { $0.executable == "/usr/bin/scp" })?
+                .arguments
+                .last
+        )
+        let uploadedRemotePath = try XCTUnwrap(firstSCPDestination.split(separator: ":", maxSplits: 1).last)
+        let cleanupInvocation = try XCTUnwrap(
+            invocations.first(where: { $0.executable == "/usr/bin/ssh" })
+        )
+        let cleanupCommand = cleanupInvocation.arguments.joined(separator: " ")
+
+        XCTAssertTrue(cleanupCommand.contains(String(uploadedRemotePath)))
+    }
+
     func testDetectsForegroundSSHSessionForTTY() {
         let session = TerminalSSHSessionDetector.detectForTesting(
             ttyName: "/dev/ttys004",