fix: ConnectionSettings secret migration hardening

[Copilot]

Three follow-up hardening items from the PR #377 consensus review. The core risk: Save() swallowed all exceptions silently, File.Exists() was used as a proxy for write success, and migration failures were invisible.

Changes

• Save() returns bool — true on successful write, false on any exception. Callers that ignore the return value are unaffected.

• Keychain cleanup now gated on Save() success — replaces the File.Exists(SettingsPath) check, which was unsafe: if Save() failed (disk full, permissions, etc.) but an older file existed, File.Exists would return true and Keychain entries would be deleted without the recovered values persisted.

  // 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 */ }

• Migration failures logged — outer catch {} in RecoverSecretsFromSecureStorage changed to catch (Exception ex) with Debug.WriteLine + append to ~/.polypilot/crash.log, matching the existing crash-log convention.

• Sync-over-async comment — ReadSecureStorage gets an explanatory comment on the intentional Task.Run(...).GetAwaiter().GetResult() pattern (avoids SynchronizationContext deadlock; acceptable for a one-time migration path).

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• 4myvsblobprodcus32.vsblob.vsassets.io
  • Triggering command: /usr/bin/dotnet dotnet restore --no-dependencies /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/55BB33C8E1046D06DB6BC8FB401555C0/missingpackages_workingdir --packages /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/missingpackages /p:DisableImplicitNuGetFallbackFolder=true --verbosity normal --configfile /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/nugetconfig/nuget.config --force (dns block)
• 4vyvsblobprodcus361.vsblob.vsassets.io
  • Triggering command: /usr/bin/dotnet dotnet restore --no-dependencies /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/55BB33C8E1046D06DB6BC8FB401555C0/missingpackages_workingdir --packages /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/missingpackages /p:DisableImplicitNuGetFallbackFolder=true --verbosity normal --configfile /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/nugetconfig/nuget.config --force (dns block)
• 5dkvsblobprodcus355.vsblob.vsassets.io
  • Triggering command: /usr/bin/dotnet dotnet restore --no-dependencies /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/55BB33C8E1046D06DB6BC8FB401555C0/missingpackages_workingdir --packages /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/missingpackages /p:DisableImplicitNuGetFallbackFolder=true --verbosity normal --configfile /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/nugetconfig/nuget.config --force (dns block)
• 7tjvsblobprodcus341.vsblob.vsassets.io
  • Triggering command: /usr/bin/dotnet dotnet restore --no-dependencies /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/55BB33C8E1046D06DB6BC8FB401555C0/missingpackages_workingdir --packages /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/missingpackages /p:DisableImplicitNuGetFallbackFolder=true --verbosity normal --configfile /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/nugetconfig/nuget.config --force (dns block)
• kijvsblobprodcus387.vsblob.vsassets.io
  • Triggering command: /usr/bin/dotnet dotnet restore --no-dependencies /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/55BB33C8E1046D06DB6BC8FB401555C0/missingpackages_workingdir --packages /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/missingpackages /p:DisableImplicitNuGetFallbackFolder=true --verbosity normal --configfile /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/nugetconfig/nuget.config --force (dns block)
• pc2vsblobprodcus360.vsblob.vsassets.io
  • Triggering command: /usr/bin/dotnet dotnet restore --no-dependencies /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/55BB33C8E1046D06DB6BC8FB401555C0/missingpackages_workingdir --packages /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/missingpackages /p:DisableImplicitNuGetFallbackFolder=true --verbosity normal --configfile /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/nugetconfig/nuget.config --force (dns block)
• se1vsblobprodcus349.vsblob.vsassets.io
  • Triggering command: /usr/bin/dotnet dotnet restore --no-dependencies /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/55BB33C8E1046D06DB6BC8FB401555C0/missingpackages_workingdir --packages /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/missingpackages /p:DisableImplicitNuGetFallbackFolder=true --verbosity normal --configfile /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/nugetconfig/nuget.config --force (dns block)
• vb4vsblobprodcus33.vsblob.vsassets.io
  • Triggering command: /usr/bin/dotnet dotnet restore --no-dependencies /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/8764B2CEA58873189746604E1AFC56D2/missingpackages_workingdir --packages /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/missingpackages /p:DisableImplicitNuGetFallbackFolder=true --verbosity normal --configfile /tmp/codeql-scratch-abbd4936ea2bb08e/dbs/csharp/working/nugetconfig/nuget.config --force (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Follow-up: ConnectionSettings secret migration hardening</issue_title>
<issue_description>## 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.</issue_description>

Comments on the Issue (you are @copilot in this section)

• Fixes Follow-up: ConnectionSettings secret migration hardening #379

---

📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

[PureWeen]

Multi-Model Consensus Review — PR #383 (5 models)

Addresses issue #379 (follow-up from PR #377 consensus review). Changes: Save() returns bool, Keychain cleanup gated on saved instead of File.Exists(), migration failures logged to crash.log, sync-over-async comment added.

CI: ⚠️ No CI configured

Tests: 2589 passed, 3 failures (all pre-existing, pass in isolation — unrelated to this PR's changes)

---

Consensus Findings (2+ models)

Sev	File:Line	Models	Description
🟢	ConnectionSettings.cs:274	3/5	File.WriteAllText is truncate-then-write, not atomic. Save() returning true means no exception was thrown, but a crash or power loss mid-write can produce a corrupt/zero-byte settings.json. In theory, Keychain deletion could proceed against a partially-written file. In practice, .NET raises IOException on most partial-write conditions, so this is largely theoretical on Mac Catalyst. A write-to-temp + File.Move(temp, dest, overwrite:true) would make the stated guarantee fully correct. Non-blocking — the PR is strictly better than before.
🟢
🟢	ConnectionSettingsTests.cs	2/5	No tests cover new code paths: Save() returning false (e.g., read-only path), migration skipping Keychain removal when saved==false, or catch branch writing to crash.log.

Test isolation note (Gemini flagged as HIGH — not a new issue)

GetPolyPilotDir() has always been independent of CopilotService.SetBaseDirForTesting(). The project's existing convention handles this by prohibiting tests from calling ConnectionSettings.Save() or ConnectionSettings.Load() (documented in SessionSidebar.razor instructions). This PR adds a crash.log write via the same pre-existing GetPolyPilotDir(), but only in the #if MACCATALYST migration catch path — which tests don't exercise. Not introduced by this PR.

---

Issues Addressed from #379

#	Item	Status
1	File.Exists(SettingsPath) weak proxy	✅ FIXED — replaced with bool saved = settings.Save()
2	Sync-over-async comment	✅ FIXED — comment added to ReadSecureStorage
3	Silent exception swallowing	✅ FIXED — catch (Exception ex) with Debug.WriteLine + crash.log

---

Recommended Action: ✅ Approve

The core data-loss fix is correct and strictly better than before. All three items from issue #379 are addressed. The consensus findings above are all LOW severity — the File.WriteAllText atomicity concern is theoretical on Mac Catalyst, and the other two are polish items. None are blocking.