fix: website build under TypeScript 7, menu bar panel, and S3 client leak - #27
Open
lollipopkit wants to merge 82 commits into
Open
fix: website build under TypeScript 7, menu bar panel, and S3 client leak#27lollipopkit wants to merge 82 commits into
lollipopkit wants to merge 82 commits into
Conversation
Cloudflare Pages has been failing on main since the dependency bump in #25 raised typescript to ^7.0.2: [typesafe-i18n] generating files for TypeScript version: '7.0.x' TypeError: ts.createProgram is not a function TypeScript 7 ships no stable programmatic API, so typesafe-i18n 5.27.1 cannot transpile the locale files. Upstream tracks this in codingcommons/typesafe-i18n#794 with a fix proposed in #795, neither merged. The workaround suggested there patches typesafe-i18n to use the Bun runtime, which does not apply here because Cloudflare installs dependencies with npm. The generator is dropped from the build instead: all eight files it produces are committed, and it reports "all files are up to date" on every run, so the build does not need it. Run `bun run typesafe-i18n` manually after changing translations. Note this does not repair the generator itself — it still fails under TypeScript 7. Until upstream lands a fix, regenerating requires temporarily installing typescript 6. The build also now installs through bun. Cloudflare detects only npm from the environment and runs `npm install`, so bun.lock never applied and dependency versions were free to drift between local and deployed builds; verified by installing a mismatched vite with npm and watching the build correct it.
Deploying mfuse with
|
| Latest commit: |
8533e87
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8291b6e3.mfuse.pages.dev |
| Branch Preview URL: | https://fix-website-typescript7-buil.mfuse.pages.dev |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 54 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR updates website tooling, connection configuration, credential flows, lifecycle coordination, S3 connections, mount operations, application behavior, localization, and macOS views. ChangesConnection platform and interfaces
Sequence Diagram(s)sequenceDiagram
participant ConnectionEditorSheet
participant ContentView
participant ConnectionManager
participant FileProviderMountProvider
ConnectionEditorSheet->>ContentView: submit connection save
ContentView->>ConnectionManager: save configuration and credentials
ConnectionManager->>FileProviderMountProvider: register or restore provider state
FileProviderMountProvider-->>ConnectionManager: registration result
ConnectionManager-->>ContentView: save result
ContentView-->>ConnectionEditorSheet: dismiss editor or show error
sequenceDiagram
participant ConnectionManager
participant S3FileSystem
participant ConnectivityProbe
participant AWSClient
ConnectionManager->>S3FileSystem: connect
S3FileSystem->>ConnectivityProbe: probe endpoint
ConnectivityProbe-->>S3FileSystem: probe result
S3FileSystem->>AWSClient: create client
AWSClient-->>S3FileSystem: connection or classified error
S3FileSystem-->>ConnectionManager: filesystem result
Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
The batch action row and the footer separated buttons with Spacer(), which
distributes the leftover space rather than the buttons themselves. Because the
labels differ in width ("Open MFuse" vs "Quit"), the icons ended up unevenly
spaced — measured 230px between the first two and 192px between the last two —
and the row read as off-centre.
Both rows now use equal-width cells, so each button centres within its own share
of the width. The footer's tap target covers its whole cell instead of just the
glyph and label.
lollipopkit
dismissed stale reviews from winnowl[bot] and coderabbitai[bot]
via
August 2, 2026 17:01
279c403
The connection list lived in a ScrollView, which collapsed to zero height and took every row with it — the panel jumped straight from the header to the batch actions even with connections configured. A ScrollView takes its ideal height from the space its parent offers rather than from its content, while a menu bar window sizes itself to its content. Neither side can resolve first, so the list got no height at all; `.frame(maxHeight:)` only capped it and supplied nothing. The rows are now placed directly and only move into a fixed-height ScrollView once there are more than seven, which also stops short lists from scrolling pointlessly. The detail view's Unmount button becomes an icon, with a tooltip and an accessibility label since the text no longer carries the meaning.
![winnowl[bot]](https://avatars.githubusercontent.com/in/4312382?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
✅ No blocking issues found — approving.
🛠️ To have the bot fix these findings, comment @winnowl fix.
📋 Additional findings from this change (not shown inline) (5)
- 🟡 Medium An Open-in-Finder operation survives detail selection changes and can reveal the previously selected connection after its view has disappeared.
- 🔵 Low The icon-only refresh control does not expose the localized “Refresh Finder listing” text as its accessibility label, unlike the adjacent icon-only unmount control.
- 🟡 Medium The long-list branch is height-bounded but not rendering-bounded: it places every connection row in a regular
VStack, so opening the menu and every published mount-state transition eagerly builds and diffs the full connection list rather than only the rows visible in the 320-point viewport. In addition, the same update performs several independent O(n) passes throughconnectionsformountedCountandmountingCount. With hundreds or thousands of saved/synced connections, a single transition can therefore stall menu opening/animation despite only roughly seven rows being visible; usingLazyVStackand deriving aggregate counts once per render/state snapshot would avoid this. Evidence: the scrolling branch wraps the sharedVStackatMenuBarView.swiftlines 89-100, each row callseffectiveMountState, and lines 319-325 independently filter the entire array; there is no connection-count cap inConnectionManager.add. This would be disproven if the product enforces a small hard maximum connection count before data reaches this view, or profiling demonstrates that SwiftUI lazily instantiates this regularVStackin the supported menu-window configuration. - 🟡 Medium A clean build is now forced to fetch many locked tarballs from
registry.npmmirror.com, a third-party mirror, rather than an explicitly configured trusted registry; deployments that deny that host fail before Vite, and approving it expands the build supply-chain trust boundary. - 🟡 Medium The new detail-view repair task can race a user-initiated disconnect and resurrect a stale mounted state after unmount completes.
repairMountStateawaitsmountURL, then recreates the symlink and unconditionally writes.mounted; becauseConnectionManageris main-actor isolated but reentrant across awaits,disconnectcan remove the symlink/domain and write.unmountedin between, after which the older repair continuation overwrites it. The UI can therefore show the connection as mounted and recreate its Finder symlink even though the provider domain was just disconnected. The repair should be generation/cancellation guarded (and ideally revalidate the provider state) before creating the symlink or committing.mounted.
🗑️ Suppressed and duplicate diagnostics (2)
- The new detail-view task can let a stale repair overwrite a newer mount transition or mount error, so the controls can show “mounted” and enable Finder/unmount/refresh after the user has disconnected, or before a concurrent connect has finished.
- The build script now requires Bun unconditionally, but the website package does not declare or pin Bun (there is no
packageManager/enginesentry or checked-in deployment/CI setup that provisions it), so a standard Node-only build runner will fail before Vite is invoked.
🤖 Prompt for AI agents — all findings (5)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Additional findings on this change (not posted inline) (5)
In MFuse/Views/ConnectionDetailView.swift around line 74, address this finding:
An Open-in-Finder operation survives detail selection changes and can reveal the previously selected connection after its view has disappeared.
In MFuse/Views/ConnectionDetailView.swift around line 135, address this finding:
The icon-only refresh control does not expose the localized “Refresh Finder listing” text as its accessibility label, unlike the adjacent icon-only unmount control.
In MFuse/Views/MenuBarView.swift around line 89, address this finding:
The long-list branch is height-bounded but not rendering-bounded: it places every connection row in a regular `VStack`, so opening the menu and every published mount-state transition eagerly builds and diffs the full connection list rather than only the rows visible in the 320-point viewport. In addition, the same update performs several independent O(n) passes through `connections` for `mountedCount` and `mountingCount`. With hundreds or thousands of saved/synced connections, a single transition can therefore stall menu opening/animation despite only roughly seven rows being visible; using `LazyVStack` and deriving aggregate counts once per render/state snapshot would avoid this. Evidence: the scrolling branch wraps the shared `VStack` at `MenuBarView.swift` lines 89-100, each row calls `effectiveMountState`, and lines 319-325 independently filter the entire array; there is no connection-count cap in `ConnectionManager.add`. This would be disproven if the product enforces a small hard maximum connection count before data reaches this view, or profiling demonstrates that SwiftUI lazily instantiates this regular `VStack` in the supported menu-window configuration.
In website/package.json around line 9, address this finding:
A clean build is now forced to fetch many locked tarballs from `registry.npmmirror.com`, a third-party mirror, rather than an explicitly configured trusted registry; deployments that deny that host fail before Vite, and approving it expands the build supply-chain trust boundary.
In MFuse/Views/ConnectionDetailView.swift around line 55, address this finding:
The new detail-view repair task can race a user-initiated disconnect and resurrect a stale mounted state after unmount completes. `repairMountState` awaits `mountURL`, then recreates the symlink and unconditionally writes `.mounted`; because `ConnectionManager` is main-actor isolated but reentrant across awaits, `disconnect` can remove the symlink/domain and write `.unmounted` in between, after which the older repair continuation overwrites it. The UI can therefore show the connection as mounted and recreate its Finder symlink even though the provider domain was just disconnected. The repair should be generation/cancellation guarded (and ideally revalidate the provider state) before creating the symlink or committing `.mounted`.
📜 Review details
Model
- gpt-5.6-sol
Coverage
- scopes: 3/4 complete
Two separate causes, both from the app running as an accessory once its window is closed. Settings opened without activating the app first, unlike Open MFuse right above it, so the window appeared behind whatever was frontmost and the click looked ignored. dismissMenuBarPanel closed NSApp.keyWindow, but an accessory app's menu bar panel never becomes key — keyWindow is nil and nothing closed, leaving the panel covering the action it had just triggered. It now falls back to the frontmost panel-level window. Also stops the S3 backend from crashing the extension. connect() had no idempotency guard, so the extension's connect-timeout retry built a second AWSClient over the first; the abandoned one was released without a shutdown, which trips an assertion in AWSClient.deinit and killed MFuseProvider three times during testing. Clients are now reused, and every path that drops one routes through a single shutdown helper that tolerates alreadyShutdown.
lollipopkit
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🛠️ To have the bot fix these findings, comment @winnowl fix.
🔎 Confirmed findings (1)
- 🟡 Medium
dismissMenuBarPanel()can hide an unrelated main app window and leave the MenuBarExtra panel open because it unconditionally orders outNSApp.keyWindowbefore looking for the visible panel. (inline)
📋 Additional findings from this change (not shown inline) (9)
- 🟡 Medium A mounted presentation can remain green and actionable after Finder resolution proves that no reachable mount exists. If the File Provider domain was removed/disconnected externally (or its symlink and recorded mount path disappeared) while
effectiveMountStateis still.mounted,resolveFinderURLreturns nil; this action silently does nothing and leaves Reveal, Refresh, and Unmount visible with status still reporting the stale path. There is no feedback or state repair on the nil branch. This would be false if another guaranteed observer updatesmountStatesbeforeresolveFinderURLcan return nil, or if a nil result is surfaced to the user/state by code outside this action. - 🟡 Medium Batch Mount All can execute a stale mount target after it has already become mounted because filtering and execution are separated and ConnectionManager.connect does not revalidate effective mount state.
- 🟡 Medium Rapid or stale Unmount All/row actions can run disconnect for the same connection concurrently or after it is already unmounted, issuing duplicate mount-provider teardown and allowing a later duplicate failure to overwrite a successful unmount with an error state.
- 🟡 Medium “Open MFuse” can create an additional main window instead of restoring the main window that was hidden when the app transitioned to menu-bar-only mode. The shutdown-cancel path retains all windows and only calls orderOut, while the footer then invokes openWindow on a WindowGroup, whose open action is a request for another group window. Repeating Command-Q (which is canceled into accessory mode) followed by Open MFuse can therefore accumulate hidden/duplicate main windows rather than reopening the intended existing interface.
- 🟡 Medium
connect()is actor-reentrant across the connectivity-probeawait, so overlapping lifecycle calls can both leak a successfully probed client and resurrect connected state after a disconnect. For example, connect A creates client A and suspends inlistObjectsV2; connect B enters while all stored state is still nil, creates client B, and both probes succeed. A publishes its pair, then B overwrites it without shutting client A down, so A's last references are released withoutAWSClient.shutdown(). Likewise, adisconnect()that runs while A is probing sees no published client and returns, after which A resumes and publishesisConnected == true, contrary to the completed disconnect. This would be false if actor methods could not interleave atawait, or if an in-progress generation/token prevented stale attempts from publishing and owned cleanup of every local client; neither mechanism is present. - 🟡 Medium The new connectivity probe lists the whole bucket because it omits the configured remote-path prefix. A valid filesystem configuration rooted at (for example)
team-a/can use an IAM/bucket policy that permitss3:ListBucketonly whens3:prefixstarts withteam-a/; all filesystem enumeration under that configured root is then authorized becauses3Key(for:)prependsconfig.remotePath, butconnect()now sends an unprefixed ListObjectsV2 and fails with AccessDenied/authenticationFailed. This rejects a supported least-privilege configuration before it can connect. The claim would be false if MFuse requires bucket-wide ListBucket permission regardless ofremotePath, but the operational requests consistently scope keys/prefixes throughs3Key(for:)and no such requirement is documented in the configuration model. - 🔵 Low The lifecycle behavior added in this scope has no deterministic test coverage: the MFuseS3 test target still contains only a placeholder and does not exercise successful/failed probes, retry after failure, overlapping or idempotent connect/disconnect, or shutdown counts. Consequently the reentrant overwrite/disconnect scenarios and exactly-once shutdown invariant are not guarded by the test suite. This would be false if lifecycle tests existed elsewhere against the concrete
S3FileSystem, but repository search finds only this MFuseS3 placeholder test and no other references testingS3FileSystem. - 🟠 High Generic S3 probe failures are copied verbatim into a user-visible
RemoteFileSystemError.connectionFailed, so secrets contained in an SDK/transport error description are neither removed nor marked private before propagation. - 🟠 High The build script now has a hard runtime dependency on Bun, but the repository neither declares a Bun version nor provisions Bun in any CI/deployment configuration, so invoking the website's advertised build script on a normal Node-based build worker fails before dependency installation with
bun: command not found.
♻️ Previously reported (still present) (3)
- 🟠 High Launching
repairMountStatewhenever the detail appears can overwrite a newer unmount result and make the detail return to a false mounted presentation.repairMountStateawaitsmountURL/createSymlinkand then unconditionally writes.mounted, but it has no cancellation or connection-generation check; becauseConnectionManageris MainActor-isolated,disconnectcan run during either await, finish by setting.unmounted, and then the older repair continuation can set.mountedagain (even if symlink recreation failed). The header will consequently re-enable Reveal, Refresh, and Unmount for a domain that was just disconnected. This would be false only if the mount-provider operations are proven never to suspend/interleave with disconnect, or repair is otherwise serialized/invalidated by disconnect. - 🔵 Low The icon-only refresh control has a tooltip but no action accessibility label, so VoiceOver receives the SF Symbol-derived name (for example, “arrow clockwise”) rather than “Refresh Finder listing.” This makes the mounted-state action ambiguous to nonvisual users, while the adjacent icon-only unmount control correctly supplies both
.helpand.accessibilityLabel. The claim would be false if the supported SwiftUI/macOS versions are demonstrated to promote.helpto the button's accessible title rather than merely AXHelp. - 🟡 Medium A clean build now downloads a substantial part of the locked graph from
registry.npmmirror.comrather than the default npm registry, creating an undocumented third-party package-source dependency and causing frozen clean builds to fail in deployment networks that only allow the expected npm registry.
🗑️ Suppressed and duplicate diagnostics (2)
- Panel dismissal can hide the main application window instead of the menu bar extra whenever an unrelated normal window remains key.
- The S3 test target still contains only its placeholder test, so none of the new classification behavior is covered by the required unit cases.
🤖 Prompt for AI agents — all findings (13)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Findings on this change (also posted as inline comments) (1)
In MFuse/Views/MenuBarView.swift around line 355, address this finding:
`dismissMenuBarPanel()` can hide an unrelated main app window and leave the MenuBarExtra panel open because it unconditionally orders out `NSApp.keyWindow` before looking for the visible panel.
## Additional findings on this change (not posted inline) (9)
In MFuse/Views/ConnectionDetailView.swift around line 75, address this finding:
A mounted presentation can remain green and actionable after Finder resolution proves that no reachable mount exists. If the File Provider domain was removed/disconnected externally (or its symlink and recorded mount path disappeared) while `effectiveMountState` is still `.mounted`, `resolveFinderURL` returns nil; this action silently does nothing and leaves Reveal, Refresh, and Unmount visible with status still reporting the stale path. There is no feedback or state repair on the nil branch. This would be false if another guaranteed observer updates `mountStates` before `resolveFinderURL` can return nil, or if a nil result is surfaced to the user/state by code outside this action.
In MFuse/Views/MenuBarView.swift around line 112, address this finding:
Batch Mount All can execute a stale mount target after it has already become mounted because filtering and execution are separated and ConnectionManager.connect does not revalidate effective mount state.
In MFuse/Views/MenuBarView.swift around line 140, address this finding:
Rapid or stale Unmount All/row actions can run disconnect for the same connection concurrently or after it is already unmounted, issuing duplicate mount-provider teardown and allowing a later duplicate failure to overwrite a successful unmount with an error state.
In MFuse/Views/MenuBarView.swift around line 168, address this finding:
“Open MFuse” can create an additional main window instead of restoring the main window that was hidden when the app transitioned to menu-bar-only mode. The shutdown-cancel path retains all windows and only calls orderOut, while the footer then invokes openWindow on a WindowGroup, whose open action is a request for another group window. Repeating Command-Q (which is canceled into accessory mode) followed by Open MFuse can therefore accumulate hidden/duplicate main windows rather than reopening the intended existing interface.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 85, address this finding:
`connect()` is actor-reentrant across the connectivity-probe `await`, so overlapping lifecycle calls can both leak a successfully probed client and resurrect connected state after a disconnect. For example, connect A creates client A and suspends in `listObjectsV2`; connect B enters while all stored state is still nil, creates client B, and both probes succeed. A publishes its pair, then B overwrites it without shutting client A down, so A's last references are released without `AWSClient.shutdown()`. Likewise, a `disconnect()` that runs while A is probing sees no published client and returns, after which A resumes and publishes `isConnected == true`, contrary to the completed disconnect. This would be false if actor methods could not interleave at `await`, or if an in-progress generation/token prevented stale attempts from publishing and owned cleanup of every local client; neither mechanism is present.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 84, address this finding:
The new connectivity probe lists the whole bucket because it omits the configured remote-path prefix. A valid filesystem configuration rooted at (for example) `team-a/` can use an IAM/bucket policy that permits `s3:ListBucket` only when `s3:prefix` starts with `team-a/`; all filesystem enumeration under that configured root is then authorized because `s3Key(for:)` prepends `config.remotePath`, but `connect()` now sends an unprefixed ListObjectsV2 and fails with AccessDenied/authenticationFailed. This rejects a supported least-privilege configuration before it can connect. The claim would be false if MFuse requires bucket-wide ListBucket permission regardless of `remotePath`, but the operational requests consistently scope keys/prefixes through `s3Key(for:)` and no such requirement is documented in the configuration model.
In Packages/MFuseS3/Tests/MFuseS3Tests/S3FileSystemTests.swift around line 5, address this finding:
The lifecycle behavior added in this scope has no deterministic test coverage: the MFuseS3 test target still contains only a placeholder and does not exercise successful/failed probes, retry after failure, overlapping or idempotent connect/disconnect, or shutdown counts. Consequently the reentrant overwrite/disconnect scenarios and exactly-once shutdown invariant are not guarded by the test suite. This would be false if lifecycle tests existed elsewhere against the concrete `S3FileSystem`, but repository search finds only this MFuseS3 placeholder test and no other references testing `S3FileSystem`.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 134, address this finding:
Generic S3 probe failures are copied verbatim into a user-visible `RemoteFileSystemError.connectionFailed`, so secrets contained in an SDK/transport error description are neither removed nor marked private before propagation.
In website/package.json around line 9, address this finding:
The build script now has a hard runtime dependency on Bun, but the repository neither declares a Bun version nor provisions Bun in any CI/deployment configuration, so invoking the website's advertised build script on a normal Node-based build worker fails before dependency installation with `bun: command not found`.
## Previously reported and still present (3)
In MFuse/Views/ConnectionDetailView.swift around line 55, address this finding:
Launching `repairMountState` whenever the detail appears can overwrite a newer unmount result and make the detail return to a false mounted presentation. `repairMountState` awaits `mountURL`/`createSymlink` and then unconditionally writes `.mounted`, but it has no cancellation or connection-generation check; because `ConnectionManager` is MainActor-isolated, `disconnect` can run during either await, finish by setting `.unmounted`, and then the older repair continuation can set `.mounted` again (even if symlink recreation failed). The header will consequently re-enable Reveal, Refresh, and Unmount for a domain that was just disconnected. This would be false only if the mount-provider operations are proven never to suspend/interleave with disconnect, or repair is otherwise serialized/invalidated by disconnect.
In MFuse/Views/ConnectionDetailView.swift around line 135, address this finding:
The icon-only refresh control has a tooltip but no action accessibility label, so VoiceOver receives the SF Symbol-derived name (for example, “arrow clockwise”) rather than “Refresh Finder listing.” This makes the mounted-state action ambiguous to nonvisual users, while the adjacent icon-only unmount control correctly supplies both `.help` and `.accessibilityLabel`. The claim would be false if the supported SwiftUI/macOS versions are demonstrated to promote `.help` to the button's accessible title rather than merely AXHelp.
In website/bun.lock, address this finding:
A clean build now downloads a substantial part of the locked graph from `registry.npmmirror.com` rather than the default npm registry, creating an undocumented third-party package-source dependency and causing frozen clean builds to fail in deployment networks that only allow the expected npm registry.
📜 Review details
Model
- gpt-5.6-sol
Coverage
- scopes: 5/7 complete
| if let key = NSApp.keyWindow { | ||
| key.orderOut(nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
dismissMenuBarPanel() can hide an unrelated main app window and leave the MenuBarExtra panel open because it unconditionally orders out NSApp.keyWindow before looking for the visible panel.
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
⚠️ SwiftUI does not publicly document the concrete NSWindow subclass or key-window behavior used by MenuBarExtraStyle.window, so this relies on the non-key panel behavior already observed and documented in the changed helper's comment.
🤖 Prompt for AI agents
In MFuse/Views/MenuBarView.swift, address this finding:
`dismissMenuBarPanel()` can hide an unrelated main app window and leave the MenuBarExtra panel open because it unconditionally orders out `NSApp.keyWindow` before looking for the visible panel.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| } | |
| if let key = NSApp.keyWindow, | |
| key.level.rawValue > NSWindow.Level.normal.rawValue { | |
| key.orderOut(nil) | |
| return | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@MFuse/Views/MenuBarView.swift`:
- Around line 351-361: Update the dismissal logic in the menu-bar action to
resolve the specific MenuBarExtra panel NSWindow and call orderOut(nil) only on
that panel. Remove the generic NSApp.keyWindow and first elevated-window
fallback so main or Settings windows cannot be hidden.
In `@Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift`:
- Around line 45-56: Serialize the S3FileSystem lifecycle by coordinating
connect() and disconnect() around the in-flight listObjectsV2 operation:
coalesce concurrent connect() calls, prevent a later connect() from replacing an
active client, and have disconnect() cancel or await any pending connection
before returning. Update the lifecycle state near awsClient and s3, and add
tests covering concurrent connects plus disconnect during a suspended connect,
ensuring no client is published after disconnect.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e99b204c-2cff-4fa4-928d-c734f0a08227
📒 Files selected for processing (3)
MFuse/Views/ConnectionDetailView.swiftMFuse/Views/MenuBarView.swiftPackages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: winnowl/review
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
MFuse/Views/MenuBarView.swift (5)
8-11: LGTM!
80-102: LGTM!
127-131: LGTM!Also applies to: 149-149
170-179: LGTM!
201-207: LGTM!MFuse/Views/ConnectionDetailView.swift (1)
98-110: LGTM!Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift (2)
87-87: LGTM!
98-108: 🩺 Stability & AvailabilitySuppress only verified-benign shutdown errors.
This helper catches every error from
AWSClient.shutdown()and then lets callers release the last client reference. If Soto 7.14.0 reports a transport or cancellation failure, cleanup is not proven complete and theAWSClient.deinitassertion can still occur.disconnect()is also declaredasync throws, but this helper prevents it from reporting shutdown failures.Ignore only the known
alreadyShutdowncase, or surface/log other failures while preserving the original connection error. Soto requires explicitAWSClient.shutdown()before deinitialization. (soto.codes)Confirm the Soto 7.14.0 error contract and add a failed-shutdown test.
Comment on lines
+351
to
+361
| // The menu bar panel does not become the key window while the app runs as an | ||
| // accessory, so closing `NSApp.keyWindow` silently does nothing and the panel | ||
| // stays open over the action it just triggered. Fall back to the frontmost | ||
| // panel-level window, which is the menu bar extra itself. | ||
| if let key = NSApp.keyWindow { | ||
| key.orderOut(nil) | ||
| return | ||
| } | ||
| NSApp.windows | ||
| .first { $0.isVisible && $0.level.rawValue > NSWindow.Level.normal.rawValue }? | ||
| .orderOut(nil) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'MenuBarExtra|NSPanel|NSWindow|windowLevel|keyWindow|orderOut|dismissMenuBarPanel' --glob '*.swift' .Repository: lollipopkit/mfuse
Length of output: 19816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- MenuBarView structure and dismissal context ---'
ast-grep outline MFuse/Views/MenuBarView.swift --match 'MenuBarView' --view expanded
sed -n '1,80p;330,365p' MFuse/Views/MenuBarView.swift
printf '%s\n' '--- App scene and window declarations ---'
sed -n '130,205p;220,255p' MFuse/MFuseApp.swift
printf '%s\n' '--- Window-related symbols and identifiers ---'
rg -n -C 5 'WindowGroup|Window\(|Settings|MenuBarExtra|windowIdentifier|identifier|NSWindow|keyWindow|mainWindow|orderOut' MFuse --glob '*.swift'Repository: lollipopkit/mfuse
Length of output: 46459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class Window:
name: str
visible: bool
level: int
normal = 0
main = Window("main", True, normal)
settings = Window("settings", True, normal)
panel = Window("menu-bar panel", True, 100)
def target(windows, key):
if key is not None:
return key.name
return next((w.name for w in windows if w.visible and w.level > normal), None)
cases = [
("main is key while panel is visible", [main, panel], main),
("settings is key while panel is visible", [settings, panel], settings),
("no key and multiple elevated windows", [
Window("other elevated window", True, 200), panel
], None),
]
for label, windows, key in cases:
selected = target(windows, key)
print(f"{label}: selected={selected!r}")
PY
printf '%s\n' '--- Declared application scenes and dismissal implementation ---'
python3 - <<'PY'
from pathlib import Path
for path in [Path("MFuse/MFuseApp.swift"), Path("MFuse/Views/MenuBarView.swift")]:
text = path.read_text()
print(path)
for i, line in enumerate(text.splitlines(), 1):
if ("WindowGroup" in line or "Settings {" in line or
"MenuBarExtra" in line or "if let key = NSApp.keyWindow" in line or
"NSApp.windows" in line):
print(f"{i}: {line}")
PYRepository: lollipopkit/mfuse
Length of output: 690
🌐 Web query:
Apple AppKit NSApplication keyWindow windows documentation and SwiftUI MenuBarExtra window style panel behavior
💡 Result:
In AppKit, the keyWindow property of NSApplication represents the window currently designated to receive keyboard events [1][2]. Only one window in the application can be the key window at any given time [2]. When a window is the key window, AppKit highlights its title bar to indicate that it is the active focus for user input [2]. A window's status as the key window changes automatically based on user interactions, such as clicking on the window [2]. Regarding SwiftUI's MenuBarExtra with the.window style, this modifier renders the content in a popover-like, chromeless window anchored to the menu bar icon [3][4][5]. Because MenuBarExtra behaves differently from standard application windows, developers often encounter challenges accessing the underlying NSWindow or detecting its visibility state using standard SwiftUI lifecycle modifiers like scenePhase [6][7][8]. Key behavioral points for MenuBarExtra with.window style include: - Lifecycle Limitations: Standard modifiers such as.onAppear or.onDisappear may not reliably trigger in the way they do for primary application windows because the MenuBarExtra window's lifecycle is managed by the system in response to menu bar interactions [7]. - Key Window Interaction: When a MenuBarExtra window is open, it may become the key window (or influence the app's key window state) [6]. Developers sometimes observe that checking NSApplication.shared.keyWindow is a functional workaround for detecting when the MenuBarExtra window is visible to the user [6]. - Lack of First-Party API: SwiftUI does not currently provide native, stable API to programmatically control the presentation state or directly access the underlying NSWindow of a MenuBarExtra [8]. Consequently, some developers rely on third-party libraries or KVO (Key-Value Observing) on the NSApplication.keyWindow property to manage these interactions [6][8].
Citations:
- 1: https://developer.apple.com/documentation/appkit/nsapplication/keywindow
- 2: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/WinPanel/Concepts/ChangingMainKeyWindow.html
- 3: https://developer.apple.com/documentation/swiftui/menubarextrastyle/window
- 4: https://developer.apple.com/documentation/swiftui/menubarextra
- 5: https://developer.apple.com/videos/play/wwdc2022/10061/
- 6: https://damian.fyi/swift/2022/12/29/detecting-when-a-swiftui-menubarextra-with-window-style-is-opened.html
- 7: macOS 15 Sequoia | .onChange(of:) value track issue orchetect/MenuBarExtraAccess#13
- 8: https://github.com/orchetect/MenuBarExtraAccess
Dismiss only the MenuBarExtra panel.
If the main or Settings window is key, NSApp.keyWindow.orderOut(nil) hides that window. If no key window exists, the first elevated window is not guaranteed to be the menu-bar panel. Resolve the panel’s NSWindow and call orderOut(nil) only on that window.
🤖 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 `@MFuse/Views/MenuBarView.swift` around lines 351 - 361, Update the dismissal
logic in the menu-bar action to resolve the specific MenuBarExtra panel NSWindow
and call orderOut(nil) only on that panel. Remove the generic NSApp.keyWindow
and first elevated-window fallback so main or Settings windows cannot be hidden.
resolveFinderURL required FileManager.fileExists to succeed on the mount URL before returning it. The app is sandboxed and cannot reliably stat paths under ~/Library/CloudStorage, so that check failed for exactly the mounts it was meant to guard, the function fell through to nil, and the callers — which can only ignore a nil — did nothing at all. Finder opens those paths regardless of whether this process can stat them, so the mount URL is now returned as soon as it resolves. The mount-path fallback drops the same check for the same reason. A nil result is now logged with the locations that were tried, instead of leaving no trace of why the action was a no-op.
The path was always the App Group container, so every row showed the same truncated "/Users/…/Library/Contain…" prefix and never got far enough to say which mount it belonged to. The menu bar row now carries just the name and status, since the panel is a quick switcher. The sidebar keeps the address, which does identify the connection, and loses only the path line.
There was a problem hiding this comment.
Actionable comments posted: 0
✅ No blocking issues found — approving.
🛠️ To have the bot fix these findings, comment @winnowl fix.
📋 Additional findings from this change (not shown inline) (17)
- 🟡 Medium The mounted detail button remains enabled for the entire asynchronous disconnect and every click starts another unstructured task, while
ConnectionManager.disconnecthas no per-ID in-flight exclusion; rapid clicks can issue overlapping symlink/domain/filesystem disconnect operations and produce order-dependent final.unmountedversus.errorUI state. - 🟡 Medium A user-requested Finder refresh discards every enumerator error with
try?, producing neither user feedback nor a log entry, so a failed refresh is indistinguishable from success while the screen continues to show the mount as healthy. - 🔵 Low No UI or view-state tests exercise
ConnectionDetailView, so the required mounted/mounting/unmounted/error rendering matrix, mounted-only action availability, and accessibility metadata are all unverified by this change. - 🟡 Medium The repository has no lifecycle/menu-bar tests for the newly relied-on dismissal, activation, and termination paths, so the scope's required test alignment (key-window/fallback dismissal, app/Settings activation, duplicate Quit, cancellation versus full termination) is unmet.
- 🟠 High disconnect() can return successfully while an already-started connect() is still in flight, and that connect can subsequently publish a live client, leaving the filesystem connected after the explicit disconnect.
- 🟡 Medium disconnect() shuts down the shared AWSClient while actor methods that already copied the S3 service across an await may still be using it, so actor isolation does not prevent in-flight operations from running against an already-shut-down client.
- 🟡 Medium connect() treats empty access-key and secret strings as valid credentials, allowing endpoint/network errors to mask an invalid credential set as connectionFailed instead of deterministically returning authenticationFailed.
- 🟡 Medium All AWSClient.shutdown errors are swallowed, so explicit disconnect reports success even when teardown failed and the backend cannot establish that the client was cleanly shut down.
- 🟡 Medium Ranged reads can return more bytes than the requested
lengthwhen S3 or a compatible endpoint ignores/mishandles the Range header, because the implementation accepts up tolength + 1024body bytes and returns the entire collected body without validating or truncating it. - 🟡 Medium
itemInfo(at:)does not special-case the remote filesystem root, so a configured base prefix can cause the mount root to be reported as a file rather than the required directory. - 🟠 High Deleting a path that has both an exact object and descendant objects deletes the descendants and leaves the exact file, even though the filesystem resolves that path as a file.
- 🟠 High Multi-object delete can return HTTP success with per-object errors, but the implementation discards the response and reports the deletion successful while failed objects remain.
- 🟠 High Copy and move preflight is vulnerable to a destination creation race: CopyObject is issued without an atomic destination create-only condition and can overwrite an object created after the preflight.
- 🟡 Medium The copy-source encoder leaves Unicode letters and digits unescaped because CharacterSet.alphanumerics is Unicode-wide, producing a non-ASCII CopySource value instead of S3-required UTF-8 percent encoding for keys such as 文档.txt.
- 🟠 High Moving a directory into one of its own descendants copies under the source prefix and then recursively deletes that prefix, deleting the newly created destination while returning success.
- 🟡 Medium Opening a connection's detail view can turn an intentionally disconnected File Provider domain back into a UI-level
mountedstate without reconnecting it. The new.taskcallsrepairMountState, which treats any non-nilmountURLas proof of a live mount, butdisconnectdeliberately keeps the domain registered andFileProviderMountProvider.mountURLonly checks that the domain exists before asking for its root URL. A disconnected registered domain can therefore still yield a URL; merely navigating to the detail screen then overwrites.unmountedwith.mounted, exposing Open/Unmount controls and corrupting the menu/sidebar counts. Repair should consultdomainStates().isDisconnected(as startup sync does), or otherwise verify provider connection state before setting.mounted. - 🟡 Medium Mounted rows always advertise the convenience symlink path even though the mount lifecycle explicitly permits
.mountedaftercreateSymlinkreturns nil or throws, and Finder resolution now falls back to the real mount URL in that case. Thus a name collision, symlink creation failure, or sandbox reachability failure leaves Menu Bar/Sidebar showing a path that does not exist while Open in Finder targets a different location. The displayed path should come from the resolved reachable URL/mount state, or the state should retain whether symlink creation succeeded.
♻️ Previously reported (still present) (7)
- 🟡 Medium
repairMountStatecannot repair a stale positive mount state: when the provider reports no mount URL (or throws), it leaves the existing.mountedstate untouched, so the detail screen can continue presenting Finder/refresh/unmount actions for a domain that was disconnected outside the app. - 🟡 Medium The appearance repair task can race an unmount and overwrite its result with
.mounted:repairMountStateawaits provider work and then unconditionally sets mounted without checking a connection generation or whether disconnect began in the meantime. - 🟡 Medium Unmount All can race another disconnect (or a newer remount) for the same connection, because each task calls the reentrant
ConnectionManager.disconnectwith no per-ID in-flight exclusion or operation generation check. For example, while the batch call is suspended inremoveSymlink, a row/sidebar disconnect can enter too; one removal can then make the other report an error after the domain was successfully unmounted. Likewise, a user mount started while the old disconnect is suspended can complete before that old disconnect resumes, after which the stale disconnect tears down the newly mounted domain and overwrites its state with.unmountedor.error. - 🟡 Medium
dismissMenuBarPanel()does not identify the menu-bar-extra window: it orders out any current key window, and with no key window it orders out the first visible above-normal-level window. Consequently a menu action can hide the main window, Settings, or an unrelated floating/AppKit panel instead of (or while failing to dismiss) the menu-bar extra. - 🟡 Medium Concurrent calls to connect() can each create and validate a separate AWSClient, after which the later completion overwrites the first successful client's retained state without shutting that first client down.
- 🟡 Medium The S3 test target still contains only a placeholder, so none of the new error-mapping or lifecycle behavior—including the actor-reentrancy failure paths—is exercised.
- 🟠 High The fallback connection error exposes the complete SDK error description in a RemoteFileSystemError, allowing credential-bearing request or server-provided text to reach caller-visible errors and public File Provider logs.
🗑️ Suppressed and duplicate diagnostics (1)
- The S3 test target contains only a placeholder, so none of the scope-required mocked-S3 cases are exercised.
🤖 Prompt for AI agents — all findings (24)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Additional findings on this change (not posted inline) (17)
In MFuse/Views/ConnectionDetailView.swift around line 100, address this finding:
The mounted detail button remains enabled for the entire asynchronous disconnect and every click starts another unstructured task, while `ConnectionManager.disconnect` has no per-ID in-flight exclusion; rapid clicks can issue overlapping symlink/domain/filesystem disconnect operations and produce order-dependent final `.unmounted` versus `.error` UI state.
In MFuse/Views/ConnectionDetailView.swift around line 130, address this finding:
A user-requested Finder refresh discards every enumerator error with `try?`, producing neither user feedback nor a log entry, so a failed refresh is indistinguishable from success while the screen continues to show the mount as healthy.
In MFuse/Views/ConnectionDetailView.swift around line 5, address this finding:
No UI or view-state tests exercise `ConnectionDetailView`, so the required mounted/mounting/unmounted/error rendering matrix, mounted-only action availability, and accessibility metadata are all unverified by this change.
In MFuse/MFuseApp.swift around line 244, address this finding:
The repository has no lifecycle/menu-bar tests for the newly relied-on dismissal, activation, and termination paths, so the scope's required test alignment (key-window/fallback dismissal, app/Settings activation, duplicate Quit, cancellation versus full termination) is unmet.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 94, address this finding:
disconnect() can return successfully while an already-started connect() is still in flight, and that connect can subsequently publish a live client, leaving the filesystem connected after the explicit disconnect.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 141, address this finding:
disconnect() shuts down the shared AWSClient while actor methods that already copied the S3 service across an await may still be using it, so actor isolation does not prevent in-flight operations from running against an already-shut-down client.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 57, address this finding:
connect() treats empty access-key and secret strings as valid credentials, allowing endpoint/network errors to mask an invalid credential set as connectionFailed instead of deterministically returning authenticationFailed.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 101, address this finding:
All AWSClient.shutdown errors are swallowed, so explicit disconnect reports success even when teardown failed and the backend cannot establish that the client was cleanly shut down.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 265, address this finding:
Ranged reads can return more bytes than the requested `length` when S3 or a compatible endpoint ignores/mishandles the Range header, because the implementation accepts up to `length + 1024` body bytes and returns the entire collected body without validating or truncating it.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 208, address this finding:
`itemInfo(at:)` does not special-case the remote filesystem root, so a configured base prefix can cause the mount root to be reported as a file rather than the required directory.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 350, address this finding:
Deleting a path that has both an exact object and descendant objects deletes the descendants and leaves the exact file, even though the filesystem resolves that path as a file.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 343, address this finding:
Multi-object delete can return HTTP success with per-object errors, but the implementation discards the response and reports the deletion successful while failed objects remain.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 441, address this finding:
Copy and move preflight is vulnerable to a destination creation race: CopyObject is issued without an atomic destination create-only condition and can overwrite an object created after the preflight.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 492, address this finding:
The copy-source encoder leaves Unicode letters and digits unescaped because CharacterSet.alphanumerics is Unicode-wide, producing a non-ASCII CopySource value instead of S3-required UTF-8 percent encoding for keys such as 文档.txt.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 363, address this finding:
Moving a directory into one of its own descendants copies under the source prefix and then recursively deletes that prefix, deleting the newly created destination while returning success.
In MFuse/Views/ConnectionDetailView.swift around line 55, address this finding:
Opening a connection's detail view can turn an intentionally disconnected File Provider domain back into a UI-level `mounted` state without reconnecting it. The new `.task` calls `repairMountState`, which treats any non-nil `mountURL` as proof of a live mount, but `disconnect` deliberately keeps the domain registered and `FileProviderMountProvider.mountURL` only checks that the domain exists before asking for its root URL. A disconnected registered domain can therefore still yield a URL; merely navigating to the detail screen then overwrites `.unmounted` with `.mounted`, exposing Open/Unmount controls and corrupting the menu/sidebar counts. Repair should consult `domainStates().isDisconnected` (as startup sync does), or otherwise verify provider connection state before setting `.mounted`.
In MFuse/Views/MenuBarView.swift around line 247, address this finding:
Mounted rows always advertise the convenience symlink path even though the mount lifecycle explicitly permits `.mounted` after `createSymlink` returns nil or throws, and Finder resolution now falls back to the real mount URL in that case. Thus a name collision, symlink creation failure, or sandbox reachability failure leaves Menu Bar/Sidebar showing a path that does not exist while Open in Finder targets a different location. The displayed path should come from the resolved reachable URL/mount state, or the state should retain whether symlink creation succeeded.
## Previously reported and still present (7)
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 643, address this finding:
`repairMountState` cannot repair a stale positive mount state: when the provider reports no mount URL (or throws), it leaves the existing `.mounted` state untouched, so the detail screen can continue presenting Finder/refresh/unmount actions for a domain that was disconnected outside the app.
In MFuse/Views/ConnectionDetailView.swift around line 56, address this finding:
The appearance repair task can race an unmount and overwrite its result with `.mounted`: `repairMountState` awaits provider work and then unconditionally sets mounted without checking a connection generation or whether disconnect began in the meantime.
In MFuse/Views/MenuBarView.swift, address this finding:
Unmount All can race another disconnect (or a newer remount) for the same connection, because each task calls the reentrant `ConnectionManager.disconnect` with no per-ID in-flight exclusion or operation generation check. For example, while the batch call is suspended in `removeSymlink`, a row/sidebar disconnect can enter too; one removal can then make the other report an error after the domain was successfully unmounted. Likewise, a user mount started while the old disconnect is suspended can complete before that old disconnect resumes, after which the stale disconnect tears down the newly mounted domain and overwrites its state with `.unmounted` or `.error`.
In MFuse/Views/MenuBarView.swift around line 355, address this finding:
`dismissMenuBarPanel()` does not identify the menu-bar-extra window: it orders out any current key window, and with no key window it orders out the first visible above-normal-level window. Consequently a menu action can hide the main window, Settings, or an unrelated floating/AppKit panel instead of (or while failing to dismiss) the menu-bar extra.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 85, address this finding:
Concurrent calls to connect() can each create and validate a separate AWSClient, after which the later completion overwrites the first successful client's retained state without shutting that first client down.
In Packages/MFuseS3/Tests/MFuseS3Tests/S3FileSystemTests.swift around line 5, address this finding:
The S3 test target still contains only a placeholder, so none of the new error-mapping or lifecycle behavior—including the actor-reentrancy failure paths—is exercised.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 134, address this finding:
The fallback connection error exposes the complete SDK error description in a RemoteFileSystemError, allowing credential-bearing request or server-provided text to reach caller-visible errors and public File Provider logs.
📜 Review details
Model
- gpt-5.6-sol
Coverage
- scopes: 7/8 complete
There was a problem hiding this comment.
Actionable comments posted: 0
✅ No blocking issues found — approving.
🛠️ To have the bot fix these findings, comment @winnowl fix.
📋 Additional findings from this change (not shown inline) (10)
- 🔵 Low When
mountURL(for:)throws or returns nil, the fallback accepts any reachable symbolic link at the generated shortcut pathname without checking that its resolved destination is a managed File Provider mount. For example, an unmanaged process/item can leave<symlinkBase>/<sanitized-name>-<uuid>pointing to any existing directory; if provider lookup is temporarily unavailable, Reveal in Finder selects that directory. This is inconsistent withFileProviderMountProvider.shouldRemoveManagedSymlink, which only treats a matching filename as managed when its resolved destination is under~/Library/CloudStorage. Evidence:hasReachableLinkonly reads the destination and callsfileExists, while the fallback returns the URL immediately. The claim would be false if access to every possible configured symlink base were guaranteed to be exclusive to this provider and no unmanaged entry could exist there, or if the resolver validated the destination against the managed mount destination/current recorded mount before returning it. - 🟡 Medium A reveal can recreate a convenience symlink after a concurrent disconnect has removed it, leaving a shortcut behind for an unmounted domain. Concrete sequence: Reveal obtains a nonnil mount URL and suspends in
createSymlink; while suspended,disconnectremoves the symlink and disconnects the domain; the reveal call then resumes and creates/returns the link.ConnectionManageris MainActor-isolated, but actor reentrancy at these awaits permits this ordering, and unlike mount-resolution tasks this reveal operation is neither tracked/cancelled nor guarded by connection generation/mount state after the await. The subsequent Finder activation can also use a now-disconnected target. Evidence: the resolver unconditionally callscreateSymlinkand returns after awaits, whiledisconnectseparately callsremoveSymlinkand has no synchronization with reveal tasks. The claim would be false if the mount provider serialized create/remove so creation could not complete after disconnect, or if disconnect/cancellation made every in-flightcreateSymlinkfail before filesystem creation;MountProviderprovides no such guarantee andFileProviderMountProviderhas no serialization/cancellation check around creation. - 🟡 Medium Unmount can be initiated concurrently by repeated presses because the button remains visible/enabled throughout
disconnectand each press creates an independent unstructuredTask; unlikeconnect,ConnectionManager.disconnecthas no per-ID in-flight guard.disconnectdoes not change mount state until after awaiting symlink removal and File Provider disconnect, so a double-click can run two removals/disconnects. With the real provider, both removals can observe the same symlink and one can fail after the other removes it; that failure is accumulated as a cleanup failure and can leave.errorafter the domain was successfully disconnected, producing misleading UI. Evidence is the delayed finalsetMountState(.unmounted...), the absence of a guard indisconnect, andremoveManagedSymlinkIfNeededusing a check-then-remove sequence. This would be false if the UI disabled/replaced the control immediately or the manager serialized/coalesced disconnects, neither of which occurs here. - 🟠 High When there is no key window, dismissal can order out an unrelated visible floating/panel-level window because the fallback selects the first window above normal level rather than identifying the MenuBarExtra window.
- 🟡 Medium An effective
.errorrow is always presented as mountable, even when the error was caused by a failed disconnect that left the existing filesystem alive; in that state the exposed Mount action is contradictory and is a guaranteed no-op. - 🟠 High Concurrent removals can corrupt the saved connection list: if removal A is awaiting credential deletion while removal B succeeds, then a credential-deletion failure for A restores A's full pre-removal
previousConnectionssnapshot, resurrecting B in memory and storage even though B's credential was successfully deleted. - 🟡 Medium The sidebar's richer mount indicator is visual-only for mounting and error states, so VoiceOver users cannot determine those states from the row: both are conveyed only by the color of an unlabeled
Circle, while the folder icon is hidden for both. - 🟠 High Two overlapping connect calls can each create and validate an AWSClient; the later completion overwrites the first successful client/service without shutting the first client down.
- 🟡 Medium The centralized shutdown helper releases a client even when shutdown fails for a reason other than an already-shutdown condition, leaving the exact deinit assertion/event-loop leak that the lifecycle change is intended to prevent.
- 🟠 High Connection error classification ignores Soto's structured AWSErrorType.errorCode and relies entirely on String(describing:), so authentication errors from providers whose descriptions do not contain the exact code text are misclassified as transient connection failures.
♻️ Previously reported (still present) (8)
- 🟡 Medium The new navigation repair can overwrite a completed unmount with
.mounted:repairMountStateawaitsmountProvider.mountURL, then unconditionally creates a symlink and sets mounted without checking task cancellation, the connection generation, or the current domain/mount state. A concrete sequence is: opening a mounted detail starts the.task; the user presses Unmount whilemountURLis suspended;disconnectremoves the symlink/disconnects and sets.unmounted; then the cancellation-insensitive repair resumes with the URL it obtained and sets.mounted, restoring mounted-only controls and potentially the symlink for a disconnected domain. This is exposed by adding repair on detail navigation. Evidence is the unconditional post-awaitsetMountState(.mounted...)inConnectionManager.repairMountState, whereas connect's mount-resolution path explicitly callsTask.checkCancellation()and disconnect advances the connection generation. This would be false if every supportedMountProvider.mountURLwere guaranteed to abort on task cancellation and never return after a concurrent disconnect, but that guarantee is absent from the protocol and the repair code does not enforce it. - 🔵 Low The refresh control is icon-only but has no explicit accessibility label. Its
.help("Refresh Finder listing")supplies a pointer tooltip, while the accessibility element is derived fromImage(systemName: "arrow.clockwise"); VoiceOver therefore does not get the localized action name required for this control (and may announce the symbol description rather than the intended Finder-refresh action). The neighboring icon-only Unmount button explicitly adds both.helpand.accessibilityLabel, demonstrating the expected pattern. This would be false if SwiftUI on every supported macOS version promoted.helpinto the button's accessibility title, but.helpis tooltip/help metadata rather than the explicit VoiceOver label used elsewhere in this view. - 🟠 High Panel dismissal can hide the main application window instead of the menu-bar panel whenever another window remains key.
- 🟡 Medium Repeated Unmount All clicks (or an Unmount All click racing a row unmount) can dispatch concurrent disconnect operations for the same connection, because the UI leaves mounted rows/actions enabled until disconnect finishes and ConnectionManager has no in-flight disconnect guard. For example, reopening the panel during a slow File Provider disconnect and clicking Unmount All again invokes removeSymlink/disconnect twice; provider operations may fail or race and one invocation can publish an erroneous mount error after the other succeeds.
- 🟠 High A disconnect that arrives while connect is awaiting the S3 probe is lost, and the suspended connect can subsequently publish a connected service after disconnect has returned.
- 🟡 Medium The generic mapping embeds an unredacted SDK error description into a user/loggable RemoteFileSystemError, allowing endpoint credentials, signed URLs, authorization material, or provider-echoed secrets to reach public logs.
- 🔵 Low The required MFuseS3 lifecycle and error-classification tests were not added; the package declares an MFuseS3Tests target but the repository has no corresponding test sources.
- 🟠 High The production build now mandates Bun, but the website does not pin a Bun version anywhere. A clean deployment image with no Bun fails immediately with
bun: not found, while an image whose provider-selected Bun predates support for the committed textbun.lockcan fail the frozen install before Vite runs. The repository-wide searches found nopackageManager,BUN_VERSION, orbun-versiondeclaration and no deployment workflow/config that installs Bun;website/package.jsononly invokes it. This would be disproven if the actual deployment configuration outside this repository guarantees and pins a compatible Bun release for every build.
🗑️ Suppressed and duplicate diagnostics (1)
- Running the newly added frozen install from a clean cache makes the deployment contact
registry.npmmirror.com, a third-party registry host, for many locked artifacts rather than consistently using the standard npm registry. For example,website/bun.lockpins@jridgewell/gen-mapping(line 37),gsap(line 193), and numerous native optional packages to explicithttps://registry.npmmirror.com/...tarball URLs. A deployment with the usual npm-registry-only egress policy will therefore fail before Vite, and a policy that forbids third-party registry mirrors is violated even though integrity hashes pin artifact bytes. This would be disproven if the deployment policy explicitly approves and allows npmmirror (or guarantees a complete Bun package cache on every clean build).
🤖 Prompt for AI agents — all findings (18)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Additional findings on this change (not posted inline) (10)
In MFuse/Services/ConnectionManager+Finder.swift around line 31, address this finding:
When `mountURL(for:)` throws or returns nil, the fallback accepts any reachable symbolic link at the generated shortcut pathname without checking that its resolved destination is a managed File Provider mount. For example, an unmanaged process/item can leave `<symlinkBase>/<sanitized-name>-<uuid>` pointing to any existing directory; if provider lookup is temporarily unavailable, Reveal in Finder selects that directory. This is inconsistent with `FileProviderMountProvider.shouldRemoveManagedSymlink`, which only treats a matching filename as managed when its resolved destination is under `~/Library/CloudStorage`. Evidence: `hasReachableLink` only reads the destination and calls `fileExists`, while the fallback returns the URL immediately. The claim would be false if access to every possible configured symlink base were guaranteed to be exclusive to this provider and no unmanaged entry could exist there, or if the resolver validated the destination against the managed mount destination/current recorded mount before returning it.
In MFuse/Services/ConnectionManager+Finder.swift around line 24, address this finding:
A reveal can recreate a convenience symlink after a concurrent disconnect has removed it, leaving a shortcut behind for an unmounted domain. Concrete sequence: Reveal obtains a nonnil mount URL and suspends in `createSymlink`; while suspended, `disconnect` removes the symlink and disconnects the domain; the reveal call then resumes and creates/returns the link. `ConnectionManager` is MainActor-isolated, but actor reentrancy at these awaits permits this ordering, and unlike mount-resolution tasks this reveal operation is neither tracked/cancelled nor guarded by connection generation/mount state after the await. The subsequent Finder activation can also use a now-disconnected target. Evidence: the resolver unconditionally calls `createSymlink` and returns after awaits, while `disconnect` separately calls `removeSymlink` and has no synchronization with reveal tasks. The claim would be false if the mount provider serialized create/remove so creation could not complete after disconnect, or if disconnect/cancellation made every in-flight `createSymlink` fail before filesystem creation; `MountProvider` provides no such guarantee and `FileProviderMountProvider` has no serialization/cancellation check around creation.
In MFuse/Views/ConnectionDetailView.swift around line 99, address this finding:
Unmount can be initiated concurrently by repeated presses because the button remains visible/enabled throughout `disconnect` and each press creates an independent unstructured `Task`; unlike `connect`, `ConnectionManager.disconnect` has no per-ID in-flight guard. `disconnect` does not change mount state until after awaiting symlink removal and File Provider disconnect, so a double-click can run two removals/disconnects. With the real provider, both removals can observe the same symlink and one can fail after the other removes it; that failure is accumulated as a cleanup failure and can leave `.error` after the domain was successfully disconnected, producing misleading UI. Evidence is the delayed final `setMountState(.unmounted...)`, the absence of a guard in `disconnect`, and `removeManagedSymlinkIfNeeded` using a check-then-remove sequence. This would be false if the UI disabled/replaced the control immediately or the manager serialized/coalesced disconnects, neither of which occurs here.
In MFuse/Views/MenuBarView.swift around line 349, address this finding:
When there is no key window, dismissal can order out an unrelated visible floating/panel-level window because the fallback selects the first window above normal level rather than identifying the MenuBarExtra window.
In MFuse/Views/SidebarView.swift around line 123, address this finding:
An effective `.error` row is always presented as mountable, even when the error was caused by a failed disconnect that left the existing filesystem alive; in that state the exposed Mount action is contradictory and is a guaranteed no-op.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 183, address this finding:
Concurrent removals can corrupt the saved connection list: if removal A is awaiting credential deletion while removal B succeeds, then a credential-deletion failure for A restores A's full pre-removal `previousConnections` snapshot, resurrecting B in memory and storage even though B's credential was successfully deleted.
In MFuse/Views/SidebarView.swift around line 105, address this finding:
The sidebar's richer mount indicator is visual-only for mounting and error states, so VoiceOver users cannot determine those states from the row: both are conveyed only by the color of an unlabeled `Circle`, while the folder icon is hidden for both.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 94, address this finding:
Two overlapping connect calls can each create and validate an AWSClient; the later completion overwrites the first successful client/service without shutting the first client down.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 104, address this finding:
The centralized shutdown helper releases a client even when shutdown fails for a reason other than an already-shutdown condition, leaving the exact deinit assertion/event-loop leak that the lifecycle change is intended to prevent.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 118, address this finding:
Connection error classification ignores Soto's structured AWSErrorType.errorCode and relies entirely on String(describing:), so authentication errors from providers whose descriptions do not contain the exact code text are misclassified as transient connection failures.
## Previously reported and still present (8)
In MFuse/Views/ConnectionDetailView.swift around line 55, address this finding:
The new navigation repair can overwrite a completed unmount with `.mounted`: `repairMountState` awaits `mountProvider.mountURL`, then unconditionally creates a symlink and sets mounted without checking task cancellation, the connection generation, or the current domain/mount state. A concrete sequence is: opening a mounted detail starts the `.task`; the user presses Unmount while `mountURL` is suspended; `disconnect` removes the symlink/disconnects and sets `.unmounted`; then the cancellation-insensitive repair resumes with the URL it obtained and sets `.mounted`, restoring mounted-only controls and potentially the symlink for a disconnected domain. This is exposed by adding repair on detail navigation. Evidence is the unconditional post-await `setMountState(.mounted...)` in `ConnectionManager.repairMountState`, whereas connect's mount-resolution path explicitly calls `Task.checkCancellation()` and disconnect advances the connection generation. This would be false if every supported `MountProvider.mountURL` were guaranteed to abort on task cancellation and never return after a concurrent disconnect, but that guarantee is absent from the protocol and the repair code does not enforce it.
In MFuse/Views/ConnectionDetailView.swift around line 135, address this finding:
The refresh control is icon-only but has no explicit accessibility label. Its `.help("Refresh Finder listing")` supplies a pointer tooltip, while the accessibility element is derived from `Image(systemName: "arrow.clockwise")`; VoiceOver therefore does not get the localized action name required for this control (and may announce the symbol description rather than the intended Finder-refresh action). The neighboring icon-only Unmount button explicitly adds both `.help` and `.accessibilityLabel`, demonstrating the expected pattern. This would be false if SwiftUI on every supported macOS version promoted `.help` into the button's accessibility title, but `.help` is tooltip/help metadata rather than the explicit VoiceOver label used elsewhere in this view.
In MFuse/Views/MenuBarView.swift around line 345, address this finding:
Panel dismissal can hide the main application window instead of the menu-bar panel whenever another window remains key.
In MFuse/Views/MenuBarView.swift around line 135, address this finding:
Repeated Unmount All clicks (or an Unmount All click racing a row unmount) can dispatch concurrent disconnect operations for the same connection, because the UI leaves mounted rows/actions enabled until disconnect finishes and ConnectionManager has no in-flight disconnect guard. For example, reopening the panel during a slow File Provider disconnect and clicking Unmount All again invokes removeSymlink/disconnect twice; provider operations may fail or race and one invocation can publish an erroneous mount error after the other succeeds.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 85, address this finding:
A disconnect that arrives while connect is awaiting the S3 probe is lost, and the suspended connect can subsequently publish a connected service after disconnect has returned.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 134, address this finding:
The generic mapping embeds an unredacted SDK error description into a user/loggable RemoteFileSystemError, allowing endpoint credentials, signed URLs, authorization material, or provider-echoed secrets to reach public logs.
In Packages/MFuseS3/Package.swift around line 22, address this finding:
The required MFuseS3 lifecycle and error-classification tests were not added; the package declares an MFuseS3Tests target but the repository has no corresponding test sources.
In website/package.json around line 9, address this finding:
The production build now mandates Bun, but the website does not pin a Bun version anywhere. A clean deployment image with no Bun fails immediately with `bun: not found`, while an image whose provider-selected Bun predates support for the committed text `bun.lock` can fail the frozen install before Vite runs. The repository-wide searches found no `packageManager`, `BUN_VERSION`, or `bun-version` declaration and no deployment workflow/config that installs Bun; `website/package.json` only invokes it. This would be disproven if the actual deployment configuration outside this repository guarantees and pins a compatible Bun release for every build.
📜 Review details
Model
- gpt-5.6-sol
Coverage
- scopes: 5/6 complete
Sidebar rows signalled mount state three times over: the backend icon turned green, a folder glyph faded in, and the trailing dot changed colour. The icon and the folder glyph are gone; the dot alone carries it, and the backend icon keeps a stable tint so it reads as an identifier rather than a status light. The detail view's Mount section only appears on failure. While mounted it repeated the container path, but it is the sole place a mount error is shown, so it stays for that case instead of being removed outright.
There was a problem hiding this comment.
Actionable comments posted: 0
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Unresolved from previous review (1) — not approved until fixed
- Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift: Deleting a path that has both an exact object and descendant objects deletes the descendants and leaves the exact file, even though the filesystem resolves that path as a file.
📋 Additional findings from this change (not shown inline) (6)
- 🟡 Medium An appearance-time mount repair can overwrite a later successful unmount and put the detail view back into a false
.mountedstate. - 🟡 Medium Removing a connection while it is still in the connection handshake can report success and permanently delete the configuration even when cleanup of the just-created filesystem later fails.
- 🔵 Low Removal alerts discard the localized description supplied by removal errors and can show enum/debug representations instead of localized user-facing context.
- 🟡 Medium
createFiledoes not fail when the RemoteItem path already exists as a directory. Iffoo/is a marker or there are objects belowfoo/,If-None-Match: *on the distinct keyfoosucceeds, leaving both a file and directory at RemotePath/foo; subsequent enumeration can return duplicate paths anditemInfoselects the file. Evidence is that creation checks only exact keyfoo, whereas the copy/move collision helper separately checks both exact key andfoo/prefix. This would be false if the contract defines directory existence as irrelevant to create-file collisions, contrary to its “fails if exists” namespace semantics. - 🟡 Medium Missing-file errors are not mapped consistently to
RemoteFileSystemError.notFound. Both full and rangedreadFilecallgetObjectand propagate its raw AWSError on a 404/NoSuchKey, despite this type having anisNotFoundErrorhelper anditemInfomapping the same condition to.notFound(path). File Provider callers therefore cannot classify a missing read through the RemoteFileSystem error contract. This would be false if Soto itself throwsRemoteFileSystemError.notFound, which it cannot do because that is an MFuseCore-specific type. - 🟡 Medium Full-object reads have no effective body-size bound:
collect(upTo: .max)buffers the entire S3 response in extension memory before creating anotherDatavalue. Opening or otherwise requesting a multi-gigabyte S3 object can exhaust memory and terminate the File Provider process rather than returning a controlled error or relying on ranged reads. The ranged overload is bounded, making the unbounded full-read path explicit. This would be false if all callers enforce a small object-size limit before calling this method; the public RemoteFileSystem API and this method contain no such precondition.
♻️ Previously reported (still present) (17)
- 🟡 Medium When File Provider mount URL resolution fails or returns nil, the fallback can reveal an unrelated reachable directory because it treats any reachable symbolic link at the generated shortcut pathname as the connection's existing managed symlink, without validating its destination against the recorded mount path.
- 🟡 Medium Starting unmount leaves the detail view's unmount, Finder, and refresh controls enabled for the entire asynchronous disconnect, allowing duplicate disconnects and mounted-only actions to race the teardown.
- 🟡 Medium The panel-dismissal helper can hide an unrelated application window instead of the menu-bar-extra panel.
- 🟡 Medium Repeated unmount actions can run concurrent disconnects for the same connection because the UI leaves the row and batch action enabled as mounted for the entire asynchronous disconnect.
- 🟡 Medium Finder reveal can recreate/open a stale mount location after the connection has been removed or edited while URL resolution is suspended.
- 🟠 High
connect()leavesawsClientands3nil while awaiting the connectivity probe, so actor reentrancy permits concurrent connects and disconnects to violate lifecycle invariants. Two connects can each create a client; if B succeeds then A succeeds, A overwrites B and B is dropped without shutdown. A disconnect that runs during A's probe sees no client and returns, after which A publishes a connected service. - 🟡 Medium Generic connection failures expose the complete SDK error description in a stable
RemoteFileSystemError.connectionFailedmessage. Soto's S3 error description incorporates the remote service's error message, and transport errors can contain the custom endpoint; this text is subsequently UI-visible and logged publicly by retry handling, allowing endpoint details or server-echoed request/signing data to escape. - 🟡 Medium Credential validation only checks that the access-key fields are non-nil, not that they are non-empty. A directly constructed S3 filesystem (including the main app path, which does not use the extension's separate non-empty guard) can therefore create an AWS client and issue a signed probe with empty credentials instead of deterministically returning
.authenticationFailed; depending on the endpoint/error representation this can be misreported as a transient connection failure and retried. - 🟡 Medium The shutdown wrapper suppresses every
AWSClient.shutdown()error, after which failed-connect and disconnect paths release their only reference to the client. If shutdown fails before the client's resources are closed, a live client is dropped (and can trigger the Soto deinit assertion the change is intended to prevent), whiledisconnect()still reports success so the caller cannot retry cleanup. - 🔵 Low The S3 package declares an
MFuseS3Teststarget but contains no S3 test sources, so none of the required lifecycle/error-contract cases are covered. In particular, the concurrent-connect/disconnect overwrite and failed-shutdown behavior can regress without a deterministic test double or local endpoint exercising repeat connect, stale cleanup, failed probes, idempotent disconnect, authentication codes, NoSuchBucket, and network failures. - 🟠 High Moving a directory into one of its own descendants can report success while deleting all of the moved data. For example, with source
/acontainingxand absent destination/a/b, the destination check passes,copyItemcopiesa/xtoa/b/x, and thendelete(at: /a)recursively deletes botha/xand the newly createda/b/x. There is nodestination.isDescendant(of: source)guard before the copy. This would be false if callers are contractually prohibited from requesting descendant destinations and that prohibition is enforced before this backend is called; the public RemoteFileSystem contract shown here has no such restriction. - 🟠 High Deletion targets the directory form before determining which RemoteItem the path denotes, causing wrong-object deletion when S3 contains both
fooandfoo/....itemInfo(/foo)reports the exact object as a file, butdelete(/foo)listsfoo/, deletes all descendants, setsdeletedDirectoryObjects, and deliberately skips deleting the exactfooobject. Thus deleting the reported file destroys unrelated descendant data and leaves the file in place. This would be false if the implementation prevented such dual forms from existing, butcreateFilecurrently permits creatingfoobeside an existingfoo/prefix and arbitrary S3 buckets can already contain both. - 🟡 Medium An in-flight operation can continue with a stale S3 service after disconnect/reconnect because actor methods are reentrant across
await. Recursive copy capturess3once, then awaits each list/copy request;disconnectcan interleave, clear and shut down that client, and a later continuation still invokes the captured service (even if a reconnect has installed a different one), yielding partial copies or requests against a shut-down client.requireS3only validates at method entry. This would be false if disconnect is externally serialized with every filesystem operation, but no such invariant appears in the RemoteFileSystem contract and actor isolation alone does not provide it across awaits. - 🟡 Medium Unicode letters in a CopyObject source key are not reliably percent-encoded because the allowed set is
CharacterSet.alphanumerics, which includes non-ASCII Unicode scalars, while S3's CopyObject source header requires the UTF-8 key to be URL-encoded. A key such ascafé/資料.txtcan therefore be sent with literal Unicode incopySource, causing signature/header rejection on AWS or stricter compatible services even though spaces and ASCII reserved characters are escaped. This would be false if Foundation'saddingPercentEncodingescaped every non-ASCII scalar despite those scalars being explicitly present in the allowed CharacterSet, or if Soto re-encodes this modeled header after receipt. - 🟡 Medium The frozen production install is not reproducible from the official npm registry alone: the committed lockfile pins numerous tarballs, including the direct
gsapdependency and nativelightningcsspackages used by Vite/Tailwind, tohttps://registry.npmmirror.com/.... In a CI/deployment network that allowlists only registry.npmjs.org (or when this third-party mirror is unavailable),bun install --frozen-lockfilefails beforevite build; it also makes the build trust a registry source not declared or reviewed in package.json. Evidence includes the explicit GSAP URL at the anchor and explicit mirror URLs formagic-string,postcss,lightningcss-*, andvitefu, while package.json's build always performs the frozen install. This claim would be false if the supported deployment policy explicitly permits and trusts npmmirror and clean installs on every supported platform demonstrably do not fetch the lockfile's explicit URLs. - 🟡 Medium Opening a connection detail can race its automatic mount-state repair against an unmount. The view starts
repairMountStatein a task, while the header can concurrently calldisconnect;repairMountStateawaitsmountURL/symlink creation and then unconditionally writes.mountedwithout a connection-generation or current-state check. If the user unmounts while that await is in flight, disconnect can finish and set.unmounted, only for the stale repair to overwrite it with.mounted, leaving all views showing Finder/unmount actions for a domain that was just disconnected. - 🟡 Medium The detail view's automatic repair can turn an intentionally disconnected registered File Provider domain back into a user-visible
.mountedstate.repairMountStateconsiders any non-nilmountURLsufficient, whereassyncMountsfirst checksdomainStates()and explicitly keepsisDisconnecteddomains unmounted. Because registration is retained on normal disconnect, opening details later can bypass that disconnected-state check and expose Mount/Reveal controls inconsistent with the actual domain state.
🤖 Prompt for AI agents — all findings (24)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (1)
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift, address this finding:
Deleting a path that has both an exact object and descendant objects deletes the descendants and leaves the exact file, even though the filesystem resolves that path as a file.
## Additional findings on this change (not posted inline) (6)
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 651, address this finding:
An appearance-time mount repair can overwrite a later successful unmount and put the detail view back into a false `.mounted` state.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 112, address this finding:
Removing a connection while it is still in the connection handshake can report success and permanently delete the configuration even when cleanup of the just-created filesystem later fails.
In MFuse/Views/SidebarView.swift around line 152, address this finding:
Removal alerts discard the localized description supplied by removal errors and can show enum/debug representations instead of localized user-facing context.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 288, address this finding:
`createFile` does not fail when the RemoteItem path already exists as a directory. If `foo/` is a marker or there are objects below `foo/`, `If-None-Match: *` on the distinct key `foo` succeeds, leaving both a file and directory at RemotePath `/foo`; subsequent enumeration can return duplicate paths and `itemInfo` selects the file. Evidence is that creation checks only exact key `foo`, whereas the copy/move collision helper separately checks both exact key and `foo/` prefix. This would be false if the contract defines directory existence as irrelevant to create-file collisions, contrary to its “fails if exists” namespace semantics.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift, address this finding:
Missing-file errors are not mapped consistently to `RemoteFileSystemError.notFound`. Both full and ranged `readFile` call `getObject` and propagate its raw AWSError on a 404/NoSuchKey, despite this type having an `isNotFoundError` helper and `itemInfo` mapping the same condition to `.notFound(path)`. File Provider callers therefore cannot classify a missing read through the RemoteFileSystem error contract. This would be false if Soto itself throws `RemoteFileSystemError.notFound`, which it cannot do because that is an MFuseCore-specific type.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 246, address this finding:
Full-object reads have no effective body-size bound: `collect(upTo: .max)` buffers the entire S3 response in extension memory before creating another `Data` value. Opening or otherwise requesting a multi-gigabyte S3 object can exhaust memory and terminate the File Provider process rather than returning a controlled error or relying on ranged reads. The ranged overload is bounded, making the unbounded full-read path explicit. This would be false if all callers enforce a small object-size limit before calling this method; the public RemoteFileSystem API and this method contain no such precondition.
## Previously reported and still present (17)
In MFuse/Services/ConnectionManager+Finder.swift around line 31, address this finding:
When File Provider mount URL resolution fails or returns nil, the fallback can reveal an unrelated reachable directory because it treats any reachable symbolic link at the generated shortcut pathname as the connection's existing managed symlink, without validating its destination against the recorded mount path.
In MFuse/Views/ConnectionDetailView.swift around line 98, address this finding:
Starting unmount leaves the detail view's unmount, Finder, and refresh controls enabled for the entire asynchronous disconnect, allowing duplicate disconnects and mounted-only actions to race the teardown.
In MFuse/Views/MenuBarView.swift around line 345, address this finding:
The panel-dismissal helper can hide an unrelated application window instead of the menu-bar-extra panel.
In MFuse/Views/MenuBarView.swift around line 288, address this finding:
Repeated unmount actions can run concurrent disconnects for the same connection because the UI leaves the row and batch action enabled as mounted for the entire asynchronous disconnect.
In MFuse/Services/ConnectionManager+Finder.swift around line 19, address this finding:
Finder reveal can recreate/open a stale mount location after the connection has been removed or edited while URL resolution is suspended.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 85, address this finding:
`connect()` leaves `awsClient` and `s3` nil while awaiting the connectivity probe, so actor reentrancy permits concurrent connects and disconnects to violate lifecycle invariants. Two connects can each create a client; if B succeeds then A succeeds, A overwrites B and B is dropped without shutdown. A disconnect that runs during A's probe sees no client and returns, after which A publishes a connected service.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 134, address this finding:
Generic connection failures expose the complete SDK error description in a stable `RemoteFileSystemError.connectionFailed` message. Soto's S3 error description incorporates the remote service's error message, and transport errors can contain the custom endpoint; this text is subsequently UI-visible and logged publicly by retry handling, allowing endpoint details or server-echoed request/signing data to escape.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 57, address this finding:
Credential validation only checks that the access-key fields are non-nil, not that they are non-empty. A directly constructed S3 filesystem (including the main app path, which does not use the extension's separate non-empty guard) can therefore create an AWS client and issue a signed probe with empty credentials instead of deterministically returning `.authenticationFailed`; depending on the endpoint/error representation this can be misreported as a transient connection failure and retried.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 103, address this finding:
The shutdown wrapper suppresses every `AWSClient.shutdown()` error, after which failed-connect and disconnect paths release their only reference to the client. If shutdown fails before the client's resources are closed, a live client is dropped (and can trigger the Soto deinit assertion the change is intended to prevent), while `disconnect()` still reports success so the caller cannot retry cleanup.
In Packages/MFuseS3/Package.swift around line 22, address this finding:
The S3 package declares an `MFuseS3Tests` target but contains no S3 test sources, so none of the required lifecycle/error-contract cases are covered. In particular, the concurrent-connect/disconnect overwrite and failed-shutdown behavior can regress without a deterministic test double or local endpoint exercising repeat connect, stale cleanup, failed probes, idempotent disconnect, authentication codes, NoSuchBucket, and network failures.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 362, address this finding:
Moving a directory into one of its own descendants can report success while deleting all of the moved data. For example, with source `/a` containing `x` and absent destination `/a/b`, the destination check passes, `copyItem` copies `a/x` to `a/b/x`, and then `delete(at: /a)` recursively deletes both `a/x` and the newly created `a/b/x`. There is no `destination.isDescendant(of: source)` guard before the copy. This would be false if callers are contractually prohibited from requesting descendant destinations and that prohibition is enforced before this backend is called; the public RemoteFileSystem contract shown here has no such restriction.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 350, address this finding:
Deletion targets the directory form before determining which RemoteItem the path denotes, causing wrong-object deletion when S3 contains both `foo` and `foo/...`. `itemInfo(/foo)` reports the exact object as a file, but `delete(/foo)` lists `foo/`, deletes all descendants, sets `deletedDirectoryObjects`, and deliberately skips deleting the exact `foo` object. Thus deleting the reported file destroys unrelated descendant data and leaves the file in place. This would be false if the implementation prevented such dual forms from existing, but `createFile` currently permits creating `foo` beside an existing `foo/` prefix and arbitrary S3 buckets can already contain both.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 375, address this finding:
An in-flight operation can continue with a stale S3 service after disconnect/reconnect because actor methods are reentrant across `await`. Recursive copy captures `s3` once, then awaits each list/copy request; `disconnect` can interleave, clear and shut down that client, and a later continuation still invokes the captured service (even if a reconnect has installed a different one), yielding partial copies or requests against a shut-down client. `requireS3` only validates at method entry. This would be false if disconnect is externally serialized with every filesystem operation, but no such invariant appears in the RemoteFileSystem contract and actor isolation alone does not provide it across awaits.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 492, address this finding:
Unicode letters in a CopyObject source key are not reliably percent-encoded because the allowed set is `CharacterSet.alphanumerics`, which includes non-ASCII Unicode scalars, while S3's CopyObject source header requires the UTF-8 key to be URL-encoded. A key such as `café/資料.txt` can therefore be sent with literal Unicode in `copySource`, causing signature/header rejection on AWS or stricter compatible services even though spaces and ASCII reserved characters are escaped. This would be false if Foundation's `addingPercentEncoding` escaped every non-ASCII scalar despite those scalars being explicitly present in the allowed CharacterSet, or if Soto re-encodes this modeled header after receipt.
In website/bun.lock, address this finding:
The frozen production install is not reproducible from the official npm registry alone: the committed lockfile pins numerous tarballs, including the direct `gsap` dependency and native `lightningcss` packages used by Vite/Tailwind, to `https://registry.npmmirror.com/...`. In a CI/deployment network that allowlists only registry.npmjs.org (or when this third-party mirror is unavailable), `bun install --frozen-lockfile` fails before `vite build`; it also makes the build trust a registry source not declared or reviewed in package.json. Evidence includes the explicit GSAP URL at the anchor and explicit mirror URLs for `magic-string`, `postcss`, `lightningcss-*`, and `vitefu`, while package.json's build always performs the frozen install. This claim would be false if the supported deployment policy explicitly permits and trusts npmmirror and clean installs on every supported platform demonstrably do not fetch the lockfile's explicit URLs.
In MFuse/Views/ConnectionDetailView.swift around line 53, address this finding:
Opening a connection detail can race its automatic mount-state repair against an unmount. The view starts `repairMountState` in a task, while the header can concurrently call `disconnect`; `repairMountState` awaits `mountURL`/symlink creation and then unconditionally writes `.mounted` without a connection-generation or current-state check. If the user unmounts while that await is in flight, disconnect can finish and set `.unmounted`, only for the stale repair to overwrite it with `.mounted`, leaving all views showing Finder/unmount actions for a domain that was just disconnected.
In MFuse/Views/ConnectionDetailView.swift around line 54, address this finding:
The detail view's automatic repair can turn an intentionally disconnected registered File Provider domain back into a user-visible `.mounted` state. `repairMountState` considers any non-nil `mountURL` sufficient, whereas `syncMounts` first checks `domainStates()` and explicitly keeps `isDisconnected` domains unmounted. Because registration is retained on normal disconnect, opening details later can bypass that disconnected-state check and expose Mount/Reveal controls inconsistent with the actual domain state.
📜 Review details
Model
- gpt-5.6-sol
Coverage
- scopes: 7/7 complete
Actions closed the panel by calling orderOut on its window directly. The status item has no idea that happened, so it kept drawing itself selected while no panel was open. Nothing dismisses the panel by hand any more. Mount, unmount and Finder now leave it up, so the row's state change is visible where it was triggered, and Open MFuse, Settings and Quit activate the app, which makes the system close the panel on its own and reset the status item with it. Mount state is also no longer painted onto controls. The backend icon and the mount/unmount buttons keep a fixed tint, and colour is reserved for the status dot, which is the one element whose only job is to report state. The detail view's Mount button loses its prominent style for the same reason.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MFuse/Views/SidebarView.swift (1)
99-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose
MountStatein the sidebar row.Add a localized accessibility value for each
mountcase, including.error. Do not usemount.statusTextalone because.mountedreturns the mount path instead of a state name. Add a non-color state cue if the dot remains the only visible indicator.🤖 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 `@MFuse/Views/SidebarView.swift` around lines 99 - 100, Update the sidebar row in SidebarView to expose a localized accessibility value derived from every MountState case, including .error, rather than relying solely on mount.statusText because .mounted represents the path. If the colored status dot remains the only visible state indicator, add a non-color cue as well.
🤖 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.
Outside diff comments:
In `@MFuse/Views/SidebarView.swift`:
- Around line 99-100: Update the sidebar row in SidebarView to expose a
localized accessibility value derived from every MountState case, including
.error, rather than relying solely on mount.statusText because .mounted
represents the path. If the colored status dot remains the only visible state
indicator, add a non-color cue as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57247ab1-db0e-4864-866c-2cc1834a3efa
📒 Files selected for processing (3)
MFuse/Views/ConnectionDetailView.swiftMFuse/Views/MenuBarView.swiftMFuse/Views/SidebarView.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: winnowl/review
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
MFuse/Views/MenuBarView.swift (1)
8-11: LGTM!Also applies to: 80-103, 126-131, 164-179, 196-202, 213-229, 244-244, 277-287, 328-328
MFuse/Views/SidebarView.swift (1)
85-89: LGTM!MFuse/Views/ConnectionDetailView.swift (1)
38-46: LGTM!Also applies to: 96-107, 117-117, 139-139
There was a problem hiding this comment.
Actionable comments posted: 0
✅ No blocking issues found — approving.
🛠️ To have the bot fix these findings, comment @winnowl fix.
📋 Additional findings from this change (not shown inline) (8)
- 🟡 Medium The async resolver does not revalidate the connection configuration or mounted state after awaiting the mount provider, so actor reentrancy can cause Finder to open a stale connection URL after a concurrent rename, update, or disconnect.
- 🟡 Medium Detail-load repair only upgrades state when
mountURLreturns a URL and never repairs a stale mounted state to unmounted/error, so extension-side disconnect/removal or lookup failure can leave the header showing mounted-only actions indefinitely. Concrete scenario: MFuse has cached.mounted, the File Provider domain is disconnected or removed outside the app, and the user opens the detail;mountURLreturns nil (missing domain) or throws, repair merely returns/logs, andmount.isMountedremains true, exposing Open in Finder, Unmount, and Refresh while hiding Mount. Evidence:repairMountStatecontains only a.mountedstate assignment; its nil and catch paths make no shared-state update, and the detail UI gates all three actions solely oneffectiveMountState.isMounted. This would be false ifmountURLwere guaranteed to return a valid URL for every cached mounted state and extension-side state could never change independently, butMountProviderexplicitly supports registered/disconnected domains andsyncMountsalready handles missing/disconnected domains as unmounted. - 🟡 Medium The enabled Edit action during
.mountingcan save a new configuration while the existingconnectattempt continues with the old configuration, allowing the stale attempt to win and mount the old endpoint after the row already displays the new one. - 🔵 Low There are no UI tests covering the sidebar state/action and removal behavior required by this scope, including mounting-disabled actions, mounted reveal/unmount, error remount, mixed batch states, selected-item removal, or the failure alert/dismissal path.
- 🟡 Medium Missing-bucket and empty-bucket configuration failures are classified as transient
.connectionFailederrors.RemoteFileSystemError.isTransientConnectionFailurereturns true for every.connectionFailed, so File Provider's bootstrap loop retries these deterministic configuration errors; after retry it maps them toNSFileProviderError.serverUnreachable, misrepresenting an invalid/nonexistent bucket as a network outage. The bucket cannot become valid between those immediate attempts without replacing the immutable config. - 🟠 High Copy and move cannot handle valid S3 objects larger than 5 GB because every file is copied with one CopyObject request. AWS S3 limits a single CopyObject operation to 5 GB; larger objects require multipart upload with UploadPartCopy. Thus copying a 6 GB file (or a directory containing one) fails even though S3 supports the object, and move cannot fulfill normal object semantics. Evidence: the file and recursive-directory paths both funnel every key through exactly one
s3.copyObject(request)with no size check or multipart fallback. This limitation was introduced with the S3 copy implementation and violates S3 request-limit compatibility. It would be disproven if supported services/Soto transparently transform CopyObject into multipart copy, but CopyObject is a direct modeled S3 operation and does not do so. - 🟡 Medium The required S3 semantic regression coverage was not added: the package declares an
MFuseS3Teststarget, but the repository has no MFuseS3 test sources and none of the changed files are tests. Therefore pagination, same-prefix siblings, empty directories, range edge cases, collision/412 behavior, special-character copy sources, recursive operations, and partial move failures are all unverified, including the destructive cases identified in this review. Evidence: repository search findsMFuseS3Testsonly in Package.swift and the changed-file list contains no test file. This is directly related to the change and violates the explicit test-alignment obligation. It would be disproven by test sources outside the reviewed repository or generated/injected tests that exercise these paths, but no such suite is represented here. - 🟡 Medium A newly created S3 connection with a custom endpoint that omits an explicit port can connect to the wrong port. The editor hides the port field for S3 but still builds
ConnectionConfig.portfromBackendType.defaultPort(443);S3FileSystemnow consumesconfig.s3Endpoint, whose compatibility resolver appends that stored port whenever it differs from the endpoint scheme default. Thus enteringhttp://localhostin a new config becomeshttp://localhost:443, rather than using HTTP's port 80. The compatibility behavior intended for legacy endpoint-plus-port records is being applied to new records because the producer does not distinguish or normalize them.
♻️ Previously reported (still present) (16)
- 🟡 Medium When provider URL lookup is unavailable, the resolver can reveal an unrelated filesystem location through a stale or user-created symlink at the connection's shortcut path, because link reachability is the only validation performed.
- 🟡 Medium The detail-load repair can overwrite a newer disconnect/connect result because it performs an untracked provider request and unconditionally writes
.mountedafterward. Concrete scenario: the detail appears with a mounted state andrepairMountStatesuspends inmountURL; the user clicks Unmount,disconnectadvances the connection generation and finishes by setting.unmounted; then the earliermountURLrequest returns a URL and repair sets the connection back to.mounted, re-exposing Finder, refresh, and eject controls even though the domain was disconnected. Evidence: the new.task(id: config.id)starts repair, whilerepairMountStatehas no cancellation check, generation snapshot, operation identity, or state validation around its awaits, unlikeconnect; its final write is unconditional. This would be false if the mount provider guaranteed every request begun before disconnect is cancelled/returns nil and cannot complete after disconnect, but that guarantee is absent fromMountProviderand from the File Provider implementation. - 🟡 Medium The newly added Refresh control silently discards enumerator failures and leaves the mounted presentation unchanged, so a user-requested refresh can fail with no shared error state or feedback and the UI misleadingly appears to have completed the action. Concrete scenario: a domain was removed or the File Provider extension is unavailable while cached mount state is
.mounted; clicking Refresh makessignalEnumeratorthrowdomainNotFound/managerNotFound,try?suppresses it, and all mounted-only controls remain visible. Evidence: the button action is exactlytry? await connectionManager.mountProvider?.signalEnumerator(for: config), whereassyncMountsconverts the same signal failure into bothConnectionState.errorandMountState.errorand sets extension-setup state when applicable. This would be false if refresh failures were contractually harmless and followed by an independent state reconciliation/visible feedback, but the provider API is throwing and this action schedules neither. - 🟡 Medium Unmount and refresh remain enabled throughout their asynchronous operations and
disconnecthas no per-connection in-flight guard, permitting repeated clicks to launch conflicting provider calls and allowing a later duplicate failure to replace a successful result. Concrete scenario: double-click Unmount while the first call is suspended inremoveSymlink/providerdisconnect; both invocations run because state remains.mounted; one provider disconnect succeeds and sets.unmounted, while the duplicate throws because the domain is already disconnected, then sets.error, presenting a failure despite the requested unmount having succeeded. Evidence: unlikeconnect,disconnectdoes not insert/check an in-flight ID, and it does not set an intermediate state before its first await; the detail button creates a fresh unstructured task for every click and is only replaced afterisMountedchanges. This would be false if all MountProvider disconnect/remove operations were guaranteed idempotent and nonthrowing under concurrent duplicate calls, but the protocol declares both throwing and the manager already handles their failures. - 🔵 Low The scope adds four MountState-dependent detail branches and a navigation-keyed repair task without any corresponding UI/state tests, leaving the required branch visibility and task-rerun behavior unverified. In particular, no test would fail if the error Section were accidentally hidden, mounted refresh/reveal controls appeared while unmounted, or
.task(id:)stopped rerunning on selection changes. Evidence: repository search finds no ConnectionDetailView test or instantiation outside production ContentView, and none of the changed files adds tests; existing ConnectionManager tests exercise core mount state but not detail control visibility, error-section rendering, or navigation task identity. This would be false if such coverage exists under a non-textual/generated test harness not present in the reviewed repository. - 🟡 Medium Unmount actions are not made mutually exclusive, so two rapid row/"Unmount All" actions can run
ConnectionManager.disconnectconcurrently for the same connection and turn a successful unmount into an error state. - 🟡 Medium Removal failures surface raw enum/debug descriptions and an opaque UUID instead of the localized error description and user-visible mount name, so the destructive failure alert is not useful localized context.
- 🟠 High Actor reentrancy allows overlapping lifecycle calls to leak a client or resurrect a connection after disconnect.
connect()keeps its newly createdAWSClientandS3only in locals while awaiting the probe. A secondconnect()therefore also starts a client; if both probes succeed, the later actor continuation overwritesawsClient/s3without shutting down the first client. Likewise, adisconnect()that runs while the probe is suspended sees no stored client and returns, after which the original connect installs its service, violating disconnect ordering. This is also reachable from the File Provider timeout/cancellation path because cancelling the waiter does not prevent another lifecycle call from entering the actor while the SDK await is suspended. - 🟡 Medium Bootstrap fallback errors expose raw SDK/network diagnostics to user-visible and public-log surfaces. For every unmapped failure,
mapConnectionErrorembedsString(describing: error)in.connectionFailed; endpoint/DNS/TLS errors commonly include the configured endpoint hostname, URL, peer address, or request context. File Provider then puts this string inNSLocalizedDescriptionKey, and both bootstrap retry paths log descriptions with public privacy, contrary to the requirement that sensitive endpoint or signed-request details not escape. A safe mapper should classify by typed error/code and emit a bounded diagnostic rather than forwarding the complete SDK description. - 🔵 Low The scope's lifecycle and error-classification behavior has no executable coverage: the MFuseS3 test target still contains only an empty placeholder. Thus regressions in typed error mapping, successful connection installation, failed-probe shutdown, overlapping/cancelled connect, partial-state recovery, and disconnect shutdown are not deterministically checked, despite these being the central invariants of the change.
- 🔴 Critical Moving a directory into one of its own descendants can report success while deleting both the original and the newly copied destination. For example, with source
/acontaininga/fileand absent destination/a/new, the destination check passes;copyItemcopiesa/filetoa/new/file, thendelete(at: /a)recursively deletes every key undera/, includinga/new/file. Evidence:moveunconditionally callscopyItemfollowed bydelete, and directory copy/delete use source prefixa/;RemotePathalready exposesisDescendant(of:)but no such guard is made. This is introduced/exposed by the new recursive S3 move implementation and violates data integrity. It would be disproven if an upstream caller or S3 service invariant rejects descendant destinations before this method, but the publicRemoteFileSystem.moveAPI and this implementation show no such enforcement. - 🔴 Critical Deleting or moving a file can delete an unrelated virtual directory that has the same lexical basename. S3 permits both object
fooand objects underfoo/.itemInfo(/foo)identifies the exact object as a file, butdelete(/foo)first listsfoo/; if any child exists it deletes those children and never deletes objectfoo. Consequently a file move copiesfoo, then removes the unrelatedfoo/...subtree, leaves the source file in place, and returns success. Evidence:deletechooses directory solely from whether the slash-terminated prefix has contents, whilemovepreviously classified the source but does not pass that classification to deletion. This change introduced/exposed destructive alias handling. It would be disproven only if the system guarantees that exact file objects and same-name virtual-directory prefixes can never coexist, which S3 itself does not guarantee and the collision/enumeration code explicitly checks both forms. - 🟠 High A paginated enumerate/copy/delete operation can continue issuing requests through an S3 service whose AWS client was shut down by a reentrant
disconnect(). Each operation captureslet s3 = try requireS3()and then suspends at network awaits; actors are reentrant, sodisconnectcan clear state and immediately shut down the same client before the original method resumes and starts its next page/object request. Evidence:disconnecthas no in-flight-operation coordination or connection generation check, while enumeration and recursive loops retain and repeatedly use the captureds3. The result is mid-operation client-shutdown errors and violates the stated disconnect concurrency invariant. It would be disproven if Soto'sS3owned an independent client not affected byAWSClient.shutdown, but it is constructed from and uses that client. - 🟠 High Recursive deletion silently treats per-object failures as success because S3 DeleteObjects commonly returns HTTP 200 with individual failures in the output
errorscollection; the code discards the response and setsdeletedDirectoryObjects = true. A permission/retention failure for one key can therefore makedeletereturn normally with keys remaining; during move this can partially remove the source and still report success. Evidence:_ = try await s3.deleteObjects(deleteReq)ignores the operation output and only transport-level errors are observed. This was introduced by the batch-delete implementation. It would be disproven if the Soto version converts every nonempty DeleteObjectserrorsresult into a thrown error, rather than returning the modeled response, but generated S3 APIs normally return the response for this HTTP-200 partial-failure contract. - 🟡 Medium
itemInfo(.root)does not model the bucket/mounted root as a directory. With the default empty remote root,s3Keyproduces an empty file key anditemInfofirst issues HeadObject forkey: ""; Soto/AWS can reject the required key as malformed (or the HTTP path aliases the bucket HEAD), and that error is not a not-found object error that reaches the directory-list fallback. With a configured root, an object whose key exactly equals the configured base is likewise returned as a file even though the mounted root is a directory. Evidence: there is nopath.isRootbranch before the file-first HEAD, whiles3Keyexplicitly returns an empty string for an empty root. The changed path-to-key/item-info implementation introduced this root classification failure. It would be disproven if all callers categorically avoid requesting root metadata and every supported SDK/provider reports empty-key HEAD as object 404, neither of which is guaranteed by the public API or S3 request model. - 🟡 Medium Finder URL resolution and mount-state repair can race unmounting because they independently recreate the convenience symlink without participating in ConnectionManager's connection generation/interruption checks. For example, an Open in Finder task can obtain a mount URL, then
disconnectremoves the old symlink, after whichresolveFinderURLcreates a new one while the domain is being removed; the disconnect path will not clean up that newly created link. Likewise, the detail view's repair task can set.mountedafter a concurrent unmount. With controls available in multiple windows, this can leave a stale symlink or restore a mounted UI state after unmount.
🤖 Prompt for AI agents — all findings (24)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Additional findings on this change (not posted inline) (8)
In MFuse/Services/ConnectionManager+Finder.swift around line 19, address this finding:
The async resolver does not revalidate the connection configuration or mounted state after awaiting the mount provider, so actor reentrancy can cause Finder to open a stale connection URL after a concurrent rename, update, or disconnect.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 643, address this finding:
Detail-load repair only upgrades state when `mountURL` returns a URL and never repairs a stale mounted state to unmounted/error, so extension-side disconnect/removal or lookup failure can leave the header showing mounted-only actions indefinitely. Concrete scenario: MFuse has cached `.mounted`, the File Provider domain is disconnected or removed outside the app, and the user opens the detail; `mountURL` returns nil (missing domain) or throws, repair merely returns/logs, and `mount.isMounted` remains true, exposing Open in Finder, Unmount, and Refresh while hiding Mount. Evidence: `repairMountState` contains only a `.mounted` state assignment; its nil and catch paths make no shared-state update, and the detail UI gates all three actions solely on `effectiveMountState.isMounted`. This would be false if `mountURL` were guaranteed to return a valid URL for every cached mounted state and extension-side state could never change independently, but `MountProvider` explicitly supports registered/disconnected domains and `syncMounts` already handles missing/disconnected domains as unmounted.
In MFuse/Views/SidebarView.swift around line 136, address this finding:
The enabled Edit action during `.mounting` can save a new configuration while the existing `connect` attempt continues with the old configuration, allowing the stale attempt to win and mount the old endpoint after the row already displays the new one.
In MFuse/Views/SidebarView.swift around line 6, address this finding:
There are no UI tests covering the sidebar state/action and removal behavior required by this scope, including mounting-disabled actions, mounted reveal/unmount, error remount, mixed batch states, selected-item removal, or the failure alert/dismissal path.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 132, address this finding:
Missing-bucket and empty-bucket configuration failures are classified as transient `.connectionFailed` errors. `RemoteFileSystemError.isTransientConnectionFailure` returns true for every `.connectionFailed`, so File Provider's bootstrap loop retries these deterministic configuration errors; after retry it maps them to `NSFileProviderError.serverUnreachable`, misrepresenting an invalid/nonexistent bucket as a network outage. The bucket cannot become valid between those immediate attempts without replacing the immutable config.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 446, address this finding:
Copy and move cannot handle valid S3 objects larger than 5 GB because every file is copied with one CopyObject request. AWS S3 limits a single CopyObject operation to 5 GB; larger objects require multipart upload with UploadPartCopy. Thus copying a 6 GB file (or a directory containing one) fails even though S3 supports the object, and move cannot fulfill normal object semantics. Evidence: the file and recursive-directory paths both funnel every key through exactly one `s3.copyObject(request)` with no size check or multipart fallback. This limitation was introduced with the S3 copy implementation and violates S3 request-limit compatibility. It would be disproven if supported services/Soto transparently transform CopyObject into multipart copy, but CopyObject is a direct modeled S3 operation and does not do so.
In Packages/MFuseS3/Package.swift around line 22, address this finding:
The required S3 semantic regression coverage was not added: the package declares an `MFuseS3Tests` target, but the repository has no MFuseS3 test sources and none of the changed files are tests. Therefore pagination, same-prefix siblings, empty directories, range edge cases, collision/412 behavior, special-character copy sources, recursive operations, and partial move failures are all unverified, including the destructive cases identified in this review. Evidence: repository search finds `MFuseS3Tests` only in Package.swift and the changed-file list contains no test file. This is directly related to the change and violates the explicit test-alignment obligation. It would be disproven by test sources outside the reviewed repository or generated/injected tests that exercise these paths, but no such suite is represented here.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 28, address this finding:
A newly created S3 connection with a custom endpoint that omits an explicit port can connect to the wrong port. The editor hides the port field for S3 but still builds `ConnectionConfig.port` from `BackendType.defaultPort` (443); `S3FileSystem` now consumes `config.s3Endpoint`, whose compatibility resolver appends that stored port whenever it differs from the endpoint scheme default. Thus entering `http://localhost` in a new config becomes `http://localhost:443`, rather than using HTTP's port 80. The compatibility behavior intended for legacy endpoint-plus-port records is being applied to new records because the producer does not distinguish or normalize them.
## Previously reported and still present (16)
In MFuse/Services/ConnectionManager+Finder.swift around line 31, address this finding:
When provider URL lookup is unavailable, the resolver can reveal an unrelated filesystem location through a stale or user-created symlink at the connection's shortcut path, because link reachability is the only validation performed.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 636, address this finding:
The detail-load repair can overwrite a newer disconnect/connect result because it performs an untracked provider request and unconditionally writes `.mounted` afterward. Concrete scenario: the detail appears with a mounted state and `repairMountState` suspends in `mountURL`; the user clicks Unmount, `disconnect` advances the connection generation and finishes by setting `.unmounted`; then the earlier `mountURL` request returns a URL and repair sets the connection back to `.mounted`, re-exposing Finder, refresh, and eject controls even though the domain was disconnected. Evidence: the new `.task(id: config.id)` starts repair, while `repairMountState` has no cancellation check, generation snapshot, operation identity, or state validation around its awaits, unlike `connect`; its final write is unconditional. This would be false if the mount provider guaranteed every request begun before disconnect is cancelled/returns nil and cannot complete after disconnect, but that guarantee is absent from `MountProvider` and from the File Provider implementation.
In MFuse/Views/ConnectionDetailView.swift around line 127, address this finding:
The newly added Refresh control silently discards enumerator failures and leaves the mounted presentation unchanged, so a user-requested refresh can fail with no shared error state or feedback and the UI misleadingly appears to have completed the action. Concrete scenario: a domain was removed or the File Provider extension is unavailable while cached mount state is `.mounted`; clicking Refresh makes `signalEnumerator` throw `domainNotFound`/`managerNotFound`, `try?` suppresses it, and all mounted-only controls remain visible. Evidence: the button action is exactly `try? await connectionManager.mountProvider?.signalEnumerator(for: config)`, whereas `syncMounts` converts the same signal failure into both `ConnectionState.error` and `MountState.error` and sets extension-setup state when applicable. This would be false if refresh failures were contractually harmless and followed by an independent state reconciliation/visible feedback, but the provider API is throwing and this action schedules neither.
In MFuse/Views/ConnectionDetailView.swift around line 97, address this finding:
Unmount and refresh remain enabled throughout their asynchronous operations and `disconnect` has no per-connection in-flight guard, permitting repeated clicks to launch conflicting provider calls and allowing a later duplicate failure to replace a successful result. Concrete scenario: double-click Unmount while the first call is suspended in `removeSymlink`/provider `disconnect`; both invocations run because state remains `.mounted`; one provider disconnect succeeds and sets `.unmounted`, while the duplicate throws because the domain is already disconnected, then sets `.error`, presenting a failure despite the requested unmount having succeeded. Evidence: unlike `connect`, `disconnect` does not insert/check an in-flight ID, and it does not set an intermediate state before its first await; the detail button creates a fresh unstructured task for every click and is only replaced after `isMounted` changes. This would be false if all MountProvider disconnect/remove operations were guaranteed idempotent and nonthrowing under concurrent duplicate calls, but the protocol declares both throwing and the manager already handles their failures.
In MFuse/Views/ConnectionDetailView.swift around line 40, address this finding:
The scope adds four MountState-dependent detail branches and a navigation-keyed repair task without any corresponding UI/state tests, leaving the required branch visibility and task-rerun behavior unverified. In particular, no test would fail if the error Section were accidentally hidden, mounted refresh/reveal controls appeared while unmounted, or `.task(id:)` stopped rerunning on selection changes. Evidence: repository search finds no ConnectionDetailView test or instantiation outside production ContentView, and none of the changed files adds tests; existing ConnectionManager tests exercise core mount state but not detail control visibility, error-section rendering, or navigation task identity. This would be false if such coverage exists under a non-textual/generated test harness not present in the reviewed repository.
In MFuse/Views/MenuBarView.swift, address this finding:
Unmount actions are not made mutually exclusive, so two rapid row/"Unmount All" actions can run `ConnectionManager.disconnect` concurrently for the same connection and turn a successful unmount into an error state.
In MFuse/Views/SidebarView.swift around line 152, address this finding:
Removal failures surface raw enum/debug descriptions and an opaque UUID instead of the localized error description and user-visible mount name, so the destructive failure alert is not useful localized context.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 85, address this finding:
Actor reentrancy allows overlapping lifecycle calls to leak a client or resurrect a connection after disconnect. `connect()` keeps its newly created `AWSClient` and `S3` only in locals while awaiting the probe. A second `connect()` therefore also starts a client; if both probes succeed, the later actor continuation overwrites `awsClient`/`s3` without shutting down the first client. Likewise, a `disconnect()` that runs while the probe is suspended sees no stored client and returns, after which the original connect installs its service, violating disconnect ordering. This is also reachable from the File Provider timeout/cancellation path because cancelling the waiter does not prevent another lifecycle call from entering the actor while the SDK await is suspended.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 134, address this finding:
Bootstrap fallback errors expose raw SDK/network diagnostics to user-visible and public-log surfaces. For every unmapped failure, `mapConnectionError` embeds `String(describing: error)` in `.connectionFailed`; endpoint/DNS/TLS errors commonly include the configured endpoint hostname, URL, peer address, or request context. File Provider then puts this string in `NSLocalizedDescriptionKey`, and both bootstrap retry paths log descriptions with public privacy, contrary to the requirement that sensitive endpoint or signed-request details not escape. A safe mapper should classify by typed error/code and emit a bounded diagnostic rather than forwarding the complete SDK description.
In Packages/MFuseS3/Tests/MFuseS3Tests/S3FileSystemTests.swift around line 5, address this finding:
The scope's lifecycle and error-classification behavior has no executable coverage: the MFuseS3 test target still contains only an empty placeholder. Thus regressions in typed error mapping, successful connection installation, failed-probe shutdown, overlapping/cancelled connect, partial-state recovery, and disconnect shutdown are not deterministically checked, despite these being the central invariants of the change.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 362, address this finding:
Moving a directory into one of its own descendants can report success while deleting both the original and the newly copied destination. For example, with source `/a` containing `a/file` and absent destination `/a/new`, the destination check passes; `copyItem` copies `a/file` to `a/new/file`, then `delete(at: /a)` recursively deletes every key under `a/`, including `a/new/file`. Evidence: `move` unconditionally calls `copyItem` followed by `delete`, and directory copy/delete use source prefix `a/`; `RemotePath` already exposes `isDescendant(of:)` but no such guard is made. This is introduced/exposed by the new recursive S3 move implementation and violates data integrity. It would be disproven if an upstream caller or S3 service invariant rejects descendant destinations before this method, but the public `RemoteFileSystem.move` API and this implementation show no such enforcement.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 350, address this finding:
Deleting or moving a file can delete an unrelated virtual directory that has the same lexical basename. S3 permits both object `foo` and objects under `foo/`. `itemInfo(/foo)` identifies the exact object as a file, but `delete(/foo)` first lists `foo/`; if any child exists it deletes those children and never deletes object `foo`. Consequently a file move copies `foo`, then removes the unrelated `foo/...` subtree, leaves the source file in place, and returns success. Evidence: `delete` chooses directory solely from whether the slash-terminated prefix has contents, while `move` previously classified the source but does not pass that classification to deletion. This change introduced/exposed destructive alias handling. It would be disproven only if the system guarantees that exact file objects and same-name virtual-directory prefixes can never coexist, which S3 itself does not guarantee and the collision/enumeration code explicitly checks both forms.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift, address this finding:
A paginated enumerate/copy/delete operation can continue issuing requests through an S3 service whose AWS client was shut down by a reentrant `disconnect()`. Each operation captures `let s3 = try requireS3()` and then suspends at network awaits; actors are reentrant, so `disconnect` can clear state and immediately shut down the same client before the original method resumes and starts its next page/object request. Evidence: `disconnect` has no in-flight-operation coordination or connection generation check, while enumeration and recursive loops retain and repeatedly use the captured `s3`. The result is mid-operation client-shutdown errors and violates the stated disconnect concurrency invariant. It would be disproven if Soto's `S3` owned an independent client not affected by `AWSClient.shutdown`, but it is constructed from and uses that client.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 343, address this finding:
Recursive deletion silently treats per-object failures as success because S3 DeleteObjects commonly returns HTTP 200 with individual failures in the output `errors` collection; the code discards the response and sets `deletedDirectoryObjects = true`. A permission/retention failure for one key can therefore make `delete` return normally with keys remaining; during move this can partially remove the source and still report success. Evidence: `_ = try await s3.deleteObjects(deleteReq)` ignores the operation output and only transport-level errors are observed. This was introduced by the batch-delete implementation. It would be disproven if the Soto version converts every nonempty DeleteObjects `errors` result into a thrown error, rather than returning the modeled response, but generated S3 APIs normally return the response for this HTTP-200 partial-failure contract.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 208, address this finding:
`itemInfo(.root)` does not model the bucket/mounted root as a directory. With the default empty remote root, `s3Key` produces an empty file key and `itemInfo` first issues HeadObject for `key: ""`; Soto/AWS can reject the required key as malformed (or the HTTP path aliases the bucket HEAD), and that error is not a not-found object error that reaches the directory-list fallback. With a configured root, an object whose key exactly equals the configured base is likewise returned as a file even though the mounted root is a directory. Evidence: there is no `path.isRoot` branch before the file-first HEAD, while `s3Key` explicitly returns an empty string for an empty root. The changed path-to-key/item-info implementation introduced this root classification failure. It would be disproven if all callers categorically avoid requesting root metadata and every supported SDK/provider reports empty-key HEAD as object 404, neither of which is guaranteed by the public API or S3 request model.
In MFuse/Services/ConnectionManager+Finder.swift around line 24, address this finding:
Finder URL resolution and mount-state repair can race unmounting because they independently recreate the convenience symlink without participating in ConnectionManager's connection generation/interruption checks. For example, an Open in Finder task can obtain a mount URL, then `disconnect` removes the old symlink, after which `resolveFinderURL` creates a new one while the domain is being removed; the disconnect path will not clean up that newly created link. Likewise, the detail view's repair task can set `.mounted` after a concurrent unmount. With controls available in multiple windows, this can leave a stale symlink or restore a mounted UI state after unmount.
📜 Review details
Model
- gpt-5.6-sol
Coverage
- scopes: 5/7 complete
Brand icons turned out not to be usable. AWS requires prior written approval for its service icons and Microsoft licenses its logos separately, so S3 and OneDrive are out; Google Drive allows its logo but forbids recolouring, which conflicts with the single-tint rows. That left brand marks for two of nine backends and symbols for the rest. Rows now read "SFTP · example.com" under the connection name. The type is spelled out, so nothing has to be abbreviated and the longest name costs only trailing truncation instead of widening a column for every row. The leading icon is gone, which also removes the SF Symbol collisions — s3 and oneDrive both drew `cloud`, and googleDrive drew the iCloud glyph. The composed string lives on ConnectionConfig rather than in each view, and drops the type when the address has already fallen back to it. OneDrive is now just "OneDrive" across all nine locales.
…r move `addressesSameServer` decides whether a synchronized edit may keep a mount up, so a false negative takes a working mount down and withholds the remount. An anonymous login sends no username: FTP sends a fixed `USER anonymous` and WebDAV omits the Authorization header, and the editor saves the field empty. A legacy config carrying a stale username therefore compared as a different server than its normalized form. An S3 endpoint port that only repeats the scheme's default is not part of the address either, so `https://host:443` and `https://host` are one server.
There was a problem hiding this comment.
Actionable comments posted: 1
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
🔎 Confirmed findings (1)
- 🟡 Medium S3 region is passed to Soto verbatim rather than normalized:
ConnectionConfig.s3Regionreturnsparameters["region"]unchanged, and the editor persistss3Regionunchanged. Thus a user enteringus-east-1(or a synced legacy config containing surrounding whitespace) createsRegion(rawValue: "us-east-1 "), which signs/routes with an invalid region even though bucket and endpoint are trimmed. This is disproven if all configuration writers and migrations guarantee a nonempty region is already whitespace-free. (inline)
⛔ Unresolved from previous review (5) — not approved until fixed
- MFuseProvider/FileProviderExtension.swift: Changing the iCloud-sync setting migrates credentials to the other Keychain sync mode, but the already-running File Provider extension continues using the
SharedCredentialStoreselected only ininit.AppSettingsStorehas no path to callswapCredentialStore, and reconnecting an existing domain need not construct a new extension instance. After enabling or disabling sync, Finder can therefore look in the old mode and report authentication failure until the extension is recreated/restarted. Refresh the provider's store fromSharedAppSettingsbefore credential lookup (or signal/recreate active domains as part of the setting transition). - MFuse/Services/DomainManager.swift: Startup reconciliation can overwrite a newer edited connection's File Provider bootstrap snapshot with the stale configuration it captured before the edit.
DomainManager.reconcileDomainsAndSymlinks()iterates a snapshot ofconnectionManager.connectionsand invokesmountProvider.ensureRegistered(config:)directly, without a current-revision/generation fence or routing throughConnectionManager.syncSavedConnectionRegistration. Since app startup is launched in an asynchronous view task, a user can save an edit aftersyncDomains()has loaded domain states but before its loop reaches that connection; the edit registers and persists the new config, then the stale startup iteration registers/persists the old config last. The row/storage show the new target while the extension later bootstraps the old target. This is disproven only if reconciliation is guaranteed to complete before all edits (it is not—the initial setup runs asynchronously after the window is constructed) orensureRegisteredindependently rejects obsolete configs (it accepts any supplied config). - MFuse/Views/ContentView.swift: When File Provider re-registration fails after a local edit, the edited row can remain published as cleanly mounted even though the active domain still serves the previous config. —
performSavepersists and publishesconfigbefore callingsyncSavedConnectionRegistration; if that call throws, its catch only assignssaveAlertand then returnsconfig. It does not roll back the edited connection or set its mount/connection state to an error, whilesyncSavedConnectionRegistrationcan throw directly frommountProvider.ensureRegistered(config:)before it disconnects or changes the existing mounted state. Thus a failed re-registration can still leave the edited row shown as mounted while the pre-edit domain remains active. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. —
syncSavedConnectionRegistrationawaitsmountProvider.ensureRegistered(config:)without registering that operation as shutdown-tracked work; shutdown can setisShuttingDownand take its teardown snapshot while that call is suspended, after which the provider can create the domain. The subsequentisRegistrableConnectioncheck only unregisters reactively and is not awaited by shutdown, so it does not prevent the post-snapshot registration. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. —syncSavedConnectionRegistrationnow detects incomplete cleanup and throws, butapplyStorageSnapshothas already assignedconnections = nextConnections; its catch only publishes an error forconfigand never restorespreviousConfig. Thus a disconnect failure still leaves the externally edited configuration visible while the old filesystem/domain runtime state remains.
📋 Additional findings from this change (not shown inline) (7)
- 🟡 Medium The editor accepts whitespace-only hosts/buckets and port 0 as valid, then serializes an unusable target and can overwrite an existing usable credential with the cleared credential state. (MFuse/Views/ConnectionEditorSheet.swift) — anchor-outside-diff
- 🟡 Medium
reloadConnectionsFromStorage()performs destructive cleanup for an externally removed row before checking whether its storage snapshot is still current. A local save that lands whileapplyStorageSnapshot()is suspended can therefore be persisted successfully but have its File Provider domain unregistered; the retry sees the now-current config unchanged and never re-registers it. (Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift) — anchor-outside-diff - 🟡 Medium
addressesSameServertreatspathStyleas a target change even when no custom S3 endpoint exists, although that parameter is not consumed on the AWS path. (Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionConfig.swift) — anchor-unreliable - 🟡 Medium Quit can complete manager shutdown while launch-time domain reconciliation is still creating or retaining a mounted File Provider domain.
performInitialSetupIfNeeded()runsdomainManager.syncDomains()in an independent.task, butapplicationShouldTerminatewaits only formanager.shutdown().DomainManager.reconcileDomainsAndSymlinks()has no shutdown fence and awaitsensureRegistered; if quit starts while that call is suspended (beforesyncMountshas populated manager mount state), shutdown snapshots the row as inactive and performs no disconnect. Reconciliation can then resume before the termination reply, register the domain, and—when its pre-read domain state was connected—skip its disconnect, leaving an active mount outside the shutdown pass. This is disproven if launch setup is cancelled/joined before the termination reply, or DomainManager is guaranteed never to resume registration after shutdown begins; neither is established by these paths. (MFuse/MFuseApp.swift) — anchor-unreliable - 🟡 Medium Credential mode changes report success and switch the active stores even when deleting the source credential fails, leaving the credential in both local and synchronizable Keychain partitions (and potentially leaving a cloud-synchronized copy after sync is disabled). (Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift) — anchor-outside-diff
- 🟡 Medium
mountURL(for:)bypasses the per-domain coordinator, so it can resolve a CloudStorage URL concurrently withensureRegistered, which is the operation that renames/moves that URL. A caller can therefore receive the pre-rename path after registration has completed (e.g. startmountURLwhilegetUserVisibleURLis suspended, then register the same domain with a new display name); unlike create/remove, this lookup has no FIFO exclusion or post-resolution validation. The claim would be false only if File Provider guarantees a domain's user-visible URL never changes duringNSFileProviderManager.addupdates, including display-name renames. (Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift) — anchor-outside-diff - 🟡 Medium Credential sync-mode transitions complete and let the setting be persisted even if cleanup of the source-mode credentials fails, leaving duplicate credentials in the old mode. (Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift) — anchor-outside-diff
♻️ Previously reported (still present) (11)
- 🟠 High Saving an edit replaces the mirrored credential before updating or disconnecting the currently mounted domain. For an edit that changes host/backend/auth target, the File Provider domain still has the previous bootstrap config during the awaited credential write, but subsequent extension work reads the new shared-keychain item by the unchanged connection UUID. This creates a window in which the newly entered secret can be sent to the old server—the exact producer/consumer mismatch the editor’s target-bound credential handling is intended to prevent. Serialize a target-changing save by first taking the existing domain down (or otherwise prevent its credential reads), then commit the new credential/config and register/remount the new domain; preserve rollback behavior. (MFuse/Views/ContentView.swift) — previously-reported
- 🟡 Medium Legacy shortcut cleanup deletes an empty directory solely because its name looks like an MFuse symlink, even though it has not been identified as an MFuse-managed link. For example, an empty user/foreign directory named
Backup-<UUID>at the legacy shortcut path is removed whenever this connection creates or removes its shortcut. This violates the ownership invariant that cleanup must leave user files and foreign non-links untouched; the claim would be false only if that legacy location is guaranteed to contain no user- or other-process-created directories. (Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift) — previously-reported - 🟡 Medium Legacy access-group migration and cleanup in SharedCredentialStore only search/delete the store's currently selected synchronization partition, so credentials in the opposite legacy partition are neither migrated nor removed. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — previously-reported
- 🟡 Medium SharedCredentialStore reports store/delete/file-migration success even when removal of the legacy cleartext credential file fails, leaving a sensitive plaintext copy behind without notifying its caller. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — previously-reported
- 🟡 Medium A normal write to SharedCredentialStore suppresses failures while deleting legacy access-group credentials, so it can report a successful replacement although an old sensitive Keychain copy remains. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — previously-reported
- 🟡 Medium An existing legacy credential file that cannot be read is silently treated as absent, so the migration reports no credential rather than surfacing the I/O failure and the sensitive cleartext file remains. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — previously-reported
- 🟡 Medium
disconnect()does not actually fence a replacement connection until the cancelled attempt has released its AWS client: itsawait inFlight?.resultyields the actor afterconnectTaskhas been cleared, so a concurrentconnect()can start and construct a new client while the old attempt is still unwinding/shutting down. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — previously-reported - 🟡 Medium Sidebar’s Unmount All can leave mounts started by a just-prior Mount All running. It builds
configsToUnmountfrom the current effective-state snapshot and excludes rows still reported.unmounted; Mount All launches eachconnectin an unstructured task-group child, so an Unmount All tap can take its snapshot before one of those children has registered its connect task. That row is excluded, then the delayed child connects it after Unmount All returns. MenuBarView explicitly avoids this race by passing every current config todisconnect, but SidebarView does not. This is disproven only ifwithTaskGroupis guaranteed to start and register every child’sconnectbefore a later main-actor UI action can evaluate the filter, which Swift task scheduling does not guarantee. (MFuse/Views/SidebarView.swift) — previously-reported - 🟡 Medium The localized cleartext-transport warning is materially mistranslated in every non-English catalog: it says that credentials are stored in plaintext in a shared configuration file, rather than that they will be transmitted in plaintext and that enabling TLS prevents that. Consequently an es/fr/id/it/ja/ko/zh user enabling an FTP-without-TLS or HTTP WebDAV mount is not told about the network exposure or the TLS remediation presented to English users. (MFuse/Localizable.xcstrings) — previously-reported
- 🔵 Low The detail view still exposes legacy usernames for backends/authentication modes that never consume them. (MFuse/Views/ConnectionDetailView.swift) — previously-reported
- 🔵 Low Five OAuth editor strings used by the Dropbox/OneDrive account flow have no entries in Localizable.xcstrings, so users of every non-English locale receive English fallbacks for account connection, reauthentication, account-connected status, and the unsupported-provider error instead of the localized UI promised by this catalog update. (MFuse/Views/ConnectionEditorSheet.swift) — previously-reported
🤖 Prompt for AI agents — all findings (24)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (5)
In MFuseProvider/FileProviderExtension.swift, address this finding:
Changing the iCloud-sync setting migrates credentials to the other Keychain sync mode, but the already-running File Provider extension continues using the `SharedCredentialStore` selected only in `init`. `AppSettingsStore` has no path to call `swapCredentialStore`, and reconnecting an existing domain need not construct a new extension instance. After enabling or disabling sync, Finder can therefore look in the old mode and report authentication failure until the extension is recreated/restarted. Refresh the provider's store from `SharedAppSettings` before credential lookup (or signal/recreate active domains as part of the setting transition).
In MFuse/Services/DomainManager.swift, address this finding:
Startup reconciliation can overwrite a newer edited connection's File Provider bootstrap snapshot with the stale configuration it captured before the edit. `DomainManager.reconcileDomainsAndSymlinks()` iterates a snapshot of `connectionManager.connections` and invokes `mountProvider.ensureRegistered(config:)` directly, without a current-revision/generation fence or routing through `ConnectionManager.syncSavedConnectionRegistration`. Since app startup is launched in an asynchronous view task, a user can save an edit after `syncDomains()` has loaded domain states but before its loop reaches that connection; the edit registers and persists the new config, then the stale startup iteration registers/persists the old config last. The row/storage show the new target while the extension later bootstraps the old target. This is disproven only if reconciliation is guaranteed to complete before all edits (it is not—the initial setup runs asynchronously after the window is constructed) or `ensureRegistered` independently rejects obsolete configs (it accepts any supplied config).
In MFuse/Views/ContentView.swift, address this finding:
When File Provider re-registration fails after a local edit, the edited row can remain published as cleanly mounted even though the active domain still serves the previous config.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Findings on this change (also posted as inline comments) (1)
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionConfig.swift around line 118, address this finding:
S3 region is passed to Soto verbatim rather than normalized: `ConnectionConfig.s3Region` returns `parameters["region"]` unchanged, and the editor persists `s3Region` unchanged. Thus a user entering `us-east-1 ` (or a synced legacy config containing surrounding whitespace) creates `Region(rawValue: "us-east-1 ")`, which signs/routes with an invalid region even though bucket and endpoint are trimmed. This is disproven if all configuration writers and migrations guarantee a nonempty region is already whitespace-free.
## Additional findings on this change (not posted inline) (7)
In MFuse/Views/ConnectionEditorSheet.swift around line 512, address this finding:
The editor accepts whitespace-only hosts/buckets and port 0 as valid, then serializes an unusable target and can overwrite an existing usable credential with the cleared credential state.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 1163, address this finding:
`reloadConnectionsFromStorage()` performs destructive cleanup for an externally removed row before checking whether its storage snapshot is still current. A local save that lands while `applyStorageSnapshot()` is suspended can therefore be persisted successfully but have its File Provider domain unregistered; the retry sees the now-current config unchanged and never re-registers it.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionConfig.swift, address this finding:
`addressesSameServer` treats `pathStyle` as a target change even when no custom S3 endpoint exists, although that parameter is not consumed on the AWS path.
In MFuse/MFuseApp.swift, address this finding:
Quit can complete manager shutdown while launch-time domain reconciliation is still creating or retaining a mounted File Provider domain. `performInitialSetupIfNeeded()` runs `domainManager.syncDomains()` in an independent `.task`, but `applicationShouldTerminate` waits only for `manager.shutdown()`. `DomainManager.reconcileDomainsAndSymlinks()` has no shutdown fence and awaits `ensureRegistered`; if quit starts while that call is suspended (before `syncMounts` has populated manager mount state), shutdown snapshots the row as inactive and performs no disconnect. Reconciliation can then resume before the termination reply, register the domain, and—when its pre-read domain state was connected—skip its disconnect, leaving an active mount outside the shutdown pass. This is disproven if launch setup is cancelled/joined before the termination reply, or DomainManager is guaranteed never to resume registration after shutdown begins; neither is established by these paths.
In Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift around line 250, address this finding:
Credential mode changes report success and switch the active stores even when deleting the source credential fails, leaving the credential in both local and synchronizable Keychain partitions (and potentially leaving a cloud-synchronized copy after sync is disabled).
In Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift around line 289, address this finding:
`mountURL(for:)` bypasses the per-domain coordinator, so it can resolve a CloudStorage URL concurrently with `ensureRegistered`, which is the operation that renames/moves that URL. A caller can therefore receive the pre-rename path after registration has completed (e.g. start `mountURL` while `getUserVisibleURL` is suspended, then register the same domain with a new display name); unlike create/remove, this lookup has no FIFO exclusion or post-resolution validation. The claim would be false only if File Provider guarantees a domain's user-visible URL never changes during `NSFileProviderManager.add` updates, including display-name renames.
In Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift around line 245, address this finding:
Credential sync-mode transitions complete and let the setting be persisted even if cleanup of the source-mode credentials fails, leaving duplicate credentials in the old mode.
## Previously reported and still present (11)
In MFuse/Views/ContentView.swift around line 190, address this finding:
Saving an edit replaces the mirrored credential before updating or disconnecting the currently mounted domain. For an edit that changes host/backend/auth target, the File Provider domain still has the previous bootstrap config during the awaited credential write, but subsequent extension work reads the new shared-keychain item by the unchanged connection UUID. This creates a window in which the newly entered secret can be sent to the old server—the exact producer/consumer mismatch the editor’s target-bound credential handling is intended to prevent. Serialize a target-changing save by first taking the existing domain down (or otherwise prevent its credential reads), then commit the new credential/config and register/remount the new domain; preserve rollback behavior.
In Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift around line 551, address this finding:
Legacy shortcut cleanup deletes an empty directory solely because its name looks like an MFuse symlink, even though it has not been identified as an MFuse-managed link. For example, an empty user/foreign directory named `Backup-<UUID>` at the legacy shortcut path is removed whenever this connection creates or removes its shortcut. This violates the ownership invariant that cleanup must leave user files and foreign non-links untouched; the claim would be false only if that legacy location is guaranteed to contain no user- or other-process-created directories.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 174, address this finding:
Legacy access-group migration and cleanup in SharedCredentialStore only search/delete the store's currently selected synchronization partition, so credentials in the opposite legacy partition are neither migrated nor removed.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 120, address this finding:
SharedCredentialStore reports store/delete/file-migration success even when removal of the legacy cleartext credential file fails, leaving a sensitive plaintext copy behind without notifying its caller.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 200, address this finding:
A normal write to SharedCredentialStore suppresses failures while deleting legacy access-group credentials, so it can report a successful replacement although an old sensitive Keychain copy remains.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 102, address this finding:
An existing legacy credential file that cannot be read is silently treated as absent, so the migration reports no credential rather than surfacing the I/O failure and the sensitive cleartext file remains.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 440, address this finding:
`disconnect()` does not actually fence a replacement connection until the cancelled attempt has released its AWS client: its `await inFlight?.result` yields the actor after `connectTask` has been cleared, so a concurrent `connect()` can start and construct a new client while the old attempt is still unwinding/shutting down.
In MFuse/Views/SidebarView.swift around line 65, address this finding:
Sidebar’s Unmount All can leave mounts started by a just-prior Mount All running. It builds `configsToUnmount` from the current effective-state snapshot and excludes rows still reported `.unmounted`; Mount All launches each `connect` in an unstructured task-group child, so an Unmount All tap can take its snapshot before one of those children has registered its connect task. That row is excluded, then the delayed child connects it after Unmount All returns. MenuBarView explicitly avoids this race by passing every current config to `disconnect`, but SidebarView does not. This is disproven only if `withTaskGroup` is guaranteed to start and register every child’s `connect` before a later main-actor UI action can evaluate the filter, which Swift task scheduling does not guarantee.
In MFuse/Localizable.xcstrings around line 4669, address this finding:
The localized cleartext-transport warning is materially mistranslated in every non-English catalog: it says that credentials are stored in plaintext in a shared configuration file, rather than that they will be transmitted in plaintext and that enabling TLS prevents that. Consequently an es/fr/id/it/ja/ko/zh user enabling an FTP-without-TLS or HTTP WebDAV mount is not told about the network exposure or the TLS remediation presented to English users.
In MFuse/Views/ConnectionDetailView.swift around line 32, address this finding:
The detail view still exposes legacy usernames for backends/authentication modes that never consume them.
In MFuse/Views/ConnectionEditorSheet.swift around line 375, address this finding:
Five OAuth editor strings used by the Dropbox/OneDrive account flow have no entries in Localizable.xcstrings, so users of every non-English locale receive English fallbacks for account connection, reauthentication, account-connected status, and the unsupported-provider error instead of the localized UI promised by this catalog update.
📜 Review details
Model
- gpt-5.6-terra
Coverage
- scopes: 8/8 complete
| /// editor stores as "nothing". | ||
| public var s3Region: String { | ||
| parameters["region"] ?? Self.defaultS3Region | ||
| } |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionConfig.swift, address this finding:
S3 region is passed to Soto verbatim rather than normalized: `ConnectionConfig.s3Region` returns `parameters["region"]` unchanged, and the editor persists `s3Region` unchanged. Thus a user entering `us-east-1 ` (or a synced legacy config containing surrounding whitespace) creates `Region(rawValue: "us-east-1 ")`, which signs/routes with an invalid region even though bucket and endpoint are trimmed. This is disproven if all configuration writers and migrations guarantee a nonempty region is already whitespace-free.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| } | |
| public var s3Region: String { | |
| Self.trimmedParameter(parameters["region"]) ?? Self.defaultS3Region | |
| } |
The editor stores the Region field as typed and an emptied one as "", and `s3Region` handed that straight to `Region(rawValue:)`, so "us-east-1 " or "" signed and routed requests for a region that does not exist — while the bucket and the endpoint beside it were already trimmed. Blank now reads as absent, which is the default region the editor means by an untouched field.
There was a problem hiding this comment.
Actionable comments posted: 2
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
🔎 Confirmed findings (2)
- 🟡 Medium A cancelled, not-yet-started connect task can tear down a client published by a replacement attempt.
performConnect()unconditionally treats anyawsClientas its own stale client, with no check that its task is still the registered attempt. For example, create connect A, letdisconnect()cancel and clear A before A's task body receives the actor, then start B and let B publish; when delayed A entersperformConnect, it shuts down and clears B's client before observing its own cancellation. B subsequently fails its identity guard despite having successfully probed, violating replacement fencing and disconnect/connect correctness. This would be false only if task creation were guaranteed to run A's actor-isolated body before the actor can processdisconnect()and B, which Swift task scheduling does not guarantee. (inline) - 🟡 Medium The fallback error classifier can misclassify an unreachable custom endpoint as an authentication failure because it scans the entire unstructured SDK description for generic auth words. With a self-hosted endpoint such as
https://unauthorized.internal.examplethat fails at DNS/TCP, the transport error can contain that endpoint (the code comments already acknowledge endpoint diagnostics occur in descriptions); since it carries noAWSErrorTypecode/status,normalized.contains("unauthorized")returns.authenticationFailedrather than the safe reachability category. This violates the required distinction between authentication and endpoint failures. The claim would be false if every no-code transport error description is guaranteed never to include endpoint text, contrary to the stated rationale for not exposing raw descriptions. (inline)
⛔ Unresolved from previous review (6) — not approved until fixed
- MFuse/Views/ContentView.swift: Saving an edit that changes the connection target writes the new credential to the shared File Provider credential store before the old target's domain is disconnected or its bootstrap config is replaced, so the extension can authenticate the old target with the new target's secret.
- MFuseProvider/FileProviderExtension.swift: Changing the iCloud-sync setting migrates credentials to the other Keychain sync mode, but the already-running File Provider extension continues using the
SharedCredentialStoreselected only ininit.AppSettingsStorehas no path to callswapCredentialStore, and reconnecting an existing domain need not construct a new extension instance. After enabling or disabling sync, Finder can therefore look in the old mode and report authentication failure until the extension is recreated/restarted. Refresh the provider's store fromSharedAppSettingsbefore credential lookup (or signal/recreate active domains as part of the setting transition). - MFuse/Services/DomainManager.swift: Startup reconciliation can overwrite a newer edited connection's File Provider bootstrap snapshot with the stale configuration it captured before the edit.
DomainManager.reconcileDomainsAndSymlinks()iterates a snapshot ofconnectionManager.connectionsand invokesmountProvider.ensureRegistered(config:)directly, without a current-revision/generation fence or routing throughConnectionManager.syncSavedConnectionRegistration. Since app startup is launched in an asynchronous view task, a user can save an edit aftersyncDomains()has loaded domain states but before its loop reaches that connection; the edit registers and persists the new config, then the stale startup iteration registers/persists the old config last. The row/storage show the new target while the extension later bootstraps the old target. This is disproven only if reconciliation is guaranteed to complete before all edits (it is not—the initial setup runs asynchronously after the window is constructed) orensureRegisteredindependently rejects obsolete configs (it accepts any supplied config). - MFuse/Views/ContentView.swift: When File Provider re-registration fails after a local edit, the edited row can remain published as cleanly mounted even though the active domain still serves the previous config. —
performSavecommitsconnectionManager.update(config, expecting: openedConfig)before callingsyncSavedConnectionRegistration; if that call throws, the catch at current lines 227–241 only assignssaveAlertand then returnsconfig. It does not publish an error/unmounted state or roll back the edited connection, so an already-mounted row can still appear clean while its domain retains the prior registration. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. —
syncSavedConnectionRegistrationchecksisRegistrableConnectionbeforeawait mountProvider.ensureRegistered, but that registration is not tracked or cancelled byshutdown(). If shutdown setsisShuttingDownand takes its teardown snapshot whileensureRegisteredis suspended, the provider can still finish registering the domain afterward; the subsequent guard only attempts a best-efforttry? await mountProvider.unregister, which shutdown neither awaits nor includes in its snapshot. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. —applyStorageSnapshot()still assignsconnections = nextConnectionsbefore invokingsyncSavedConnectionRegistration; although that method now throwscleanupFailedafterisCleanupComplete, the reload catch only publishes an error againstconfigand never restorespreviousConfig, so a failed disconnect can still leave the edited config visible while the old runtime state remains.
📋 Additional findings from this change (not shown inline) (2)
- 🟡 Medium SharedCredentialStore reports successful store/delete operations even when cleanup of the legacy cleartext credential file fails, leaving a usable secret on disk without surfacing the failure. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — anchor-unreliable
- 🟡 Medium The FTP editor still persists a Passive Mode selection even though the FTP backend never reads
parameters["passive"], so selecting active mode has no effect and the saved configuration does not describe the connection behavior. (MFuse/Views/ConnectionEditorSheet.swift) — anchor-outside-diff
♻️ Previously reported (still present) (5)
- 🟡 Medium
reloadConnectionsFromStorage()can unregister a connection that a concurrent local save has retained, then treat the stale reload as discarded without restoring or re-registering its domain. The revision fence is applied only after destructive cleanup has already run. (Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift) — previously-reported - 🟡 Medium Legacy access-group migration only probes the store's active synchronization partition, so a credential saved by an older build in the other partition is reported absent and never migrated. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — previously-reported
- 🟡 Medium A successful store can silently leave a credential in a legacy Keychain access group because legacy cleanup discards deletion failures. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — previously-reported
- 🟡 Medium Legacy cleartext credential-file deletion failures are logged but suppressed, so migration/store/delete can report success while the plaintext source remains in the App Group container. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — previously-reported
- 🔵 Low OAuth configuration failures are not localized because the two runtime lookup keys have no entry in any packaged MFuseCore
Localizable.stringscatalog. (Packages/MFuseCore/Sources/MFuseCore/Connection/OAuthClientConfiguration.swift) — previously-reported
🤖 Prompt for AI agents — all findings (15)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (6)
In MFuse/Views/ContentView.swift, address this finding:
Saving an edit that changes the connection target writes the new credential to the shared File Provider credential store before the old target's domain is disconnected or its bootstrap config is replaced, so the extension can authenticate the old target with the new target's secret.
In MFuseProvider/FileProviderExtension.swift, address this finding:
Changing the iCloud-sync setting migrates credentials to the other Keychain sync mode, but the already-running File Provider extension continues using the `SharedCredentialStore` selected only in `init`. `AppSettingsStore` has no path to call `swapCredentialStore`, and reconnecting an existing domain need not construct a new extension instance. After enabling or disabling sync, Finder can therefore look in the old mode and report authentication failure until the extension is recreated/restarted. Refresh the provider's store from `SharedAppSettings` before credential lookup (or signal/recreate active domains as part of the setting transition).
In MFuse/Services/DomainManager.swift, address this finding:
Startup reconciliation can overwrite a newer edited connection's File Provider bootstrap snapshot with the stale configuration it captured before the edit. `DomainManager.reconcileDomainsAndSymlinks()` iterates a snapshot of `connectionManager.connections` and invokes `mountProvider.ensureRegistered(config:)` directly, without a current-revision/generation fence or routing through `ConnectionManager.syncSavedConnectionRegistration`. Since app startup is launched in an asynchronous view task, a user can save an edit after `syncDomains()` has loaded domain states but before its loop reaches that connection; the edit registers and persists the new config, then the stale startup iteration registers/persists the old config last. The row/storage show the new target while the extension later bootstraps the old target. This is disproven only if reconciliation is guaranteed to complete before all edits (it is not—the initial setup runs asynchronously after the window is constructed) or `ensureRegistered` independently rejects obsolete configs (it accepts any supplied config).
In MFuse/Views/ContentView.swift, address this finding:
When File Provider re-registration fails after a local edit, the edited row can remain published as cleanly mounted even though the active domain still serves the previous config.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Findings on this change (also posted as inline comments) (2)
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 229, address this finding:
A cancelled, not-yet-started connect task can tear down a client published by a replacement attempt. `performConnect()` unconditionally treats any `awsClient` as its own stale client, with no check that its task is still the registered attempt. For example, create connect A, let `disconnect()` cancel and clear A before A's task body receives the actor, then start B and let B publish; when delayed A enters `performConnect`, it shuts down and clears B's client before observing its own cancellation. B subsequently fails its identity guard despite having successfully probed, violating replacement fencing and disconnect/connect correctness. This would be false only if task creation were guaranteed to run A's actor-isolated body before the actor can process `disconnect()` and B, which Swift task scheduling does not guarantee.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 349, address this finding:
The fallback error classifier can misclassify an unreachable custom endpoint as an authentication failure because it scans the entire unstructured SDK description for generic auth words. With a self-hosted endpoint such as `https://unauthorized.internal.example` that fails at DNS/TCP, the transport error can contain that endpoint (the code comments already acknowledge endpoint diagnostics occur in descriptions); since it carries no `AWSErrorType` code/status, `normalized.contains("unauthorized")` returns `.authenticationFailed` rather than the safe reachability category. This violates the required distinction between authentication and endpoint failures. The claim would be false if every no-code transport error description is guaranteed never to include endpoint text, contrary to the stated rationale for not exposing raw descriptions.
## Additional findings on this change (not posted inline) (2)
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift, address this finding:
SharedCredentialStore reports successful store/delete operations even when cleanup of the legacy cleartext credential file fails, leaving a usable secret on disk without surfacing the failure.
In MFuse/Views/ConnectionEditorSheet.swift around line 984, address this finding:
The FTP editor still persists a Passive Mode selection even though the FTP backend never reads `parameters["passive"]`, so selecting active mode has no effect and the saved configuration does not describe the connection behavior.
## Previously reported and still present (5)
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 1163, address this finding:
`reloadConnectionsFromStorage()` can unregister a connection that a concurrent local save has retained, then treat the stale reload as discarded without restoring or re-registering its domain. The revision fence is applied only after destructive cleanup has already run.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 176, address this finding:
Legacy access-group migration only probes the store's active synchronization partition, so a credential saved by an older build in the other partition is reported absent and never migrated.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 205, address this finding:
A successful store can silently leave a credential in a legacy Keychain access group because legacy cleanup discards deletion failures.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 120, address this finding:
Legacy cleartext credential-file deletion failures are logged but suppressed, so migration/store/delete can report success while the plaintext source remains in the App Group container.
In Packages/MFuseCore/Sources/MFuseCore/Connection/OAuthClientConfiguration.swift around line 42, address this finding:
OAuth configuration failures are not localized because the two runtime lookup keys have no entry in any packaged MFuseCore `Localizable.strings` catalog.
📜 Review details
Model
- gpt-5.6-terra
Coverage
- scopes: 7/7 complete
| // A previous attempt can leave a client behind — the File Provider extension | ||
| // times out and retries `connect()`, and Soto asserts in `AWSClient.deinit` if a | ||
| // client is released without being shut down. | ||
| if let stale = awsClient { |
There was a problem hiding this comment.
🔀 Concurrency | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift, address this finding:
A cancelled, not-yet-started connect task can tear down a client published by a replacement attempt. `performConnect()` unconditionally treats any `awsClient` as its own stale client, with no check that its task is still the registered attempt. For example, create connect A, let `disconnect()` cancel and clear A before A's task body receives the actor, then start B and let B publish; when delayed A enters `performConnect`, it shuts down and clears B's client before observing its own cancellation. B subsequently fails its identity guard despite having successfully probed, violating replacement fencing and disconnect/connect correctness. This would be false only if task creation were guaranteed to run A's actor-isolated body before the actor can process `disconnect()` and B, which Swift task scheduling does not guarantee.
| "unauthorized" | ||
| ] | ||
| if authenticationIndicators.contains(where: { normalized.contains($0) }) { | ||
| if Self.authenticationIndicators.contains(where: { normalized.contains($0) }) { |
There was a problem hiding this comment.
🔍 Error Handling | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift, address this finding:
The fallback error classifier can misclassify an unreachable custom endpoint as an authentication failure because it scans the entire unstructured SDK description for generic auth words. With a self-hosted endpoint such as `https://unauthorized.internal.example` that fails at DNS/TCP, the transport error can contain that endpoint (the code comments already acknowledge endpoint diagnostics occur in descriptions); since it carries no `AWSErrorType` code/status, `normalized.contains("unauthorized")` returns `.authenticationFailed` rather than the safe reachability category. This violates the required distinction between authentication and endpoint failures. The claim would be false if every no-code transport error description is guaranteed never to include endpoint text, contrary to the stated rationale for not exposing raw descriptions.
…ss as a verdict Creating the task does not run its body, so an attempt can be cancelled and replaced before it reaches its first line — and it then shut down the client the replacement had published, as if it were its own leftover. `disconnect()` cancels the attempt it de-registers in the turn that clears it, and only a cleared attempt can be replaced, so checking cancellation before touching published state is what tells the two apart. The fallback classifier also scanned the whole SDK description for words like "unauthorized". A transport failure carries no error code and names the host it could not reach, so an endpoint called `unauthorized.internal.example` was reported as bad credentials. The configured endpoint, its host and the bucket are taken out of the description before the scan.
There was a problem hiding this comment.
Actionable comments posted: 0
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
⛔ Unresolved from previous review (6) — not approved until fixed
- MFuse/Views/ContentView.swift: Saving an edit that changes the connection target writes the new credential to the shared File Provider credential store before the old target's domain is disconnected or its bootstrap config is replaced, so the extension can authenticate the old target with the new target's secret.
- MFuseProvider/FileProviderExtension.swift: Changing the iCloud-sync setting migrates credentials to the other Keychain sync mode, but the already-running File Provider extension continues using the
SharedCredentialStoreselected only ininit.AppSettingsStorehas no path to callswapCredentialStore, and reconnecting an existing domain need not construct a new extension instance. After enabling or disabling sync, Finder can therefore look in the old mode and report authentication failure until the extension is recreated/restarted. Refresh the provider's store fromSharedAppSettingsbefore credential lookup (or signal/recreate active domains as part of the setting transition). - MFuse/Services/DomainManager.swift: Startup reconciliation can overwrite a newer edited connection's File Provider bootstrap snapshot with the stale configuration it captured before the edit.
DomainManager.reconcileDomainsAndSymlinks()iterates a snapshot ofconnectionManager.connectionsand invokesmountProvider.ensureRegistered(config:)directly, without a current-revision/generation fence or routing throughConnectionManager.syncSavedConnectionRegistration. Since app startup is launched in an asynchronous view task, a user can save an edit aftersyncDomains()has loaded domain states but before its loop reaches that connection; the edit registers and persists the new config, then the stale startup iteration registers/persists the old config last. The row/storage show the new target while the extension later bootstraps the old target. This is disproven only if reconciliation is guaranteed to complete before all edits (it is not—the initial setup runs asynchronously after the window is constructed) orensureRegisteredindependently rejects obsolete configs (it accepts any supplied config). - MFuse/Views/ContentView.swift: When File Provider re-registration fails after a local edit, the edited row can remain published as cleanly mounted even though the active domain still serves the previous config. — The local-edit path still persists the new config before calling
syncSavedConnectionRegistration. IfensureRegistered(or another early registration step) throws while the old domain is active,syncSavedConnectionRegistrationexits without changing the mount state, and this catch only assigns an alert; it does not mark the row errored or unmount/otherwise reconcile it. Consequently the sidebar can continue to show the edited row as green/mounted while the active File Provider domain still serves the prior registration. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. — The lifecycle gate is checked before the suspending registration call, but it does not cover that call atomically.
syncSavedConnectionRegistrationcan passisRegistrableConnectionwhile shutdown has not started, then suspend inmountProvider.ensureRegistered; shutdown can setisShuttingDownand snapshot teardown IDs during that suspension, and the registration can complete afterward. The later guard only unregisters the domain after it has already been registered, so the reported post-snapshot registration consequence remains possible. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
📋 Additional findings from this change (not shown inline) (1)
- 🟠 High An existing File Provider domain can be lost when the add-refresh retry fails. (Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift) — anchor-outside-diff
♻️ Previously reported (still present) (1)
- 🟡 Medium Legacy cleanup deletes a user-created empty directory solely because its name matches MFuse's filename pattern. (Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift) — anchor-unreliable
❓ Low-evidence leads (not confirmed — verify before acting) (1)
- Non-TLS WebDAV configurations default to and continue using port 443, even though the filesystem switches the scheme to plain HTTP (whose default is port 80). A new WebDAV mount with TLS disabled and an empty port is built by
makeConfigwithBackendType.webdav.defaultPort(443), andWebDAVFileSystem.connect()then constructshttp://host:443; similarly toggling TLS off does not adjust an existing default port. This makes the editor's advertised default/actual endpoint incorrect and commonly connects to the wrong service. (Packages/MFuseCore/Sources/MFuseCore/Connection/BackendType.swift)
🤖 Prompt for AI agents — all findings (8)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (6)
In MFuse/Views/ContentView.swift, address this finding:
Saving an edit that changes the connection target writes the new credential to the shared File Provider credential store before the old target's domain is disconnected or its bootstrap config is replaced, so the extension can authenticate the old target with the new target's secret.
In MFuseProvider/FileProviderExtension.swift, address this finding:
Changing the iCloud-sync setting migrates credentials to the other Keychain sync mode, but the already-running File Provider extension continues using the `SharedCredentialStore` selected only in `init`. `AppSettingsStore` has no path to call `swapCredentialStore`, and reconnecting an existing domain need not construct a new extension instance. After enabling or disabling sync, Finder can therefore look in the old mode and report authentication failure until the extension is recreated/restarted. Refresh the provider's store from `SharedAppSettings` before credential lookup (or signal/recreate active domains as part of the setting transition).
In MFuse/Services/DomainManager.swift, address this finding:
Startup reconciliation can overwrite a newer edited connection's File Provider bootstrap snapshot with the stale configuration it captured before the edit. `DomainManager.reconcileDomainsAndSymlinks()` iterates a snapshot of `connectionManager.connections` and invokes `mountProvider.ensureRegistered(config:)` directly, without a current-revision/generation fence or routing through `ConnectionManager.syncSavedConnectionRegistration`. Since app startup is launched in an asynchronous view task, a user can save an edit after `syncDomains()` has loaded domain states but before its loop reaches that connection; the edit registers and persists the new config, then the stale startup iteration registers/persists the old config last. The row/storage show the new target while the extension later bootstraps the old target. This is disproven only if reconciliation is guaranteed to complete before all edits (it is not—the initial setup runs asynchronously after the window is constructed) or `ensureRegistered` independently rejects obsolete configs (it accepts any supplied config).
In MFuse/Views/ContentView.swift, address this finding:
When File Provider re-registration fails after a local edit, the edited row can remain published as cleanly mounted even though the active domain still serves the previous config.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Additional findings on this change (not posted inline) (1)
In Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift around line 162, address this finding:
An existing File Provider domain can be lost when the add-refresh retry fails.
## Previously reported and still present (1)
In Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift, address this finding:
Legacy cleanup deletes a user-created empty directory solely because its name matches MFuse's filename pattern.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- scopes: 5/7 complete
A save writes the new credential into the store the File Provider extension reads before it can replace the bootstrap config that extension reads it against, so the old server could be authenticated with a secret issued for the new one. A target change now takes the connection down first and puts the mount back once the switch is registered; a teardown that leaves runtime state behind fails the save with nothing written. Alongside it, four states that read as settled when they are not: - The extension chose its Keychain sync mode once per instance, so turning iCloud sync on or off left a running one looking where the items no longer are. It now reads the setting on every credential access. - Startup reconciliation registered the connection list it set out with, so a save landing mid-pass was overwritten by the pre-edit config. It re-reads each connection before registering it and re-checks afterwards. - A registration that failed for a mounted connection left the row reporting a clean mount while its domain still served the previous config. - The add-refresh retry removed the registered domain and, when the re-add failed or was cancelled, left none — while every caller reads that failure as "the registration is unchanged". What it removed goes back.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MFuse/Views/ContentView.swift (1)
147-162: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the selected config after a queued save commits.
If two saves use the same editor, the first save dismisses the editor and selects its config. The second save then commits, but the presentation check prevents it from updating
selectedConnection. The detail view can show the first config after the connection list contains the second config.Update selection when the selected connection has the same ID. Keep editor dismissal conditional on
presentationID.Proposed fix
await MainActor.run { - guard editorPresentation?.id == presentationID else { return } - selectedConnection = config - editorPresentation = nil + if selectedConnection?.id == config.id { + selectedConnection = config + } + if editorPresentation?.id == presentationID { + editorPresentation = nil + } }🤖 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 `@MFuse/Views/ContentView.swift` around lines 147 - 162, Update the selection update path in ContentView’s queued save flow so a commit can refresh selectedConnection even after the editor has already been dismissed. The issue is that the presentation check is blocking the second save from selecting its committed config when it has the same ID as the current selection; adjust the logic around performSave/selectedConnection so matching IDs still update the selection while keeping editor dismissal gated by presentationID.
🤖 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.
Outside diff comments:
In `@MFuse/Views/ContentView.swift`:
- Around line 147-162: Update the selection update path in ContentView’s queued
save flow so a commit can refresh selectedConnection even after the editor has
already been dismissed. The issue is that the presentation check is blocking the
second save from selecting its committed config when it has the same ID as the
current selection; adjust the logic around performSave/selectedConnection so
matching IDs still update the selection while keeping editor dismissal gated by
presentationID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bedeb28-c37a-443f-a941-728d1ce7eb7b
📒 Files selected for processing (6)
MFuse/Services/DomainManager.swiftMFuse/Views/ContentView.swiftMFuseProvider/FileProviderExtension.swiftPackages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swiftPackages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swiftPackages/MFuseCore/Tests/MFuseCoreTests/ConnectionManagerTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift
- Packages/MFuseCore/Tests/MFuseCoreTests/ConnectionManagerTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: winnowl/review
🔇 Additional comments (3)
MFuseProvider/FileProviderExtension.swift (1)
117-139: LGTM!MFuse/Services/DomainManager.swift (1)
13-15: LGTM!Also applies to: 91-99, 141-168, 251-253
Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift (1)
1627-1633: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSensitive Data Exposure (CWE-522): Insufficiently Protected Credentials
Reachability: External
Reachability path
● Entry Packages/MFuseCore/Sources/MFuseCore/Mount/FileProviderMountProvider.swift:152 performEnsureRegistered: The refresh took the registered domain out to put a fresh one in its │ ▼ ● Sink Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swiftKeep a target-change fence until the new target is registered.
prepareForTargetChange(_:)clears the teardown state before it returns.performSavethen stores the new credential before it updates the configuration. A concurrentconnect(_:)can use the oldConnectionConfigwith the new credential and authenticate the previous endpoint.
Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift#L1627-L1633: create a per-ID target-change lease that blocks or joins new connection and registration work until the save commits or rolls back.MFuse/Views/ContentView.swift#L190-L201: hold that lease across credential storage, configuration persistence, and registration. Release it withdeferon every success and failure path.
There was a problem hiding this comment.
Actionable comments posted: 3
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
🔎 Confirmed findings (3)
- 🟠 High A failed credential read does not prevent Save and can destroy the previously stored secret:
loadStoredCredentialIfNeededsets an error but leavesstoredCredentialnil, whilebuildCredentialreturns an empty/new credential andperformSaveunconditionally stores it. For example, an existing password mount whose Keychain read transiently fails can be edited only in its name and saved, replacing the working password with an empty credential. (inline) - 🟠 High Cancellation after the probe has published its AWSClient can leak that client: the starter's cancellation handler only calls relinquishConnectAttempt(), which merely cancels the already-completed task, and the success path intentionally skips Task.checkCancellation when there are no joiners. Thus a caller cancelled in the publication-to-resumption window can return success while awsClient remains live and no later disconnect is guaranteed. (inline)
- 🟡 Medium A cancelled starter can cancel a shared attempt before a concurrently arriving joiner has registered, causing the non-cancelled joiner to receive CancellationError instead of allowing the needed probe to continue. (inline)
⛔ Unresolved from previous review (2) — not approved until fixed
- Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. — The saved-registration operation is still not admitted into shutdown's tracked lifecycle work.
syncSavedConnectionRegistrationchecksisRegistrableConnectionbefore the suspendingensureRegisteredcall, butshutdown()can setisShuttingDownand takeidsNeedingTeardownafter that check while this call is suspended. The registration can then complete after the shutdown snapshot; the later guard only performs compensatingunregisterand therefore does not prevent the domain from being registered after the snapshot. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. — The cleanup failure is now detected and thrown, but the original defect remains:applyStorageSnapshot()assignsconnections = nextConnectionsbefore registering the edited config, and its catch only publishes an error.syncSavedConnectionRegistrationalso does not restorepreviousConfig; after the active-mount path'sdisconnect(config.id, using: previousConfig), a failed cleanup reachesguard isCleanupComplete ... else { throw ... }while the edited config remains visible. Thus a failed old-runtime cleanup can still leave the new config committed in the UI rather than rolling back to the previous config.
📋 Additional findings from this change (not shown inline) (2)
- 🟡 Medium Finder reveal falls back to a cached symlink or mount path after a failed domain lookup, so an externally removed/disconnected File Provider domain can still be opened as if mounted. In
resolveFinderURL,try? await mountProvider.mountURL(for:)collapses both a valid nil/not-found result and provider errors (includingdomainNotFound/manager failures) into the fallback branch;canRevealMountonly checks local mount state, teardown/removal flags, and config membership. If the domain is removed behind the app whilemountStates[id]remains.mountedand the old symlink target is still reachable (or cachedmountPathexists), the fallback returns it and Finder opens stale/disconnected storage. This would be false only if every provider lookup failure is guaranteed to coincide with a local teardown state before this code runs, which external File Provider removal does not guarantee. (MFuse/Services/ConnectionManager+Finder.swift) — anchor-outside-diff - 🟡 Medium
npm run checkis expected to fail on the browser project becausecheckJstype-checkssrc/main.js, but the Svelte component import has no ambient*.sveltemodule declaration. TypeScript therefore reports TS2307 for./App.sveltebefore the build can pass, so the documented check/build workflow is not runnable on a clean checkout. (website/src/main.js) — anchor-outside-diff
♻️ Previously reported (still present) (1)
- 🟡 Medium The editor's required-field validation accepts whitespace-only names, hosts, and S3 buckets, even though serialization trims/normalizes those values (and the S3 backend treats a whitespace bucket as empty). (MFuse/Views/ConnectionEditorSheet.swift) — previously-reported
❓ Low-evidence leads (not confirmed — verify before acting) (1)
- Directory deletion ignores per-object failures returned by S3's successful DeleteObjects response. A multi-object delete can return HTTP success with an errors array for individual keys; this implementation discards the response, then reports delete success even though some directory objects remain. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift)
🤖 Prompt for AI agents — all findings (8)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (2)
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Findings on this change (also posted as inline comments) (3)
In MFuse/Views/ConnectionEditorSheet.swift around line 724, address this finding:
A failed credential read does not prevent Save and can destroy the previously stored secret: `loadStoredCredentialIfNeeded` sets an error but leaves `storedCredential` nil, while `buildCredential` returns an empty/new credential and `performSave` unconditionally stores it. For example, an existing password mount whose Keychain read transiently fails can be edited only in its name and saved, replacing the working password with an empty credential.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 156, address this finding:
Cancellation after the probe has published its AWSClient can leak that client: the starter's cancellation handler only calls relinquishConnectAttempt(), which merely cancels the already-completed task, and the success path intentionally skips Task.checkCancellation when there are no joiners. Thus a caller cancelled in the publication-to-resumption window can return success while awsClient remains live and no later disconnect is guaranteed.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 170, address this finding:
A cancelled starter can cancel a shared attempt before a concurrently arriving joiner has registered, causing the non-cancelled joiner to receive CancellationError instead of allowing the needed probe to continue.
## Additional findings on this change (not posted inline) (2)
In MFuse/Services/ConnectionManager+Finder.swift around line 30, address this finding:
Finder reveal falls back to a cached symlink or mount path after a failed domain lookup, so an externally removed/disconnected File Provider domain can still be opened as if mounted. In `resolveFinderURL`, `try? await mountProvider.mountURL(for:)` collapses both a valid nil/not-found result and provider errors (including `domainNotFound`/manager failures) into the fallback branch; `canRevealMount` only checks local mount state, teardown/removal flags, and config membership. If the domain is removed behind the app while `mountStates[id]` remains `.mounted` and the old symlink target is still reachable (or cached `mountPath` exists), the fallback returns it and Finder opens stale/disconnected storage. This would be false only if every provider lookup failure is guaranteed to coincide with a local teardown state before this code runs, which external File Provider removal does not guarantee.
In website/src/main.js around line 3, address this finding:
`npm run check` is expected to fail on the browser project because `checkJs` type-checks `src/main.js`, but the Svelte component import has no ambient `*.svelte` module declaration. TypeScript therefore reports TS2307 for `./App.svelte` before the build can pass, so the documented check/build workflow is not runnable on a clean checkout.
## Previously reported and still present (1)
In MFuse/Views/ConnectionEditorSheet.swift around line 512, address this finding:
The editor's required-field validation accepts whitespace-only names, hosts, and S3 buckets, even though serialization trims/normalizes those values (and the S3 backend treats a whitespace bucket as empty).
📜 Review details
Model
- gpt-5.6-luna
Coverage
- scopes: 5/7 complete
| // Saving replaces the stored credential outright, so a load this sheet | ||
| // silently swallowed would let an empty field wipe a working secret. Say so | ||
| // instead — Save stays available, but the user knows what it will write. | ||
| testResult = AppL10n.string( |
There was a problem hiding this comment.
🎯 Correctness | 🟠 High
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In MFuse/Views/ConnectionEditorSheet.swift, address this finding:
A failed credential read does not prevent Save and can destroy the previously stored secret: `loadStoredCredentialIfNeeded` sets an error but leaves `storedCredential` nil, while `buildCredential` returns an empty/new credential and `performSave` unconditionally stores it. For example, an existing password mount whose Keychain read transiently fails can be edited only in its name and saved, replacing the working password with an empty credential.
| // one interested returns success instead, because throwing would have it drop | ||
| // the only reference to a live `AWSClient` without shutting it down. Its own | ||
| // cancellation handling then disconnects it properly. | ||
| let resumedJoiners = resumeConnectWaiters(of: task, with: .success(())) |
There was a problem hiding this comment.
🔍 Resource Lifetime | 🟠 High
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
⚠️ The exact frequency of this race depends on Swift actor scheduling, but the cancellation and publication ordering is permitted and the cleanup path is absent in the shown code.
🤖 Prompt for AI agents
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift, address this finding:
Cancellation after the probe has published its AWSClient can leak that client: the starter's cancellation handler only calls relinquishConnectAttempt(), which merely cancels the already-completed task, and the success path intentionally skips Task.checkCancellation when there are no joiners. Thus a caller cancelled in the publication-to-resumption window can return success while awsClient remains live and no later disconnect is guaranteed.
| /// joined it and were never cancelled; the starter stays on `task.value` either way, so | ||
| /// they are still resumed when it finishes. | ||
| private func relinquishConnectAttempt(_ task: Task<AWSClient, Error>) { | ||
| let hasJoiners = connectWaiters.values.contains { $0.task == task } |
There was a problem hiding this comment.
🔀 Concurrency | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift, address this finding:
A cancelled starter can cancel a shared attempt before a concurrently arriving joiner has registered, causing the non-cancelled joiner to receive CancellationError instead of allowing the needed probe to continue.
…ing read as settled An S3 caller cancelled between the attempt publishing its client and its own resumption was answered with success, which left a live AWSClient that only that caller could take down — and a cancelled caller is under no obligation to. With no joiner to take it, the connection now goes down with the caller. A joiner that registered just after the starter withdrew was answered with that withdrawal's cancellation; it never withdrew, so it starts an attempt of its own instead. The editor let a save go through after the stored credential could not be read, writing the empty fields over a working secret. Save is held until the fields carry a credential of their own. It also accepted whitespace-only names, hosts and buckets, which serialization reads as blank. Finder reveal treated "no domain registered" and "the lookup failed" as one answer, so a domain removed behind the app was still opened through a stale convenience link or cached path. A save queued behind another one commits after that one dismissed the sheet, and the detail pane kept showing the revision it replaced: selection now follows the committed row, while dismissal stays tied to the sheet that started the save.
There was a problem hiding this comment.
Actionable comments posted: 3
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
🔎 Confirmed findings (3)
- 🟠 High Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned. (inline)
- 🟠 High The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved. (inline)
- 🟡 Medium The editor permits a new password-authenticated connection to be saved or tested with an empty password. (inline)
⛔ Unresolved from previous review (2) — not approved until fixed
- Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. — The new
isRegistrableConnectionchecks narrow the window but do not close it. InsyncSavedConnectionRegistration, the final check before registration is onlyguard isCurrentRevision(config) else { return }; after that,mountProvider.ensureRegistered(config:)can suspend. Shutdown can setisShuttingDownand take its teardown snapshot during that suspension, then the in-flight save resumes and registers the domain. The laterisRegistrableConnectioncheck only unregisters after the registration has already occurred, so the reported post-snapshot registration consequence remains possible. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. — The reload path still publishesconnections = nextConnectionsbefore processing changed rows, andsyncSavedConnectionRegistrationdoes not restorepreviousConfigwhen cleanup fails. In the mounted same-server edit case (remountIfMounted: true), it even callsensureRegistered(config: config)before the laterdisconnect(... using: previousConfig)check; if that disconnect leaves a filesystem/domain behind,isCleanupCompletethrows, butapplyStorageSnapshotonly records an error and leaves the edited row visible. Thus the reported consequence—failed cleanup with the new config committed while old runtime state remains—can still occur.
⚠️ Unverified risks (1)
- Mode-transition cleanup errors are explicitly discarded and the provider switches to the target stores anyway. In
setSynchronizableEnabled, failures from both source deletions are only logged insidecatchblocks, after whichprimaryandsharedStoreare replaced. If source deletion fails, the old credential remains in the old partition while the new copy is active, so migration claims completion without surfacing cleanup failure and can leave orphaned duplicate secrets. (Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift)
📋 Additional findings from this change (not shown inline) (5)
- 🟠 High SharedCredentialStore does not constrain local-mode Keychain queries to non-synchronizable items.
baseQueryaddskSecAttrSynchronizable = trueonly for.synchronizableand adds nothing for.local, whereas the paired KeychainService explicitly sets the attribute to false for local mode. Consequently local-mode reads/updates/deletes are not guaranteed to address only the local partition (and can select a synchronizable item), undermining mode isolation and allowing writes/deletes to target the wrong partition. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — anchor-outside-diff - 🟡 Medium The editor accepts a malformed nonempty S3 custom endpoint and allows Save/Test to construct a config that the S3 backend cannot use as a URL endpoint. (MFuse/Views/ConnectionEditorSheet.swift) — anchor-outside-diff
- 🟡 Medium Startup mount synchronization silently discards stale-domain removal failures, so an orphan File Provider domain can remain registered while sync reports success and provides no retry/error signal. (Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift) — anchor-outside-diff
- 🟡 Medium Removal rollback restores
connectionsand persists the row without advancingconnectionsRevision, so an overlapping reload can pass its revision fence and publish the stale post-removal snapshot, losing the restored connection in memory despite storage containing it. (Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift) — per-file-budget - 🟡 Medium MirroredCredentialProvider.store is not atomic across the primary and shared providers: it writes the primary first and does not roll it back if
sharedStore.storethrows. A shared-provider failure leaves a newer credential in the app Keychain with no corresponding extension-readable mirror, and a later read can overwrite/repair the mirror while the caller has already received an error. This violates the required rollback/data-integrity semantics for failed saves. (Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift) — anchor-outside-diff
♻️ Previously reported (still present) (3)
- 🟠 High Legacy Keychain migration in SharedCredentialStore does not probe both sync partitions.
migrateLegacyKeychainDataIfNeededcalls the overload whose query uses this store'ssyncMode, so a legacy item in the other local/synchronizable partition is invisible and remains unmigrated; this directly violates the requirement that all supported legacy partitions be discovered and cleaned. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — anchor-unreliable - 🟠 High Legacy cleartext-file cleanup failures are swallowed, so a successful
storeor legacy-file migration can return while the secret file still exists.removeLegacyCredentialFileIfPresentonly logs its removal error and has no throwing result; bothstoreandmigrateLegacyCredentialIfNeededcall it after writing the Keychain item and then report success. A read-only/permission failure on the App Group file therefore leaves a cleartext credential behind and is not surfaced or retryable as an incomplete migration. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — previously-reported - 🟡 Medium Port 0 is accepted as a valid host-based port and is persisted, although all host-based implementations pass it directly to their clients. (MFuse/Views/ConnectionEditorSheet.swift) — previously-reported
🤖 Prompt for AI agents — all findings (13)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (2)
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Findings on this change (also posted as inline comments) (3)
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 940, address this finding:
Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 1018, address this finding:
The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved.
In MFuse/Views/ConnectionEditorSheet.swift around line 536, address this finding:
The editor permits a new password-authenticated connection to be saved or tested with an empty password.
## Additional findings on this change (not posted inline) (5)
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 339, address this finding:
SharedCredentialStore does not constrain local-mode Keychain queries to non-synchronizable items. `baseQuery` adds `kSecAttrSynchronizable = true` only for `.synchronizable` and adds nothing for `.local`, whereas the paired KeychainService explicitly sets the attribute to false for local mode. Consequently local-mode reads/updates/deletes are not guaranteed to address only the local partition (and can select a synchronizable item), undermining mode isolation and allowing writes/deletes to target the wrong partition.
In MFuse/Views/ConnectionEditorSheet.swift around line 532, address this finding:
The editor accepts a malformed nonempty S3 custom endpoint and allows Save/Test to construct a config that the S3 backend cannot use as a URL endpoint.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 1459, address this finding:
Startup mount synchronization silently discards stale-domain removal failures, so an orphan File Provider domain can remain registered while sync reports success and provides no retry/error signal.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 333, address this finding:
Removal rollback restores `connections` and persists the row without advancing `connectionsRevision`, so an overlapping reload can pass its revision fence and publish the stale post-removal snapshot, losing the restored connection in memory despite storage containing it.
In Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift around line 141, address this finding:
MirroredCredentialProvider.store is not atomic across the primary and shared providers: it writes the primary first and does not roll it back if `sharedStore.store` throws. A shared-provider failure leaves a newer credential in the app Keychain with no corresponding extension-readable mirror, and a later read can overwrite/repair the mirror while the caller has already received an error. This violates the required rollback/data-integrity semantics for failed saves.
## Previously reported and still present (3)
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift, address this finding:
Legacy Keychain migration in SharedCredentialStore does not probe both sync partitions. `migrateLegacyKeychainDataIfNeeded` calls the overload whose query uses this store's `syncMode`, so a legacy item in the other local/synchronizable partition is invisible and remains unmigrated; this directly violates the requirement that all supported legacy partitions be discovered and cleaned.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 120, address this finding:
Legacy cleartext-file cleanup failures are swallowed, so a successful `store` or legacy-file migration can return while the secret file still exists. `removeLegacyCredentialFileIfPresent` only logs its removal error and has no throwing result; both `store` and `migrateLegacyCredentialIfNeeded` call it after writing the Keychain item and then report success. A read-only/permission failure on the App Group file therefore leaves a cleartext credential behind and is not surfaced or retryable as an incomplete migration.
In MFuse/Views/ConnectionEditorSheet.swift around line 531, address this finding:
Port 0 is accepted as a valid host-based port and is persisted, although all host-based implementations pass it directly to their clients.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- scopes: 6/9 complete
| } | ||
|
|
||
| for config in connections where | ||
| let idsNeedingTeardown = connections.filter { config in |
There was a problem hiding this comment.
🔍 Resource Lifetime | 🟠 High
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned.
| } | ||
| Task { @MainActor in | ||
| try? await Task.sleep(nanoseconds: Self.shutdownDeadlineNanoseconds) | ||
| if resumer.resume() { |
There was a problem hiding this comment.
🔍 Resource Lifetime | 🟠 High
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved.
| return !s3Bucket.isEmpty && hasValidPort && hasRequiredAccessKeyCredentials | ||
| return !Self.isBlank(s3Bucket) && hasValidPort && hasRequiredAccessKeyCredentials | ||
| } | ||
| return !backendType.requiresServerEndpoint |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In MFuse/Views/ConnectionEditorSheet.swift, address this finding:
The editor permits a new password-authenticated connection to be saved or tested with an empty password.
…ey were passing over Quit cancelled and awaited connect, reconnect and mount work but not removals, so a removal went on writing the connection list, storage, the domain and the credential — and rolling all four back — in a process that was exiting. It is now awaited before the teardown snapshot is taken, under the same deadline as everything else. Saved-connection registration re-checks the same gate before it registers rather than only after. A removal published a shorter connection list, and its rollback published the restored one, without advancing the revision either time, so a reload holding an older snapshot still passed its fence and published over both. The legacy Keychain migration only ever probed the partition this store writes to, so an item written while iCloud sync was in the other state stayed invisible: never migrated, never cleaned up, and the mount stops authenticating. Both partitions are probed now, and the item is deleted from the one it was found in. A legacy cleartext file that cannot be deleted is emptied instead of left readable, and local-mode queries state their partition the way KeychainService does. Stale-domain removal during mount sync said nothing when it failed. The editor accepted an empty password, a malformed S3 endpoint and port 0, each of which is handed to a backend that cannot use it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift`:
- Around line 132-143: Replace the atomic Data().write fallback in the legacy
credential cleanup path with an in-place FileHandle truncation of the existing
file, preserving the current fault logging on failure. Add a regression test in
SharedStorageTests covering deletion failure while truncation remains permitted,
and verify the retained legacy credential file is emptied.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37f4e2f6-c55b-4177-8ee9-5c6fbf8c312f
📒 Files selected for processing (8)
MFuse/Services/ConnectionManager+Finder.swiftMFuse/Views/ConnectionEditorSheet.swiftMFuse/Views/ContentView.swiftPackages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swiftPackages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swiftPackages/MFuseCore/Tests/MFuseCoreTests/ConnectionManagerTests.swiftPackages/MFuseS3/Sources/MFuseS3/S3FileSystem.swiftPackages/MFuseS3/Tests/MFuseS3Tests/S3FileSystemTests.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- MFuse/Services/ConnectionManager+Finder.swift
- Packages/MFuseS3/Tests/MFuseS3Tests/S3FileSystemTests.swift
- Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift
- MFuse/Views/ContentView.swift
- MFuse/Views/ConnectionEditorSheet.swift
- Packages/MFuseCore/Tests/MFuseCoreTests/ConnectionManagerTests.swift
- Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: winnowl/review
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift (1)
14-22: LGTM!Also applies to: 35-35, 52-52, 186-222, 233-253, 320-327, 346-353, 364-383
There was a problem hiding this comment.
Actionable comments posted: 1
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
🔎 Confirmed findings (1)
- 🟡 Medium The editor treats semantically equivalent S3 address spellings as a target change and clears the loaded access keys: for an existing
https://s3.example.commount, changing the endpoint field tohttps://s3.example.com:443(or trimming bucket/region whitespace) changesServerIdentity.s3Endpoint/the raw fields even thoughConnectionConfigand the S3 backend resolve these to the same endpoint/bucket/region. The key fields are then emptied and Save can no longer preserve the credential without re-entry. (inline)
⛔ Unresolved from previous review (5) — not approved until fixed
- Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift: SharedCredentialStore reports successful store/delete operations even when cleanup of the legacy cleartext credential file fails, leaving a usable secret on disk without surfacing the failure. — The current
storeanddeletemethods still callremoveLegacyCredentialFileIfPresentwithout propagating an error, and that helper remains non-throwing. IfremoveItemfails and the fallbackData().write(..., .atomic)also fails, the helper only logs a fault and returns, so the operation still reports success while the original cleartext file (and usable secret) can remain on disk. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved. — The defect remains:
withShutdownDeadlineresumes the shutdown waiter when the timer wins, but it neither cancels nor awaits theTaskrunningwork(). The current code explicitly logs that it is abandoning cleanup, sodisconnect/mount-state teardown can continue aftershutdown()has returned and retain unresolved cleanup obligations (even though other guards reduce some publication races). - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned. —
shutdown()now snapshotsremovalTasksand waits for each removal viarunConcurrentlyForShutdown, but that wait is explicitly abandoned aftershutdownDeadlineNanoseconds. IfperformRemoveis suspended in a cancellation-ignoring storage, provider, or credential operation,withShutdownDeadlineresumes shutdown while the removal task remains active, so it can still mutate storage/provider state after shutdown returns. The currentwithShutdownDeadlineimplementation even logs that it is “abandoning” timed-out cleanup. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. — The lifecycle check is still a check-then-await race.
syncSavedConnectionRegistrationverifiesisRegistrableConnectionwhile shutdown is false, then suspends inmountProvider.ensureRegistered; shutdown can setisShuttingDownand take its teardown snapshots during that suspension, after which the provider call can register the domain. The later guard only attempts best-effort unregistration after registration has already occurred, and the sync task is not tracked or awaited byshutdown(so a hung/late provider call can leave the post-snapshot registration behind). - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. — The current code now checksisCleanupCompleteand throws when the disconnect fails, butapplyStorageSnapshothas already setconnections = nextConnectionsbefore registration (the publication occurs before the changed-config loop). Its catch only publishes an error state; it does not restorepreviousConfigor otherwise remove the edited row. Thus a mounted externally edited connection whose old filesystem/domain cleanup fails still remains visible with the new config while old runtime state is retained, so the reported consequence remains possible.
📋 Additional findings from this change (not shown inline) (7)
- 🟠 High A new Google Drive connection can be saved with no OAuth credential, leaving a mount that can never connect and offering no sign-in path afterward. (MFuse/Views/ConnectionEditorSheet.swift) — anchor-outside-diff
- 🟠 High Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations.
invalidate()takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrentruntimeContext()can create a new bootstrap task aftertake()and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took. (MFuseProvider/FileProviderExtension.swift) — anchor-outside-diff - 🟡 Medium Google Drive validation accepts whitespace-only OAuth Client ID and Redirect URI because it tests isEmpty rather than trimmed emptiness; such values are persisted and later fail OAuth refresh despite the editor reporting the configuration valid. (MFuse/Views/ConnectionEditorSheet.swift) — anchor-outside-diff
- 🟡 Medium Startup cleanup silently discards symlink-removal failures, so the reconciliation pass can report success while an orphaned managed shortcut remains and no retry is scheduled. In
DomainManager.removeStaleDomainsAndSymlinks, each orphan is removed withtry? fm.removeItem(at:); the same pattern exists inConnectionManager.cleanupOrphanedSymlinks. If the filesystem returns a permission or transient I/O error, the managed link remains in the user-visible shortcuts directory, whilesyncDomains()does not add an error and later startup may not retry it (the directory entry is still skipped silently). This violates the cleanup/error-handling and convergence obligations. This would be false if the shortcuts directory were guaranteed writable and removal could never fail, or if another guaranteed periodic cleanup retried failed removals. (MFuse/Services/DomainManager.swift) — anchor-outside-diff - 🟡 Medium The File Provider bootstrap timeout does not actually bound a connect that ignores cancellation.
withOperationTimeoutraces the operation against a sleeping timeout task, callsgroup.cancelAll(), and returns the result, but a throwing task group waits for all child tasks to finish before leaving its scope. Therefore, whenfileSystem.connect()blocks or ignores cancellation,runtimeContext()remains stuck beyond the advertised 15 seconds;invalidate()also waits on that bootstrap task after cancelling it and can leave the extension's filesystem/cache resources alive indefinitely. The claim would be false only if every backend's connect is guaranteed to promptly observe cancellation (including hangs at the underlying I/O layer). (MFuseProvider/FileProviderExtension.swift) — anchor-outside-diff - 🟡 Medium Malformed or otherwise undecodable Keychain credential data is not mapped to an authentication/bootstrap failure in the File Provider extension.
SharedCredentialStore.credentialthrows the decoder error,requireCredentialpropagates it, andnsError(from:)falls through toerror as NSErrorrather than returning NSFileProviderError.notAuthenticated. Thus a corrupt credential causes a generic provider error (and can be treated as server-unreachable) instead of the required authentication failure, with no recovery path for the domain. (MFuseProvider/FileProviderExtension.swift) — anchor-outside-diff - 🟡 Medium A credential mode transition can report success while leaving the old partition populated, creating duplicate credentials and a mixed state. (Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift) — anchor-unreliable
🤖 Prompt for AI agents — all findings (13)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (5)
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift, address this finding:
SharedCredentialStore reports successful store/delete operations even when cleanup of the legacy cleartext credential file fails, leaving a usable secret on disk without surfacing the failure.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Findings on this change (also posted as inline comments) (1)
In MFuse/Views/ConnectionEditorSheet.swift around line 863, address this finding:
The editor treats semantically equivalent S3 address spellings as a target change and clears the loaded access keys: for an existing `https://s3.example.com` mount, changing the endpoint field to `https://s3.example.com:443` (or trimming bucket/region whitespace) changes `ServerIdentity.s3Endpoint`/the raw fields even though `ConnectionConfig` and the S3 backend resolve these to the same endpoint/bucket/region. The key fields are then emptied and Save can no longer preserve the credential without re-entry.
## Additional findings on this change (not posted inline) (7)
In MFuse/Views/ConnectionEditorSheet.swift around line 524, address this finding:
A new Google Drive connection can be saved with no OAuth credential, leaving a mount that can never connect and offering no sign-in path afterward.
In MFuseProvider/FileProviderExtension.swift around line 242, address this finding:
Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations. `invalidate()` takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrent `runtimeContext()` can create a new bootstrap task after `take()` and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took.
In MFuse/Views/ConnectionEditorSheet.swift around line 525, address this finding:
Google Drive validation accepts whitespace-only OAuth Client ID and Redirect URI because it tests isEmpty rather than trimmed emptiness; such values are persisted and later fail OAuth refresh despite the editor reporting the configuration valid.
In MFuse/Services/DomainManager.swift around line 212, address this finding:
Startup cleanup silently discards symlink-removal failures, so the reconciliation pass can report success while an orphaned managed shortcut remains and no retry is scheduled. In `DomainManager.removeStaleDomainsAndSymlinks`, each orphan is removed with `try? fm.removeItem(at:)`; the same pattern exists in `ConnectionManager.cleanupOrphanedSymlinks`. If the filesystem returns a permission or transient I/O error, the managed link remains in the user-visible shortcuts directory, while `syncDomains()` does not add an error and later startup may not retry it (the directory entry is still skipped silently). This violates the cleanup/error-handling and convergence obligations. This would be false if the shortcuts directory were guaranteed writable and removal could never fail, or if another guaranteed periodic cleanup retried failed removals.
In MFuseProvider/FileProviderExtension.swift around line 150, address this finding:
The File Provider bootstrap timeout does not actually bound a connect that ignores cancellation. `withOperationTimeout` races the operation against a sleeping timeout task, calls `group.cancelAll()`, and returns the result, but a throwing task group waits for all child tasks to finish before leaving its scope. Therefore, when `fileSystem.connect()` blocks or ignores cancellation, `runtimeContext()` remains stuck beyond the advertised 15 seconds; `invalidate()` also waits on that bootstrap task after cancelling it and can leave the extension's filesystem/cache resources alive indefinitely. The claim would be false only if every backend's connect is guaranteed to promptly observe cancellation (including hangs at the underlying I/O layer).
In MFuseProvider/FileProviderExtension.swift around line 718, address this finding:
Malformed or otherwise undecodable Keychain credential data is not mapped to an authentication/bootstrap failure in the File Provider extension. `SharedCredentialStore.credential` throws the decoder error, `requireCredential` propagates it, and `nsError(from:)` falls through to `error as NSError` rather than returning NSFileProviderError.notAuthenticated. Thus a corrupt credential causes a generic provider error (and can be treated as server-unreachable) instead of the required authentication failure, with no recovery path for the domain.
In Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift, address this finding:
A credential mode transition can report success while leaving the old partition populated, creating duplicate credentials and a mixed state.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- scopes: 5/8 complete
| host: host, | ||
| port: port, | ||
| username: username, | ||
| s3Endpoint: s3Endpoint, |
There was a problem hiding this comment.
🔒 Security | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
⚠️ The exact pull-request diff is not available, but the affected editor identity comparison and credential-clearing behavior are present in the changed file.
🤖 Prompt for AI agents
In MFuse/Views/ConnectionEditorSheet.swift, address this finding:
The editor treats semantically equivalent S3 address spellings as a target change and clears the loaded access keys: for an existing `https://s3.example.com` mount, changing the endpoint field to `https://s3.example.com:443` (or trimming bucket/region whitespace) changes `ServerIdentity.s3Endpoint`/the raw fields even though `ConnectionConfig` and the S3 backend resolve these to the same endpoint/bucket/region. The key fields are then emptied and Save can no longer preserve the credential without re-entry.
…e nothing bounded - SharedCredentialStore: truncate a legacy credential file in place when it cannot be deleted; an atomic write needed the same directory permission the removal was just refused, so the secret stayed readable. - ConnectionEditorSheet: resolve the S3 endpoint, bucket and region the way ConnectionConfig does before deciding the target moved, so an equivalent spelling no longer clears the loaded access keys. - ConnectionEditorSheet: sign in to Google Drive from the sheet. Nothing in the app ever issued the prompt it promised "after saving", so a new mount was saved with no token and could never connect. Blank OAuth client fields are rejected and trimmed the way they are stored. - DomainManager / ConnectionManager: report a managed symlink that could not be removed instead of discarding the failure. - FileProviderExtension: refuse to build a runtime context after invalidate(), bound an operation whose work ignores cancellation, and report an undecodable credential as an authentication failure. - MirroredCredentialProvider: roll back and report a credential mode transition whose source cleanup failed, rather than leaving both partitions populated.
There was a problem hiding this comment.
Actionable comments posted: 2
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
🔎 Confirmed findings (2)
- 🟠 High Google Drive re-authorization can pair a new account's access token with the previous account's refresh token.
buildCredential()treats any OAuth response without a refresh token as safe to inheritsavedCredentialForCurrentTarget?.password, butGoogleOAuthProviderdoes not return or record an account identity, so authorizing account B with the same client/redirect URI and an omitted refresh token preserves account A's refresh token. The mount then initially uses B's token but later refreshes with A's token, violating account isolation and potentially failing or authenticating as the wrong account. (inline) - 🟡 Medium The committed website check/build pipeline does not type-check Svelte component script blocks, so a production build can pass the declared checks while Svelte-only type errors remain undetected. (inline)
⛔ Unresolved from previous review (6) — not approved until fixed
- MFuseProvider/FileProviderExtension.swift: Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations.
invalidate()takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrentruntimeContext()can create a new bootstrap task aftertake()and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took. — The bootstrap store now closes the creation door:takeForInvalidation()setsisInvalidated, andcurrentOrCreaterejects later callers. However, the reported defect also includes operations that already obtained the context. Operations still receive a plainFileProviderRuntimeContextfromruntimeContext()and are not registered or awaited byinvalidate(), which only awaits the bootstrap task before disconnecting and closing the context. Thus an in-flight operation can still use the filesystem or stores while cleanup tears them down. - Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift: SharedCredentialStore reports successful store/delete operations even when cleanup of the legacy cleartext credential file fails, leaving a usable secret on disk without surfacing the failure. — The cleanup helper still has a non-throwing signature and both callers ignore its outcome. If
removeItemfails and the fallbackFileHandle/truncatealso fails, the helper only logs a fault and returns;storeanddeletethen return successfully while the cleartext file remains readable. The fallback reduces the failure cases but does not eliminate the reported consequence or surface it to the caller. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved. — The defect remains in
withShutdownDeadline: it resumes the continuation when the sleeper wins, but never cancels or awaits theTask { await work() }. ThusrunConcurrentlyForShutdownandshutdown()can return whiledisconnect/mount cleanup is still suspended; that work can later continue andperformDisconnectstill publishes states or performs provider cleanup, while failures or a permanently hanging operation leave cleanup incomplete. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned. —
shutdown()now snapshotsremovalTasksand awaits each removal, but it does so throughrunConcurrentlyForShutdown/withShutdownDeadline, which resumes after five seconds even if the removal has not finished. The removal task is not cancelled, so a credential/provider/storage operation that outlives that deadline can still mutate state aftershutdown()returns; the current comments explicitly describe this as an abandoned pass. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. —
syncSavedConnectionRegistrationnow checksisRegistrableConnectionbefore and after its suspending teardown, and that gate rejects new work onceshutdown()setsisShuttingDown. However, an already-entered call can still be suspended insidemountProvider.ensureRegistered(config:)when shutdown sets the flag and snapshots teardown IDs. The shutdown snapshot does not track or await this registration call (the connection may otherwise be disconnected), soensureRegisteredcan complete and register the domain after the snapshot; only afterward does the save resume, notice the gate, and attemptunregister. If that unregister is still suspended—or shutdown has already returned—the domain is not covered by the shutdown teardown snapshot. Thus the reported in-flight-registration lifecycle race remains possible. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. — The lifecycle failure is now thrown viaguard isCleanupComplete(...), so the old silent-success path is addressed, but the original visible-config defect remains:applyStorageSnapshot()still assignsconnections = nextConnectionsbefore synchronizing changed rows, and its catch only records an error state. It does not restorepreviousConfig(or otherwise keep the old row visible) when cleanup/registration fails. Thus an externally edited row can still remain visible with the new config while the old filesystem/domain runtime state remains.
⚠️ Unverified risks (1)
- The editor's credential target does not include Dropbox/OneDrive OAuth account metadata, so changing the account labels does not invalidate or reload the saved token for the new account.
ConnectionConfig.addressesSameServertreats every non-addressing parameter (includingoauthAccountName/oauthAccountEmail) as identity, butserverIdentityonly populates the Google client fields and leaves the account fields out for the bundled OAuth backends. A saved Dropbox/OneDrive credential can therefore remain eligible forsavedCredentialForCurrentTargetafter the target account metadata changes and be written back with the new account labels. This would be false if those account fields were provably immutable after authorization and could never differ during an edit, sync, or reauthorization. (MFuse/Views/ConnectionEditorSheet.swift)
📋 Additional findings from this change (not shown inline) (3)
- 🟡 Medium Initial domain reconciliation failures are not shown to the user.
performInitialSetupIfNeeded()catchesdomainManager.syncDomains()errors and only callsNSLog, so a registration/disconnect/stale-domain failure during launch leaves the UI potentially showing unregistered or disconnected state with no localized actionable message or retry affordance. The later mount sync is best-effort/log-only as well. This violates the error-handling obligation for domain-sync failures. (MFuse/MFuseApp.swift) — anchor-outside-diff - 🟡 Medium Unsupported locale values in the query string or localStorage override the browser preference with the default locale instead of being rejected and allowing the next source in the precedence chain to be tried. For example, with navigator.languages[0] = 'zh-CN' and localStorage set to 'fr', getInitialLocale() returns 'en'; with ?lang=fr it also returns 'en'. This makes invalid persisted/bookmarked values prevent a supported browser locale from being selected, and the later mount persists that fallback and replaces the URL. (website/src/lib/i18n.js) — anchor-outside-diff
- 🔵 Low The sidebar's icon-only Add and overflow controls have no accessibility labels or help tooltips. VoiceOver therefore exposes the
plusandellipsis.circlebuttons without an actionable name, and pointer users get no tooltip, unlike the icon-only mount/reveal/refresh controls elsewhere. This leaves the sidebar's app-facing controls not understandable under the accessibility obligation. (MFuse/Views/SidebarView.swift) — anchor-outside-diff
♻️ Previously reported (still present) (1)
- 🔵 Low S3 region input is not normalized before persistence:
buildParameterscompares the raw text to"us-east-1"and stores any other value verbatim, including"", whitespace, or padded values.isValidperforms no region validation, while runtimes3Regiontrims/normalizes these values. Thus saving an emptied or padded default region writesregion: ""/region: " us-east-1 "into connections.json instead of the resolved default or an omitted key, and arbitrary malformed region strings are accepted and passed to Soto for signing. This would be false if another persistence layer canonicalized the parameter map afterbuildParameters; the shown save path directly passes its result toConnectionConfig. (MFuse/Views/ConnectionEditorSheet.swift) — previously-reported
❓ Low-evidence leads (not confirmed — verify before acting) (1)
- Batch Unmount All can miss mounts launched by a preceding Mount All. The action computes
configsToMountand then startsconnecttasks asynchronously; before those tasks publish.connecting/.mounting, Sidebar's Unmount All filters only rows whose effective state is mounted or mounting, so it can select nothing and finish. The pendingconnecttasks then proceed and mount after Unmount All has completed. The same admission race exists in the menu-bar batch because it snapshots all IDs butConnectionManager.disconnectdoes not cancel a connect that has not yet registered itsconnectTask; a connect can start after disconnect returns. (MFuse/Views/SidebarView.swift)
🤖 Prompt for AI agents — all findings (12)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (6)
In MFuseProvider/FileProviderExtension.swift, address this finding:
Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations. `invalidate()` takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrent `runtimeContext()` can create a new bootstrap task after `take()` and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift, address this finding:
SharedCredentialStore reports successful store/delete operations even when cleanup of the legacy cleartext credential file fails, leaving a usable secret on disk without surfacing the failure.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Findings on this change (also posted as inline comments) (2)
In MFuse/Views/ConnectionEditorSheet.swift around line 786, address this finding:
Google Drive re-authorization can pair a new account's access token with the previous account's refresh token. `buildCredential()` treats any OAuth response without a refresh token as safe to inherit `savedCredentialForCurrentTarget?.password`, but `GoogleOAuthProvider` does not return or record an account identity, so authorizing account B with the same client/redirect URI and an omitted refresh token preserves account A's refresh token. The mount then initially uses B's token but later refreshes with A's token, violating account isolation and potentially failing or authenticating as the wrong account.
In website/package.json around line 12, address this finding:
The committed website check/build pipeline does not type-check Svelte component script blocks, so a production build can pass the declared checks while Svelte-only type errors remain undetected.
## Additional findings on this change (not posted inline) (3)
In MFuse/MFuseApp.swift around line 159, address this finding:
Initial domain reconciliation failures are not shown to the user. `performInitialSetupIfNeeded()` catches `domainManager.syncDomains()` errors and only calls `NSLog`, so a registration/disconnect/stale-domain failure during launch leaves the UI potentially showing unregistered or disconnected state with no localized actionable message or retry affordance. The later mount sync is best-effort/log-only as well. This violates the error-handling obligation for domain-sync failures.
In website/src/lib/i18n.js around line 43, address this finding:
Unsupported locale values in the query string or localStorage override the browser preference with the default locale instead of being rejected and allowing the next source in the precedence chain to be tried. For example, with navigator.languages[0] = 'zh-CN' and localStorage set to 'fr', getInitialLocale() returns 'en'; with ?lang=fr it also returns 'en'. This makes invalid persisted/bookmarked values prevent a supported browser locale from being selected, and the later mount persists that fallback and replaces the URL.
In MFuse/Views/SidebarView.swift around line 38, address this finding:
The sidebar's icon-only Add and overflow controls have no accessibility labels or help tooltips. VoiceOver therefore exposes the `plus` and `ellipsis.circle` buttons without an actionable name, and pointer users get no tooltip, unlike the icon-only mount/reveal/refresh controls elsewhere. This leaves the sidebar's app-facing controls not understandable under the accessibility obligation.
## Previously reported and still present (1)
In MFuse/Views/ConnectionEditorSheet.swift around line 1083, address this finding:
S3 region input is not normalized before persistence: `buildParameters` compares the raw text to `"us-east-1"` and stores any other value verbatim, including `""`, whitespace, or padded values. `isValid` performs no region validation, while runtime `s3Region` trims/normalizes these values. Thus saving an emptied or padded default region writes `region: ""`/`region: " us-east-1 "` into connections.json instead of the resolved default or an omitted key, and arbitrary malformed region strings are accepted and passed to Soto for signing. This would be false if another persistence layer canonicalized the parameter map after `buildParameters`; the shown save path directly passes its result to `ConnectionConfig`.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- scopes: 5/7 complete
| // Google issues a refresh token on the first consent and may withhold it on a | ||
| // later one. Dropping the stored one then would leave a mount that works until | ||
| // the access token expires and can never renew it. | ||
| guard oauthCredential.password == nil, |
There was a problem hiding this comment.
🔒 Security | 🟠 High
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In MFuse/Views/ConnectionEditorSheet.swift, address this finding:
Google Drive re-authorization can pair a new account's access token with the previous account's refresh token. `buildCredential()` treats any OAuth response without a refresh token as safe to inherit `savedCredentialForCurrentTarget?.password`, but `GoogleOAuthProvider` does not return or record an account identity, so authorizing account B with the same client/redirect URI and an omitted refresh token preserves account A's refresh token. The mount then initially uses B's token but later refreshes with A's token, violating account isolation and potentially failing or authenticating as the wrong account.
| "dev": "vite", | ||
| "typesafe-i18n": "typesafe-i18n --no-watch", | ||
| "build": "bun run typesafe-i18n && vite build", | ||
| "check": "tsc --noEmit -p jsconfig.json && tsc --noEmit -p jsconfig.node.json", |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In website/package.json, address this finding:
The committed website check/build pipeline does not type-check Svelte component script blocks, so a production build can pass the declared checks while Svelte-only type errors remain undetected.
- Google Drive: read the account after authorization (drive/v3/about under the scope already granted), record it in the config, and carry a stored refresh token over only when the sign-in names the account it was issued for. The same client authorizes whichever account the user picks, so an omitted refresh token used to pair one account's access token with another's refresh token. - S3: resolve the region the way the backend does before storing it, so an emptied or padded field is no longer written verbatim. - SharedCredentialStore: report a legacy cleartext file that could neither be removed nor emptied instead of answering "stored"/"deleted" over it. The read path stays best-effort so a stuck file cannot take a mount down. - Quit: cancel the cleanup pass the deadline gives up on, rather than leaving it to write state after shutdown has reported the connections torn down. - Launch: surface a failed File Provider domain reconciliation with a retry, instead of logging it and leaving the window showing state the system does not have. - Sidebar: name the icon-only add and overflow controls for VoiceOver and as tooltips. - Website: skip a locale value the bundle cannot resolve so the next source in the chain is tried, rather than answering with the default.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift (1)
303-314: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAttempt every legacy partition before throwing.
deleteKeychainDatathrows for statuses other thanerrSecSuccessanderrSecItemNotFound. A synchronizable query can returnerrSecMissingEntitlement, so the current loop stops before later partitions are attempted. Collect the errors and throw after all partitions complete.🤖 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 `@Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift` around lines 303 - 314, Update deleteLegacyKeychainData to attempt every legacyAccessGroup and legacySyncMode combination even when deleteKeychainData throws; collect any thrown errors during iteration, then throw after both loops finish, while preserving successful and not-found deletion behavior.
🧹 Nitpick comments (1)
MFuse/MFuseApp.swift (1)
179-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated failure message.
performInitialSetupIfNeededandretryDomainSyncbuild the identical message from the same key, the same fallback, and the same argument. Extract one helper so the key and the fallback stay in one place.♻️ Proposed refactor
+ private func domainSyncFailureMessage(_ error: Error) -> String { + AppL10n.string( + "app.error.startupDomainSyncFailed", + fallback: "MFuse could not reconcile its File Provider domains at launch: %@. Mounts may be missing or show the wrong state until this succeeds.", + error.localizedDescription + ) + }Then use it at both sites:
- startupDomainSyncFailure = AppL10n.string( - "app.error.startupDomainSyncFailed", - fallback: "MFuse could not reconcile its File Provider domains at launch: %@. Mounts may be missing or show the wrong state until this succeeds.", - error.localizedDescription - ) + startupDomainSyncFailure = domainSyncFailureMessage(error)Also applies to: 224-228
🤖 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 `@MFuse/MFuseApp.swift` around lines 179 - 183, Extract the shared startup domain-sync failure message construction from performInitialSetupIfNeeded and retryDomainSync into a single helper. Centralize the AppL10n key, fallback text, and error.localizedDescription argument there, then have both call sites use the helper.
🤖 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 `@MFuse/MFuseApp.swift`:
- Around line 105-121: Update MenuBarView to handle a non-nil
startupDomainSyncFailure when the last window closes by exposing an alert with
the failure message and retryDomainSync action, or explicitly clear the state
during last-window closure. Ensure the failure cannot remain hidden without
either a retry opportunity or intentional dismissal.
In
`@Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift`:
- Around line 58-64: Update the fallback text in the errorDescription property
for credential.error.modeTransitionCleanupFailed to state that the mode
transition did not complete, rather than saying credentials were moved; retain
the existing failure details interpolation.
---
Outside diff comments:
In `@Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift`:
- Around line 303-314: Update deleteLegacyKeychainData to attempt every
legacyAccessGroup and legacySyncMode combination even when deleteKeychainData
throws; collect any thrown errors during iteration, then throw after both loops
finish, while preserving successful and not-found deletion behavior.
---
Nitpick comments:
In `@MFuse/MFuseApp.swift`:
- Around line 179-183: Extract the shared startup domain-sync failure message
construction from performInitialSetupIfNeeded and retryDomainSync into a single
helper. Centralize the AppL10n key, fallback text, and
error.localizedDescription argument there, then have both call sites use the
helper.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67e56a36-2305-44b9-a305-e762324950a2
📒 Files selected for processing (25)
MFuse/Localizable.xcstringsMFuse/MFuseApp.swiftMFuse/Services/DomainManager.swiftMFuse/Views/ConnectionEditorSheet.swiftMFuse/Views/SidebarView.swiftMFuseProvider/FileProviderExtension.swiftMFuseProvider/Localizable.xcstringsPackages/MFuseCore/Sources/MFuseCore/Connection/ConnectionConfig.swiftPackages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swiftPackages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swiftPackages/MFuseCore/Sources/MFuseCore/Resources/en.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/es.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/fr.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/id.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/it.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/ja.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/ko.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/zh-Hans.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/zh-Hant.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swiftPackages/MFuseCore/Tests/MFuseCoreTests/SharedStorageTests.swiftPackages/MFuseGoogleDrive/Package.swiftPackages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swiftPackages/MFuseGoogleDrive/Tests/MFuseGoogleDriveTests/GoogleDriveFileSystemTests.swiftwebsite/src/lib/i18n.js
🚧 Files skipped from review as they are similar to previous changes (6)
- Packages/MFuseCore/Sources/MFuseCore/Resources/zh-Hans.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Resources/es.lproj/Localizable.strings
- MFuse/Services/DomainManager.swift
- MFuse/Views/SidebarView.swift
- Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift
- MFuse/Views/ConnectionEditorSheet.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: winnowl/review
🔇 Additional comments (22)
Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift (1)
269-285: 🎯 Functional CorrectnessKeep cleanup over
connectionIDs. Both stores treat missing credentials as successful deletion, so IDs without credentials do not cause rollback.> Likely an incorrect or invalid review comment.website/src/lib/i18n.js (2)
60-69: LGTM!
32-41: 🎯 Functional CorrectnessNo issue found:
'zh-CN'and'en'are generated locales. The generated types and locale loaders include both values.> Likely an incorrect or invalid review comment.Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionConfig.swift (1)
248-248: LGTM!Also applies to: 272-272
Packages/MFuseCore/Sources/MFuseCore/Resources/id.lproj/Localizable.strings (1)
3-3: LGTM!Also applies to: 43-44
Packages/MFuseCore/Sources/MFuseCore/Resources/it.lproj/Localizable.strings (1)
3-3: LGTM!Also applies to: 43-44, 46-48, 51-52, 56-56
Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift (1)
163-204: LGTM!Also applies to: 437-442
Packages/MFuseCore/Tests/MFuseCoreTests/SharedStorageTests.swift (1)
26-33: LGTM!Also applies to: 52-84, 274-355
MFuse/MFuseApp.swift (1)
43-45: LGTM!Also applies to: 148-150, 309-336, 357-367
MFuseProvider/FileProviderExtension.swift (1)
26-41: LGTM!Also applies to: 54-57, 68-80, 170-207, 209-253, 343-343, 810-823, 1020-1021, 1070-1076
MFuseProvider/Localizable.xcstrings (1)
4-16: LGTM!Packages/MFuseCore/Sources/MFuseCore/Resources/en.lproj/Localizable.strings (1)
3-3: LGTM!Also applies to: 43-44, 46-48, 51-52, 56-56
Packages/MFuseCore/Sources/MFuseCore/Resources/fr.lproj/Localizable.strings (1)
3-3: LGTM!Also applies to: 43-44, 46-48, 51-52, 56-56
Packages/MFuseCore/Sources/MFuseCore/Resources/ja.lproj/Localizable.strings (1)
3-3: LGTM!Also applies to: 43-44, 46-48, 51-52, 56-56
Packages/MFuseCore/Sources/MFuseCore/Resources/ko.lproj/Localizable.strings (1)
3-3: LGTM!Also applies to: 43-44, 46-48, 51-52, 56-56
Packages/MFuseCore/Sources/MFuseCore/Resources/zh-Hant.lproj/Localizable.strings (1)
3-3: LGTM!Also applies to: 43-44, 46-48, 51-52, 56-56
MFuse/Localizable.xcstrings (2)
243-360: LGTM!Also applies to: 777-835, 1367-1425, 2487-2545, 7214-7272
3842-3842: 🎯 Functional CorrectnessNo remaining references to
editor.message.googleSignInAfterSavingexist.Packages/MFuseGoogleDrive/Package.swift (1)
11-12: LGTM!Also applies to: 21-21
Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift (2)
23-30: LGTM!Also applies to: 56-65, 141-167, 187-187, 217-217
31-40: 🔒 Security & PrivacyReachability path
● Entry MFuse/Views/ConnectionEditorSheet.swift:1348 authorizeOAuthAccount: Authorized against the client the sheet holds, trimmed the way │ ▼ ● Sink Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swiftDo not raise this account-binding issue. Missing or empty email data cannot match a saved account, and empty account metadata is not persisted.
Packages/MFuseGoogleDrive/Tests/MFuseGoogleDriveTests/GoogleDriveFileSystemTests.swift (1)
1-9: LGTM!Also applies to: 10-35, 37-51, 53-67
…d keep the retry reachable SharedCredentialStore.deleteLegacyKeychainData returned at the first refusal, leaving the remaining access group / sync mode combinations holding a credential the caller had just been told was deleted. Every combination is attempted now; failures are logged and the first one is still thrown. ModeTransitionCleanupError said the credentials had been moved. The transition is rolled back before the error is thrown, so nothing was moved — the text now says the change was rolled back. All nine locales updated. The startup domain sync failure was only reachable from the main window's alert. Closing the window leaves MFuse in the menu bar, so a retry that failed after that had nowhere to report and no way to be retried until a window was opened again. MenuBarView carries the same message with Retry and Dismiss, sharing the one piece of state with the alert. Also extracts the duplicated failure message construction into a single helper.
Owner
Author
|
@winnowl review |
There was a problem hiding this comment.
Actionable comments posted: 0
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
⛔ Unresolved from previous review (5) — not approved until fixed
- MFuseProvider/FileProviderExtension.swift: Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations.
invalidate()takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrentruntimeContext()can create a new bootstrap task aftertake()and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took. —BootstrapTaskStore.takeForInvalidation()now closes the door for laterruntimeContext()calls, so the new-context leak race is addressed. However, existing operations are still not registered or awaited: an operation can obtain the context fromruntimeContext(), andinvalidate()then awaits only the bootstrap task before disconnecting and closing that context's filesystem and stores. That operation can consequently continue using the torn-down context, so the reported teardown/use-after-close defect remains. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved. — The deadline still returns without awaiting the underlying teardown task.
withShutdownDeadlineresumes its continuation when the timer wins, then only callsworkTask.cancel(); cancellation is cooperative, so a disconnect/provider operation that ignores cancellation can continue andperformDisconnectcan still publish state or finish cleanup aftershutdown()has returned. The changed log text does not remove the original consequence. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned. —
shutdown()now snapshotsremovalTasksand waits on each removal, but the wait is only deadline-bounded: the shutdown worker awaitsmanager.removalTasks[id]?.result, whilewithShutdownDeadlinecancels only its localworkTaskwhen the five-second deadline fires. It never cancels the underlying removal task inremovalTasks. Therefore a removal suspended inperformRemove(for example in provider or credential I/O that ignores cancellation) can continue deleting/restoring storage and provider state aftershutdown()has returned, which is the reported consequence. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. —
syncSavedConnectionRegistrationnow checksisRegistrableConnectionbefore and after its suspension points, but it does not make the registration operation part of shutdown’s tracked lifecycle work.mountProvider.ensureRegistered(config:)can suspend;shutdown()can setisShuttingDownand takeidsNeedingTeardownwhile that call is suspended, and the call can then complete and register the domain afterward. The post-registration guard only attempts an unregister after the late registration; it does not prevent the registration from occurring after the teardown snapshot (and that cleanup is itself untracked). - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. — The lifecycle failure is now detected and thrown (guard isCleanupComplete ... else { throw ... }), but the original observable defect remains in reload:connections = nextConnectionspublishes the edited config before changed-registration cleanup, and the catch only records an error without restoringpreviousConfigor its domain/bootstrap state. Thus a failed disconnect can still leave the old runtime state while the UI retains the new config.
🤖 Prompt for AI agents — all findings (5)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (5)
In MFuseProvider/FileProviderExtension.swift, address this finding:
Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations. `invalidate()` takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrent `runtimeContext()` can create a new bootstrap task after `take()` and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- scopes: 1/7 complete
- the dig, synthesis stage ran out of wall clock, so the rest was left unexplored
…sing it Extension teardown shut the door on new runtime contexts but not on the operations already holding one: `invalidate()` disconnected the filesystem and closed the metadata cache, the content cache and the anchor store while a create, a move or a delete was still running against them. `MetadataCache.close()` drops the handle and leaves every later write a no-op, so the cache invalidation such a mutation makes on its way out was silently discarded — and the next extension instance opened the same database and went on serving the entry for an item that is no longer there until it expired. Operations now take the context as a counted lease, raised before the context is asked for so an operation cannot slip between the drain and the door, and released from a `defer` whether it returned, threw or was cancelled. Enumeration takes one too. Teardown drains those leases before it closes anything, bounded at five seconds and logged when it gives up: the system releases the instance shortly after `invalidate()`, so a backend that never returns from a cancelled read must not hold the SQLite handles open for the rest of the process's life.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MFuseProvider/FileProviderExtension.swift (1)
127-143: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding against a repeated
finish().
FileProviderRuntimeContextLeaseis a copyable struct. A secondfinish()on a copy decrementsactiveCountbelow zero.waitUntilIdlethen never observesactiveCount == 0, so the drain always runs to the 5-second timeout and closes the context under a live operation. The current call sites callfinish()once from adefer, so this is defensive only.A class with a
deinit-backed or flag-backed release removes the risk. A simpler option is to clamp inRuntimeContextActivity.end().♻️ Optional clamp in
RuntimeContextActivity.end()func end() { let waiter: CheckedContinuation<Void, Never>? = lock.withLock { - activeCount -= 1 + assert(activeCount > 0, "RuntimeContextActivity.end() called without a matching begin()") + activeCount = max(0, activeCount - 1) guard activeCount == 0 else { return nil } return takeIdleWaiterLocked() } waiter?.resume() }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 370b8d34-bcb1-4332-9ded-36b264cf4e06
📒 Files selected for processing (16)
MFuse/Localizable.xcstringsMFuse/MFuseApp.swiftMFuse/Views/MenuBarView.swiftMFuseProvider/FileProviderEnumerator.swiftMFuseProvider/FileProviderExtension.swiftPackages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swiftPackages/MFuseCore/Sources/MFuseCore/Resources/en.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/es.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/fr.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/id.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/it.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/ja.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/ko.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/zh-Hans.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Resources/zh-Hant.lproj/Localizable.stringsPackages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift
🚧 Files skipped from review as they are similar to previous changes (12)
- Packages/MFuseCore/Sources/MFuseCore/Resources/zh-Hant.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift
- MFuse/Localizable.xcstrings
- Packages/MFuseCore/Sources/MFuseCore/Resources/ja.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Resources/zh-Hans.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Resources/it.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Resources/ko.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Resources/es.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Resources/en.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Resources/fr.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Resources/id.lproj/Localizable.strings
- Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: winnowl/review
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
MFuse/MFuseApp.swift (2)
148-159: LGTM!Also applies to: 187-187, 236-236
205-211: 📐 Maintainability & Code QualityNo localization changes are required.
AppL10n.string(_:fallback:_:)accepts variadicCVarArgarguments and applies them withString(format:). All five keys exist, and every localized value forapp.error.startupDomainSyncFailedpreserves%@.MFuse/Views/MenuBarView.swift (1)
12-31: LGTM!Also applies to: 71-117, 137-160, 183-210, 224-239, 256-262, 273-288
MFuseProvider/FileProviderEnumerator.swift (1)
11-14: LGTM!Also applies to: 26-26, 95-97, 190-192, 284-286
MFuseProvider/FileProviderExtension.swift (2)
68-125: LGTM!Also applies to: 338-338, 396-396, 422-433, 486-488, 514-516, 575-577, 647-649, 717-719, 761-761, 958-996, 1206-1212
907-919: 🎯 Functional CorrectnessNo change required.
SharedCredentialStoreProvider.credential(for:)forwardsDecodingErrorwithout wrapping it.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
⛔ Unresolved from previous review (5) — not approved until fixed
- MFuseProvider/FileProviderExtension.swift: Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations.
invalidate()takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrentruntimeContext()can create a new bootstrap task aftertake()and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took. — The new invalidation barrier prevents contexts from being handed out aftertakeForInvalidation()and tracks leases that are already held, but teardown still closes the context after a bounded five-second drain.awaitInFlightOperations()useswithOperationTimeout(seconds: Self.invalidationDrainTimeoutSeconds, ...); when that timeout expires it only logs the failure, andinvalidate()immediately proceeds todisconnect()and closes the caches/stores. An operation that holds a lease and does not return within that bound can therefore still run against the context while it is being torn down, so the reported use-after-close consequence remains possible. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved. — The defect remains:
withShutdownDeadlinestill resumes shutdown when the timer wins, then only callsworkTask.cancel()on the unstructured cleanup task. Cancellation is cooperative and does not terminate a provider/filesystem operation that ignores cancellation, sodisconnect(and its underlying cleanup) can continue aftershutdown()returns and can still leave cleanup obligations unresolved or publish later state. The current timeout branch explicitly retains the abandon behavior:if resumer.resume() { workTask.cancel(); ... }. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned. —
shutdown()now snapshotsremovalTasksand waits on each task, but the wait is wrapped inwithShutdownDeadline. When the five-second deadline fires, that helper cancels only its localworkTask; the unstructured removal task stored inremovalTasksis not cancelled. Thus a removal suspended inmountProvider.unregister, credential access, or storage work can continue mutating provider/storage aftershutdown()returns. The removal task is only removed by its own eventual completion, so the original consequence remains possible on timeout. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. — The added
isRegistrableConnectionchecks reduce the orphaning window, but they do not prevent the reported race. A save can pass the second pre-registration check, then suspend inmountProvider.ensureRegistered(config:);shutdown()can setisShuttingDownand take its teardown snapshot during that suspension. WhenensureRegisteredresumes, it still registers the domain after the snapshot. The post-registration check only attempts an asynchronous best-effortunregister; shutdown does not track or await this registration operation, so the lifecycle gate still does not cover the in-flight save. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. — The lifecycle failure is now propagated bysyncSavedConnectionRegistration: afterawait disconnect(config.id, using: previousConfig), it checksisCleanupCompleteand throws on a failed teardown. However,applyStorageSnapshotstill publishesconnections = nextConnectionsbefore invoking that registration, and its catch only sets an error state; it does not restorepreviousConfig(or the prior domain/bootstrap state). Thus a failed disconnect can still leave the edited config visible while the old runtime state remains, which is the reported consequence.
📋 Additional findings from this change (not shown inline) (1)
- 🟡 Medium A failed Google Drive token refresh during connect() is surfaced by the File Provider extension as server-unreachable instead of notAuthenticated. In GoogleDriveFileSystem.connect() the 401 branch calls GoogleOAuthProvider.refresh directly; a revoked/invalid refresh token makes Google return HTTP 400, which refresh() throws as GoogleDriveError.oauthFailed. That error propagates out of connect() (the catch only clears accessToken and rethrows), and FileProviderExtension.nsError(from:) has no mapping for GoogleDriveError — its default case maps any non-RemoteFileSystemError to NSFileProviderError.serverUnreachable. So when a user's refresh token is revoked (revoked app access, invalid_grant), the extension's bootstrap reports the mount as "server unreachable" rather than asking the user to re-authenticate, violating the scope obligation that refresh failures surface as authenticationFailed, never as server-unreachable. This is the extension bootstrap path (bootstrapRuntimeContext -> connectFileSystemWithRetry -> fileSystem.connect()), so it is the primary way a Google Drive mount starts. (Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleDriveFileSystem.swift) — anchor-unreliable
❓ Low-evidence leads (not confirmed — verify before acting) (1)
- The OAuth authorization-code flow's security-critical paths have no direct tests: PKCE verifier/challenge generation, callback state validation, and the token refresh/refresh-token carry-over persistence (GoogleOAuthProvider.authorize, GoogleOAuthProvider.refresh, GoogleDriveFileSystem refresh with onCredentialUpdated). GoogleDriveFileSystemTests.swift contains only a placeholder and two account-lookup tests (with a mock URLSession); the state-validation guard in GoogleOAuthProvider.authorize (line ~129) and the refresh persistence in GoogleDriveFileSystem (connect and refreshAccessToken, which write the refreshed token via onCredentialUpdated into the mirrored/shared store) are never exercised, so a regression in state validation or in the refresh-token persistence ("a re-auth that returns no new refresh token keeps the stored one") would ship uncaught. This is the test-alignment gap the scope explicitly calls out. (Packages/MFuseGoogleDrive/Tests/MFuseGoogleDriveTests/GoogleDriveFileSystemTests.swift)
🤖 Prompt for AI agents — all findings (6)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (5)
In MFuseProvider/FileProviderExtension.swift, address this finding:
Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations. `invalidate()` takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrent `runtimeContext()` can create a new bootstrap task after `take()` and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Additional findings on this change (not posted inline) (1)
In Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleDriveFileSystem.swift, address this finding:
A failed Google Drive token refresh during connect() is surfaced by the File Provider extension as server-unreachable instead of notAuthenticated. In GoogleDriveFileSystem.connect() the 401 branch calls GoogleOAuthProvider.refresh directly; a revoked/invalid refresh token makes Google return HTTP 400, which refresh() throws as GoogleDriveError.oauthFailed. That error propagates out of connect() (the catch only clears accessToken and rethrows), and FileProviderExtension.nsError(from:) has no mapping for GoogleDriveError — its default case maps any non-RemoteFileSystemError to NSFileProviderError.serverUnreachable. So when a user's refresh token is revoked (revoked app access, invalid_grant), the extension's bootstrap reports the mount as "server unreachable" rather than asking the user to re-authenticate, violating the scope obligation that refresh failures surface as authenticationFailed, never as server-unreachable. This is the extension bootstrap path (bootstrapRuntimeContext -> connectFileSystemWithRetry -> fileSystem.connect()), so it is the primary way a Google Drive mount starts.
📜 Review details
Model
- gpt-5.6-luna, deepseek-v4-flash
Coverage
- 1 of 13 areas reviewed
…reachable server A refresh token Google has stopped honouring — access revoked, password changed, `invalid_grant` — is refused with HTTP 400, and `GoogleOAuthProvider.refresh` reported every non-200 as `GoogleDriveError.oauthFailed`. `connect()` rethrows that untouched, so it reached the extension unmatched by `nsError(from:)` and the mount reported a problem that had nothing to do with the network, while the one thing that fixes it — signing in again — was never asked for. This is the bootstrap path, so it is how a Google Drive mount comes up in the first place; the runtime paths already converted a failed refresh to `authenticationFailed`. A 400 or 401 from the token endpoint now throws `RemoteFileSystemError.authenticationFailed`, which the extension maps to `notAuthenticated` and the app reports as an authentication failure. Anything else the endpoint answers is Google failing to serve the request rather than the grant being gone, and stays a `GoogleDriveError`: sending the user through a sign-in that changes nothing is its own kind of wrong. Only the status code is logged, never the response body.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@Packages/MFuseGoogleDrive/Tests/MFuseGoogleDriveTests/GoogleDriveFileSystemTests.swift`:
- Around line 54-78: Extend
googleOAuthProviderReportsRevokedRefreshTokenAsAuthenticationFailure to also
exercise the HTTP 401 response path using an invalid_grant token error. Reuse
the existing provider refresh assertion pattern and verify that HTTP 401 maps to
RemoteFileSystemError.authenticationFailed, while preserving the current HTTP
400 coverage.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8720d8e-9a36-47b6-9568-3f5d1c5aae61
📒 Files selected for processing (2)
Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swiftPackages/MFuseGoogleDrive/Tests/MFuseGoogleDriveTests/GoogleDriveFileSystemTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: winnowl/review
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
Packages/MFuseGoogleDrive/Tests/MFuseGoogleDriveTests/GoogleDriveFileSystemTests.swift (2)
2-2: LGTM!
80-96: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 0
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
⛔ Unresolved from previous review (5) — not approved until fixed
- MFuseProvider/FileProviderExtension.swift: Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations.
invalidate()takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrentruntimeContext()can create a new bootstrap task aftertake()and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took. — The bootstrap-store invalidation barrier now prevents a new context aftertakeForInvalidation(), and leases track operations that acquired a context. However, teardown still proceeds after a fixed five-second timeout even if those leases remain active:awaitInFlightOperations()catches the timeout andinvalidate()immediately disconnects/closes every context resource. Thus an operation that continues beyond the drain timeout can still use the old filesystem/cache/anchor/state stores while they are being torn down, so the reported use-after-close consequence remains possible. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved. — The deadline still resumes shutdown without awaiting the underlying
workTask: on timeout it only callsworkTask.cancel()and returns. Cancellation is cooperative, whileperformDisconnectand its provider/filesystem operations do not consistently check cancellation before their later state/publication steps. A non-cooperative cleanup can therefore continue aftershutdown()returns, publish teardown state or otherwise touch resources, and remain unresolved exactly as described. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned. — Shutdown now snapshots and awaits the removals already in
removalTasks, butremove(_:)still has no shutdown guard and shutdown does not re-snapshot removals after its suspension points. A remove admitted afterisShuttingDownis set (for example while shutdown awaits the initial removal deadline, or during later teardown waits) can therefore runperformRemoveand mutate storage/provider state aftershutdown()returns. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
- Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. — The cleanup failure is now detected and thrown bysyncSavedConnectionRegistrationviaguard isCleanupComplete(...), but the original visibility defect remains:applyStorageSnapshotpublishesconnections = nextConnectionsbefore it runs the changed-config registration loop. If the subsequent disconnect fails, the catch only records an error state; it does not restorepreviousConfig(or otherwise replace the edited row), so the UI still shows the new config while old runtime state may remain.
📋 Additional findings from this change (not shown inline) (1)
- 🟡 Medium In
DomainManager.reconcileDomainsAndSymlinks, the decision to disconnect a domain is made fromexistingStatesByID, a snapshot taken once before the loop, and the disconnect for each connection is executed afterregisterCurrentRevisionsuspends (eachensureRegisteredperforms anNSFileProviderManager.addand, in the refresh path, up to ~1.5s of sleeps). If the user explicitly mounts a connection during that window — the pass runs while the window is up (ContentView.task→performInitialSetupIfNeeded) — the domain is already connected by the time the loop reaches the disconnect step, butshouldRemainDisconnectedwas computed from the stale snapshot (existingState.isDisconnected == true, or the domain absent from the snapshot). The pass then callsmountProvider.disconnect(config:)(DomainManager.swift line ~120) unconditionally, tearing down the mount the user just brought up. The disconnect step has no re-check of the live domain state and no generation/fence against a concurrentconnect/reconnect(it calls the provider directly, bypassingConnectionManager.disconnect's task tracking), so a freshly mounted domain is silently unmounted during startup reconciliation — violating the obligation that newly registered/explicitly mounted domains stay mounted after explicit user action. (MFuse/Services/DomainManager.swift) — anchor-outside-diff
🤖 Prompt for AI agents — all findings (6)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (5)
In MFuseProvider/FileProviderExtension.swift, address this finding:
Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations. `invalidate()` takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrent `runtimeContext()` can create a new bootstrap task after `take()` and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
The shutdown deadline abandons disconnect work while its underlying task continues, allowing post-shutdown cleanup/publication and leaving cleanup obligations unresolved.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Additional findings on this change (not posted inline) (1)
In MFuse/Services/DomainManager.swift around line 118, address this finding:
In `DomainManager.reconcileDomainsAndSymlinks`, the decision to disconnect a domain is made from `existingStatesByID`, a snapshot taken once before the loop, and the disconnect for each connection is executed after `registerCurrentRevision` suspends (each `ensureRegistered` performs an `NSFileProviderManager.add` and, in the refresh path, up to ~1.5s of sleeps). If the user explicitly mounts a connection during that window — the pass runs while the window is up (`ContentView.task` → `performInitialSetupIfNeeded`) — the domain is already connected by the time the loop reaches the disconnect step, but `shouldRemainDisconnected` was computed from the stale snapshot (`existingState.isDisconnected == true`, or the domain absent from the snapshot). The pass then calls `mountProvider.disconnect(config:)` (DomainManager.swift line ~120) unconditionally, tearing down the mount the user just brought up. The disconnect step has no re-check of the live domain state and no generation/fence against a concurrent `connect`/`reconnect` (it calls the provider directly, bypassing `ConnectionManager.disconnect`'s task tracking), so a freshly mounted domain is silently unmounted during startup reconciliation — violating the obligation that newly registered/explicitly mounted domains stay mounted after explicit user action.
📜 Review details
Model
- gpt-5.6-luna, deepseek-v4-flash
Coverage
- 1 of 15 areas reviewed
…fusal Startup reconciliation decided each disconnect from a domain-state snapshot taken once before the loop, while every step between the two suspends — ensureRegistered re-adds the domain and can spend seconds doing it. A mount the user started inside that window was missing from the snapshot, so the pass classified the domain as one to leave disconnected and tore down the mount that had just come up. The active mounts are now captured alongside the domain states, and the disconnect is skipped only for a connection that became active after that point. Google refuses a revoked refresh token with either HTTP 400 or 401 and the caller cannot tell which; only the 400 path was covered.
Owner
Author
|
@winnowl review |
There was a problem hiding this comment.
Actionable comments posted: 4
🛠️ To have the bot fix these findings, comment @winnowl fix.
⛔ Files ignored due to path filters (2)
website/bun.lockis excluded by!**/*.lockwebsite/package-lock.jsonis excluded by!**/package-lock.json
🔎 Confirmed findings (4)
- 🟠 High A save can restore an obsolete credential onto a newer config revision after its config update loses a race. (inline)
- 🟠 High A cancelled starter can leave the AWS client published after the only remaining joiner has also cancelled. (inline)
- 🟡 Medium Save-time domain registration failures are only shown in a dismissible alert and are not retained for reconciliation or retry in the running app. (inline)
- 🟡 Medium Legacy Keychain cleanup failures are silently discarded after a successful write, so an unreadable/unremovable item in a legacy access-group or either sync partition can remain while the caller is told the credential was stored successfully. (inline)
⛔ Unresolved from previous review (4) — not approved until fixed
- MFuseProvider/FileProviderExtension.swift: Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations.
invalidate()takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrentruntimeContext()can create a new bootstrap task aftertake()and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took. — The currentBootstrapTaskStorecloses the door before creating new contexts, and leases normally make teardown wait for operations; however,awaitInFlightOperations()uses a five-second timeout and explicitly continues after timeout, theninvalidate()disconnects and closes the context even when the logger reports operations are still in flight. Therefore the reported use-after-close consequence can still occur for an operation that outlives that timeout. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned. — Still-present: shutdown now snapshots and awaits removal tasks, but the removal is awaited through
manager.removalTasks[id]?.resultinside the deadline-bounded wrapper. When the deadline fires,withShutdownDeadlinecancelsworkTask, which does not cancel the independently stored removal task; a suspended removal can therefore resume and perform its later unregister, storage, or credential mutations after shutdown has returned. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot. — still-present:
syncSavedConnectionRegistrationchecksisShuttingDownbefore starting registration and again afterensureRegistered, butensureRegistereditself is an awaited untracked operation. Shutdown snapshots its teardown IDs without waiting for this save task, so a call already suspended inensureRegisteredcan finish after the snapshot and create the domain. The post-registration guard may unregister it afterward, but it does not prevent the reported post-snapshot registration or ensure shutdown waits for its cleanup. - Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift: A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and
syncSavedConnectionRegistrationdoes not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row toconnections; registration then callsdisconnect(id, using: previousConfig), which only publishes an error and returns, and unconditionally calls the nonthrowingconnect(id). If the lingering filesystem still cannot disconnect,connectalso only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branchawait disconnect(...); await connect(...); returnaround lines 1478-1481, with neither anisCleanupCompletecheck nor rollback topreviousConfig. The claim would be false if disconnect/connect failures were converted to thrown results and reload restoredpreviousConfig(including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return. — still-present:applyStorageSnapshotassignsconnections = nextConnectionsbefore callingsyncSavedConnectionRegistration. Although the current registration now checksisCleanupCompleteand throws instead of remounting, its catch does not restorepreviousConfig; it keeps the edited row and only publishes an error state. Therefore a failed teardown can still leave old runtime state under a newly visible config, which is the reported observable defect.
📋 Additional findings from this change (not shown inline) (35)
- 🟠 High A stale authorization result can still be applied to a changed editor target.
connectOAuthAccount()checks!Task.isCancelledbefore suspending onMainActor.run, but the target-change handler cancels the task and clears OAuth state; if the authorization task passes that check and the target changes before the queued main-actor closure executes, the closure unconditionally writes the old credential/account into the new target. This violates the late-result guard and can save/use a token authorized for the previous client/redirect target. The claim is false only if cancellation and the queued main-actor closure are guaranteed to be serialized such that target changes cannot occur between the check and closure execution. (MFuse/Views/ConnectionEditorSheet.swift) — anchor-unreliable - 🟠 High A canceled Google authorization can still update the editor for a newer target because the completion closure only checks cancellation before scheduling
MainActor.run, and performs no cancellation or target-identity check inside the queued closure. For example, authorization finishes, the task passesguard !Task.isCancelled, then the user changes the client/redirect/backend (which callsclearOAuthAuthorizationState()and cancels the task) before the main-actor block runs; the stale block then writesoauthCredentialand account labels into the newly selected target. This can make the new target appear connected and save the token authorized for the old target. The claim would be false only if this completion is guaranteed to execute synchronously on the same actor with no intervening target change, which is not guaranteed byawait MainActor.run. (MFuse/Views/ConnectionEditorSheet.swift) — anchor-outside-diff - 🟠 High Copying or moving a directory into one of its own descendants is not rejected; the source listing uses the source prefix while each copied object is added under that same prefix, so pagination can observe newly created descendants and recursively copy indefinitely or amplify data before move deletion. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — anchor-outside-diff
- 🟠 High DeleteObjects response errors are ignored, so a partial bulk deletion can be reported as successful and the caller may proceed (including move(), which then deletes the source path) despite objects remaining undeleted. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — anchor-outside-diff
- 🟠 High
copy/moveallow a directory to be copied into one of its own descendants. For source/aand a currently absent destination/a/b, the source listing includesa/...while the operation createsa/b/...; pagination can then copy newly-created descendants again (or otherwise produce an unbounded/duplicated copy). Formove, the finaldelete(at: /a)recursively deletes the newly-created/a/bsubtree as part of the source, so the move loses its destination. The invariant that a directory move/copy destination must not be the source or a descendant is not enforced. This would be disproven if callers guarantee destinations are never descendants, or if the backend contract explicitly permits destructive descendant moves. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — per-file-budget - 🟠 High Deleting a path that has both an object at the exact key and descendants under the slash-prefixed key leaves the exact object behind while deleting its descendants. For example, with objects
foo(file) andfoo/bar(child),delete(/foo)listsfoo/, deletesfoo/bar, setsdeletedDirectoryObjects = true, and skips the laterDeleteObject(foo)branch.itemInfoalso defines/fooas a file first, so this is a directly reachable inconsistent state rather than an unsupported path. This would be disproven if the storage invariant forbids an exact object andkey + "/"descendants from coexisting. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — per-file-budget - 🟠 High Destination nonexistence is not atomic with the subsequent CopyObject. If another client creates the destination after
ensureDestinationDoesNotExistreturns but beforecopyObject, S3 CopyObject overwrites that object; formove, the source is then deleted as well, so the concurrent destination data is silently replaced and cannot be recovered. The destination-existence invariant is therefore only true at check time, not at commit time. This would be disproven if all buckets enforce an external lock preventing concurrent writers, or if the S3-compatible server rejects CopyObject to an existing key (standard S3 does not). (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — anchor-unreliable - 🟠 High
syncDomainsdirectly invokesmountProvider.disconnect(config:)after deciding a domain should remain disconnected, rather than routing throughConnectionManager.disconnect. A userconnectcan begin after the reconciliation snapshot/check and before this direct call; the provider disconnect then tears down the newly started mount while ConnectionManager's state/connection task continues and may later publish mounted state or schedule resolution. The manager's disconnect-task/generation cancellation and await protocol is bypassed, producing a state/domain mismatch at startup or during retry reconciliation. (MFuse/Services/DomainManager.swift) — anchor-outside-diff - 🟠 High Deleting a directory can skip objects on multi-page listings. The code obtains a continuation token for page N, deletes all keys from page N, then uses that token against the now-mutated keyspace. S3 continuation tokens are tied to the listing position, not a snapshot; removing keys before the token can shift the boundary and cause later keys to be skipped. For a directory with more than one page,
deletecan return success while objects remain. This would be disproven if every supported S3 implementation guarantees continuation tokens enumerate an immutable snapshot despite concurrent deletion. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — per-file-budget - 🟡 Medium The public initializer can create and persist an authentication method unsupported by the selected backend. For example,
ConnectionConfig(name:..., backendType: .s3, ...)defaults to.password, although S3 only supports.accessKey; a valid access-key credential paired with that config is then rejected by File Provider's password-shaped validation while S3 construction expects accessKeyID/secretAccessKey. Any non-editor caller (import, tests, sync or API use) that uses the default produces an unusable persisted connection. (Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionConfig.swift) — anchor-outside-diff - 🟡 Medium A failed target-changing save does not roll back the runtime teardown performed before the write. (MFuse/Views/ContentView.swift) — anchor-outside-diff
- 🟡 Medium Mirrored writes and deletes are not atomic across the primary and shared stores, so a partial failure leaves the two app-facing credential stores divergent even though the operation barrier serializes later operations. (Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift) — anchor-outside-diff
- 🟡 Medium The callback is authenticated only by its state value and does not verify that the returned URL matches the configured redirect URI (scheme, host, path, and port).
ASWebAuthenticationSessionis configured with onlycallbackURLScheme, and any URL delivered for that scheme is accepted before extractingstateandcode. A callback such ascom.example.mfuse://attacker/wrong-path?state=<current-state>&code=<code>can therefore be exchanged, violating the obligation to reject mismatched callback schemes/redirect targets. This would be disproven if the platform guaranteed that ASWebAuthenticationSession always returns a callback URL whose complete redirect URI is identical to the authorization request, rather than only enforcing the registered scheme. (Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift) — anchor-outside-diff - 🟡 Medium Refresh classifies every HTTP 400 or 401 response as
RemoteFileSystemError.authenticationFailed, regardless of the provider error payload. Thus a 400invalid_client, malformed request, or other client/configuration error is treated as a revoked grant and prompts reauthentication; the intended revoked-grant classification should be based on the OAuth error (for exampleinvalid_grant), while server/network failures remain distinct. This would be disproven only if Google’s token endpoint contract guarantees that every 400/401 response for this request is exclusively a revoked grant. (Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift) — inline-budget - 🟡 Medium Cancelling the editor's
oauthAuthorizationTaskdoes not cancel the underlying browser session.authorize()stores the session and awaits a checked continuation, but has no cancellation handler that callsASWebAuthenticationSession.cancel(). The editor cancels the task on target changes/disappearance; the provider task can consequently leave the browser authorization UI running until the user completes it, and the continuation remains in flight. This is false only if ASWebAuthenticationSession is guaranteed to be automatically canceled when the awaiting Swift task is canceled (the session is not otherwise tied to that task). (Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift) — inline-budget - 🟡 Medium
currentAccountincludes the raw Google response body inGoogleDriveError.oauthFailedfor every non-200 response, and refresh similarly includes the raw token endpoint body for non-400/401 responses. These messages are later assigned directly to the editor's user-visibletestResult(testResult = error.localizedDescription), so provider response details can be surfaced unnecessarily (and may contain diagnostic or sensitive fields). The claim is false only if Google guarantees these response bodies never contain sensitive or user-unfriendly data and the localized error is never shown outside diagnostics. (Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift) — inline-budget - 🟡 Medium Save remains enabled while a Google re-authentication is in progress, and the editor continues to treat the previously saved token as the connected account. An existing mount can therefore be opened, click Re-authenticate (intending to replace it with the account selected in the browser), and immediately click Save before authorization completes; the old account/token is persisted and the in-progress authorization is silently discarded by the subsequent sheet dismissal. This violates the flow's implied re-authentication/save ordering and can leave the user believing the newly selected account was saved when it was not. The claim would be false if Save is blocked elsewhere (for example by a parent-level modal action guard) or if saving is explicitly intended to be allowed and documented as retaining the old account while authorization runs. (MFuse/Views/ConnectionEditorSheet.swift) — inline-budget
- 🟡 Medium Raw Google token-endpoint response bodies are embedded in
GoogleDriveError.oauthFailedfor non-200 refresh responses (and account lookup failures). If such an error reaches a File Provider operation,nsError(from:)falls through toerror as NSError, preserving the localized description, while operation handlers also logerror.localizedDescriptionwith public privacy. A provider response containing an email, tenant/account details, or other diagnostic data can therefore be written to public extension logs and/or returned to File Provider/UI instead of being reduced to a safe generic error. (Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift) — inline-budget - 🟡 Medium A persisted or newly saved remote path containing surrounding whitespace is not normalized consistently: the editor preserves it in ConnectionConfig.remotePath, while s3Key trims only '/' characters. For example remotePath " photos/ " produces keys rooted at " photos/ /..." rather than the configured logical prefix "photos/...", breaking enumeration and all file operations. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — inline-budget
- 🟡 Medium Fallback classification scrubs the configured bucket before scanning authentication indicators. If the configured bucket is itself an indicator (for example bucket "accessdenied") and the SDK supplies an unstructured authentication error with description "AccessDenied", removing the bucket token leaves no indicator and the error is misclassified as an unreachable endpoint instead of authenticationFailed. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — inline-budget
- 🟡 Medium A whitespace-only (or whitespace-padded) S3 Remote Path is not normalized consistently: the editor persists it because
isEmptyis false, while the S3 key builder removes only/, not whitespace. Thus a field that visually represents an empty/root path (for example" ") makes the root listing use prefix" ", and"/photos/ "targetsphotos/rather thanphotos. This means the saved value and the effective S3 prefix do not follow the same blank/path normalization rule. (MFuse/Views/ConnectionEditorSheet.swift) — inline-budget - 🟡 Medium The fallback scrubber can erase the actual authentication indicator when the bucket or endpoint contains that indicator, causing real credential failures to be classified as unreachable endpoint failures. For example, a bucket named
accessdeniedand an SDK/transport description ofAccessDeniedis transformed to an empty string beforeauthenticationIndicatorsis checked, somapConnectionErrorreturns.connectionFailedinstead of.authenticationFailed; the UI will advise network troubleshooting rather than credential repair. The same collision occurs for endpoint hosts containingunauthorizedor other indicator words. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — inline-budget - 🟡 Medium With a non-empty configured remote prefix,
itemInfo(at: .root)can report the root as a file instead of a directory. For config remotePathtenant, if an object exists at keytenant(alongside normal keys undertenant/), the first HEAD usess3Key(root, isDirectory:false)=tenantand returns.file; callers then treat the mounted root as a file and directory copy/delete behavior is wrong. Root should be a directory namespace regardless of an exact object at the prefix key. This would be disproven if the backend guarantees that no object can exist at the configured prefix key. (Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift) — inline-budget - 🟡 Medium The changed
editor.warning.cleartextCredentialstranslations still describe credentials being stored in a shared configuration file, while the call site now uses the warning for FTP/WebDAV with TLS disabled and English correctly warns that credentials are transmitted in cleartext. A non-English user can therefore miss the active network confidentiality risk and the instruction to enable TLS. (MFuse/Localizable.xcstrings) — inline-budget - 🟡 Medium Browser locale discovery ignores supported locales after the first browser language, so the documented browser fallback is not actually preference-aware. For example, with
navigator.languages = ['fr-FR', 'zh-CN']and no valid query/storage value,getInitialLocale()normalizes onlyfr-FRand returns the defaulten, instead of selecting the later supportedzh-CN. This makes the result depend on an unsupported first entry and violates deterministic supported-locale filtering/precedence when browsers provide an ordered list. (website/src/lib/i18n.js) — inline-budget - 🟡 Medium The declared
typesafe-i18nnpm script is not executable with the project's frozen dependency set: it runstypesafe-i18n@5.27.1, whose generator calls the removedts.createProgramAPI, against the locked TypeScript 7.0.2. Thus a contributor following the documentednpm run typesafe-i18ncommand cannot regenerate translations from the project itself; the README's external TypeScript 5.9.3 workaround confirms the script is knowingly broken. This violates the npm-script/buildability obligation unless the script is removed/disabled or made compatible with the locked toolchain. (website/package.json) — inline-budget - 🟡 Medium The provider extension chooses its
SharedCredentialStoresync mode from the live shared setting on every access, while the app'ssetSynchronizableEnabledmigrates credentials before persisting the setting. During a mode toggle, a File Provider request can observe the new setting (or an already changed process-shared setting) before the migration is complete and read the target partition, get no credential, and report authentication failure; conversely a token refresh can write into the target partition while the app's transition later rolls back/cleans it. The two processes have no transition marker or mount fencing, so an in-flight provider operation can authenticate against a partition whose migration outcome is not yet committed. (MFuseProvider/FileProviderExtension.swift) — inline-budget - 🟡 Medium HTTP response bodies are copied verbatim into user-visible GoogleDriveError descriptions. A non-200 account lookup or non-400/401 token refresh constructs messages such as
...: \(bodyDescription), and the File Provider fallback returns the original NSError rather than sanitizing it; a server response containing internal details, echoed request data, or attacker-controlled markup can therefore be logged/displayed through connection and File Provider error handling. (Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift) — inline-budget - 🟡 Medium Startup credential mirroring failures are not surfaced as actionable state. When the primary credential exists but the shared/File Provider copy fails, the app continues startup after logging the snapshot error, then can register/auto-mount domains whose extension cannot read the credential. The user sees a mount/authentication failure rather than a clear credential-sync failure and has no prompted recovery path, leaving the persisted config and credential copies inconsistent. (MFuse/MFuseApp.swift) — anchor-unreliable
- 🟡 Medium Google Drive server failures and transport failures are not consistently classified as File Provider server-unreachable errors. For example, a token refresh receiving HTTP 503 throws GoogleDriveError.oauthFailed, and a URLSession network error from GoogleDriveFileSystem is rethrown unchanged; nsError(from:) only translates RemoteFileSystemError and otherwise returns the original NSError. Consequently File Provider receives a non-NSFileProviderErrorDomain error (and in the OAuth case the Google response text), so the provider layer cannot reliably surface these as server/network failures. (MFuseProvider/FileProviderExtension.swift) — anchor-unreliable
- 🟡 Medium Sidebar Unmount All still has a snapshot race with a concurrent Mount All. Each action independently filters
connectionManager.connectionsbefore its task group runs; if Unmount All evaluates while a connection is still unmounted and Mount All then admits/connects it, the unmount snapshot omits it and the newly started mount completes after the batch, leaving a mounted connection despite the user's later Unmount All request. The effective-state helper only fixes tasks already present at the filter instant, not this admission race. (MFuse/Views/SidebarView.swift) — inline-budget - 🟡 Medium Legacy migration is only attempted when the current access-group/mode has no item; if a current item exists while an old-access-group item remains in the other local or synchronizable partition, reads return the current credential and never probe or remove the stale legacy item. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — anchor-unreliable
- 🟡 Medium After an expired access token, every refresh failure during a normal file operation is converted to
RemoteFileSystemError.authenticationFailed, including transient network errors and Google token-endpoint 5xx responses. Thus a temporary outage while refreshing a token causes File Provider to returnnotAuthenticated(and prompt/recover as if the user must sign in) rather than a retryable/server-unreachable error. This differs fromconnect(), which rethrows non-400/401 OAuth failures, so the same refresh failure is mapped inconsistently depending on when it occurs. (Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleDriveFileSystem.swift) — anchor-unreliable - 🟡 Medium The app localization catalog does not contain the OAuth editor keys used by ConnectionEditorSheet, so those calls always fall back to English rather than resolving the target catalog (and cannot provide the promised non-English translations). (MFuse/Views/ConnectionEditorSheet.swift) — anchor-unreliable
- 🔵 Low When a connection is mounting, ConnectionDetailView replaces the mount button with an unlabeled
ProgressView. VoiceOver therefore receives no meaningful control/status label for the in-progress lifecycle state (unlike MenuBarView and SidebarView, which explicitly label their mounting indicators), so a user navigating the detail header cannot determine that mounting is in progress or what the control represents. This would be disproven if the enclosing header or platform accessibility automatically exposed this ProgressView as a localized mounting status on supported macOS versions. (MFuse/Views/ConnectionDetailView.swift) — inline-budget
♻️ Previously reported (still present) (6)
- 🟠 High Startup reconciliation can disconnect a mount explicitly started after its active-state check, because the check and provider disconnect are separated by an un-fenced async boundary. (MFuse/Services/DomainManager.swift) — previously-reported
- 🟠 High The shutdown deadline does not cancel the actual per-connection teardown task, so shutdown can return while disconnect continues and later publishes state or touches provider resources after shutdown. (Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift) — previously-reported
- 🟡 Medium An existing but unreadable legacy cleartext credential file is treated as if it does not exist, so a read returns nil successfully and leaves the legacy secret file in place without surfacing cleanup/migration failure. (Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift) — previously-reported
- 🟡 Medium MFuseCore's explicit Chinese fallback still fails for valid regional Chinese identifiers not on its allowlist, such as
zh-MY: candidates becomezh-MY,zh-MY, and barezh, but the package ships onlyzh-Hans.lprojandzh-Hant.lproj, so the helper returns the English fallback instead of selecting a Chinese resource. This violates regional Chinese resolution unlesszh-MYis explicitly unsupported. (Packages/MFuseCore/Sources/MFuseCore/Localization/MFuseCoreL10n.swift) — previously-reported - 🟡 Medium Reveal can still open a mount after the user has completed Unmount.
resolveFinderURLperforms its finalcanRevealMountcheck and returns a URL, but the caller then yields back to the main actor beforeNSWorkspace.activateFileViewerSelectingruns. A disconnect can remove the symlink/domain and publish unmounted in that gap; the already-returned URL is nevertheless activated, so Reveal can take the user to a stale/nonexistent Cloud Storage location after unmount. This would be disproven only if disconnect were serialized through activation or the returned URL carried a lifecycle token that the activation path revalidated. (MFuse/Services/ConnectionManager+Finder.swift) — previously-reported - 🟡 Medium The detail view's mounting control is an unlabeled icon-free ProgressView. While a connection is mounting,
mountButtonreplaces the labeled Mount/Unmount button withProgressView()without a help string or accessibility label, so VoiceOver and other accessibility clients cannot identify the lifecycle state/control, contrary to the surrounding controls' explicit labels. (MFuse/Views/ConnectionDetailView.swift) — anchor-unreliable
❓ Low-evidence leads (not confirmed — verify before acting) (1)
- Chinese locale resolution is inconsistent across targets: MFuseCore explicitly maps regional Chinese identifiers to script bundles, but AppL10n and the provider use
NSLocalizedStringwith.mainand no equivalent script mapping; with azh-TW/zh-HKorzh-CNuser locale, app/provider resources can fall back differently from core resources because onlyzh-Hans.lproj/zh-Hant.lprojare shipped. (MFuse/Support/AppL10n.swift)
🤖 Prompt for AI agents — all findings (49)
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
## Unresolved from the previous review — these block approval, fix them first (4)
In MFuseProvider/FileProviderExtension.swift, address this finding:
Invalidation does not synchronize teardown with later runtime-context creation or in-flight operations. `invalidate()` takes/cancels the current bootstrap task, but there is no invalidated/cleanup barrier: a concurrent `runtimeContext()` can create a new bootstrap task after `take()` and while cleanup is still disconnecting/closing the old context, and operations that already obtained the old context are not tracked and can continue while its filesystem, caches, anchor store, and state store are being closed. This permits use-after-close/torn-down context and leaks the newly created context because the cleanup only owns the task it took.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown does not account for in-flight removals, so a removal can continue mutating storage and provider state after shutdown has returned.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
Shutdown's lifecycle gate does not cover saved-connection registration, allowing an in-flight save to register a File Provider domain after shutdown has taken its teardown snapshot.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift, address this finding:
A changed external config replaces the old visible config before cleanup/remount is known to have succeeded, and `syncSavedConnectionRegistration` does not propagate cleanup/connect failure. Concrete case: an existing mounted connection's UUID is edited externally, but its filesystem or File Provider disconnect fails. Reload first assigns the edited row to `connections`; registration then calls `disconnect(id, using: previousConfig)`, which only publishes an error and returns, and unconditionally calls the nonthrowing `connect(id)`. If the lingering filesystem still cannot disconnect, `connect` also only publishes an error and returns, so registration reports success and the UI retains the new config even though old runtime state remains. This violates the requirement that failed cleanup keep the old connection visible rather than silently committing a changed config. Evidence is the unconditional assignment in reload around line 1078 and the branch `await disconnect(...); await connect(...); return` around lines 1478-1481, with neither an `isCleanupComplete` check nor rollback to `previousConfig`. The claim would be false if disconnect/connect failures were converted to thrown results and reload restored `previousConfig` (including domain/bootstrap state), or if those lifecycle methods guaranteed success whenever they return.
## Findings on this change (also posted as inline comments) (4)
In MFuse/Views/ContentView.swift around line 219, address this finding:
A save can restore an obsolete credential onto a newer config revision after its config update loses a race.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 162, address this finding:
A cancelled starter can leave the AWS client published after the only remaining joiner has also cancelled.
In MFuse/Views/ContentView.swift around line 259, address this finding:
Save-time domain registration failures are only shown in a dismissible alert and are not retained for reconciliation or retry in the running app.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 293, address this finding:
Legacy Keychain cleanup failures are silently discarded after a successful write, so an unreadable/unremovable item in a legacy access-group or either sync partition can remain while the caller is told the credential was stored successfully.
## Additional findings on this change (not posted inline) (35)
In MFuse/Views/ConnectionEditorSheet.swift, address this finding:
A stale authorization result can still be applied to a changed editor target. `connectOAuthAccount()` checks `!Task.isCancelled` before suspending on `MainActor.run`, but the target-change handler cancels the task and clears OAuth state; if the authorization task passes that check and the target changes before the queued main-actor closure executes, the closure unconditionally writes the old credential/account into the new target. This violates the late-result guard and can save/use a token authorized for the previous client/redirect target. The claim is false only if cancellation and the queued main-actor closure are guaranteed to be serialized such that target changes cannot occur between the check and closure execution.
In MFuse/Views/ConnectionEditorSheet.swift around line 1324, address this finding:
A canceled Google authorization can still update the editor for a newer target because the completion closure only checks cancellation before scheduling `MainActor.run`, and performs no cancellation or target-identity check inside the queued closure. For example, authorization finishes, the task passes `guard !Task.isCancelled`, then the user changes the client/redirect/backend (which calls `clearOAuthAuthorizationState()` and cancels the task) before the main-actor block runs; the stale block then writes `oauthCredential` and account labels into the newly selected target. This can make the new target appear connected and save the token authorized for the old target. The claim would be false only if this completion is guaranteed to execute synchronously on the same actor with no intervening target change, which is not guaranteed by `await MainActor.run`.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 741, address this finding:
Copying or moving a directory into one of its own descendants is not rejected; the source listing uses the source prefix while each copied object is added under that same prefix, so pagination can observe newly created descendants and recursively copy indefinitely or amplify data before move deletion.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 707, address this finding:
DeleteObjects response errors are ignored, so a partial bulk deletion can be reported as successful and the caller may proceed (including move(), which then deletes the source path) despite objects remaining undeleted.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 722, address this finding:
`copy`/`move` allow a directory to be copied into one of its own descendants. For source `/a` and a currently absent destination `/a/b`, the source listing includes `a/...` while the operation creates `a/b/...`; pagination can then copy newly-created descendants again (or otherwise produce an unbounded/duplicated copy). For `move`, the final `delete(at: /a)` recursively deletes the newly-created `/a/b` subtree as part of the source, so the move loses its destination. The invariant that a directory move/copy destination must not be the source or a descendant is not enforced. This would be disproven if callers guarantee destinations are never descendants, or if the backend contract explicitly permits destructive descendant moves.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 714, address this finding:
Deleting a path that has both an object at the exact key and descendants under the slash-prefixed key leaves the exact object behind while deleting its descendants. For example, with objects `foo` (file) and `foo/bar` (child), `delete(/foo)` lists `foo/`, deletes `foo/bar`, sets `deletedDirectoryObjects = true`, and skips the later `DeleteObject(foo)` branch. `itemInfo` also defines `/foo` as a file first, so this is a directly reachable inconsistent state rather than an unsupported path. This would be disproven if the storage invariant forbids an exact object and `key + "/"` descendants from coexisting.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift, address this finding:
Destination nonexistence is not atomic with the subsequent CopyObject. If another client creates the destination after `ensureDestinationDoesNotExist` returns but before `copyObject`, S3 CopyObject overwrites that object; for `move`, the source is then deleted as well, so the concurrent destination data is silently replaced and cannot be recovered. The destination-existence invariant is therefore only true at check time, not at commit time. This would be disproven if all buckets enforce an external lock preventing concurrent writers, or if the S3-compatible server rejects CopyObject to an existing key (standard S3 does not).
In MFuse/Services/DomainManager.swift around line 137, address this finding:
`syncDomains` directly invokes `mountProvider.disconnect(config:)` after deciding a domain should remain disconnected, rather than routing through `ConnectionManager.disconnect`. A user `connect` can begin after the reconciliation snapshot/check and before this direct call; the provider disconnect then tears down the newly started mount while ConnectionManager's state/connection task continues and may later publish mounted state or schedule resolution. The manager's disconnect-task/generation cancellation and await protocol is bypassed, producing a state/domain mismatch at startup or during retry reconciliation.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 711, address this finding:
Deleting a directory can skip objects on multi-page listings. The code obtains a continuation token for page N, deletes all keys from page N, then uses that token against the now-mutated keyspace. S3 continuation tokens are tied to the listing position, not a snapshot; removing keys before the token can shift the boundary and cause later keys to be skipped. For a directory with more than one page, `delete` can return success while objects remain. This would be disproven if every supported S3 implementation guarantees continuation tokens enumerate an immutable snapshot despite concurrent deletion.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionConfig.swift around line 331, address this finding:
The public initializer can create and persist an authentication method unsupported by the selected backend. For example, `ConnectionConfig(name:..., backendType: .s3, ...)` defaults to `.password`, although S3 only supports `.accessKey`; a valid access-key credential paired with that config is then rejected by File Provider's password-shaped validation while S3 construction expects accessKeyID/secretAccessKey. Any non-editor caller (import, tests, sync or API use) that uses the default produces an unusable persisted connection.
In MFuse/Views/ContentView.swift around line 201, address this finding:
A failed target-changing save does not roll back the runtime teardown performed before the write.
In Packages/MFuseCore/Sources/MFuseCore/Connection/MirroredCredentialProvider.swift around line 165, address this finding:
Mirrored writes and deletes are not atomic across the primary and shared stores, so a partial failure leaves the two app-facing credential stores divergent even though the operation barrier serializes later operations.
In Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift around line 103, address this finding:
The callback is authenticated only by its state value and does not verify that the returned URL matches the configured redirect URI (scheme, host, path, and port). `ASWebAuthenticationSession` is configured with only `callbackURLScheme`, and any URL delivered for that scheme is accepted before extracting `state` and `code`. A callback such as `com.example.mfuse://attacker/wrong-path?state=<current-state>&code=<code>` can therefore be exchanged, violating the obligation to reject mismatched callback schemes/redirect targets. This would be disproven if the platform guaranteed that ASWebAuthenticationSession always returns a callback URL whose complete redirect URI is identical to the authorization request, rather than only enforcing the registered scheme.
In Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift around line 208, address this finding:
Refresh classifies every HTTP 400 or 401 response as `RemoteFileSystemError.authenticationFailed`, regardless of the provider error payload. Thus a 400 `invalid_client`, malformed request, or other client/configuration error is treated as a revoked grant and prompts reauthentication; the intended revoked-grant classification should be based on the OAuth error (for example `invalid_grant`), while server/network failures remain distinct. This would be disproven only if Google’s token endpoint contract guarantees that every 400/401 response for this request is exclusively a revoked grant.
In Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift around line 105, address this finding:
Cancelling the editor's `oauthAuthorizationTask` does not cancel the underlying browser session. `authorize()` stores the session and awaits a checked continuation, but has no cancellation handler that calls `ASWebAuthenticationSession.cancel()`. The editor cancels the task on target changes/disappearance; the provider task can consequently leave the browser authorization UI running until the user completes it, and the continuation remains in flight. This is false only if ASWebAuthenticationSession is guaranteed to be automatically canceled when the awaiting Swift task is canceled (the session is not otherwise tied to that task).
In Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift around line 165, address this finding:
`currentAccount` includes the raw Google response body in `GoogleDriveError.oauthFailed` for every non-200 response, and refresh similarly includes the raw token endpoint body for non-400/401 responses. These messages are later assigned directly to the editor's user-visible `testResult` (`testResult = error.localizedDescription`), so provider response details can be surfaced unnecessarily (and may contain diagnostic or sensitive fields). The claim is false only if Google guarantees these response bodies never contain sensitive or user-unfriendly data and the localized error is never shown outside diagnostics.
In MFuse/Views/ConnectionEditorSheet.swift around line 460, address this finding:
Save remains enabled while a Google re-authentication is in progress, and the editor continues to treat the previously saved token as the connected account. An existing mount can therefore be opened, click Re-authenticate (intending to replace it with the account selected in the browser), and immediately click Save before authorization completes; the old account/token is persisted and the in-progress authorization is silently discarded by the subsequent sheet dismissal. This violates the flow's implied re-authentication/save ordering and can leave the user believing the newly selected account was saved when it was not. The claim would be false if Save is blocked elsewhere (for example by a parent-level modal action guard) or if saving is explicitly intended to be allowed and documented as retaining the old account while authorization runs.
In Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift around line 212, address this finding:
Raw Google token-endpoint response bodies are embedded in `GoogleDriveError.oauthFailed` for non-200 refresh responses (and account lookup failures). If such an error reaches a File Provider operation, `nsError(from:)` falls through to `error as NSError`, preserving the localized description, while operation handlers also log `error.localizedDescription` with public privacy. A provider response containing an email, tenant/account details, or other diagnostic data can therefore be written to public extension logs and/or returned to File Provider/UI instead of being reduced to a safe generic error.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 815, address this finding:
A persisted or newly saved remote path containing surrounding whitespace is not normalized consistently: the editor preserves it in ConnectionConfig.remotePath, while s3Key trims only '/' characters. For example remotePath " photos/ " produces keys rooted at " photos/ /..." rather than the configured logical prefix "photos/...", breaking enumeration and all file operations.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 418, address this finding:
Fallback classification scrubs the configured bucket before scanning authentication indicators. If the configured bucket is itself an indicator (for example bucket "accessdenied") and the SDK supplies an unstructured authentication error with description "AccessDenied", removing the bucket token leaves no indicator and the error is misclassified as an unreachable endpoint instead of authenticationFailed.
In MFuse/Views/ConnectionEditorSheet.swift around line 637, address this finding:
A whitespace-only (or whitespace-padded) S3 Remote Path is not normalized consistently: the editor persists it because `isEmpty` is false, while the S3 key builder removes only `/`, not whitespace. Thus a field that visually represents an empty/root path (for example `" "`) makes the root listing use prefix `" "`, and `"/photos/ "` targets `photos/ ` rather than `photos`. This means the saved value and the effective S3 prefix do not follow the same blank/path normalization rule.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 419, address this finding:
The fallback scrubber can erase the actual authentication indicator when the bucket or endpoint contains that indicator, causing real credential failures to be classified as unreachable endpoint failures. For example, a bucket named `accessdenied` and an SDK/transport description of `AccessDenied` is transformed to an empty string before `authenticationIndicators` is checked, so `mapConnectionError` returns `.connectionFailed` instead of `.authenticationFailed`; the UI will advise network troubleshooting rather than credential repair. The same collision occurs for endpoint hosts containing `unauthorized` or other indicator words.
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift around line 572, address this finding:
With a non-empty configured remote prefix, `itemInfo(at: .root)` can report the root as a file instead of a directory. For config remotePath `tenant`, if an object exists at key `tenant` (alongside normal keys under `tenant/`), the first HEAD uses `s3Key(root, isDirectory:false)` = `tenant` and returns `.file`; callers then treat the mounted root as a file and directory copy/delete behavior is wrong. Root should be a directory namespace regardless of an exact object at the prefix key. This would be disproven if the backend guarantees that no object can exist at the configured prefix key.
In MFuse/Localizable.xcstrings around line 4846, address this finding:
The changed `editor.warning.cleartextCredentials` translations still describe credentials being stored in a shared configuration file, while the call site now uses the warning for FTP/WebDAV with TLS disabled and English correctly warns that credentials are transmitted in cleartext. A non-English user can therefore miss the active network confidentiality risk and the instruction to enable TLS.
In website/src/lib/i18n.js around line 68, address this finding:
Browser locale discovery ignores supported locales after the first browser language, so the documented browser fallback is not actually preference-aware. For example, with `navigator.languages = ['fr-FR', 'zh-CN']` and no valid query/storage value, `getInitialLocale()` normalizes only `fr-FR` and returns the default `en`, instead of selecting the later supported `zh-CN`. This makes the result depend on an unsupported first entry and violates deterministic supported-locale filtering/precedence when browsers provide an ordered list.
In website/package.json around line 11, address this finding:
The declared `typesafe-i18n` npm script is not executable with the project's frozen dependency set: it runs `typesafe-i18n@5.27.1`, whose generator calls the removed `ts.createProgram` API, against the locked TypeScript 7.0.2. Thus a contributor following the documented `npm run typesafe-i18n` command cannot regenerate translations from the project itself; the README's external TypeScript 5.9.3 workaround confirms the script is knowingly broken. This violates the npm-script/buildability obligation unless the script is removed/disabled or made compatible with the locked toolchain.
In MFuseProvider/FileProviderExtension.swift around line 237, address this finding:
The provider extension chooses its `SharedCredentialStore` sync mode from the live shared setting on every access, while the app's `setSynchronizableEnabled` migrates credentials before persisting the setting. During a mode toggle, a File Provider request can observe the new setting (or an already changed process-shared setting) before the migration is complete and read the target partition, get no credential, and report authentication failure; conversely a token refresh can write into the target partition while the app's transition later rolls back/cleans it. The two processes have no transition marker or mount fencing, so an in-flight provider operation can authenticate against a partition whose migration outcome is not yet committed.
In Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleOAuthProvider.swift around line 164, address this finding:
HTTP response bodies are copied verbatim into user-visible GoogleDriveError descriptions. A non-200 account lookup or non-400/401 token refresh constructs messages such as `...: \(bodyDescription)`, and the File Provider fallback returns the original NSError rather than sanitizing it; a server response containing internal details, echoed request data, or attacker-controlled markup can therefore be logged/displayed through connection and File Provider error handling.
In MFuse/MFuseApp.swift, address this finding:
Startup credential mirroring failures are not surfaced as actionable state. When the primary credential exists but the shared/File Provider copy fails, the app continues startup after logging the snapshot error, then can register/auto-mount domains whose extension cannot read the credential. The user sees a mount/authentication failure rather than a clear credential-sync failure and has no prompted recovery path, leaving the persisted config and credential copies inconsistent.
In MFuseProvider/FileProviderExtension.swift, address this finding:
Google Drive server failures and transport failures are not consistently classified as File Provider server-unreachable errors. For example, a token refresh receiving HTTP 503 throws GoogleDriveError.oauthFailed, and a URLSession network error from GoogleDriveFileSystem is rethrown unchanged; nsError(from:) only translates RemoteFileSystemError and otherwise returns the original NSError. Consequently File Provider receives a non-NSFileProviderErrorDomain error (and in the OAuth case the Google response text), so the provider layer cannot reliably surface these as server/network failures.
In MFuse/Views/SidebarView.swift around line 70, address this finding:
Sidebar Unmount All still has a snapshot race with a concurrent Mount All. Each action independently filters `connectionManager.connections` before its task group runs; if Unmount All evaluates while a connection is still unmounted and Mount All then admits/connects it, the unmount snapshot omits it and the newly started mount completes after the batch, leaving a mounted connection despite the user's later Unmount All request. The effective-state helper only fixes tasks already present at the filter instant, not this admission race.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift, address this finding:
Legacy migration is only attempted when the current access-group/mode has no item; if a current item exists while an old-access-group item remains in the other local or synchronizable partition, reads return the current credential and never probe or remove the stale legacy item.
In Packages/MFuseGoogleDrive/Sources/MFuseGoogleDrive/GoogleDriveFileSystem.swift, address this finding:
After an expired access token, every refresh failure during a normal file operation is converted to `RemoteFileSystemError.authenticationFailed`, including transient network errors and Google token-endpoint 5xx responses. Thus a temporary outage while refreshing a token causes File Provider to return `notAuthenticated` (and prompt/recover as if the user must sign in) rather than a retryable/server-unreachable error. This differs from `connect()`, which rethrows non-400/401 OAuth failures, so the same refresh failure is mapped inconsistently depending on when it occurs.
In MFuse/Views/ConnectionEditorSheet.swift, address this finding:
The app localization catalog does not contain the OAuth editor keys used by ConnectionEditorSheet, so those calls always fall back to English rather than resolving the target catalog (and cannot provide the promised non-English translations).
In MFuse/Views/ConnectionDetailView.swift around line 118, address this finding:
When a connection is mounting, ConnectionDetailView replaces the mount button with an unlabeled `ProgressView`. VoiceOver therefore receives no meaningful control/status label for the in-progress lifecycle state (unlike MenuBarView and SidebarView, which explicitly label their mounting indicators), so a user navigating the detail header cannot determine that mounting is in progress or what the control represents. This would be disproven if the enclosing header or platform accessibility automatically exposed this ProgressView as a localized mounting status on supported macOS versions.
## Previously reported and still present (6)
In MFuse/Services/DomainManager.swift around line 130, address this finding:
Startup reconciliation can disconnect a mount explicitly started after its active-state check, because the check and provider disconnect are separated by an un-fenced async boundary.
In Packages/MFuseCore/Sources/MFuseCore/Connection/ConnectionManager.swift around line 978, address this finding:
The shutdown deadline does not cancel the actual per-connection teardown task, so shutdown can return while disconnect continues and later publishes state or touches provider resources after shutdown.
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift around line 140, address this finding:
An existing but unreadable legacy cleartext credential file is treated as if it does not exist, so a read returns nil successfully and leaves the legacy secret file in place without surfacing cleanup/migration failure.
In Packages/MFuseCore/Sources/MFuseCore/Localization/MFuseCoreL10n.swift around line 56, address this finding:
MFuseCore's explicit Chinese fallback still fails for valid regional Chinese identifiers not on its allowlist, such as `zh-MY`: candidates become `zh-MY`, `zh-MY`, and bare `zh`, but the package ships only `zh-Hans.lproj` and `zh-Hant.lproj`, so the helper returns the English fallback instead of selecting a Chinese resource. This violates regional Chinese resolution unless `zh-MY` is explicitly unsupported.
In MFuse/Services/ConnectionManager+Finder.swift around line 94, address this finding:
Reveal can still open a mount after the user has completed Unmount. `resolveFinderURL` performs its final `canRevealMount` check and returns a URL, but the caller then yields back to the main actor before `NSWorkspace.activateFileViewerSelecting` runs. A disconnect can remove the symlink/domain and publish unmounted in that gap; the already-returned URL is nevertheless activated, so Reveal can take the user to a stale/nonexistent Cloud Storage location after unmount. This would be disproven only if disconnect were serialized through activation or the returned URL carried a lifecycle token that the activation path revalidated.
In MFuse/Views/ConnectionDetailView.swift, address this finding:
The detail view's mounting control is an unlabeled icon-free ProgressView. While a connection is mounting, `mountButton` replaces the labeled Mount/Unmount button with `ProgressView()` without a help string or accessibility label, so VoiceOver and other accessibility clients cannot identify the lifecycle state/control, contrary to the surrounding controls' explicit labels.
📜 Review details
Model
- gpt-5.6-luna
Coverage
- 7 of 7 areas reviewed
| // the old config paired with it — or a credential behind for a mount | ||
| // that was never created. Reported alongside the primary failure rather | ||
| // than swallowed: only the user can put that right. | ||
| let rollbackFailure = await restoreCredential( |
There was a problem hiding this comment.
🔀 Concurrency | 🟠 High
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In MFuse/Views/ContentView.swift, address this finding:
A save can restore an obsolete credential onto a newer config revision after its config update loses a race.
| // while the probe was still running left the probe to them, so the connection | ||
| // is established and published and only this caller no longer wants it — | ||
| // reporting the cancellation is what stops it acting on one it never took. | ||
| let resumedJoiners = resumeConnectWaiters(of: task, with: .success(())) |
There was a problem hiding this comment.
🔍 Resource Lifetime | 🟠 High
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In Packages/MFuseS3/Sources/MFuseS3/S3FileSystem.swift, address this finding:
A cancelled starter can leave the AWS client published after the only remaining joiner has also cancelled.
| fallback: "Domain Sync Issue" | ||
| ), | ||
| message: error.localizedDescription | ||
| message: AppL10n.string( |
There was a problem hiding this comment.
🎯 Correctness | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In MFuse/Views/ContentView.swift, address this finding:
Save-time domain registration failures are only shown in a dismissible alert and are not retained for reconciliation or retry in the running app.
| useDataProtectionKeychain: true | ||
| ) | ||
| for legacySyncMode in legacySyncModes { | ||
| try? deleteKeychainData( |
There was a problem hiding this comment.
🔒 Security | 🟡 Medium
🧩 Analysis
- Change relation: introduced
- Confirmation: independently-verified
- Reachable: ✅
🤖 Prompt for AI agents
In Packages/MFuseCore/Sources/MFuseCore/Shared/SharedCredentialStore.swift, address this finding:
Legacy Keychain cleanup failures are silently discarded after a successful write, so an unreadable/unremovable item in a legacy access-group or either sync partition can remain while the caller is told the credential was stored successfully.
📝 Committable suggestion — review it before committing; it is generated, not proven.
Suggested change
| try? deleteKeychainData( | |
| try deleteKeychainData( |
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.
Cloudflare Pages has been failing on
mainsince #25 raisedtypescriptto^7.0.2:TypeScript 7 ships no stable programmatic API, so typesafe-i18n 5.27.1 cannot transpile the locale files. Upstream tracks this in codingcommons/typesafe-i18n#794, with a fix proposed in #795 — neither is merged, and 5.27.1 is still the latest release. The workaround suggested in that issue patches typesafe-i18n to run locale files through the Bun runtime and is explicitly documented as "only works when used with bun", which does not apply here because Cloudflare installs dependencies with npm.
What this changes
Drops the generator from the build. All eight files it produces are committed, and it reports
all files are up to dateon every run, so the build never needed it. Runbun run typesafe-i18nmanually after changing translations.Installs through bun during the build. Cloudflare detects only npm from the environment and runs
npm install, then builds withbun run build.bun.locktherefore never applied and dependency versions were free to drift between local and deployed builds — which is how TypeScript 7 reached CI in the first place.Verification
Reproduced the failure with a clean install (the local
node_modulesstill held TypeScript 6.0.3, which masked it), then verified the fix by replaying Cloudflare's exact sequence —npm installfollowed bybun run build:To confirm the lockfile is genuinely enforced rather than incidentally correct, installed a mismatched vite with npm and watched the build correct it:
Caveat
This unblocks the build; it does not repair the generator.
bun run typesafe-i18nstill fails under TypeScript 7, so regenerating after a translation change currently requires temporarily installing typescript 6. Reverting this commit is the right move once upstream lands #795.Summary by CodeRabbit
Summary
Changes