This repository was archived by the owner on May 24, 2026. It is now read-only.
fix: restore Direct Sharing persistence on Mac Catalyst#377
Merged
Conversation
PR #341 moved ServerPassword, RemoteToken, and LanToken to SecureStorage (Keychain) on Mac Catalyst via [JsonIgnore] and #if IOS || ANDROID || MACCATALYST guards. However, Mac Catalyst runs without app sandbox (disabled in Entitlements.plist), making Keychain unreliable. The password was silently lost on restart, so StartDirectSharingIfEnabled() would skip due to empty password. Fix: - Change SecureStorage guards from IOS || ANDROID || MACCATALYST to IOS || ANDROID only — Mac Catalyst is a desktop platform - Add one-time RecoverSecretsFromSecureStorage() for MACCATALYST to recover any passwords already migrated to Keychain by PR 341 - Only clean up Keychain entries after verifying JSON was written - Add 4 regression tests for secret serialization on desktop Verified via MauiDevFlow: enabled Direct Sharing, relaunched app, confirmed bridge auto-started with 'Stop Direct Sharing' visible. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
Multi-Model Consensus Review (5-model × 5-agent)CI Status:
|
…failure Addresses PR review finding #1 (critical): if ReadSecureStorage fails transiently for one secret but succeeds for another, the blanket SecureStorage.Remove() would destroy the unrecovered secret. Now each Keychain entry is only removed if that specific value was successfully recovered. Also removes the no-op verify.Contains() guard (finding #2) since per-key tracking makes it unnecessary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
Multi-Model Consensus Re-Review -- Round 2 (5-model dispatch)Tests: 2,575 passed, 0 failed ✅CI:
|
| # | Finding | Status |
|---|---|---|
| 1 | 🔴 Unconditional Keychain wipe (verify.Contains always true) |
✅ FIXED -- per-key recovered* booleans gate each Remove() |
| 2 | 🟡 Task.Run(...).GetAwaiter().GetResult() deadlock risk |
Task.Run mitigates SyncContext deadlock) |
| 3 | 🟡 Verification guard was a no-op | ✅ FIXED -- removed entirely |
| 4 | 🟢 #if MACCATALYST untestable |
N/A -- inherent limitation, accepted |
New Findings (consensus 2+ models)
| Sev | File:Line | Description |
|---|---|---|
| 🟡 | ConnectionSettings.cs:316 |
File.Exists(SettingsPath) is a weak save-success proxy. Save() swallows all exceptions. If Save() fails but a prior settings file exists, File.Exists returns true and Keychain entries are deleted without persisting recovered values. Narrow edge case (requires disk failure + pre-existing file). Fix: have Save() return bool, or re-read file to confirm secrets present. |
| 🟢 | ConnectionSettings.cs:327 |
Outer catch { } silently swallows all failures -- no log or diagnostic trace for migration failures. |
| 🟢 | ConnectionSettings.cs:332 |
Sync-over-async blocks main thread 3× per Load() -- not a deadlock but causes UI jank during one-time migration. |
Verdict: ✅ Approve (with tracked follow-ups)
The CRITICAL data-loss bug is properly fixed with a clean per-key recovery+cleanup design. The remaining File.Exists weakness is a narrow edge case. The Task.Run pattern is pre-existing and used identically in iOS/Android. Ship-worthy -- both MODERATEs can be tracked follow-ups.
Review by PR Review Squad (5-model consensus: claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex)
arisng
pushed a commit
to arisng/PolyPilot
that referenced
this pull request
Apr 4, 2026
## Problem PR PureWeen#341 moved `ServerPassword`, `RemoteToken`, and `LanToken` to SecureStorage (Keychain) on Mac Catalyst via `[JsonIgnore]` and `#if IOS || ANDROID || MACCATALYST` guards. However, Mac Catalyst runs **without app sandbox** (disabled in `Entitlements.plist`), making Keychain unreliable. The password was silently dropped from `settings.json` on save, and never reliably recovered from Keychain on load, so `StartDirectSharingIfEnabled()` would skip due to empty password. **Result:** Direct Sharing was always disabled after every restart despite being enabled by the user. ## Fix 1. **Changed SecureStorage guards** from `#if IOS || ANDROID || MACCATALYST` → `#if IOS || ANDROID` for property definitions, `Save()`, and `Load()` — Mac Catalyst is a desktop platform and should use plain JSON like Windows. 2. **Added one-time reverse migration** (`RecoverSecretsFromSecureStorage`) for `MACCATALYST` to recover any passwords already migrated to Keychain by PR PureWeen#341. Only cleans up Keychain entries after verifying JSON was successfully written (addresses code review finding about data loss if `Save()` fails). 3. **Added 4 regression tests** validating that secret fields serialize to JSON on desktop platforms. ## Verification - ✅ All 2575 tests pass - ✅ Built and relaunched via `relaunch.sh` - ✅ MauiDevFlow CDP verified: enabled Direct Sharing → relaunched → `Stop Direct Sharing` button visible (bridge auto-started) - ✅ `settings.json` confirmed: `ServerPassword` present and `DirectSharingEnabled: true` persisted across restart --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions Bot
added a commit
that referenced
this pull request
Apr 28, 2026
Three follow-up items from the PR #377 consensus review: 1. Save() now returns bool (true on success, false on failure). The Keychain cleanup in RecoverSecretsFromSecureStorage is gated on Save()'s return value instead of File.Exists(SettingsPath), which was unsafe: a prior settings file could exist even if the current Save() failed (e.g., disk full), causing Keychain entries to be deleted without the recovered values persisted. 2. The outer catch {} in RecoverSecretsFromSecureStorage now logs to Debug.WriteLine and appends to ~/.polypilot/crash.log, matching the existing crash-log convention. 3. ReadSecureStorage gets a doc comment explaining the intentional sync-over-async Task.Run pattern (one-time migration, not hot path). Unit tests added for Save() return value behavior. Integration test added for Settings page navigation. Fixes #379 Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
pushed a commit
that referenced
this pull request
Apr 29, 2026
Three follow-up hardening items from the PR #377 consensus review (5-model review squad). The core risk: `Save()` swallowed all exceptions silently, `File.Exists()` was used as a proxy for write success, and migration failures were invisible. ## Changes ### 1. `Save()` returns `bool` (MODERATE — data-loss prevention) `Save()` now returns `true` on successful write, `false` on any exception. Callers that ignore the return value are unaffected (backward compatible — `void` callers still compile). The Keychain cleanup in `RecoverSecretsFromSecureStorage` is now gated on `Save()`'s return value instead of `File.Exists(SettingsPath)`: ```csharp // Before — weak proxy: old file can exist even if this Save() failed settings.Save(); if (File.Exists(SettingsPath)) { /* delete keychain entries */ } // After — direct success signal bool saved = settings.Save(); if (saved) { /* delete keychain entries */ } ``` This prevents data loss when `Save()` fails (disk full, permissions error) but an older settings file already exists on disk. ### 2. Migration failures logged (LOW — observability) The outer `catch {}` in `RecoverSecretsFromSecureStorage` now captures the exception and: - Writes to `Debug.WriteLine` for IDE output - Appends to `~/.polypilot/crash.log` matching the existing crash-log convention Previously, a partial migration failure was completely silent — no diagnostic trace. ### 3. Sync-over-async comment (LOW — documentation) `ReadSecureStorage` now has a doc comment explaining the intentional `Task.Run(...).GetAwaiter().GetResult()` pattern: it avoids `SynchronizationContext` deadlock, runs only during the one-time migration on `Load()`, and the tradeoff is acceptable vs. making `Load()` async. ## Tests - **Unit tests**: 3 new tests for `Save()` return value behavior (`Save_ReturnsTrue_OnSuccess`, `Save_ReturnsFalse_OnFailure`, `Save_ReturnsFalse_WhenPathIsReadOnly`) - **Integration test**: `SettingsPersistenceTests` — navigates to Settings page, verifies it renders correctly - All 3579 unit tests pass ✅ - Integration tests build successfully ✅ ## Files Changed | File | Change | |------|--------| | `PolyPilot/Models/ConnectionSettings.cs` | `Save()` → `bool`, migration logging, sync-over-async doc | | `PolyPilot.Tests/ConnectionSettingsTests.cs` | 3 new tests for Save() return value | | `PolyPilot.IntegrationTests/SettingsPersistenceTests.cs` | New integration test for Settings page | - Fixes #379 > [!WARNING] > <details> > <summary><strong>⚠️ Firewall blocked 1 domain</strong></summary> > > The following domain was blocked by the firewall during workflow execution: > > - `192.0.2.1` > > To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "192.0.2.1" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details> > Generated by [Agent Fix](https://github.com/PureWeen/PolyPilot/actions/runs/25066485519/agentic_workflow) for issue #379 · ● 15.6M · [◷](https://github.com/search?q=repo%3APureWeen%2FPolyPilot+%22gh-aw-workflow-id%3A+agent-fix%22&type=pullrequests) <!-- gh-aw-agentic-workflow: Agent Fix, engine: copilot, model: claude-opus-4.6, id: 25066485519, workflow_id: agent-fix, run: https://github.com/PureWeen/PolyPilot/actions/runs/25066485519 --> <!-- gh-aw-workflow-id: agent-fix --> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> 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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Problem
PR #341 moved
ServerPassword,RemoteToken, andLanTokento SecureStorage (Keychain) on Mac Catalyst via[JsonIgnore]and#if IOS || ANDROID || MACCATALYSTguards. However, Mac Catalyst runs without app sandbox (disabled inEntitlements.plist), making Keychain unreliable. The password was silently dropped fromsettings.jsonon save, and never reliably recovered from Keychain on load, soStartDirectSharingIfEnabled()would skip due to empty password.Result: Direct Sharing was always disabled after every restart despite being enabled by the user.
Fix
Changed SecureStorage guards from
#if IOS || ANDROID || MACCATALYST→#if IOS || ANDROIDfor property definitions,Save(), andLoad()— Mac Catalyst is a desktop platform and should use plain JSON like Windows.Added one-time reverse migration (
RecoverSecretsFromSecureStorage) forMACCATALYSTto recover any passwords already migrated to Keychain by PR Improve bridge startup reliability and token validation #341. Only cleans up Keychain entries after verifying JSON was successfully written (addresses code review finding about data loss ifSave()fails).Added 4 regression tests validating that secret fields serialize to JSON on desktop platforms.
Verification
relaunch.shStop Direct Sharingbutton visible (bridge auto-started)settings.jsonconfirmed:ServerPasswordpresent andDirectSharingEnabled: truepersisted across restart