fix: improve error diagnostics, log exposure, and session error display (issues #420, #424, #425)

[PureWeen]

Summary

Fixes three issues filed by @roji against PolyPilot v1.0.11 (installed via Homebrew).

Fixes #420, Fixes #424, Fixes #425

---

Issue #420 — Crash on macOS 15.7.4 (FoundationModels missing)

Root cause: Microsoft.Maui.Essentials.AI v10.0.50-ci.main.26126.2 bundles a native framework with a hard (LC_LOAD_DYLIB) dependency on FoundationModels.framework, which only exists on macOS 26+. On macOS 15.x, dyld aborts at launch before any managed code runs.

Fix: Updated Microsoft.Maui.Essentials.AI to v10.0.50-ci.main.26157.2, which links FoundationModels as LC_LOAD_WEAK_DYLIB — the app loads on older macOS and gracefully degrades (PolyPilot doesn't call any EssentialsAI APIs directly).

---

Issue #424 — "Failed to start server" with no useful error info

Changes:

• ServerManager.StartServerAsync now captures stderr into a LastError property instead of discarding it silently
• IServerManager exposes string? LastError { get; }
• All 8 FallbackNotice error assignments in CopilotService now use a shared BuildServerFallbackNotice(error, logPath, reason) helper that consistently surfaces: the error reason, the actual error text (when available), the path to ~/.polypilot/event-diagnostics.log, and Settings guidance
• Settings page StartServer() button now shows the actual error with a 10s dismiss (e.g. "Failed to start server: Port 4321 already in use") instead of the generic message
• Added "View Logs" button in both the Dashboard fallback banner and the Settings Persistent Server section — opens ~/.polypilot/ in Finder/Explorer

---

Issue #425 — Session creation error hidden on first attempt

Root cause: CreateSessionForm.TriggerCreate() reset isExpanded = false immediately after OnCreate.InvokeAsync(). Blazor parameter updates (CreateError) arrive in the next render cycle, so the form collapsed before the error could be shown. On the second click, the persisted error state became visible.

Fix:

• Removed auto-collapse from TriggerCreate()
• Added public void CollapseOnSuccess() method to CreateSessionForm
• SessionSidebar.HandleCreateSession and HandleCreateGroup now call CollapseOnSuccess() only after verifying success — error leaves the form open and visible immediately

---

Tests

• 4 new unit tests for BuildServerFallbackNotice: verifies error text, log path, empty-error handling, and custom reason string
• Fixed pre-existing flaky test race (UrgencySortTests missing [Collection("BaseDir")] attribute)
• All 2927 tests pass

[PureWeen]

🔍 PR Review Squad — Round 1

PR Summary

Three-fix PR addressing reported issues:

• PolyPilot crashes after installation on MacOS 15.7.4 with library missing (FoundationModels) #420: Bump Microsoft.Maui.Essentials.AI 26126.2 → 26157.2 (weak link for FoundationModels.framework, fixes macOS 15 crash) ✅
• "Failed to start server" on startup without any clarification or indication where to get more info #424: ServerManager.LastError captures stderr; BuildServerFallbackNotice() helper consolidates all 8 FallbackNotice assignments
• Can't create session: "Service not initialized. Call InitializeAsync first." #425: CreateSessionForm no longer auto-collapses after OnCreate.InvokeAsync(); parent calls CollapseOnSuccess() on success only

CI Status

⚠️ No checks configured on branch

Build & Tests

• ✅ Build: 0 errors
• ✅ BuildServerFallbackNotice unit tests pass (4 new)
• ✅ UrgencySortTests flaky fix ([Collection("BaseDir")]) verified

Findings

#	Severity	Location	Description	Consensus
N1	🟡 MODERATE	CopilotService.cs:905,1202,1227,1299,1320	BuildServerFallbackNotice() template always says "fell back to Embedded mode. Your sessions won't persist across restarts." — but 5 of 7 call sites do NOT switch to Embedded mode. Lines 905/1202/1227/1299/1320 set FallbackNotice while remaining in Persistent mode with IsInitialized = false. Only lines 845 and 914 actually do CurrentMode = ConnectionMode.Embedded. This misleads users into thinking their sessions are running in a degraded-but-alive Embedded fallback when the server is actually down.	4/5
N2	🟢 MINOR	Dashboard.razor:OpenLogsFolder	No else branch for Linux/GTK — silently does nothing on non-Mac/non-Windows. Settings.OpenLogsFolder already has an else { ShowStatus("Logs at: ...", ...) } fallback. Copy that pattern to Dashboard.	4/5
N3	🟢 MINOR	ServerManager.cs:130	string.Join("\n", stderrLines) is called immediately after the 15s timeout without awaiting the stderr drain task (t2). The ConcurrentQueue is thread-safe (no crash), but late-arriving stderr lines may be missing from LastError. Acceptable for best-effort diagnostics — the most common error ("port in use") is typically written immediately at process start.	4/5

Cleared Concerns

• Form field reset regression: ✅ NOT a bug — Collapse() (line 243) explicitly resets branchInput, prInput, sessionDirectory, initialPrompt, selectedWorktreeId, and nameManuallyEdited. All 5 models confirmed.
• CollapseOnSuccess() null-safety: ✅ createSessionFormRef?.CollapseOnSuccess() null-conditional is correct.
• Process disposal: ✅ Task.WhenAll(t1, t2).ContinueWith(_ => process.Dispose()) correctly disposes the OS handle when both streams close, while the detached server process keeps running.

Suggested Fix for N1

The simplest fix is to split the template — the reason parameter already differentiates the cases, so the "fell back to Embedded mode" clause can be conditional:

internal static string BuildServerFallbackNotice(string? serverError, string logPath, string reason = "couldn't start", bool embeddedFallback = true)
{
    var detail = string.IsNullOrEmpty(serverError) ? "" : $"\n\nError: {serverError}";
    var fallbackClause = embeddedFallback
        ? " — fell back to Embedded mode. Your sessions won't persist across restarts."
        : " — reconnection failed.";
    return $"Persistent server {reason}{fallbackClause}{detail}\n\nLogs: {logPath}\n\nGo to Settings → Save & Reconnect to fix.";
}

Then pass embeddedFallback: false at the 5 non-Embedded call sites.

Test Coverage

4 new unit tests cover BuildServerFallbackNotice. No test currently covers the "Embedded vs non-Embedded" message distinction — worth adding one per the fix above.

Verdict: ⚠️ Request changes

N1 is a genuine semantic bug in the error message for 5 of 7 call sites. N2 and N3 are non-blocking minor nits.

[PureWeen]

PR #427 -- Round 2 Re-Review (Aggregated)

New commit since Round 1: 0574dc30 -- "fix: address PR review feedback"

Round 1 Findings Status

#	Finding	Status
M1 🟡	BuildServerFallbackNotice misleading "fell back to Embedded mode" at 5/7 non-Embedded call sites	✅ FIXED -- new embeddedFallback param; 5 non-Embedded sites pass false, 2 actual fallback sites use true
M2 🟡	Dashboard.OpenLogsFolder missing Linux/GTK branch	✅ FIXED -- added else branch showing log path inline for 8s
N1 🟢	stderr snapshot race in ServerManager.StartServerAsync	✅ FIXED -- added await Task.WhenAny(t2, Task.Delay(500)) grace window

Build & Tests

• Mac Catalyst: ✅ 25 warnings, 0 errors
• Tests: ✅ All pass (6 new BuildServerFallbackNotice tests including EmbeddedFallback True/False variants)

New Findings: None

All Round 1 issues addressed correctly. No new issues found.

Verdict: ✅ Approve

Review by PR Review Squad (5 workers, multi-model consensus)

[PureWeen]

PR #427 — Round 3 Review

New commit since Round 2 (✅ Approve): 2ce12dd6 — "fix: add 5 MB rotation to crash.log to prevent unbounded growth"

New Commit Analysis

The new commit adds 8 lines to LogException() in MauiProgram.cs to rotate crash.log at 5 MB (move to .old, one backup kept). This matches the existing pattern used by plugin.log (5 MB rotation) and event-diagnostics.log (10 MB delete).

Potential edge cases checked:

• Concurrent rotation: LogException is called from UnhandledException/UnobservedTaskException handlers (background threads). If two fire simultaneously, one File.Move succeeds and one fails silently — AppendAllText then creates a new file. No crash, no data loss.
• Failed File.Delete on backup: If the .old file is locked, File.Move also fails silently, and the log continues growing past 5 MB. Acceptable "best effort" behavior — identical to the plugin.log pattern.
• new FileInfo(CrashLogPath) overhead: Negligible since crash logging is rare.

Round 1 + Round 2 Findings: All ✅ FIXED (from prior reviews)

#	Finding	Status
M1 🟡	BuildServerFallbackNotice misleading "fell back to Embedded mode" at 5/7 non-Embedded sites	✅ FIXED
M2 🟡	Dashboard.OpenLogsFolder missing Linux/GTK branch	✅ FIXED
N1 🟢	stderr snapshot race in ServerManager.StartServerAsync	✅ FIXED

New Findings: None

CI: No checks configured on branch.

Verdict: ✅ Approve

New commit is clean and consistent with existing log management patterns.

Review by PR Review Squad — Worker 5

[PureWeen]

PR #427 — Round 4 Re-Review

New commits since Round 3 (✅ Approved):

• 6aa617e8 — move View Logs from Dashboard FallbackNotice banner → new Settings Diagnostics section
• 4d9dde84 — add Diagnostics nav item to Settings sidebar

Tests: ✅ 2929 passing, 0 failing
CI: No checks configured on branch

---

Prior Findings Status

#	Finding	Status
N1 🟡	BuildServerFallbackNotice misleading "Embedded mode" at 5/7 non-Embedded sites	✅ FIXED
N2 🟡	Dashboard.OpenLogsFolder missing Linux/GTK branch	✅ FIXED (removed from Dashboard; Settings version has else branch)
N3 🟢	stderr snapshot race in ServerManager.StartServerAsync	✅ FIXED

---

New Findings

🟢 MINOR — console.log not created on Windows (5/5 models)

Settings.razor — Diagnostics log file guide

The guide lists console.log as "General app output — verbose debug logging from the running app." This file is created by relaunch.sh which redirects nohup ... > ~/.polypilot/console.log 2>&1. relaunch.sh is a macOS-only bash script — Windows users launch differently. The Diagnostics section is already PlatformHelper.IsDesktop-gated, but Windows is desktop. A user on Windows following this guide won't find console.log.

Suggested fix: Add "(macOS only — written by relaunch.sh)" qualifier, or conditionally render that list item only on macOS.

---

🟢 MINOR — GoToSettings() indentation regression (4/5 models)

Dashboard.razor:714

The method declaration has 8-space leading indent while its body and all surrounding methods use 4-space. Introduced as a diff artifact of the OpenLogsFolder removal. Cosmetic only — compiles fine.

---

🟢 MINOR (not consensus) — Scroll-spy ids missing settings-diagnostics

Settings.razor:812

var ids = ['settings-connection','settings-cli','settings-ui','settings-developer'];

settings-diagnostics is not observed by the IntersectionObserver, so the sidebar nav won't auto-highlight "Diagnostics" when the user scrolls there manually. However, this is pre-existing behavior — settings-plugins is also absent from this list on main. Clicking the nav item works correctly (explicit activeCategory = "diagnostics" assignment). Not new debt.

---

Not Flagged / Confirmed Clean

• UX: View Logs removed from banner — 3/5 models accepted this. The FallbackNotice text already includes the log path inline (Logs: ~/.polypilot/event-diagnostics.log), and the "Settings" button provides navigation. Acceptable tradeoff. ✅
• Form field reset regression (Can't create session: "Service not initialized. Call InitializeAsync first." #425 fix) — Collapse() resets all fields; CollapseOnSuccess() correctly delegates. ✅
• crash.log rotation (5 MB) — consistent with plugin.log pattern. ✅
• BuildServerFallbackNotice helper — correct embeddedFallback usage at all 7 call sites. ✅
• Microsoft.Maui.Essentials.AI bump — weak link fix confirmed. ✅

---

Verdict: ✅ Approve

Two new commits are clean. The console.log documentation inaccuracy on Windows is the only consensus finding and it's minor — the file just won't be there, which is harmless. All substantive prior findings remain fixed. Good to merge.

Review by PR Review Squad — Round 4 (5 workers: 2× claude-opus-4.6, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex)