File Provider: add strict writes and contained remote items - #33
Conversation
Configure Remux with the accepted shared App Group root and explicit application and shared Keychain access groups. Keep the credential service identity centralized while allowing the existing Keychain store to target either access group through the accepted structured query path. Preserve the exact accepted F1 test boundary: shared-root resolution, explicit access-group query construction, and shared configuration lookup. The focused suite first failed at the expected missing symbols, then passed 3 tests with 0 failures and 0 skips. A Remux simulator build also succeeded. Generate the app entitlement, plist values, and Xcode project membership from the accepted configuration with XcodeGen 2.44.1; consecutive generations were deterministic. Shared-state migration, live application/shared Keychain separation, and all File Provider extension wiring remain deferred to F2 and later branches.
Copy profiles, credentials, and hostname-bound trust into the shared container without deleting application-local source state. Verify copied credentials and repository/trust snapshots before atomically writing the migration marker, so failed attempts remain retryable and completed attempts are idempotent. Expose only the application/shared Keychain store factory at this layer. Migration construction, authoritative shared-repository selection, and lifecycle activation remain intentionally absent for the final app-integration layer. Preserve the SSH setup rollback-only trust APIs while adding bulk migration access and binding accepted trust to both server identity and hostname. Verification: accepted tests produced the expected missing-helper compile RED; the focused migration, shared-storage, and trusted-host suites passed 11 tests with 0 failures and 0 skips; the Remux simulator build succeeded. XcodeGen 2.44.1 was deterministic, runtime production delta is 124 lines, and all complete blobs/hunks match their accepted provenance.
Transplant the accepted inactive File Provider domain model exactly as reviewed. Eligible records require a saved SSH credential and trusted host identity matching both server ID and hostname. UUID-derived domain identifiers remain stable, display-name changes reconcile as remove/add pairs, and a FIFO actor gate serializes concurrent callers. Keep this leaf deliberately unreachable: it links FileProvider.framework into the app target but adds no dependency factory, live instance, RootModel or lifecycle invocation, host-mutation hook, extension target, or compatibility path. Registry and storage failures propagate after the gate is released; no new retry or cleanup behavior is introduced. The accepted three-test blob covers eligibility, deterministic add/rename/remove behavior with concurrent reconciliation serialized to one mutation, and host-change removal until replacement hostname trust. The tests-first build failed at the expected missing F3 types. The final focused suite passed 3/3 with no failures or skips, and the Remux iPhone 17 simulator build succeeded. XcodeGen 2.44.1 generated the same PBX blob twice.
Move transport startup tracing into a dedicated source that supplies the accepted no-op implementation when REMUX_FILE_PROVIDER_EXTENSION is active. Keep Ghostty tracing out of extension-conditioned SSH cleanup and SFTP setup, and expose root-key construction from the server and resolved-auth pair needed by extension callers. Regenerate deterministic app-target membership for the new source with XcodeGen 2.44.1. The existing SSH transport and exec selectors pass 74/74 and the normal iPhone 17 simulator build succeeds through XcodeBuildMCP with the required compiler wrappers. No test file changes are included. Global conditioned CLI builds remain a separate target-local extension gate because the setting propagates into SwiftPM dependencies and fails inside Crypto before Remux is compiled.
Define structured SFTP file type, metadata, directory-entry, readable-file, and read-only client values for listings and downloads. Bound downloads to monotonic chunks and ensure success, timeout, and cancellation close child handles and clean partial files. Reuse the existing connection lease and operation-timeout boundaries. Evidence: TerminalPreviewFileLoaderTests passed 7/7 at baseline; the focused suites retained the expected missing-read-layer compile failure before production and then passed 15/15; the normal iPhone 17 simulator build passed. XcodeGen 2.44.1 produced deterministic test membership.
Implement strict exclusive SFTP upload creation together with explicit directory creation, rename, file removal, and empty-directory removal operations. Normalize Citadel write statuses into typed permission and unsupported-mutation errors. Close remote upload handles on cancellation without issuing later mutations, and keep terminal attachments on the shared SFTP upload behavior. Evidence: the write tests produced a compile failure on the absent contracts before the source changes; all 38 focused tests passed afterward; the normal Remux iPhone 17 simulator build passed with isolated DerivedData and the required compiler probes; exact blob, scope, line-count, project-file, and diff checks passed.
Reconstruct the accepted remote-item model as one reviewable F7 commit directly on the strict SFTP write transport boundary. This moves only the immutable accepted bytes for canonical relative paths, File Provider identifier encoding, contained symlink projection, remote-item metadata/versioning, and their 12 focused tests. Accepted provenance: - FileProviderRemotePath.swift: 16813f6 - FileProviderRemoteItem.swift: 859c016 - FileProviderRemoteItemTests.swift: f8527a8 - source commits: df3cd2c, f2f82db, and 9fcd3fd Verification: - parent File Provider storage/domain suites: 10/10 passing - tests-first RED: FileProviderRemoteItemTests failed only for the absent accepted model types - GREEN: FileProviderRemoteItemTests 12/12 passing - normal iPhone 17 simulator build passed - XcodeGen 2.44.1 was deterministic across two generations - project.yml remains byte-identical to the parent - the 12 generated PBX membership lines are an accepted-oracle subset This deliberately excludes FileProviderRemoteService changes, stable opaque identity work, writable projection contracts, File Provider extension activation, compatibility behavior, and any newly authored product or test logic.
📝 WalkthroughWalkthroughChangesThe project adds FileProvider support with shared app-group storage, keychain access groups, migration, domain reconciliation, remote item models, and SFTP file operations. It also adds extension-specific tracing behavior and comprehensive async tests. FileProvider integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f9e708885
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| case .write: | ||
| return try await connection.remuxOpenFileForWriting( | ||
| atPath: remotePath, | ||
| flags: [.write, .create, .forceCreate] |
There was a problem hiding this comment.
When a terminal attachment write times out after the server creates its remote file, GhosttyAttachmentTransfer.upload deliberately skips cleanup for .operationTimedOut, while the path builder reuses the deterministic .<filename>.part name on the next attempt. Opening every upload with .forceCreate makes that retry—and all later uploads of the same filename—fail on the stale partial file; keep exclusive creation specific to File Provider writes or otherwise retain retry/cleanup behavior for attachment staging.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
RemuxApp/Sources/Persistence/ApplicationStorage.swift (1)
57-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or use the
fileManagerparameter.
sharedRemuxRootacceptsfileManagerbut never uses it.remuxRootuses itsfileManagerto create the directory, so the two functions behave differently for the same parameter name. Either create the directory here, or drop the parameter.♻️ Option: create the shared root directory
) throws -> URL { guard let containerURL = containerURL(appGroupIdentifier) else { throw FileProviderSharedConfigurationError.missingSharedContainer } - return containerURL.appendingPathComponent("Remux", isDirectory: true) + let root = containerURL.appendingPathComponent("Remux", isDirectory: true) + try fileManager.createDirectory(at: root, withIntermediateDirectories: true) + return root }🤖 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 `@RemuxApp/Sources/Persistence/ApplicationStorage.swift` around lines 57 - 71, Update sharedRemuxRoot so its fileManager parameter is either removed or used consistently; prefer creating the “Remux” directory through fileManager before returning the URL, matching remuxRoot’s behavior and preserving its existing error handling.RemuxApp/Sources/Persistence/SSHCredentialStore.swift (1)
166-188: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
kSecAttrAccessiblewhen adding shared credential keychain items.
saveCredentialcreates new Shared credential items withSecItemAdd, but the query used for both update and add carries nokSecAttrAccessible, so new shared items use the default unlocked-only accessibility. File Provider background sync can read Shared items while the device is locked, which fails witherrSecInteractionNotAllowed. Add an explicit accessibility value, such askSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, on the add path only; do not set it in the match query.🤖 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 `@RemuxApp/Sources/Persistence/SSHCredentialStore.swift` around lines 166 - 188, Update saveCredential and the add-item attributes around query to set kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly only when constructing attributes for SecItemAdd. Keep query’s match fields unchanged so the accessibility value is not used for lookup or update matching.RemuxApp/Sources/Persistence/TrustedHostStore.swift (1)
88-92: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDocument that
replaceIdentitiesbypasses trust validation.
restoreIdentitycarries an explicit warning at lines 59-62 that it bypasses host-key challenge and trust-transition validation.replaceIdentitiesis a wider bypass. It overwrites the whole trust store with no validation. Add an equivalent doc comment so future callers do not use it to establish new trust.📝 Proposed doc comment
+ /// Replaces the entire trust store contents. + /// + /// This intentionally bypasses host-key challenge and trust-transition + /// validation. Callers must not use it to establish new trust. It exists + /// for migration and snapshot restore only. func replaceIdentities(_ identities: [TrustedHostIdentity]) throws {🤖 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 `@RemuxApp/Sources/Persistence/TrustedHostStore.swift` around lines 88 - 92, Add a doc comment immediately above replaceIdentities(_:) stating that it bypasses host-key challenge and trust-transition validation, overwrites the entire trust store without validation, and must not be used to establish new trust.RemuxAppTests/FileProviderDomainReconcilerTests.swift (1)
113-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests write real keychain items without guaranteed cleanup. Both test files save credentials through a live
KeychainSSHCredentialStoreunder a randomized service name, and neither guarantees deletion. Temporary directories are reclaimed by the OS, but keychain items persist, so every run accumulates orphan entries on developer machines and on CI.
RemuxAppTests/FileProviderDomainReconcilerTests.swift#L113-L138: replace the liveKeychainSSHCredentialStoreat line 122 with an in-memorySSHCredentialStore. These tests verify domain reconciliation, not keychain behaviour, so nothing is lost.RemuxAppTests/FileProviderSharedStorageTests.swift#L37-L53: keep the live store, because the test verifies real access-group separation. Move the deletes at lines 51-52 into anaddTeardownBlockregistered before the first save, so cleanup runs when an earlier call throws.🤖 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 `@RemuxAppTests/FileProviderDomainReconcilerTests.swift` around lines 113 - 138, The test setup in RemuxAppTests/FileProviderDomainReconcilerTests.swift, within init, should use the in-memory SSHCredentialStore instead of KeychainSSHCredentialStore. In RemuxAppTests/FileProviderSharedStorageTests.swift, preserve the live store required by the access-group test, but move both credential deletions into an addTeardownBlock registered before the first save so cleanup runs even when setup throws.RemuxAppTests/FileProviderSharedStorageMigratorTests.swift (1)
49-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the concrete error in the migration failure test.
XCTAssertThrowsErrorAsyncdiscards the thrown error at line 220. The test at line 49 therefore passes for any failure, including an unrelated file-system error raised bysaveIdentitybeforeFailingOnceCredentialStoreis ever reached. That would silently hide a regression in the credential copy path.Forward the error to a caller-supplied handler and assert the expected case.
♻️ Proposed change to surface the thrown error
private func XCTAssertThrowsErrorAsync( _ expression: () async throws -> Void, file: StaticString = `#filePath`, - line: UInt = `#line` + line: UInt = `#line`, + _ errorHandler: (Error) -> Void = { _ in } ) async { do { try await expression() XCTFail("expected error", file: file, line: line) - } catch {} + } catch { + errorHandler(error) + } }Then tighten the call site.
Failureis currently private toFailingOnceCredentialStore; expose it or assert on the store's own marker:await XCTAssertThrowsErrorAsync { try await fixture.migrator.migrateIfNeeded() + } errorHandler: { error in + XCTAssertFalse(error is FileProviderSharedStorageMigrationError, "expected the store failure, not a verification failure") }Also applies to: 212-221
🤖 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 `@RemuxAppTests/FileProviderSharedStorageMigratorTests.swift` around lines 49 - 51, Update XCTAssertThrowsErrorAsync to accept and invoke a caller-supplied error handler with the thrown error, then tighten the migration failure test around fixture.migrator.migrateIfNeeded() to assert the expected FailingOnceCredentialStore failure rather than merely any thrown error; expose its Failure type or use the store’s existing failure marker as appropriate.
🤖 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 `@RemuxApp/Sources/FileProvider/FileProviderDomainReconciler.swift`:
- Around line 42-60: Update FileProviderDomainReconciler.reconcile() to track
requests received while reconciliationTask is active, then execute one
additional reconcileDomains() pass after the current task completes when such a
request is pending. Preserve serialized mutation execution and clear the pending
state appropriately; update testReconcileAddsRenamesAndRemovesToMatchDesiredSet
to expect two requests while retaining the maximumConcurrentMutationCount == 1
assertion.
- Around line 81-89: Update reconcileDomains() to distinguish removed or moved
domains from renamed-only records. Keep serverID, trusted-host, and credential
changes on the registry.remove/add path, but for records whose only change is
displayName, reuse the existing domain and update it through the add operation
without removing it first.
In `@RemuxApp/Sources/FileProvider/FileProviderRemoteItem.swift`:
- Around line 14-20: Guard the timestamp conversion in contentVersion by
replacing the trapping Int64($0.timeIntervalSince1970) conversion with
Int64(exactly:) and an appropriate fallback or clamping behavior. Verify
RemuxSFTPFileMetadata.modificationDate construction sites first, but ensure
malformed, non-finite, or out-of-range remote timestamps cannot crash the
process.
In `@RemuxApp/Sources/SSH/RemuxCitadelSFTPClient.swift`:
- Around line 460-472: Update isNoSuchFile to unwrap SFTPError.errorStatus and
evaluate its contained status code, reusing the same status-code unwrapping
logic as normalizedWriteError. Ensure direct RemuxSFTPClientError.noSuchFile and
SFTPMessage.Status handling remain supported so normalizedReadError,
ensureDirectoryExists, and exists recognize missing paths consistently.
In `@RemuxAppTests/RemuxSFTPReadOnlyClientTests.swift`:
- Around line 410-413: Update the readData fixture method to return empty Data
when chunks is exhausted instead of calling removeFirst() on an empty array;
preserve recording each request and return the next chunk when available.
---
Nitpick comments:
In `@RemuxApp/Sources/Persistence/ApplicationStorage.swift`:
- Around line 57-71: Update sharedRemuxRoot so its fileManager parameter is
either removed or used consistently; prefer creating the “Remux” directory
through fileManager before returning the URL, matching remuxRoot’s behavior and
preserving its existing error handling.
In `@RemuxApp/Sources/Persistence/SSHCredentialStore.swift`:
- Around line 166-188: Update saveCredential and the add-item attributes around
query to set kSecAttrAccessible to
kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly only when constructing
attributes for SecItemAdd. Keep query’s match fields unchanged so the
accessibility value is not used for lookup or update matching.
In `@RemuxApp/Sources/Persistence/TrustedHostStore.swift`:
- Around line 88-92: Add a doc comment immediately above replaceIdentities(_:)
stating that it bypasses host-key challenge and trust-transition validation,
overwrites the entire trust store without validation, and must not be used to
establish new trust.
In `@RemuxAppTests/FileProviderDomainReconcilerTests.swift`:
- Around line 113-138: The test setup in
RemuxAppTests/FileProviderDomainReconcilerTests.swift, within init, should use
the in-memory SSHCredentialStore instead of KeychainSSHCredentialStore. In
RemuxAppTests/FileProviderSharedStorageTests.swift, preserve the live store
required by the access-group test, but move both credential deletions into an
addTeardownBlock registered before the first save so cleanup runs even when
setup throws.
In `@RemuxAppTests/FileProviderSharedStorageMigratorTests.swift`:
- Around line 49-51: Update XCTAssertThrowsErrorAsync to accept and invoke a
caller-supplied error handler with the thrown error, then tighten the migration
failure test around fixture.migrator.migrateIfNeeded() to assert the expected
FailingOnceCredentialStore failure rather than merely any thrown error; expose
its Failure type or use the store’s existing failure marker as appropriate.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 14fdfdd7-41e8-4ae1-8d2e-2df75fbdef44
📒 Files selected for processing (24)
Remux.xcodeproj/project.pbxprojRemuxApp/Info.plistRemuxApp/Remux.entitlementsRemuxApp/Sources/App/RemuxAppDependencies.swiftRemuxApp/Sources/FileProvider/FileProviderDomainReconciler.swiftRemuxApp/Sources/FileProvider/FileProviderRemoteItem.swiftRemuxApp/Sources/FileProvider/FileProviderRemotePath.swiftRemuxApp/Sources/Persistence/ApplicationStorage.swiftRemuxApp/Sources/Persistence/FileProviderSharedStorageMigrator.swiftRemuxApp/Sources/Persistence/SSHCredentialStore.swiftRemuxApp/Sources/Persistence/TrustedHostStore.swiftRemuxApp/Sources/SSH/RemuxCitadelSFTPClient.swiftRemuxApp/Sources/SSH/RemuxSFTPClient.swiftRemuxApp/Sources/SSH/RemuxSSHRootService.swiftRemuxApp/Sources/SSH/RemuxTransportStartupTrace.swiftRemuxApp/Sources/Tmux/GhosttyRuntimeTrace.swiftRemuxAppTests/FileProviderDomainReconcilerTests.swiftRemuxAppTests/FileProviderRemoteItemTests.swiftRemuxAppTests/FileProviderSharedStorageMigratorTests.swiftRemuxAppTests/FileProviderSharedStorageTests.swiftRemuxAppTests/GhosttyTerminalDisconnectReasonClassifierTests.swiftRemuxAppTests/RemuxSFTPReadOnlyClientTests.swiftRemuxAppTests/TerminalPreviewFileLoaderTests.swiftproject.yml
💤 Files with no reviewable changes (1)
- RemuxApp/Sources/Tmux/GhosttyRuntimeTrace.swift
| func reconcile() async throws { | ||
| if let reconciliationTask { | ||
| try await reconciliationTask.value | ||
| return | ||
| } | ||
|
|
||
| let task = Task { | ||
| try await self.reconcileDomains() | ||
| } | ||
| reconciliationTask = task | ||
|
|
||
| do { | ||
| try await task.value | ||
| reconciliationTask = nil | ||
| } catch { | ||
| reconciliationTask = nil | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Coalescing can drop a reconciliation that was requested mid-pass.
reconcileDomains reads the snapshot at line 63. A caller that invokes reconcile() after that read joins the in-flight task at line 44 and returns when it finishes. That caller's changes are never read.
Concretely: the app saves an edited server, then calls reconcile(). If a pass is already running and has passed line 63, the new host or display name is not applied. The domain stays stale until an unrelated later call.
Track a pending-request flag and run one more pass when a request arrives during an active pass.
🔧 Proposed coalescing that honours late requests
private var reconciliationTask: Task<Void, Error>?
+ private var reconciliationRequested = false
func reconcile() async throws {
if let reconciliationTask {
+ reconciliationRequested = true
try await reconciliationTask.value
+ if reconciliationRequested {
+ try await reconcile()
+ }
return
}
+ reconciliationRequested = false
let task = Task {
try await self.reconcileDomains()
}
reconciliationTask = task
do {
try await task.value
reconciliationTask = nil
+ reconciliationRequested = false
} catch {
reconciliationTask = nil
+ reconciliationRequested = false
throw error
}
}Note that this changes the observable counts in testReconcileAddsRenamesAndRemovesToMatchDesiredSet. The second concurrent caller would drive a second pass, so recordsRequestCount becomes 2. Update that assertion, and keep the maximumConcurrentMutationCount == 1 assertion, which still holds.
🤖 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 `@RemuxApp/Sources/FileProvider/FileProviderDomainReconciler.swift` around
lines 42 - 60, Update FileProviderDomainReconciler.reconcile() to track requests
received while reconciliationTask is active, then execute one additional
reconcileDomains() pass after the current task completes when such a request is
pending. Preserve serialized mutation execution and clear the pending state
appropriately; update testReconcileAddsRenamesAndRemovesToMatchDesiredSet to
expect two requests while retaining the maximumConcurrentMutationCount == 1
assertion.
| for record in existingRecords | ||
| where desiredRecords.first(where: { $0.serverID == record.serverID }) != record { | ||
| try await registry.remove(serverID: record.serverID) | ||
| } | ||
|
|
||
| for record in desiredRecords | ||
| where existingRecords.first(where: { $0.serverID == record.serverID }) != record { | ||
| try await registry.add(record) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
NSFileProviderManager change existing domain displayName without removing domain
💡 Result:
Yes, you can update the display name of an existing NSFileProviderDomain without removing the domain by calling the add(:completionHandler:) method again with the same identifier [1]. According to technical documentation and implementation patterns, if a domain with a specific identifier already exists, calling the add(:completionHandler:) method on NSFileProviderManager will update the domain's properties—including its displayName and hidden state—rather than creating a new one or failing [1]. This allows you to programmatically refresh or change the display name as seen in the user interface without triggering the overhead or potential data implications of a full domain removal and re-registration [1]. Note that while this is the intended mechanism for updating domain metadata, some developers have reported limitations regarding how consistently this name is reflected in specific parts of the Finder sidebar, sometimes noting that the application name or other factors may influence the final appearance in the user interface [2]. If you encounter issues where the name does not immediately update, ensure that the identifier remains exactly the same so the system recognizes the request as an update to an existing domain [1].
Citations:
- 1: https://docs.rs/objc2-file-provider/latest/objc2_file_provider/struct.NSFileProviderManager.html
- 2: https://origin-devforums.apple.com/forums/thread/824932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reconciler and nearby tests without executing repository code.
fd -a 'FileProviderDomainReconciler.swift|FileProvider.*Reconcile|.*Reconcile.*' . | sed 's#^\./##' | while read -r f; do
echo "===== $f ====="
wc -l "$f"
done
echo "===== Relevant source ====="
sed -n '1,140p' RemuxApp/Sources/FileProvider/FileProviderDomainReconciler.swift 2>/dev/null || true
echo "===== Test mentions ====="
rg -n "testReconcileAddsRenamesAndRemovesToMatchDesiredSet|displayName|reconcile|removes?|adds?|renames?" Tests remux app Sources -S || true
echo "===== Manifest/deployment target evidence ====="
fd 'Package.swift|Project.swift|Info.plist|deployment_target|iOSDeploymentTarget|macOSDeploymentTarget' . | sed 's#^\./##' | while read -r f; do
echo "--- $f ---"
sed -n '1,220p' "$f" | rg -n "deployment_target|iOS|macOS|XCFramework|NSFileProvider|FileProvider|swift-tools|targets|ios|macos" -C 3 || true
doneRepository: h3nock/remux
Length of output: 5702
🌐 Web query:
Apple NSFileProviderManager add domain exists display name update same identifier domain manager add existing domain display name
💡 Result:
When using the NSFileProviderManager class to register domains, if you attempt to add a domain using an identifier that is already registered, the add(_:completionHandler:) method (or addDomain:completionHandler: in Objective-C) will update the display name and hidden state of the existing domain [1]. This operation will succeed provided that the domain is not backed by an NSFileProviderReplicatedExtension at a disk location that already exists, which would result in an NSFileWriteFileExistsError [1]. In summary, calling the method with the same identifier is the intended way to update the display name of an existing file provider domain [1]. Developers have reported, however, that the actual display name visible in the Finder sidebar may not always reflect the value provided in the NSFileProviderDomain object as expected, leading to requests for more granular control over sidebar naming [2].
Citations:
- 1: https://docs.rs/objc2-file-provider/latest/objc2_file_provider/struct.NSFileProviderManager.html
- 2: https://origin-devforums.apple.com/forums/thread/824932
🌐 Web query:
"NSFileProviderManager add" existing domain display name update existing domain
💡 Result:
Yes, you can update the display name of an existing NSFileProviderDomain by calling the NSFileProviderManager.add(_:completionHandler:) method again with the same domain identifier [1][2]. When you call add(_:completionHandler:) with a NSFileProviderDomain object that shares an identifier with a domain already registered in the NSFileProviderManager, the system will update the display name (and hidden state) of that existing domain rather than failing [1][2]. It is important to note that while updating the display name via this method is supported, attempting to "add" a domain that already exists (with the same identifier) and expects to create new underlying disk locations or specific configurations may fail with an NSFileWriteFileExistsError if the system cannot perform the requested operation (such as re-initializing replicated storage) [1][2]. Always ensure that the NSFileProviderDomain object passed to the method carries the correct, matching identifier you wish to update [1][2].
Citations:
- 1: https://docs.rs/objc2-file-provider/latest/objc2_file_provider/struct.NSFileProviderManager.html
- 2: https://docs.rs/objc2-file-provider/latest/src/objc2_file_provider/generated/Extension.rs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== Directory tree around test/source ====="
find . -maxdepth 4 \( -path './.git' -o -path './Build' -o -path './.build' \) -prune -o \( -type d -o -type f \) -print | sort | rg 'FileProvide|RemuxApp' || true
echo "===== Test outline/source relevant lines ====="
wc -l RemuxAppTests/FileProviderDomainReconcilerTests.swift
sed -n '1,270p' RemuxAppTests/FileProviderDomainReconcilerTests.swift
echo "===== Complete source for registry/test classes ====="
sed -n '120,230p' RemuxApp/Sources/FileProvider/FileProviderDomainReconciler.swift
echo "===== Project manifest files ====="
git ls-files | rg '(^|/)(Project\.swift|Package\.swift|.*\.xcodeproj/.*|Info\.plist|Info\.Plist)$' | while read -r f; do
echo "--- $f ---"
sed -n '1,220p' "$f" | rg -n "deployment_target|iOS|macOS|Swift|FileProvider|InfoPlist|MinimumOSVersion|Info.plist" -C 4 || true
doneRepository: h3nock/remux
Length of output: 36704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== Build settings from project file ====="
python3 - <<'PY'
from pathlib import Path
p = Path('Remux.xcodeproj/project.pbxproj')
for i, line in enumerate(p.read_text().splitlines(), 1):
if any(key in line for key in ['IPHONEOS_DEPLOYMENT_TARGET', 'MACOSX_DEPLOYMENT_TARGET', 'PLATFORM_PREFERRED_VERSION', 'COMBINE_HIDPI_IMAGES', 'BASE_SDK']):
print(f"{i:5}: {line}")
PY
echo "===== Read-only state model for current remove/add condition ====="
python3 - <<'PY'
existing = [
("s1", "Old Password"),
("s2", "Key"),
]
desired = [
("s1", "Password"),
("s2", "Key"),
]
def first_where(records, field, value):
return next((r for r in records if r[0] == value), None)
remove = [r for r in existing if first_where(desired, "serverID", r[0]) != r]
add = [r for r in desired if first_where(existing, "serverID", r[0]) != r]
print("remove_count", len(remove), "add_count", len(add))
print("server1_in_remove", any(r[0] == "s1" for r in remove))
print("server1_in_add", any(r[0] == "s1" and r[1] == "Password" for r in add))
PYRepository: h3nock/remux
Length of output: 700
🌐 Web query:
Apple NSFileProviderDomain update domain display name addDomain already exists NSFileProviderManager addDomain
💡 Result:
In the File Provider framework, you can update the display name of an existing NSFileProviderDomain by calling the NSFileProviderManager.add(_:completionHandler:) method again using the same domain identifier [1]. When you call addDomain for a domain identifier that is already registered, the system does not treat it as an error [1]. Instead, it updates the properties of that domain, such as the display name and hidden state, to match the new NSFileProviderDomain object provided in the call [1]. Key details regarding this process include: - Consistency: You must use the same unique identifier (NSFileProviderDomainIdentifier) for the domain you wish to update [1][2]. - Behavior: If the system recognizes the identifier as already existing, the addDomain call will succeed and apply the new configuration (e.g., updated displayName) [1]. - Limitations: While this method updates the display name, developers have noted that actual control over the Finder sidebar display can be complex and may not always behave as expected depending on how the system manages the domain alias [3]. - Errors: You should be aware that if the domain is backed by a specific disk location (e.g., using a replicated extension) and there are conflicts with that underlying disk location, the call might fail with errors such as NSFileWriteFileExistsError [1]. Always ensure your implementation handles the completion handler error to debug any issues during the update process [4][5].
Citations:
- 1: https://docs.rs/objc2-file-provider/latest/objc2_file_provider/struct.NSFileProviderManager.html
- 2: https://developer.apple.com/documentation/fileprovider/nsfileproviderdomain/init(identifier:displayname:)
- 3: https://origin-devforums.apple.com/forums/thread/824932
- 4: https://developer.apple.com/documentation/fileprovider/nsfileprovidermanager?language=o_8
- 5: https://developer.apple.com/forums/thread/700241
Use the existing domain for screen-name updates.
reconcileDomains() removes the server domain whenever server.displayName changes, then adds the same identifier again. NSFileProviderManager.add(domain) can update an existing domain by ID, so split the comparison into removed/moved identifiers and renamed-only records. Keep only identifier, trusted-host, and credential changes on the remove/add path.
🤖 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 `@RemuxApp/Sources/FileProvider/FileProviderDomainReconciler.swift` around
lines 81 - 89, Update reconcileDomains() to distinguish removed or moved domains
from renamed-only records. Keep serverID, trusted-host, and credential changes
on the registry.remove/add path, but for records whose only change is
displayName, reuse the existing domain and update it through the add operation
without removing it first.
| var contentVersion: Data { | ||
| var data = Data() | ||
| data.appendString(type.rawValue) | ||
| data.appendOptional(size) | ||
| data.appendOptional(modificationDate.map { Int64($0.timeIntervalSince1970) }) | ||
| return data | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the Int64 conversion of timeIntervalSince1970.
Int64($0.timeIntervalSince1970) traps if the Double is NaN, infinite, or outside the Int64 range. modificationDate originates from remote SFTP attributes, so a malicious or broken server can supply an out-of-range timestamp and crash the process. Use Int64(exactly:) with a fallback, or clamp the value.
🛡️ Proposed fix
- data.appendOptional(modificationDate.map { Int64($0.timeIntervalSince1970) })
+ data.appendOptional(modificationDate.map { date -> Int64 in
+ let seconds = date.timeIntervalSince1970.rounded(.down)
+ guard seconds.isFinite else { return 0 }
+ return Int64(seconds.clamped(to: Double(Int64.min)...Double(Int64.max)))
+ })Verify how RemuxSFTPFileMetadata.modificationDate is produced. If it is always derived from a bounded 32-bit SFTP timestamp, the trap is unreachable and this can be closed.
#!/bin/bash
# Locate the metadata model and every construction site of modificationDate.
fd -t f 'RemuxSFTPClient.swift|RemuxCitadelSFTPClient.swift' --exec cat -n {}
rg -n -C4 'modificationDate' --glob '*.swift'🤖 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 `@RemuxApp/Sources/FileProvider/FileProviderRemoteItem.swift` around lines 14 -
20, Guard the timestamp conversion in contentVersion by replacing the trapping
Int64($0.timeIntervalSince1970) conversion with Int64(exactly:) and an
appropriate fallback or clamping behavior. Verify
RemuxSFTPFileMetadata.modificationDate construction sites first, but ensure
malformed, non-finite, or out-of-range remote timestamps cannot crash the
process.
| private func isNoSuchFile(_ error: Error) -> Bool { | ||
| if case .noSuchFile = error as? RemuxSFTPClientError { | ||
| return true | ||
| } | ||
| guard let status = error as? SFTPMessage.Status else { | ||
| return false | ||
| } | ||
| return status.errorCode == .noSuchFile | ||
| } | ||
|
|
||
| private func normalizedReadError(_ error: Error, path: String) -> Error { | ||
| isNoSuchFile(error) ? RemuxSFTPClientError.noSuchFile(path) : error | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect Citadel SFTP error throwing sites to confirm the wrapper types.
set -euo pipefail
fd -t d -i 'citadel' . | head
rg -n -C4 'case errorStatus|throw SFTPError|throw response|SFTPMessage.Status' --iglob '*.swift' | head -80Repository: h3nock/remux
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -ux
printf 'Tracked files matching RemuxCitadelSFTPClient.swift:\n'
git ls-files | rg 'RemuxCitadelSFTPClient.swift|Citadel|citadel' || true
printf '\nCandidate Swift files in SSH/SFTP related directories:\n'
fd -e swift . . 2>/dev/null | rg -i 'sftp|citadel|ssh|remux' | head -200 || true
printf '\nSearch status-code related terms in Swift files:\n'
rg -n -C3 'case \.errorStatus|errorStatus|SFTPStatusCode|SFTPMessage\.Status|normalizedWriteError|isNoSuchFile|normalizedReadError|removeFileIfExists|ensureDirectoryExists|metadata\(|listDirectory\(|withFile\(' -g '*.swift' . | head -200 || trueRepository: h3nock/remux
Length of output: 29488
🏁 Script executed:
#!/bin/bash
set -ux
printf 'RemuxCitadelSFTPClient.swift relevant sections:\n'
sed -n '1,340p' RemuxApp/Sources/SSH/RemuxCitadelSFTPClient.swift | cat -n
printf '\nRemuxSFTPClient.swift relevant sections:\n'
sed -n '1,260p' RemuxApp/Sources/SSH/RemuxSFTPClient.swift | cat -nRepository: h3nock/remux
Length of output: 24315
🌐 Web query:
Citadel SFTP Swift errorStatus SFTPError errorStatus SFTPMessage Status
💡 Result:
In the Citadel Swift library, SFTPError.errorStatus is an enum case used to represent a failure returned by the SFTP server [1][2]. This error case carries an associated value of type SFTPMessage.Status, which provides the specific status information returned by the server [1][3]. When an SFTP request (such as opening a file or setting attributes) is sent, the client checks the response received from the server [4]. If the server responds with a status message indicating an error (i.e., the error code is not.ok), the library throws this SFTPError.errorStatus case [4]. You can handle this error by catching the SFTPError and using a switch statement to access the underlying status, as demonstrated in the following example: do { try await sftp.openFile(filePath: "/path/to/file", flags:.read) } catch let error as SFTPError { switch error { case.errorStatus(let status): // status is an SFTPMessage.Status object print("SFTP error code: (status.errorCode)") print("SFTP error message: (status.message)") case.fileHandleInvalid: print("Invalid file handle") default: print("Other error: (error)") } } catch { print("General error: (error)") } The SFTPMessage.Status object typically contains an errorCode (indicating the nature of the error, such as permission denied or file not found) and a message (a descriptive string provided by the server) [4][3].
Citations:
- 1: https://swiftinit.org/docs/orlandos-nl.citadel/citadel/sftperror
- 2: https://swiftpackageindex.com/orlandos-nl/citadel/0.11.0/documentation/citadel/sftperror/invalidresponse
- 3: https://context7.com/orlandos-nl/citadel/llms.txt
- 4: https://github.com/orlandos-nl/Citadel/blob/main/Sources/Citadel/SFTP/Client/SFTPClient.swift
🏁 Script executed:
#!/bin/bash
set -ux
printf 'Remaining RemuxCitadelSFTPClient.swift section with error handling only:\n'
sed -n '340,490p' RemuxApp/Sources/SSH/RemuxCitadelSFTPClient.swift | cat -n
printf '\nProject files with Citade/Citadel references:\n'
rg -n 'Citadel|package\.dependencies|orlandos-nl|Citadel\.xcframework' -g '*' . | head -200 || true
printf '\nAny local docs/swiftpm lock/package references:\n'
git ls-files | rg -i 'Package.swift|Package.resolved|Package.swift.lock|README|CHANGELOG|Docs|docs|README.md|LICENSE' || trueRepository: h3nock/remux
Length of output: 23183
🌐 Web query:
h3nock Citadel SFTPError errorStatus enum SFTPMessage.Status
💡 Result:
In the Citadel SSH/SFTP library, SFTPError.errorStatus is an enum case that encapsulates an SFTPMessage.Status object [1][2]. This error is thrown when an SFTP operation returns a status message from the server that indicates a failure, meaning the returned error code is not.ok [3]. The SFTPMessage.Status struct contains an errorCode property, which provides specific information about the failure (such as.noSuchFile) [3][4]. Regarding the h3nock reference, repositories such as h3nock/remux utilize the Citadel library for SSH/SFTP functionality [5][6]. References to h3nock/Citadel often point to forks or pins of the official orlandos-nl/Citadel repository, which provides the underlying SFTP error handling mechanism [7][5][8]. Example usage for handling this error: do { try await sftp.openFile(filePath: "/path/to/file", flags:.read) } catch let error as SFTPError { switch error { case.errorStatus(let status): print("SFTP error code: (status.errorCode)") default: print("Other error: (error)") } }
Citations:
- 1: https://swiftinit.org/docs/orlandos-nl.citadel/citadel/sftperror
- 2: https://context7.com/orlandos-nl/citadel/llms.txt
- 3: https://github.com/orlandos-nl/Citadel/blob/main/Sources/Citadel/SFTP/Client/SFTPClient.swift
- 4: https://cephalochromoscope.net/a5f489ef-971a-4e71-a542-6092cbdac126
- 5: https://github.com/h3nock/remux
- 6: https://cephalochromoscope.net/6f92ca07-cf15-4f48-b0f5-7d6c4d43e386
- 7: https://cephalochromoscope.net/98bf9022-84e9-4f88-a309-cca7c7919e03
- 8: https://cephalochromoscope.net/752bfb5d-ad1f-4fd2-815c-a6443b5f4c58
Handle SFTPError.errorStatus in isNoSuchFile.
Citadel wraps server SFTP failures in SFTPError.errorStatus; isNoSuchFile only checks SFTPMessage.Status. A missing-path read error in this shape leaves read/withFile errors unnormalized, and ensureDirectoryExists/exists miss the create branch because both rely on isNoSuchFile. Share the status-code unwrapping between isNoSuchFile and normalizedWriteError.
🤖 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 `@RemuxApp/Sources/SSH/RemuxCitadelSFTPClient.swift` around lines 460 - 472,
Update isNoSuchFile to unwrap SFTPError.errorStatus and evaluate its contained
status code, reusing the same status-code unwrapping logic as
normalizedWriteError. Ensure direct RemuxSFTPClientError.noSuchFile and
SFTPMessage.Status handling remain supported so normalizedReadError,
ensureDirectoryExists, and exists recognize missing paths consistently.
| func readData(from offset: UInt64, length: UInt32) -> Data { | ||
| recordedRequests.append(.init(offset: offset, length: Int(length))) | ||
| return chunks.removeFirst() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid a fatal trap when the fixture runs out of chunks.
chunks.removeFirst() traps on an empty array. If a regression makes downloadFile issue one more read than expected, the test process crashes and the rest of the test bundle does not run. Return an empty Data instead, so the assertions report the failure.
💚 Proposed fix
func readData(from offset: UInt64, length: UInt32) -> Data {
recordedRequests.append(.init(offset: offset, length: Int(length)))
- return chunks.removeFirst()
+ guard !chunks.isEmpty else { return Data() }
+ return chunks.removeFirst()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func readData(from offset: UInt64, length: UInt32) -> Data { | |
| recordedRequests.append(.init(offset: offset, length: Int(length))) | |
| return chunks.removeFirst() | |
| } | |
| func readData(from offset: UInt64, length: UInt32) -> Data { | |
| recordedRequests.append(.init(offset: offset, length: Int(length))) | |
| guard !chunks.isEmpty else { return Data() } | |
| return chunks.removeFirst() | |
| } |
🤖 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 `@RemuxAppTests/RemuxSFTPReadOnlyClientTests.swift` around lines 410 - 413,
Update the readData fixture method to return empty Data when chunks is exhausted
instead of calling removeFirst() on an empty array; preserve recording each
request and return the next chunk when available.
1 participant
Reviewability experiment
This is an attempt to make the File Provider work more reviewable by presenting the existing atomic work as a small, coherent stack. If this shape or boundary does not feel right, please say so instead of spending effort reviewing an unhelpful presentation—we can reshape it.
This is a history/presentation rewrite of the already-tested implementation. It does not add new product behavior.
Intended review slice
codex/file-provider-review/read-transportcodex/file-provider-review/write-transportThe commits in this branch are the atomic commits intended for this slice:
Stack mechanics
All PRs in this stack intentionally target
main. The branches themselves remain sequential, so GitHub may show earlier ancestry in a later PR until its parents merge. Please review the commit range above as the intended slice and merge these PRs in order; after a parent lands, GitHub will reduce the next PR diff to its incremental changes.Verification
The final rewritten leaf was verified with 1164 tests passed, 0 failed, and 1 skipped.
xcodegen generateproduced no project diff, and the final tree is identical to the previously tested simplified result.Summary by CodeRabbit
New Features
Bug Fixes