From 43c61f6e63ea1481611154ab431d117aa23a6e0c Mon Sep 17 00:00:00 2001 From: Lawrence Chen <54008264+lawrencecchen@users.noreply.github.com> Date: Fri, 20 Mar 2026 21:32:21 -0700 Subject: [PATCH] Fix SSH image transfer followups (#1904) * Add regressions for SSH image transfer followups * Fix SSH image transfer followup regressions --------- Co-authored-by: Lawrence Chen --- Sources/TerminalImageTransfer.swift | 24 ++- Sources/TerminalSSHSessionDetector.swift | 172 +++++++++++++++--- Sources/Workspace.swift | 8 + cmuxTests/TerminalAndGhosttyTests.swift | 25 +++ .../WorkspaceRemoteConnectionTests.swift | 90 +++++++++ 5 files changed, 289 insertions(+), 30 deletions(-) diff --git a/Sources/TerminalImageTransfer.swift b/Sources/TerminalImageTransfer.swift index aeb00e5d..ce847304 100644 --- a/Sources/TerminalImageTransfer.swift +++ b/Sources/TerminalImageTransfer.swift @@ -166,11 +166,11 @@ enum TerminalImageTransferPlanner { switch target { case .local: - let text = fileURLs - .map { escapeForShell($0.path) } - .joined(separator: " ") - return .insertText(text) + return .insertText(insertedText(for: fileURLs)) 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 { diff --git a/Sources/TerminalSSHSessionDetector.swift b/Sources/TerminalSSHSessionDetector.swift index 3186e486..3f6d6130 100644 --- a/Sources/TerminalSSHSessionDetector.swift +++ b/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() - return remotePaths + do { + 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 - try operation.throwIfCancelled() - let normalizedLocalURL = localURL.standardizedFileURL - guard normalizedLocalURL.isFileURL else { - throw NSError(domain: "cmux.detected-ssh.drop", code: 1, userInfo: [ - NSLocalizedDescriptionKey: "dropped item is not a file URL", - ]) + var uploadedRemotePaths: [String] = [] + do { + for localURL in fileURLs { + try operation.throwIfCancelled() + let normalizedLocalURL = localURL.standardizedFileURL + 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) - 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)", - ]) - } - - return remotePath + return uploadedRemotePaths + } catch { + cleanupUploadedRemotePaths(uploadedRemotePaths) + throw error } } @@ -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) diff --git a/Sources/Workspace.swift b/Sources/Workspace.swift index da7df21c..2ce458de 100644 --- a/Sources/Workspace.swift +++ b/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), diff --git a/cmuxTests/TerminalAndGhosttyTests.swift b/cmuxTests/TerminalAndGhosttyTests.swift index 0f6a1101..b8dcd806 100644 --- a/cmuxTests/TerminalAndGhosttyTests.swift +++ b/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() diff --git a/cmuxTests/WorkspaceRemoteConnectionTests.swift b/cmuxTests/WorkspaceRemoteConnectionTests.swift index 797c96a9..ff20d388 100644 --- a/cmuxTests/WorkspaceRemoteConnectionTests.swift +++ b/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",