File Provider: add contained remote read service - #37
Open
obra wants to merge 12 commits into
Open
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.
Reconstruct the accepted F8 identity boundary as a direct child of the contained remote-item leaf. This is a move-only commit: the production and test files retain their immutable accepted blob bytes, and the project membership is the deterministic XcodeGen output. Move FileProviderRemotePath.swift blob 4f097dc, FileProviderItemProjection.swift blob 5d99e0f, FileProviderItemIdentity.swift blob 6894a71, and FileProviderRemoteItemTests.swift blob b9bf198. The identity value blob composes accepted commits a8ea8cd, e9f19c9, and 1f85860; the projection blob composes a6cc7d3 and a8ea8cd. The F7 focused baseline passed 12/12. Moving only the accepted F8 test blob produced the expected compile RED for the missing identity type and codec API. After moving the three production blobs, the same suite passed 12/12 and a normal iPhone 17 simulator build succeeded. XcodeGen 2.44.1 was deterministic across two runs, and all eight generated PBX additions are present in the accepted oracle. This leaf intentionally excludes writable projection capabilities, snapshot or mutation behavior, extension activation, live-host qualification, compatibility work, and any newly authored glue or tests.
Reconstruct the accepted snapshot-generation store and F6-compatible File Provider error mapper as one atomic review leaf. The accepted snapshot tests call the mapper, while the mapper consumes FileProviderSnapshotStoreError, so separating them would require an authored temporary seam. Move exact accepted blobs 159f2cb and 26fc78b with exact test blobs 2e43e56 and e1d0586. XcodeGen 2.44.1 deterministically adds only the 16 accepted source and test membership lines. Evidence: the F8 baseline passed 12/12; tests-first boundary discovery exposed the missing mapper dependency; the combined accepted tests then failed for only the absent store and mapper; the earlier accepted mapper proved non-exhaustive against the already-accepted F6 write transport; the exact F6-compatible mapper resolved that compile dependency; the final combined suites passed 22/22 and a normal iPhone 17 simulator build succeeded. This coherent 592-line production unit is the documented first-pass exception to the 500-line ceiling. It excludes local mutation commits, receipts, identity reservations, relocations, deleted identities, and later writable error cases owned by F10 and F13. No product, test, or project behavior was authored.
Move the accepted mutation-journal layer onto the bounded snapshot-generation parent without rewriting behavior. Snapshot mutations now persist identity reservations, relocations, deletions, refreshed directories, replay receipts, and opt-in pending signals as one generation or not at all. The commit contains only the exact accepted FileProviderSnapshotStore blob 5c319ca and exact 23-test blob dd23955, sourced from immutable commits e9f19c9, 234e3cf, 962c8b8, 28135e5, and 999e676. Relative to F9 it is +255/-1 production and +278/-0 tests; project.yml and the PBX project remain byte-identical. Verification: the F9 combined baseline passed 22/22; the accepted tests-first state failed only on missing mutation-journal APIs and associated types; the final snapshot and mapper suites passed 31/31; and the normal Remux simulator build succeeded.
Reconstruct the accepted per-domain operation coordinator as an atomic stack layer on the exact F10 mutation-journal parent. The coordinator preserves same-directory refresh coalescing, serial refresh/mutation ordering, waiter-owned cancellation, and the existing DEBUG behavior gates without adding later enumeration or signaling contracts. The production and test files are byte-identical to accepted blobs c527084 and 50abd44. Their provenance is the immutable accepted sequence 5109406, e8ec5cf, and b0b0b27, with accepted test stabilization from 5396170 and 43c75ce. XcodeGen contributes only the eight expected project-membership lines; project.yml is unchanged. Verification before commit: F10 snapshot/error-mapper baseline 31/31; test-first compile RED confined to absent coordinator/polling types; coordinator GREEN 6/6; combined F10+F11 regression 37/37; normal Remux iPhone simulator build succeeded.
Reconstruct the reviewed remote-read boundary from accepted bytes. The remote service and its tests come from the archived read-only implementation, while the Citadel provider composes the accepted read-only operation signature with the reconciled shared SSHAuthenticationMethodFactory call. Keep the provider scoped to RemuxSFTPReadOnlyClient and do not restore the obsolete RemuxSSHAuthenticationMethodFactory wrapper. Seed only the two accepted contract tests for password authentication and root-key construction. The replicated-extension-core test is intentionally omitted here and remains deferred to F21. This commit adds only the two production files, two test files, and deterministic XcodeGen membership. Verification covered the 37-test F11 baseline, an expected test-first compile failure on absent F12 types, the 51-test focused and SFTP regression gate, and a normal Remux simulator build.
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (36)
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: 1a816419cc
ℹ️ 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".
Comment on lines
+386
to
+389
| return try await connection.remuxOpenFileForWriting( | ||
| atPath: remotePath, | ||
| flags: [.write, .create, .forceCreate] | ||
| ) |
There was a problem hiding this comment.
When an attachment upload times out after creating its job-derived .part file, GhosttyAttachmentTransfer.upload deliberately skips temporary-file cleanup for operationTimedOut, and a retry uses the same path. Opening every write with .forceCreate now rejects that existing partial file, whereas the previous .truncate behavior allowed the retry to replace it, so the attachment remains unuploadable for that transfer. Use overwrite/truncate semantics for the existing attachment path, or clean up the partial file before retrying.
Useful? React with 👍 / 👎.
Comment on lines
+22
to
+27
| var metadataVersion: Data { | ||
| var data = contentVersion | ||
| data.appendString(path.relative) | ||
| data.appendString(name) | ||
| data.appendOptional(permissions) | ||
| return Data(SHA256.hash(data: data)) |
There was a problem hiding this comment.
When a symlink is replaced with another target while its type, size, permissions, and second-resolution modification time remain unchanged, both versions produced here are identical even though symlinkTargetPath changes. The snapshot delta still emits an update because the remote items differ, but File Provider receives that update with unchanged item versions and can retain the old link target; incorporate symlinkTargetRelativePath into the content or metadata version.
Useful? React with 👍 / 👎.
Comment on lines
+606
to
+610
| let previousByPath = Dictionary( | ||
| uniqueKeysWithValues: previousItems.map { ($0.remoteItem.path, $0) } | ||
| ) | ||
| let currentByPath = Dictionary( | ||
| uniqueKeysWithValues: currentItems.map { ($0.remoteItem.path, $0) } |
There was a problem hiding this comment.
When commit relocates an item, it intentionally preserves its identity while changing its path, but these path-keyed maps classify the move as both an update at the new path and a deletion at the old path. Both entries therefore carry the same NSFileProviderItemIdentifier, so a change observer can delete the item it was simultaneously told to move. Key the comparison by stable identity so relocations produce an update only, while true identity replacement at the same path produces a delete plus insert.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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/mutation-statecodex/file-provider-review/remote-read-serviceThe 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.