diff --git a/Memory/ARCHITECTURE.md b/Memory/ARCHITECTURE.md index a5959cc2..c5e7f8c5 100644 --- a/Memory/ARCHITECTURE.md +++ b/Memory/ARCHITECTURE.md @@ -152,6 +152,10 @@ recorded only after a fresh successful listing and files only after download. Failed, cancelled, stale, missing, or type-changed operations do not update it. Transfers own dedicated connections and outlive browser closure; host deletion and background expiration cancel them, and nothing is silently replayed. +Desktop and mobile upload/download preparation and enqueueing use the Remote +Infrastructure planners. Download batches retain their destination independently of +file jobs, so directory-only selections still create their empty directory trees. +App sessions retain pending-batch presentation, cancellation, and completion handling. ## Callback isolation diff --git a/Memory/REFACTOR.md b/Memory/REFACTOR.md index 3c244f23..e4b8e76a 100644 --- a/Memory/REFACTOR.md +++ b/Memory/REFACTOR.md @@ -94,3 +94,14 @@ This English-only document is the durable record of completed redundancy removal - SwiftData historical schemas remain frozen migration contracts, small structurally similar repositories and views remain separate when abstraction would not reduce code, and `RuntimeVersions` stays platform-local to avoid introducing an invalid Ghostty dependency. The 2026-09-02 completion and re-verification scans found no further independent dead-code or exact-duplication change that met the backlog's reduction and boundary-preservation rules; later work should be appended only after a fresh reference scan. + +## Fresh Refactoring Pass (2026-09-16) + +- **Reuse the shared SFTP download planner — completed.** Evidence: `rg -n 'PreparedJob|PreparedBatch|pendingBatch|keepBothLocalURL' TFM TFI Frameworks TFMTests TFITests` found only one desktop `PreparedJob`/`PreparedBatch` construction path, in `SFTPSessionModel.prepareDownload`, with direction fixed to `.download` and both remote-directory collections empty. Uploads already used `SFTPLocalUploadPlanner`. `SFTPRemoteDownloadPlanner` already owned the equivalent download traversal, local conflict policies, scoped destination access, directory creation, and transfer enqueueing for mobile. Adopted that planner in the desktop session and removed the obsolete mixed-direction batch and local helpers, retaining desktop pending-state/error handling, remote-copy policy, and transfer lifetimes. Estimated production reduction: 180 lines; actual reduction: 167 lines, or 59 lines including the added regression tests. Directory-only downloads now create their empty directory trees because the shared batch retains its destination independently of file jobs. The architecture record documents that ownership. + +Verification completed on 2026-09-16: + +- All 75 SFTP and architecture tests passed, including three new parameterized/behavioral tests covering ten file/directory conflict, subtree, empty-directory, and cancellation cases. The empty-directory regression fails against the pre-refactor `SFTPSessionModel` and passes with the final implementation. +- The full pinned TFM test action built successfully and passed 1,337 of 1,343 tests. Four failures also reproduce with the pre-refactor production source: `SettingsSyncTests.providedDatabaseCopyMigratesWithoutReset` (expects schema 15, current schema is 17), `ModelCapabilityResolverTests.reportedFieldsWinOverCodexAutoReviewOverride`, `ScrubberWebClientTests.fetchPreflightNeverEscalatesPrivateHTTP`, and `ChatComposerStateTests.aFailureNoticeIsNotAnAnswerWorthNaming`. `MetricsJourneyTests.offlineRestoredMetricsReconcileOnMainActor` passed on rerun. `SSHImportTests.indexedIdentityPlanningHandlesTheImportFileLimit` exceeded its two-second wall-clock limit when run concurrently, but passed in isolation against both versions (0.100 seconds with the final source). +- The full pinned TFI test action on iPhone 17 Pro / iOS 26.5 built successfully and passed 426 of 432 tests, including all 205 framework tests. The unchanged mobile graph retained five failures on isolated rerun: `MobileVaultFeatureTests.importRequiresAnExplicitUsernameWhenTheConfigurationOmitsIt` and four `MobileAssistantTests` cases (`completeRegistryIncludesRemoteAndBuiltInToolsOnlyForCapableModels`, `firstNewConversationSendPromotesDraftAndStartsRun`, `attachmentOnlyFirstSendPromotesTheConversation`, and `existingConversationSendStillUsesTheDraftExecutionPath`). `MobileEditorToolbarTests.iconButtonsKeepTheirNamesAndSaveLifecycle` passed on rerun. +- Strict Swift formatting across all production/test targets, `Scripts/check-zh-localization.sh`, the removed-symbol reference scan, and `git diff --check` passed. Both full test actions retained `-onlyUsePackageVersionsFromResolvedFile`; no dependency, localization, or shared production source changed. diff --git a/TFM/Features/SFTP/SFTPSessionModel.swift b/TFM/Features/SFTP/SFTPSessionModel.swift index eaf42dde..c0171b08 100644 --- a/TFM/Features/SFTP/SFTPSessionModel.swift +++ b/TFM/Features/SFTP/SFTPSessionModel.swift @@ -86,26 +86,6 @@ private struct SFTPDirectoryPresentation { @Observable final class SFTPSessionModel: Identifiable { - struct PreparedJob { - var direction: SFTPTransfer.Direction - var localURL: URL - var remotePath: String - var byteCount: Int64 - var scopedRoot: URL - var hasConflict: Bool - var conflictingTargetIsDirectory: Bool - } - - struct PreparedBatch { - var id = UUID() - var jobs: [PreparedJob] - var remoteDirectories: [String] - var remoteDirectoryConflicts: Set - var localDirectories: [URL] - var localDirectoryConflicts: Set - var conflictCount: Int - } - struct PreparedRemoteCopyJob { var sourcePath: String var destinationPath: String @@ -187,7 +167,7 @@ final class SFTPSessionModel: Identifiable { var pendingAuthentication: SSHAuthenticationPrompt? let connection = RemoteSessionConnectionController() var connectionJourney: ConnectionJourney { connection.journey } - var pendingBatch: PreparedBatch? + private var pendingDownloadBatch: SFTPRemoteDownloadBatch? var pendingUploadBatch: SFTPLocalUploadBatch? var pendingRemoteCopyBatch: PreparedRemoteCopyBatch? var operationError: String? @@ -253,11 +233,11 @@ final class SFTPSessionModel: Identifiable { var conflictCount: Int { pendingRemoteCopyBatch?.conflictCount ?? pendingUploadBatch?.conflictCount - ?? pendingBatch?.conflictCount + ?? pendingDownloadBatch?.conflictCount ?? 0 } var hasPendingBatch: Bool { - pendingUploadBatch != nil || pendingBatch != nil || pendingRemoteCopyBatch != nil + pendingUploadBatch != nil || pendingDownloadBatch != nil || pendingRemoteCopyBatch != nil } /// What the list draws. Already filtered, sorted, and formatted. var visibleRows: [SFTPRow] { directoryPresentation.visibleRows } @@ -831,7 +811,7 @@ final class SFTPSessionModel: Identifiable { func cancelPendingBatch() { pendingUploadBatch = nil - pendingBatch = nil + pendingDownloadBatch = nil pendingRemoteCopyBatch = nil } @@ -843,7 +823,7 @@ final class SFTPSessionModel: Identifiable { destination: path, client: client ) - pendingBatch = nil + pendingDownloadBatch = nil pendingRemoteCopyBatch = nil pendingUploadBatch = batch.isEmpty ? nil : batch } catch { @@ -854,48 +834,13 @@ final class SFTPSessionModel: Identifiable { func prepareDownload(_ files: [RemoteFile], to directory: URL) async { guard let client else { return } - var jobs: [PreparedJob] = [] - var directories: [URL] = [] - var directoryConflicts: Set = [] - var conflicts = 0 do { pendingUploadBatch = nil - func collect(_ remote: RemoteFile, local: URL) async throws { - if remote.isDirectory { - directories.append(local) - for child in try await client.listDirectory(at: remote.path) { - try await collect(child, local: local.appendingPathComponent(child.name)) - } - } else { - var isDirectory: ObjCBool = false - let exists = FileManager.default.fileExists(atPath: local.path, isDirectory: &isDirectory) - if exists { conflicts += 1 } - jobs.append( - PreparedJob( - direction: .download, localURL: local, remotePath: remote.path, - byteCount: remote.size, scopedRoot: directory, hasConflict: exists, - conflictingTargetIsDirectory: exists && isDirectory.boolValue - )) - } - } - for file in files { - try await collect(file, local: directory.appendingPathComponent(file.name)) - } - for directory in directories { - var isDirectory: ObjCBool = false - if FileManager.default.fileExists(atPath: directory.path, isDirectory: &isDirectory), - !isDirectory.boolValue - { - directoryConflicts.insert(directory) - conflicts += 1 - } - } - pendingRemoteCopyBatch = nil - pendingBatch = PreparedBatch( - jobs: jobs, remoteDirectories: [], remoteDirectoryConflicts: [], - localDirectories: directories, localDirectoryConflicts: directoryConflicts, - conflictCount: conflicts + let batch = try await SFTPRemoteDownloadPlanner.prepare( + files, destination: directory, client: client ) + pendingRemoteCopyBatch = nil + pendingDownloadBatch = batch } catch { operationError = error.localizedDescription } } @@ -935,7 +880,7 @@ final class SFTPSessionModel: Identifiable { jobs[index].conflictingTargetIsDirectory = kind } - pendingBatch = nil + pendingDownloadBatch = nil pendingRemoteCopyBatch = PreparedRemoteCopyBatch( source: manifest.source, jobs: jobs, remoteDirectories: preflight.directories, @@ -1082,113 +1027,15 @@ final class SFTPSessionModel: Identifiable { } return } - guard let batch = pendingBatch, let connection = transferConnection, let client else { + guard let batch = pendingDownloadBatch, let connection = transferConnection, client != nil else { return } - pendingBatch = nil + pendingDownloadBatch = nil do { - let remoteResolution = try await resolveRemoteDirectories( - batch.remoteDirectories, conflicts: batch.remoteDirectoryConflicts, - policy: policy, client: client + try SFTPRemoteDownloadPlanner.enqueue( + batch, policy: policy, hostID: hostID, connection: connection, + homeDirectory: homeResolution.resolvedDirectory, coordinator: coordinator ) - let localRoots = Set(batch.jobs.filter { $0.direction == .download }.map(\.scopedRoot)) - var skippedLocalPrefixes: [URL] = [] - var localMappings: [(URL, URL)] = [] - for root in localRoots { - let scoped = root.startAccessingSecurityScopedResource() - defer { if scoped { root.stopAccessingSecurityScopedResource() } } - for originalDirectory in batch.localDirectories where originalDirectory.path.hasPrefix(root.path) { - if skippedLocalPrefixes.contains(where: { - originalDirectory.path == $0.path - || originalDirectory.path.hasPrefix($0.path + "/") - }) { - continue - } - var directory = originalDirectory - for mapping in localMappings - where directory.path == mapping.0.path || directory.path.hasPrefix(mapping.0.path + "/") { - directory = URL( - fileURLWithPath: mapping.1.path + directory.path.dropFirst(mapping.0.path.count)) - } - if batch.localDirectoryConflicts.contains(originalDirectory) { - switch policy { - case .skip: - skippedLocalPrefixes.append(originalDirectory) - continue - case .replace: - try FileManager.default.removeItem(at: originalDirectory) - case .keepBoth: - let candidate = Self.keepBothLocalURL(originalDirectory) - localMappings.append((originalDirectory, candidate)) - directory = candidate - } - } - try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - } - } - for var job in batch.jobs { - if remoteResolution.skipped.contains(where: { - job.remotePath == $0 || job.remotePath.hasPrefix($0 + "/") - }) - || skippedLocalPrefixes.contains(where: { - job.localURL.path == $0.path || job.localURL.path.hasPrefix($0.path + "/") - }) - { - continue - } - for mapping in remoteResolution.mappings - where job.remotePath == mapping.0 || job.remotePath.hasPrefix(mapping.0 + "/") { - job.remotePath = mapping.1 + job.remotePath.dropFirst(mapping.0.count) - job.hasConflict = false - } - for mapping in localMappings - where job.localURL.path == mapping.0.path || job.localURL.path.hasPrefix(mapping.0.path + "/") { - let suffix = job.localURL.path.dropFirst(mapping.0.path.count) - job.localURL = URL(fileURLWithPath: mapping.1.path + suffix) - job.hasConflict = false - try FileManager.default.createDirectory( - at: job.localURL.deletingLastPathComponent(), withIntermediateDirectories: true - ) - } - if job.hasConflict { - if job.direction == .upload { - guard - let resolved = try await resolveRemoteConflict( - path: job.remotePath, - conflictingTargetIsDirectory: job.conflictingTargetIsDirectory, - policy: policy, client: client - ) - else { continue } - job.remotePath = resolved.path - job.hasConflict = resolved.overwrite - } else { - switch policy { - case .skip: continue - case .replace: - if job.conflictingTargetIsDirectory { - try FileManager.default.removeItem(at: job.localURL) - } - case .keepBoth: - job.localURL = Self.keepBothLocalURL(job.localURL) - } - } - } - let completion: ((SFTPTransfer.State) -> Void)? = - job.direction == .upload - ? { [weak self] state in - if state == .completed { self?.scheduleSilentRefresh() } - } - : nil - coordinator.enqueue( - direction: job.direction, batchID: batch.id, localURL: job.localURL, - remotePath: job.remotePath, byteCount: job.byteCount, hostID: hostID, - connection: connection, scopedRoot: job.scopedRoot, - overwrite: job.hasConflict && policy == .replace, - homeDirectory: homeResolution.resolvedDirectory, - completion: completion - ) - } - if remoteResolution.changed { scheduleSilentRefresh() } } catch { operationError = error.localizedDescription } } @@ -1429,20 +1276,6 @@ final class SFTPSessionModel: Identifiable { return result } - private static func keepBothLocalURL(_ url: URL) -> URL { - let ext = url.pathExtension - let base = url.deletingPathExtension() - var index = 2 - while true { - let name = - ext.isEmpty - ? "\(base.lastPathComponent).\(index)" - : "\(base.lastPathComponent).\(index).\(ext)" - let candidate = base.deletingLastPathComponent().appendingPathComponent(name) - if !FileManager.default.fileExists(atPath: candidate.path) { return candidate } - index += 1 - } - } } // MARK: - Demo data diff --git a/TFMTests/SFTPFeatureTests.swift b/TFMTests/SFTPFeatureTests.swift index 21c48416..681da773 100644 --- a/TFMTests/SFTPFeatureTests.swift +++ b/TFMTests/SFTPFeatureTests.swift @@ -791,6 +791,114 @@ struct SFTPFeatureTests { #expect(batch.conflictCount == 0) } + @Test(arguments: [TransferConflictPolicy.skip, .replace, .keepBoth], [false, true]) + func downloadingAFileResolvesExistingFilesAndDirectories( + policy: TransferConflictPolicy, existingIsDirectory: Bool + ) async throws { + let root = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) + try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: root) } + let original = root.appending(path: "report.txt") + if existingIsDirectory { + try FileManager.default.createDirectory(at: original, withIntermediateDirectories: true) + } + let preserved = existingIsDirectory ? original.appending(path: "keep.txt") : original + try Data("original".utf8).write(to: preserved) + let client = WritingSFTPClient() + let session = SFTPSessionModel( + hostID: UUID(), initialPath: "/source", client: client, configuration: testConfiguration + ) + let coordinator = SFTPTransferCoordinator(store: HostLibraryModel()) { client } + + await session.prepareDownload([remoteFile(path: "/source/report.txt", kind: .file)], to: root) + #expect(session.conflictCount == 1) + await session.enqueuePending(policy: policy, coordinator: coordinator) + for _ in 0..<200 { + guard coordinator.sftpTransfers.contains(where: { $0.state == .queued || $0.state == .running }) else { + break + } + try await Task.sleep(for: .milliseconds(10)) + } + + #expect(session.operationError == nil) + #expect(!session.hasPendingBatch) + if policy != .replace { #expect(try Data(contentsOf: preserved) == Data("original".utf8)) } + if policy == .skip { + #expect(coordinator.sftpTransfers.isEmpty) + } else { + let destination = policy == .keepBoth ? root.appending(path: "report.2.txt") : original + #expect(coordinator.sftpTransfers.map(\.state) == [.completed]) + #expect(try Data(contentsOf: destination) == Data("termind".utf8)) + } + } + + @Test(arguments: [TransferConflictPolicy.skip, .replace, .keepBoth]) + func downloadingAConflictingDirectoryAppliesThePolicyToItsWholeSubtree( + policy: TransferConflictPolicy + ) async throws { + let root = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) + try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: root) } + let original = root.appending(path: "bundle") + try Data("original".utf8).write(to: original) + let bundle = remoteFile(path: "/source/bundle", kind: .directory) + let nested = remoteFile(path: "/source/bundle/nested", kind: .directory) + let file = remoteFile(path: "/source/bundle/nested/payload.txt", kind: .file) + let sibling = remoteFile(path: "/source/bundle-other", kind: .directory) + let client = TreeSFTPClient(listings: [bundle.path: [nested], nested.path: [file]]) + let session = SFTPSessionModel( + hostID: UUID(), initialPath: "/source", client: client, configuration: testConfiguration + ) + let coordinator = SFTPTransferCoordinator(store: HostLibraryModel()) { client } + + await session.prepareDownload([bundle, sibling], to: root) + #expect(session.conflictCount == 1) + await session.enqueuePending(policy: policy, coordinator: coordinator) + + #expect(session.operationError == nil) + #expect(!session.hasPendingBatch) + #expect(try root.appending(path: "bundle-other").resourceValues(forKeys: [.isDirectoryKey]).isDirectory == true) + if policy != .replace { #expect(try Data(contentsOf: original) == Data("original".utf8)) } + if policy == .skip { + #expect(coordinator.sftpTransfers.isEmpty) + } else { + let directory = policy == .keepBoth ? root.appending(path: "bundle.2") : original + let destination = directory.appending(path: "nested/payload.txt") + #expect(coordinator.sftpTransfers.map(\.localPath) == [destination.path]) + #expect(coordinator.sftpTransfers.map(\.remotePath) == [file.path]) + #expect(coordinator.sftpTransfers.map(\.direction) == [.download]) + #expect( + try directory.appending(path: "nested").resourceValues(forKeys: [.isDirectoryKey]).isDirectory == true) + } + } + + @Test + func downloadingOnlyEmptyDirectoriesCreatesThemUnlessTheBatchWasCancelled() async throws { + let root = FileManager.default.temporaryDirectory.appending(path: UUID().uuidString) + defer { try? FileManager.default.removeItem(at: root) } + let bundle = remoteFile(path: "/source/bundle", kind: .directory) + let empty = remoteFile(path: "/source/bundle/empty", kind: .directory) + let client = TreeSFTPClient(listings: [bundle.path: [empty]]) + let session = SFTPSessionModel( + hostID: UUID(), initialPath: "/source", client: client, configuration: testConfiguration + ) + let coordinator = SFTPTransferCoordinator(store: HostLibraryModel()) { client } + + await session.prepareDownload([bundle], to: root) + #expect(session.hasPendingBatch) + session.cancelPendingBatch() + await session.enqueuePending(policy: .skip, coordinator: coordinator) + #expect(!FileManager.default.fileExists(atPath: root.path)) + + await session.prepareDownload([bundle], to: root) + await session.enqueuePending(policy: .skip, coordinator: coordinator) + + #expect(session.operationError == nil) + #expect(!session.hasPendingBatch) + #expect(coordinator.sftpTransfers.isEmpty) + #expect(try root.appending(path: "bundle/empty").resourceValues(forKeys: [.isDirectoryKey]).isDirectory == true) + } + @Test func remoteCopyPreflightFindsTargetConflicts() async { let sourceHostID = UUID()