fix: use sandboxed paths for Mac App Store build#592
Merged
Conversation
jfversluis
force-pushed
the
fix/maccatalyst-sandbox-paths
branch
2 times, most recently
from
3667dff to
48ea2c0
Compare
Owner
🔍 Multi-Model Code Review — PR #592 (Re-review after fix)
CI Status: Previous Finding Status
Current State AssessmentThe fix commit correctly resolves both critical data-loss issues by removing the What the PR now delivers:
Remaining low-severity items (non-blocking):
Test Results
Recommendation: ✅ ApproveAll critical and moderate issues from the previous review are resolved. The remaining items are low-severity, non-blocking improvements that can be addressed in follow-up PRs. The approach of trusting sandbox HOME remapping is correct, simple, and eliminates path-mismatch bugs. |
- Create PlatformPaths.GetPolyPilotDirForMacCatalyst() that returns FileSystem.AppDataDirectory/.polypilot/ for sandboxed Mac Catalyst, null for all other platforms - Update CopilotService and ConnectionSettings to check for sandbox path first, then fall back to platform-specific defaults - Remove both /.polypilot/ and /.copilot/ temporary-exception entitlements from Entitlements.AppStore.plist This change is Mac App Store-specific only. iOS, Android, and Developer ID builds use their existing path logic unchanged. The sandbox automatically remaps HOME to ~/Library/Containers/<bundle-id>/Data/, so UserProfile-based paths in the bundled copilot CLI will resolve inside the container. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address review feedback on PR #592: the initial commit only updated 2 of 12+ path sites, creating a split-brain where some services used the sandbox container while others used the home directory. Now ALL services route through PlatformPaths.GetPolyPilotDirOverride(): - CopilotService, ConnectionSettings (already done) - ServerManager, PromptLibraryService, ScheduledTaskService - ChatDatabase, AuditLogService, RepoManager - PluginLoader, ShowImageTool, PluginFileLogger - MauiProgram (crash log), App.xaml.cs (pending navigation) - MacCatalyst/Program (instance lock, navigation sidecar) - MacCatalyst/NotificationManagerService (pending navigation) Added PlatformPaths.SetForTesting() for test isolation. Added PlatformPaths.GetCopilotDirOverride() (returns sandboxnull remaps HOME so ~/.copilot/ resolves inside the container already). No migration needed: this is the first Mac App Store release. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jfversluis
force-pushed
the
fix/maccatalyst-sandbox-paths
branch
from
0f7729f to
85ba5ca
Compare
…aDirectory On Mac Catalyst, the sandbox remaps HOME (UserProfile) into ~/Library/Containers/<bundle-id>/Data/, so ~/.polypilot/ already resolves inside the container — same as ~/.copilot/. Using FileSystem.AppDataDirectory added an extra /Library/ segment, creating a path mismatch with the migration in MigrateLegacyDataIfNeeded() and causing silent data loss for upgrading users. Remove the MACCATALYST-specific FileSystem.AppDataDirectory override. GetPolyPilotDirOverride() now returns null on all platforms (test overrides still work via SetForTesting). All 12+ callsite overrides remain and correctly fall through to existing UserProfile-based logic. Also add missing PlatformPaths.cs Compile Include in test csproj. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove dead GetCopilotDirOverride() and _testCopilotDir (no callers) - Rename GetImagesDir_() to ComputeImagesDir() for clarity - Rename GetPluginsDir() to ComputePluginsDir() for consistency - Add ResetCachedPathForTesting() to PluginLoader and ShowImageTool so PlatformPaths.SetForTesting() invalidates their static caches - Wire PlatformPaths.SetForTesting() into TestSetup.cs ModuleInitializer - Update TestSetup isolation doc comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
2 participants
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.
Implement platform-specific path resolution:
The sandbox automatically remaps HOME to ~/Library/Containers//Data/, so the bundled copilot CLI (with inherit: true in helper entitlements) will resolve UserProfile paths inside the container. This allows the app to work without requiring broad home-directory entitlements that Apple rejects.