File Provider: add bounded SFTP read transport - #32
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.
📝 WalkthroughWalkthroughThe project adds File Provider shared storage, migration, domain reconciliation, expanded read-only SFTP operations, transport startup tracing, entitlements, Keychain access groups, and related tests. ChangesFile Provider and SFTP integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant FileProviderSharedStorageMigrator
participant SharedStorage
participant SSHCredentialStore
participant FileProviderDomainReconciler
participant NSFileProviderManager
App->>FileProviderSharedStorageMigrator: migrate legacy state
FileProviderSharedStorageMigrator->>SharedStorage: copy profiles and trusted hosts
FileProviderSharedStorageMigrator->>SSHCredentialStore: copy and verify credentials
FileProviderSharedStorageMigrator-->>App: write completion marker
App->>FileProviderDomainReconciler: reconcile domains
FileProviderDomainReconciler->>NSFileProviderManager: remove stale domains
FileProviderDomainReconciler->>NSFileProviderManager: add eligible domains
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: de550b4b17
ℹ️ 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".
| if let credential = try await legacyCredentials.loadCredential(identityID: identity.id) { | ||
| try await sharedCredentials.saveCredential(credential, identityID: identity.id) |
There was a problem hiding this comment.
When a migration attempt saves a credential but fails before writing the marker, a later retry after that legacy credential has been deleted skips this block and leaves the copied secret in the shared keychain. Because the final verification checks only profiles and trust, the retry can then mark migration complete, and FileProviderDomainReconciler will treat the server as eligible using a credential the user removed. Delete any destination credential when the legacy lookup returns nil, or verify credential absence before writing the marker.
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: 9
🧹 Nitpick comments (9)
RemuxApp/Sources/Persistence/TrustedHostStore.swift (1)
88-92: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument that
replaceIdentitiesbypasses host-key validation.
replaceIdentitieswrites the trust file directly. It skips the challenge and trust-transition checks thattrustHostKeyenforces.restoreIdentityabove carries a warning comment for the same reason. Add the equivalent comment so a later caller does not use this method to establish new trust.♻️ Proposed change
+ /// Replaces the whole stored identity collection. + /// + /// This intentionally bypasses host-key challenge and trust-transition + /// validation. Use it only for migration and rollback. Callers must not + /// use it to establish new trust. 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 warning comment immediately above replaceIdentities(_:) documenting that it writes identities directly and bypasses the host-key challenge and trust-transition validation enforced by trustHostKey, so callers do not use it to establish new trust. Match the warning style used by restoreIdentity.RemuxApp/Sources/Persistence/SSHCredentialStore.swift (1)
166-188: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet
kSecAttrAccessibleexplicitly for shared credentials.The query does not set
kSecAttrAccessible, so added items use the defaultkSecAttrAccessibleWhenUnlocked. The File Provider extension can be started while the device is locked. The credential read then fails witherrSecInteractionNotAllowed, and domain reconciliation treats the server as ineligible.Set the accessibility on add.
kSecAttrAccessibleAfterFirstUnlockThisDeviceOnlymatches this use case and keeps the item off backups.🔒️ Proposed change
static func query( service: String, accessGroup: String?, identityID: SSHIdentity.ID, returnData: Bool ) -> [CFString: Any] { var query: [CFString: Any] = [ kSecClass: kSecClassGenericPassword, kSecAttrService: service, kSecAttrAccount: identityID.uuidString, ]Then add the attribute in
saveCredentialon the add path only:var addQuery = query addQuery[kSecValueData] = data + addQuery[kSecAttrAccessible] = kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly let addStatus = SecItemAdd(addQuery as CFDictionary, nil)Note:
kSecAttrAccessiblemust not be part of theSecItemUpdatematch query, so keep it out ofquery.🤖 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’s add-item attributes to explicitly set kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, while leaving the shared query(_:accessGroup:identityID:returnData:) unchanged so the attribute is not used in SecItemUpdate matching.RemuxApp/Sources/Persistence/ApplicationStorage.swift (1)
57-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sharedRemuxRootignores thefileManagerparameter and does not create the directory.Two points on this API:
- The
fileManagerparameter is never used. The defaultcontainerURLclosure callsFileManager.defaultdirectly. A caller that injects a differentFileManagergets no effect, which is misleading.remuxRootcreates the directory before returning.sharedRemuxRootdoes not. Callers must create the directory themselves. Make the asymmetry explicit or align the two functions.♻️ Proposed refactor to use the injected `FileManager`
static func sharedRemuxRoot( appGroupIdentifier: String = FileProviderSharedConfiguration.appGroupIdentifier, fileManager: FileManager = .default, - containerURL: `@Sendable` (String) -> URL? = { - FileManager.default.containerURL( - forSecurityApplicationGroupIdentifier: $0 - ) - } + containerURL: (`@Sendable` (FileManager, String) -> URL?)? = nil ) throws -> URL { - guard let containerURL = containerURL(appGroupIdentifier) else { + let resolve = containerURL ?? { manager, identifier in + manager.containerURL(forSecurityApplicationGroupIdentifier: identifier) + } + guard let containerURL = resolve(fileManager, appGroupIdentifier) else { throw FileProviderSharedConfigurationError.missingSharedContainer } return containerURL.appendingPathComponent("Remux", isDirectory: true) }Keep the signature if you prefer, but then remove the unused
fileManagerparameter.🤖 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 default containerURL closure uses the injected fileManager rather than FileManager.default, and create the returned Remux directory before returning it to match remuxRoot. Preserve the existing missingSharedContainer error behavior and method signature.RemuxAppTests/FileProviderDomainReconcilerTests.swift (1)
230-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDecrement
activeMutationCountwithdefer.
Task.sleepthrows on cancellation.activeMutationCount -= 1on line 236 is then skipped, andmaximumConcurrentMutationCountreports a value that overstates the real concurrency. Usedeferso the counter always unwinds.♻️ Proposed change
private func mutate(_ mutation: Mutation, body: () -> Void) async throws { activeMutationCount += 1 maximumConcurrentMutationCount = max(maximumConcurrentMutationCount, activeMutationCount) + defer { activeMutationCount -= 1 } try await Task.sleep(for: .milliseconds(10)) body() mutations.append(mutation) - activeMutationCount -= 1 }🤖 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 230 - 237, Update the async mutate method to register a defer immediately after incrementing activeMutationCount, ensuring the counter is decremented whenever the method exits, including when Task.sleep throws due to cancellation. Remove the direct decrement at the end while preserving the existing mutation and maximum-concurrency updates.RemuxAppTests/FileProviderSharedStorageMigratorTests.swift (1)
212-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLet the helper inspect the thrown error.
The helper accepts any error.
testMigrationFailureLeavesSourceIntactDoesNotMarkAndRetriesIdempotentlytherefore passes even if the migration fails for an unintended reason, such asverificationFailed. Add an error handler so the caller can assert the specific case.♻️ Proposed change
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) + } }🤖 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 212 - 221, Update XCTAssertThrowsErrorAsync to accept an error-handling closure and invoke it with the caught error, while preserving the existing XCTFail behavior when no error is thrown. Update testMigrationFailureLeavesSourceIntactDoesNotMarkAndRetriesIdempotently to use the handler and assert the expected migration failure rather than accepting unrelated errors.RemuxApp/Sources/FileProvider/FileProviderDomainReconciler.swift (1)
132-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse one identifier rule for
addandremove.
addbuilds the identifier fromrecord.rawIdentifier.removerebuildsserverID.uuidString.lowercased()inline. The two encode the same rule in two places. If the rule changes in one place,removetargets an identifier that no longer exists, and the stale domain stays registered.Move the rule onto the identifier type.
♻️ Proposed change
+extension NSFileProviderDomainIdentifier { + fileprivate init(serverID: SavedServer.ID) { + self.init(rawValue: serverID.uuidString.lowercased()) + } +}Then use it at both sites:
func remove(serverID: SavedServer.ID) async throws { let domain = NSFileProviderDomain( - identifier: NSFileProviderDomainIdentifier(rawValue: serverID.uuidString.lowercased()), + identifier: NSFileProviderDomainIdentifier(serverID: serverID), displayName: "" )The class also holds no mutable state. Plain
Sendableis enough;@uncheckedis not required.🤖 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 132 - 136, Move the shared domain-identifier construction rule onto the identifier type used by SavedServer, then update both add and remove in FileProviderDomainReconciler to use that shared API instead of rebuilding the UUID string inline. Since the reconciler has no mutable state, replace any unnecessary `@unchecked` Sendable conformance with plain Sendable.RemuxAppTests/RemuxSFTPReadOnlyClientTests.swift (1)
214-226: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel the task before the readiness assertion.
If the polling loop times out on a loaded machine,
XCTAssertEqual(readCount, 2)fails andtask.cancel()never runs. The test then blocks ontask.valuefor the full 30-second sleep. Register the cancellation withdeferright after the task starts.💚 Proposed change
let task = Task { try await client.downloadFile(atPath: "/cancel", to: destination) { _ in } } + defer { task.cancel() }🤖 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 214 - 226, Register task cancellation with defer immediately after the download task is created in the test, before the polling loop and readiness assertion. Keep the existing cancellation call if needed only to preserve behavior without duplication, and ensure task.value cannot block for the full sleep when the assertion fails.RemuxApp/Sources/SSH/RemuxCitadelSFTPClient.swift (2)
184-197: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
linkMetadatalists the whole parent directory for one entry.Each call performs a full remote directory listing. If a caller resolves link metadata for every entry of a directory, the remote work becomes quadratic in the entry count. If the File Provider enumerator needs per-entry link metadata, reuse one listing for the whole directory instead of calling
linkMetadataper entry.🤖 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 184 - 197, The linkMetadata(atPath:) implementation performs a full parent-directory listing for each entry, causing repeated remote work. Update the surrounding directory enumeration flow to fetch listDirectory(atPath:) once per directory and reuse the resulting entries’ metadata for all links, rather than invoking linkMetadata(atPath:) separately for each entry; preserve noSuchFile handling for missing entries.
630-651: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider routing these traces through the extension-safe shim.
RemuxTransportStartupTracealready compiles to a no-op in the File Provider build. If the SFTP open timing usedtrace.stage("sftp.open"), these#if !REMUX_FILE_PROVIDER_EXTENSIONblocks would not be needed, and the app build would keep the same timing output. The same pattern repeats at lines 771-794.🤖 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 630 - 651, Replace the conditional GhosttyRuntimeTrace timing blocks around SFTPClient.open and the repeated SFTP-open timing block near the later occurrence with RemuxTransportStartupTrace.trace.stage("sftp.open"). Preserve the existing begin/end timing output in app builds while relying on the shim’s no-op behavior for File Provider builds, and remove the surrounding `#if` !REMUX_FILE_PROVIDER_EXTENSION guards.
🤖 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/Remux.entitlements`:
- Around line 9-13: Update the keychain-access-groups entries in the
entitlements configuration to confirm the bundle identifier; if it is not
dev.remux.app, make $(AppIdentifierPrefix)$(CFBundleIdentifier) the first entry
so legacy application-identifier keychain items remain readable during
migration.
In `@RemuxApp/Sources/App/RemuxAppDependencies.swift`:
- Around line 156-169: Correct the access-group mapping used to construct the
migration source in the dependency factory: ensure the application
KeychainSSHCredentialStore uses the application-identifier group that contains
legacy credentials, while shared continues using
FileProviderKeychainAccessGroup. Verify the corresponding
FileProviderSharedConfiguration symbols and keep the migration destination
unchanged.
In `@RemuxApp/Sources/FileProvider/FileProviderDomainReconciler.swift`:
- Around line 42-60: The reconcile() coalescing logic can lose changes arriving
while a pass is in flight. Add actor state to record a pending reconciliation
request; when reconcile() is called during an existing reconciliationTask, mark
it pending and await the current task, then ensure reconcileDomains() runs one
additional pass before clearing the task when pending is set. Update
testReconcileAddsRenamesAndRemovesToMatchDesiredSet to expect the additional
reconciliation request.
In `@RemuxApp/Sources/Persistence/FileProviderSharedStorageMigrator.swift`:
- Around line 70-75: Update the migration verification in migrateIfNeeded to
validate that the shared snapshot contains all legacy servers and workspaces
rather than requiring exact snapshot equality. Change the sharedTrust update
from replaceIdentities to merging the legacy trusted identities with existing
shared identities, while preserving existing entries and verifying the merged
result.
In `@RemuxApp/Sources/SSH/RemuxCitadelSFTPClient.swift`:
- Around line 151-155: Update realPath(atPath:) to catch Citadel’s missing-path
status and convert it to RemuxSFTPClientError.noSuchFile(path), matching
listDirectory, metadata, and withFile; rethrow all other errors unchanged while
preserving the existing operation timeout behavior.
In `@RemuxApp/Sources/SSH/RemuxSFTPClient.swift`:
- Around line 130-149: Bound the download loop in the SFTP transfer flow around
remoteFile.readChunk by obtaining the remote size from metadata(atPath:) or
enforcing an explicit maximum byte budget. Limit each read and stop once the
budget or reported file size is reached, while preserving cancellation checks,
local writes, offset updates, progress reporting, and EOF handling.
In `@RemuxAppTests/FileProviderDomainReconcilerTests.swift`:
- Around line 119-138: Replace the KeychainSSHCredentialStore used by the
fixture setup with the shared in-memory SSHCredentialStore helper, promoting
InMemorySSHCredentialStore from FileProviderSharedStorageMigratorTests.swift if
necessary. Keep the existing credential-saving behavior and ensure the
reconciler continues receiving it through the any SSHCredentialStore interface,
without changing production code.
In `@RemuxAppTests/FileProviderSharedStorageTests.swift`:
- Around line 37-53: In
testLiveCredentialStoresKeepApplicationSourceSeparateFromSharedDestination,
register an addTeardownBlock immediately after creating stores to delete the
application and shared credentials for the generated identityID on every exit
path, then remove the trailing happy-path delete calls.
In `@RemuxAppTests/RemuxSFTPReadOnlyClientTests.swift`:
- Around line 286-289: Update readData(from:length:) to return empty Data when
chunks is exhausted instead of calling removeFirst() on an empty array, while
preserving request recording and normal chunk consumption so requests() detects
unexpected reads.
---
Nitpick comments:
In `@RemuxApp/Sources/FileProvider/FileProviderDomainReconciler.swift`:
- Around line 132-136: Move the shared domain-identifier construction rule onto
the identifier type used by SavedServer, then update both add and remove in
FileProviderDomainReconciler to use that shared API instead of rebuilding the
UUID string inline. Since the reconciler has no mutable state, replace any
unnecessary `@unchecked` Sendable conformance with plain Sendable.
In `@RemuxApp/Sources/Persistence/ApplicationStorage.swift`:
- Around line 57-71: Update sharedRemuxRoot so its default containerURL closure
uses the injected fileManager rather than FileManager.default, and create the
returned Remux directory before returning it to match remuxRoot. Preserve the
existing missingSharedContainer error behavior and method signature.
In `@RemuxApp/Sources/Persistence/SSHCredentialStore.swift`:
- Around line 166-188: Update saveCredential’s add-item attributes to explicitly
set kSecAttrAccessible to kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly,
while leaving the shared query(_:accessGroup:identityID:returnData:) unchanged
so the attribute is not used in SecItemUpdate matching.
In `@RemuxApp/Sources/Persistence/TrustedHostStore.swift`:
- Around line 88-92: Add a warning comment immediately above
replaceIdentities(_:) documenting that it writes identities directly and
bypasses the host-key challenge and trust-transition validation enforced by
trustHostKey, so callers do not use it to establish new trust. Match the warning
style used by restoreIdentity.
In `@RemuxApp/Sources/SSH/RemuxCitadelSFTPClient.swift`:
- Around line 184-197: The linkMetadata(atPath:) implementation performs a full
parent-directory listing for each entry, causing repeated remote work. Update
the surrounding directory enumeration flow to fetch listDirectory(atPath:) once
per directory and reuse the resulting entries’ metadata for all links, rather
than invoking linkMetadata(atPath:) separately for each entry; preserve
noSuchFile handling for missing entries.
- Around line 630-651: Replace the conditional GhosttyRuntimeTrace timing blocks
around SFTPClient.open and the repeated SFTP-open timing block near the later
occurrence with RemuxTransportStartupTrace.trace.stage("sftp.open"). Preserve
the existing begin/end timing output in app builds while relying on the shim’s
no-op behavior for File Provider builds, and remove the surrounding `#if`
!REMUX_FILE_PROVIDER_EXTENSION guards.
In `@RemuxAppTests/FileProviderDomainReconcilerTests.swift`:
- Around line 230-237: Update the async mutate method to register a defer
immediately after incrementing activeMutationCount, ensuring the counter is
decremented whenever the method exits, including when Task.sleep throws due to
cancellation. Remove the direct decrement at the end while preserving the
existing mutation and maximum-concurrency updates.
In `@RemuxAppTests/FileProviderSharedStorageMigratorTests.swift`:
- Around line 212-221: Update XCTAssertThrowsErrorAsync to accept an
error-handling closure and invoke it with the caught error, while preserving the
existing XCTFail behavior when no error is thrown. Update
testMigrationFailureLeavesSourceIntactDoesNotMarkAndRetriesIdempotently to use
the handler and assert the expected migration failure rather than accepting
unrelated errors.
In `@RemuxAppTests/RemuxSFTPReadOnlyClientTests.swift`:
- Around line 214-226: Register task cancellation with defer immediately after
the download task is created in the test, before the polling loop and readiness
assertion. Keep the existing cancellation call if needed only to preserve
behavior without duplication, and ensure task.value cannot block for the full
sleep when the assertion fails.
🪄 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: b60cffc9-855b-492e-a6c9-32ab5a0db69c
📒 Files selected for processing (21)
Remux.xcodeproj/project.pbxprojRemuxApp/Info.plistRemuxApp/Remux.entitlementsRemuxApp/Sources/App/RemuxAppDependencies.swiftRemuxApp/Sources/FileProvider/FileProviderDomainReconciler.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/FileProviderSharedStorageMigratorTests.swiftRemuxAppTests/FileProviderSharedStorageTests.swiftRemuxAppTests/GhosttyTerminalDisconnectReasonClassifierTests.swiftRemuxAppTests/RemuxSFTPReadOnlyClientTests.swiftRemuxAppTests/TerminalPreviewFileLoaderTests.swiftproject.yml
💤 Files with no reviewable changes (1)
- RemuxApp/Sources/Tmux/GhosttyRuntimeTrace.swift
| <key>keychain-access-groups</key> | ||
| <array> | ||
| <string>$(AppIdentifierPrefix)dev.remux.app</string> | ||
| <string>$(AppIdentifierPrefix)dev.remux.shared</string> | ||
| </array> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The first keychain-access-groups entry becomes the default access group.
Adding this array changes the default keychain access group for every item saved without an explicit group. The default moves from the application-identifier group to $(AppIdentifierPrefix)dev.remux.app. Existing credentials were saved without a group, so they stay in the application-identifier group.
Confirm the bundle identifier. If it is not dev.remux.app, add $(AppIdentifierPrefix)$(CFBundleIdentifier) as the first entry so legacy items remain readable during migration.
🤖 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/Remux.entitlements` around lines 9 - 13, Update the
keychain-access-groups entries in the entitlements configuration to confirm the
bundle identifier; if it is not dev.remux.app, make
$(AppIdentifierPrefix)$(CFBundleIdentifier) the first entry so legacy
application-identifier keychain items remain readable during migration.
| ( | ||
| application: KeychainSSHCredentialStore( | ||
| service: service, | ||
| accessGroup: try FileProviderSharedConfiguration.applicationKeychainAccessGroup( | ||
| infoDictionary: infoDictionary | ||
| ) | ||
| ), | ||
| shared: KeychainSSHCredentialStore( | ||
| service: service, | ||
| accessGroup: try FileProviderSharedConfiguration.keychainAccessGroup( | ||
| infoDictionary: infoDictionary | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The application store may not read legacy credentials.
This factory produces the migration source (application) and destination (shared). The application store queries with an explicit kSecAttrAccessGroup of RemuxApplicationKeychainAccessGroup. Legacy items were written with no access group, so they are in the application-identifier group. The two groups match only if the bundle identifier is dev.remux.app.
This is the same root cause raised on RemuxApp/Sources/Persistence/SSHCredentialStore.swift. Verify the group mapping before merge.
🤖 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/App/RemuxAppDependencies.swift` around lines 156 - 169,
Correct the access-group mapping used to construct the migration source in the
dependency factory: ensure the application KeychainSSHCredentialStore uses the
application-identifier group that contains legacy credentials, while shared
continues using FileProviderKeychainAccessGroup. Verify the corresponding
FileProviderSharedConfiguration symbols and keep the migration destination
unchanged.
| 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.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Coalescing can drop the state change that triggered the request.
If a reconciliation is already running, reconcile() awaits that task and returns. The in-flight task read its snapshot before the new caller arrived. Any change made after that read is not reconciled.
Concrete case: the migrator saves a credential, then calls reconcile(). A reconciliation started a moment earlier is still running. The caller joins it, sees success, and the new domain is never registered. The domain appears only after some later trigger.
Track a pending request and run one more pass after the current pass finishes.
♻️ Proposed change
func reconcile() async throws {
if let reconciliationTask {
- try await reconciliationTask.value
- return
+ isFollowUpRequested = true
+ _ = try? await reconciliationTask.value
+ if let followUpTask = reconciliationTask {
+ try await followUpTask.value
+ return
+ }
}
let task = Task {
try await self.reconcileDomains()
}
reconciliationTask = task
do {
try await task.value
reconciliationTask = nil
} catch {
reconciliationTask = nil
throw error
}
+
+ if isFollowUpRequested {
+ isFollowUpRequested = false
+ try await reconcile()
+ }
}Add the flag to the actor:
private var reconciliationTask: Task<Void, Error>?
+ private var isFollowUpRequested = falseAdjust testReconcileAddsRenamesAndRemovesToMatchDesiredSet, which currently asserts recordsRequestCount == 1.
🤖 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, The reconcile() coalescing logic can lose changes arriving while
a pass is in flight. Add actor state to record a pending reconciliation request;
when reconcile() is called during an existing reconciliationTask, mark it
pending and await the current task, then ensure reconcileDomains() runs one
additional pass before clearing the task when pending is set. Update
testReconcileAddsRenamesAndRemovesToMatchDesiredSet to expect the additional
reconciliation request.
| try sharedTrust.replaceIdentities(trustedIdentities) | ||
|
|
||
| guard try await sharedProfiles.loadSnapshot() == snapshot, | ||
| try sharedTrust.loadIdentities() == trustedIdentities else { | ||
| throw FileProviderSharedStorageMigrationError.verificationFailed | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Strict snapshot equality can make the migration fail permanently.
replaceIdentities overwrites the shared trust file, and the guard requires the shared snapshot to equal the legacy snapshot exactly. Both assume the shared store is empty.
If the shared store already holds any server, workspace, or identity that is not in the legacy snapshot, the guard fails. migrateIfNeeded throws, the marker is never written, and every later launch repeats the same failure. There is no recovery path.
Verify that the shared snapshot contains the legacy records instead of requiring exact equality, and merge trust identities instead of replacing them.
♻️ Proposed change
- try sharedTrust.replaceIdentities(trustedIdentities)
+ var mergedIdentities = try sharedTrust.loadIdentities()
+ for identity in trustedIdentities
+ where !mergedIdentities.contains(where: { $0.serverID == identity.serverID }) {
+ mergedIdentities.append(identity)
+ }
+ try sharedTrust.replaceIdentities(mergedIdentities)
- guard try await sharedProfiles.loadSnapshot() == snapshot,
- try sharedTrust.loadIdentities() == trustedIdentities else {
+ let migratedSnapshot = try await sharedProfiles.loadSnapshot()
+ let migratedTrust = try sharedTrust.loadIdentities()
+ guard snapshot.servers.allSatisfy(migratedSnapshot.servers.contains),
+ snapshot.workspaces.allSatisfy(migratedSnapshot.workspaces.contains),
+ snapshot.identities.allSatisfy(migratedSnapshot.identities.contains),
+ trustedIdentities.allSatisfy(migratedTrust.contains) else {
throw FileProviderSharedStorageMigrationError.verificationFailed
}📝 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.
| try sharedTrust.replaceIdentities(trustedIdentities) | |
| guard try await sharedProfiles.loadSnapshot() == snapshot, | |
| try sharedTrust.loadIdentities() == trustedIdentities else { | |
| throw FileProviderSharedStorageMigrationError.verificationFailed | |
| } | |
| try sharedTrust.replaceIdentities(trustedIdentities) | |
| var mergedIdentities = try sharedTrust.loadIdentities() | |
| for identity in trustedIdentities | |
| where !mergedIdentities.contains(where: { $0.serverID == identity.serverID }) { | |
| mergedIdentities.append(identity) | |
| } | |
| try sharedTrust.replaceIdentities(mergedIdentities) | |
| let migratedSnapshot = try await sharedProfiles.loadSnapshot() | |
| let migratedTrust = try sharedTrust.loadIdentities() | |
| guard snapshot.servers.allSatisfy(migratedSnapshot.servers.contains), | |
| snapshot.workspaces.allSatisfy(migratedSnapshot.workspaces.contains), | |
| snapshot.identities.allSatisfy(migratedSnapshot.identities.contains), | |
| trustedIdentities.allSatisfy(migratedTrust.contains) else { | |
| throw FileProviderSharedStorageMigrationError.verificationFailed | |
| } |
🤖 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/FileProviderSharedStorageMigrator.swift` around
lines 70 - 75, Update the migration verification in migrateIfNeeded to validate
that the shared snapshot contains all legacy servers and workspaces rather than
requiring exact snapshot equality. Change the sharedTrust update from
replaceIdentities to merging the legacy trusted identities with existing shared
identities, while preserving existing entries and verifying the merged result.
| func realPath(atPath path: String) async throws -> String { | ||
| try await withOperationTimeout { | ||
| try await sftp.getRealPath(atPath: path) | ||
| try await connection.remuxRealPath(atPath: path) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize missing-path errors in realPath too.
listDirectory, metadata, and withFile convert a missing remote path into RemuxSFTPClientError.noSuchFile(path). realPath returns the raw Citadel status error. Callers that switch on RemuxSFTPClientError then miss the missing-path case for path resolution.
♻️ Proposed change
func realPath(atPath path: String) async throws -> String {
- try await withOperationTimeout {
- try await connection.remuxRealPath(atPath: path)
- }
+ do {
+ return try await withOperationTimeout {
+ try await connection.remuxRealPath(atPath: path)
+ }
+ } catch {
+ throw normalizedReadError(error, path: path)
+ }
}📝 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 realPath(atPath path: String) async throws -> String { | |
| try await withOperationTimeout { | |
| try await sftp.getRealPath(atPath: path) | |
| try await connection.remuxRealPath(atPath: path) | |
| } | |
| } | |
| func realPath(atPath path: String) async throws -> String { | |
| do { | |
| return try await withOperationTimeout { | |
| try await connection.remuxRealPath(atPath: path) | |
| } | |
| } catch { | |
| throw normalizedReadError(error, path: 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/SSH/RemuxCitadelSFTPClient.swift` around lines 151 - 155,
Update realPath(atPath:) to catch Citadel’s missing-path status and convert it
to RemuxSFTPClientError.noSuchFile(path), matching listDirectory, metadata, and
withFile; rethrow all other errors unchanged while preserving the existing
operation timeout behavior.
| try await withFile(atPath: remotePath) { remoteFile in | ||
| var offset: UInt64 = 0 | ||
| while true { | ||
| try Task.checkCancellation() | ||
| let data = try await remoteFile.readChunk( | ||
| from: offset, | ||
| length: RemuxSFTPReadableFile.maximumChunkLength | ||
| ) | ||
| try Task.checkCancellation() | ||
| guard !data.isEmpty else { break } | ||
|
|
||
| try Task.checkCancellation() | ||
| try localFile.write(contentsOf: data) | ||
| try Task.checkCancellation() | ||
|
|
||
| offset += UInt64(data.count) | ||
| await progress(Int64(min(offset, UInt64(Int64.max)))) | ||
| try Task.checkCancellation() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the total downloaded bytes.
The loop stops only when the remote returns an empty chunk. A remote path that never reports EOF, for example a character device such as /dev/zero or a growing log, makes the loop write without limit. The File Provider container can then fill the disk. Add a byte budget, or read at most the size reported by metadata(atPath:).
🛡️ Example bound
- func downloadFile(
- atPath remotePath: String,
- to localURL: URL,
- progress: `@escaping` `@Sendable` (Int64) async -> Void
- ) async throws {
+ func downloadFile(
+ atPath remotePath: String,
+ to localURL: URL,
+ maximumByteCount: UInt64 = 8 * 1024 * 1024 * 1024,
+ progress: `@escaping` `@Sendable` (Int64) async -> Void
+ ) async throws {
do {
try Data().write(to: localURL, options: .atomic)
let localFile = try FileHandle(forWritingTo: localURL)
@@
offset += UInt64(data.count)
+ guard offset <= maximumByteCount else {
+ throw RemuxSFTPClientError.oversizedReadResult(
+ requested: Int(clamping: maximumByteCount),
+ actual: Int(clamping: offset)
+ )
+ }
await progress(Int64(min(offset, UInt64(Int64.max))))🤖 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/RemuxSFTPClient.swift` around lines 130 - 149, Bound the
download loop in the SFTP transfer flow around remoteFile.readChunk by obtaining
the remote size from metadata(atPath:) or enforcing an explicit maximum byte
budget. Limit each read and stop once the budget or reported file size is
reached, while preserving cancellation checks, local writes, offset updates,
progress reporting, and EOF handling.
| let root = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent(UUID().uuidString, isDirectory: true) | ||
| let profiles = FileBackedConnectionProfileRepository(rootURL: root) | ||
| let credentials = KeychainSSHCredentialStore(service: "dev.remux.tests.\(UUID().uuidString)") | ||
| let trust = TrustedHostStore(rootURL: root) | ||
|
|
||
| let servers = [ | ||
| Self.server(id: "00000000-0000-0000-0000-000000000001", name: "Password"), | ||
| Self.server(id: "00000000-0000-0000-0000-000000000002", name: "Key"), | ||
| Self.server(id: "00000000-0000-0000-0000-000000000003", name: "Third"), | ||
| ] | ||
| let eligibility = [passwordServer, keyServer, thirdServer] | ||
|
|
||
| for (server, eligibility) in zip(servers, eligibility) { | ||
| try await profiles.saveServer(server) | ||
|
|
||
| if eligibility != .missingCredential { | ||
| try await credentials.saveCredential(.password("secret"), identityID: server.identityID) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The fixture leaves credentials in the real keychain.
Line 122 creates a KeychainSSHCredentialStore, and line 136 saves up to three credentials per fixture. Nothing deletes them. The service name is random, so each test run adds items to the developer or CI keychain that no later run can find or clean.
Use an in-memory SSHCredentialStore for this fixture, or delete the saved credentials in a teardown. FileProviderSharedStorageMigratorTests.swift already defines an InMemorySSHCredentialStore; promote it to a shared test helper and reuse it here. The reconciler depends only on any SSHCredentialStore, so no production change is needed.
🤖 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 119 -
138, Replace the KeychainSSHCredentialStore used by the fixture setup with the
shared in-memory SSHCredentialStore helper, promoting InMemorySSHCredentialStore
from FileProviderSharedStorageMigratorTests.swift if necessary. Keep the
existing credential-saving behavior and ensure the reconciler continues
receiving it through the any SSHCredentialStore interface, without changing
production code.
| func testLiveCredentialStoresKeepApplicationSourceSeparateFromSharedDestination() async throws { | ||
| let service = "dev.remux.tests.\(UUID().uuidString)" | ||
| let stores = try RemuxAppDependencies.fileProviderCredentialStores(service: service) | ||
| let identityID = UUID() | ||
|
|
||
| try await stores.application.saveCredential(.password("application"), identityID: identityID) | ||
| try await stores.shared.saveCredential(.password("shared"), identityID: identityID) | ||
|
|
||
| let applicationCredential = try await stores.application.loadCredential(identityID: identityID) | ||
| let sharedCredential = try await stores.shared.loadCredential(identityID: identityID) | ||
|
|
||
| XCTAssertEqual(applicationCredential, .password("application")) | ||
| XCTAssertEqual(sharedCredential, .password("shared")) | ||
|
|
||
| try await stores.application.deleteCredential(identityID: identityID) | ||
| try await stores.shared.deleteCredential(identityID: identityID) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check that the test target has a host application and entitlements.
fd -H 'project.yml' -x rg -n -C10 'RemuxAppTests'Repository: h3nock/remux
Length of output: 912
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list matching test file =="
fd -a 'FileProviderSharedStorageTests.swift' .
echo "== project.yml around test targets =="
cat -n project.yml | sed -n '60,130p'
echo "== Xcode project files matching RemuxAppTests/test targets =="
git ls-files | rg '(^|/)project\.yml$|\.xcodeproj|\.xcworkspace|project\.' | sed -n '1,200p'Repository: h3nock/remux
Length of output: 2840
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate remux app and app tests references =="
rg -n "Remux|RemuxAppTests|RemuxTests|hostApplication|APPLICATION_IDENTITY|keychain|entitlements|fileProviderCredentialStores|FileProviderCredentialStores" -S . --glob '!**/ghostty-remux-upstream-rebuild/**' || true
echo "== inspect candidate project files sizes =="
for f in $(fd -H 'project\.ya?ml$|project\.json$' .); do
wc -l "$f"
doneRepository: h3nock/remux
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test file around target storage and failing assertions =="
cat -n RemuxAppTests/FileProviderSharedStorageTests.swift | sed -n '1,140p'
echo "== FileProviderCredentialStores declarations =="
rg -n -C4 "FileProviderCredentialStores|fileProviderCredentialStores|deleteCredential|loadCredential|saveCredential" RemuxApp/Sources RemuxAppTests --glob '*.swift'Repository: h3nock/remux
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== RemuxAppDependencies:"
rg -n -C8 "fileProviderCredentialStores|FileProviderCredentialStores|remux tests" RemuxApp RemuxAppTests --glob '*.swift'
echo "== project.yml config around Remux and RemuxTests build settings:"
cat -n project.yml | sed -n '20,100p'Repository: h3nock/remux
Length of output: 6914
Clean the live keychain items from every teardown path.
This test writes to KeychainSSHCredentialStore on the test target path. If an assertion fails or the method exits early, the deletes on lines 51-52 never run and two random-service keychain items remain. Register the cleanup with addTeardownBlock immediately after creating the stores, then remove the trailing deletes from the happy-path block.
🤖 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/FileProviderSharedStorageTests.swift` around lines 37 - 53, In
testLiveCredentialStoresKeepApplicationSourceSeparateFromSharedDestination,
register an addTeardownBlock immediately after creating stores to delete the
application and shared credentials for the generated identityID on every exit
path, then remove the trailing happy-path delete calls.
| 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.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid a fatal trap when the chunk list is exhausted.
chunks.removeFirst() traps on an empty array. If a regression makes downloadFile perform one more read than expected, the test process crashes and the remaining tests do not report. Return an empty Data instead and let the assertion on requests() detect the extra read.
💚 Proposed change
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 286 - 289,
Update readData(from:length:) to return empty Data when chunks is exhausted
instead of calling removeFirst() on an empty array, while preserving request
recording and normal chunk consumption so requests() detects unexpected reads.
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/domain-foundationcodex/file-provider-review/read-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