Skip to content

Follow-up: ConnectionSettings secret migration hardening #379

@PureWeen

Description

@PureWeen

Context

PR #377 (fix: restore Direct Sharing persistence on Mac Catalyst) was merged with ✅ Approve. The CRITICAL data-loss bug (unconditional Keychain wipe) was fixed. These are tracked follow-up items from the multi-model consensus review.

Remaining Items

1. File.Exists(SettingsPath) is a weak save-success proxy (MODERATE)

File: ConnectionSettings.cs:316
Save() swallows all exceptions (catch { }). If Save() fails but a prior settings file exists (e.g., disk full), File.Exists returns true and Keychain entries are deleted without persisting recovered values.
Fix: Have Save() return bool, or re-read the file to confirm secrets are present before deleting Keychain entries.

2. Task.Run(...).GetAwaiter().GetResult() sync-over-async (LOW)

File: ConnectionSettings.cs:332
ReadSecureStorage blocks the main thread 3× per Load() call. Task.Run avoids the classic SynchronizationContext deadlock, but still causes UI jank during one-time migration. This is a pre-existing pattern also used in iOS/Android.
Fix: Make RecoverSecretsFromSecureStorage async, or add a comment explaining the intentional sync-over-async pattern.

3. Silent exception swallowing in migration (LOW)

File: ConnectionSettings.cs:327
The outer catch { } in RecoverSecretsFromSecureStorage silently swallows all failures. If recovery partially succeeds then fails, there's no log or diagnostic trace.
Fix: Add Debug.WriteLine or log to ~/.polypilot/crash.log on catch.

Origin

Identified by PR Review Squad (5-model consensus: claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex) during review of PR #377.

Metadata

Metadata

Labels

No labels
No labels

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions