feat: Fiesta pairing -- NIC fix, pairing string, LAN push-to-pair

[PureWeen]

What this PR does

Three improvements to Fiesta mode for the 'one PolyPilot to rule them all' multi-devbox scenario:

Feature A: NIC Selection Fix

• \GetPrimaryLocalIpAddress()\ now scores network adapters and skips virtual ones (Docker, Hyper-V, WSL, VMware)
• Prefers physical Ethernet > WiFi, private LAN IPs (192.168.x, 10.x)
• Fixes the Polish UI, Rename Sessions, Markdown Output Support, Queued Messages #1 cause of Fiesta discovery failures on dev machines

Feature B: Pairing String (copy/paste)

• Workers generate a compact \pp+\ pairing string with Copy button in Settings
• Hosts paste the string to auto-link -- no manual URL/token entry
• Works via RDP clipboard, SSH, chat, any text channel

Feature C: LAN Push-to-Pair

• 'Request Pair' button on discovered workers in Settings
• Worker shows Allow/Deny approval banner with 60s countdown
• Token flows automatically on approval -- zero manual entry
• Rate-limited /pair\ WebSocket path (5s cooldown, HTTP-level rejection)

Race condition fixes

• \LinkWorkerAndReturn()\ captures worker inside same lock scope
• Atomic rate-limit via \Interlocked.CompareExchange\ on ticks

[PureWeen]

PR #322 Review — feat: Fiesta pairing — NIC fix, pairing string, LAN push-to-pair

CI Status: No CI checks on this branch
Existing reviews: None
Models: claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex (4/5 completed at synthesis time — unanimous agreement)

---

🟡 MODERATE — ApprovePairRequestAsync: tcs.TrySetResult(true) fires even when SendAsync fails (4/4 consensus)

FiestaService.cs — ApprovePairRequestAsync

try
{
    await SendAsync(pending.Socket, BridgeMessage.Create(..., Approved = true, BridgeUrl = bridgeUrl, Token = token), CancellationToken.None);
}
catch (Exception ex)
{
    Console.WriteLine($"...");  // swallowed
}

tcs.TrySetResult(true);  // ← always runs, even when approval was never sent

When SendAsync fails (e.g., network drop between rate-limit acceptance and approval): the approval message containing the bridge URL and auth token is never delivered to the host. But tcs.TrySetResult(true) unblocks HandleIncomingPairHandshakeAsync's success path (not the timeout/deny path), so the worker UI shows "Pair request approved — worker linked!". Meanwhile the host's ReadSingleMessageAsync gets a WebSocket close frame → returns null → PairRequestResult.Unreachable. Net result: worker shows success, host shows failure, no pairing occurs. Silent discrepancy.

Fix: Move tcs.TrySetResult(true) inside the try block; call tcs.TrySetResult(false) in the catch:

try
{
    await SendAsync(pending.Socket, BridgeMessage.Create(...), CancellationToken.None);
    tcs.TrySetResult(true);
}
catch (Exception ex)
{
    Console.WriteLine($"...");
    tcs.TrySetResult(false);  // triggers denial path — host sees Denied or no-response
}

---

🟢 MINOR — NIC scoring omits RFC-1918 172.16.0.0/12 range (3/4 consensus)

FiestaService.cs — ScoreNetworkInterface + IsVirtualAdapterIp

bool isPrivateLan = ip.StartsWith("192.168.", ...) || ip.StartsWith("10.", ...);
// Missing: 172.16.0.0 – 172.31.255.255 (RFC-1918 Class B)

On corporate or university networks that use 172.16.x–172.31.x, Ethernet adapters score 60 ("not private") while WiFi at 192.168.x scores 90 — the pairing string encodes a WiFi IP instead of the wired one. This can cause pairing failures in enterprise environments.

Similarly, IsVirtualAdapterIp only blocks 172.17.x and 172.18.x; Docker networks can be allocated up to 172.31.x. The NIC name filter ("docker", "br-") already handles most cases, but combining both fixes would be thorough.

Fix: Add the 172.16.0.0/12 range to isPrivateLan:

bool isPrivateLan = ip.StartsWith("192.168.", ...) || ip.StartsWith("10.", ...)
    || (ip.StartsWith("172.", ...) && IsRfc1918_172(ip));

private static bool IsRfc1918_172(string ip)
{
    // 172.16.0.0/12 = 172.16.x.x through 172.31.x.x
    var parts = ip.Split('.');
    return parts.Length >= 2 && int.TryParse(parts[1], out var oct) && oct >= 16 && oct <= 31;
}

---

Below consensus — for author awareness

(gemini only, false positive) Crash in ParseAndLinkPairingString on short input —

---

Test Coverage

No new tests for Feature C (push-to-pair) or Feature B (pairing string parsing). Suggested additions:

1. ParseAndLinkPairingString roundtrip: GeneratePairingString → ParseAndLinkPairingString → verify linked worker fields
2. ApprovePairRequestAsync SendAsync failure: verify TCS result and host-side outcome
3. RequestPairAsync with Approved=true but null BridgeUrl: verify behavior

---

Recommended Action

⚠️ Request changes — the tcs.TrySetResult(true) on SendAsync failure (4/4 consensus) creates a UX inconsistency where the worker shows success but the host shows failure. The fix is a two-line change. The RFC-1918 172.x gap is a minor correctness issue for enterprise environments.

[PureWeen]

PR #322 Re-Review (Round 2) — Multi-Model Consensus

CI Status: ⚠️ No checks reported
Previous review: Round 1 had 2 findings. Author pushed 0cd23d4d "fix: address PR #322 review comments".

---

Previous Findings Status

Finding	Status
🔴 ApprovePairRequestAsync tcs.TrySetResult fires on failure	✅ FIXED — now correctly calls tcs.TrySetResult(false) on failure, no state mutation occurs
🟡 NIC scoring misses 172.16.0.0/12	✅ FIXED — IsRfc1918_172 correctly checks octets 16-31 for all three RFC 1918 ranges

---

New Findings (Round 2)

🔴 CRITICAL — Concurrent WebSocket sends crash approve/timeout race (2/2 reviewers)
FiestaService.cs:~808-837

When the 60s expiry timeout fires at the same instant the user clicks Approve, both paths call SendAsync on the same WebSocket concurrently. .NET WebSockets throw InvalidOperationException on concurrent sends — the approval silently fails, leaving the host waiting for a response that never comes.

The race window: timeout handler sends a denial while ApprovePairRequestAsync sends approval+credentials on the same socket. The outcome is nondeterministic.

Fix: Use tcs.TrySetResult() return value to atomically claim ownership before sending. The loser skips its send:

// In ApprovePairRequestAsync:
if (!pending.CompletionSource.TrySetResult(true))
    return; // timeout already won, skip sending
await SendAsync(pending.Socket, approveMsg, ct);

// In timeout handler:
if (!tcs.TrySetResult(false))
    return; // approval already won, skip sending
await SendAsync(ws, denyMsg, ct);

---

🟡 MODERATE — Fire-and-forget deny never delivered before socket close (2/2 reviewers)
FiestaService.cs:~427-431, ~868-875

Both the duplicate-request denial and DenyPairRequest use fire-and-forget _ = SendAsync(...) then immediately complete the TCS or return. Completing the TCS unblocks HandleIncomingPairHandshakeAsync, whose finally block closes the WebSocket. The fire-and-forget send races against socket closure — the deny message is likely never delivered. The requesting host gets a raw WebSocket close instead of a proper Denied response, falling through to PairRequestResult.Unreachable.

Fix: await the send before completing the TCS or returning.

---

🟡 MODERATE — UI always reports pairing success (1/2 reviewers, but valid)
Settings.razor:~1413-1416

await FiestaService.ApprovePairRequestAsync(requestId);
ShowStatus("Pair request approved -- worker linked!", "success", 2500); // always

ApprovePairRequestAsync swallows SendAsync exceptions and sets tcs.TrySetResult(false) but never throws or returns a success indicator. The UI unconditionally shows success. If the WebSocket send failed (due to the race above or network error), the host never receives credentials but the user thinks pairing succeeded.

Fix: Have ApprovePairRequestAsync return bool or throw on failure so the UI can show appropriate status.

---

Clean Areas (verified correct) ✅

• NIC scoring — All three RFC 1918 ranges covered correctly
• WsBridgeServer rate limiting — Interlocked.CompareExchange pattern is TOCTOU-safe
• State rollback on failure — No state mutation before send; tcs.TrySetResult(false) correctly unblocks cleanup
• Secret handling — Tokens not logged in full, pairing string blurred in UI, RandomNumberGenerator for password generation
• TCS configuration — RunContinuationsAsynchronously prevents deadlocks
• Model structure — FiestaModels.cs clean, proper public/internal split
• UI lifecycle — Event sub/unsub symmetric, cleanup on nav-away handled

Test Coverage

19 tests in FiestaPairingTests.cs. Missing scenarios:

• Concurrent approve + timeout race (the critical finding)
• Deny message delivery confirmation before socket close
• ApprovePairRequestAsync failure propagation to caller

Verdict: ⚠️ Request Changes

Both Round 1 findings are FIXED ✅. Three new findings:

1. Must fix: Concurrent WebSocket send race between approve and timeout (crash risk)
2. Must fix: Await deny sends before completing TCS / closing socket
3. Should fix: ApprovePairRequestAsync should return success/failure to UI

Workers 3 and 4 had permission issues and could not complete their assigned sub-reviews (bridge protocol and test quality). Findings above are from workers 2 and 5.

[PureWeen]

PR #322 Re-Review (Round 3) — Multi-Model Consensus (4 models: 2×Opus + Sonnet + GPT)

CI Status: ⚠️ No checks reported
Commit reviewed: acdcccbc "fix: address PR #322 round 2 review"

---

Previous Findings Status (Round 2)

#	Finding	Status
1	🔴 Concurrent WebSocket sends crash approve/timeout race	✅ FIXED — TrySetResult atomic gate in all 3 paths (approve, deny, timeout). Only winner sends. New test verifies exactly 1 send.
2	🟡 Fire-and-forget deny never delivered before socket close	✅ FIXED — DenyPairRequestAsync now awaits SendAsync after claiming TCS.
3	🟡 UI always reports pairing success	✅ FIXED — ApprovePairRequestAsync returns bool, UI branches on success/failure.

All 3 Round 2 findings are resolved. ✅

---

New Findings (Round 3)

🟡 MODERATE — HandlePairHandshakeAsync finally may close socket while winner's send is in-flight (4/4 models)
FiestaService.cs + WsBridgeServer.cs:~1259

TrySetResult resolves the TCS before the winner's SendAsync completes. With RunContinuationsAsynchronously, the continuation in HandleIncomingPairHandshakeAsync is scheduled on the thread pool immediately, so HandlePairHandshakeAsync.finally { ws.CloseAsync() } can race with the winner's send still in-flight. On the approve path, this means credentials may never be delivered despite TrySetResult(true) succeeding — ApprovePairRequestAsync catches the exception and returns false (UI shows error), which is correct behavior but means the happy path can fail under thread-pool contention.

Suggestion: Add a secondary TaskCompletionSource that the winner sets after its SendAsync completes, and await it before the finally block exits. Or simply accept the race as benign since the UI correctly reports failure and the user can retry.

---

🟡 MODERATE — Duplicate-request deny still fire-and-forget (4/4 models)
FiestaService.cs:~429-438

_ = Task.Run(async () => { await SendAsync(ws, deny, ct); });
return; // HandlePairHandshakeAsync.finally closes socket

Despite a comment claiming delivery-before-drop, the deny is launched as Task.Run and the method returns immediately. The caller's finally closes the socket, racing the send. The deny for duplicate requests is routinely lost.

Fix: Break out of the lock, then await SendAsync(...) inline before returning:

bool isDuplicate;
lock (_stateLock) { isDuplicate = _pendingPairRequests.Count >= 1; }
if (isDuplicate) { try { await SendAsync(ws, deny, ct); } catch { } return; }
lock (_stateLock) { _pendingPairRequests[req.RequestId] = pending; }

---

🟡 MODERATE — ReadSingleMessageAsync has no message size limit (4/4 models)
FiestaService.cs:~599-609

StringBuilder accumulates WebSocket frames with no cap on the unauthenticated /pair endpoint. A malicious LAN client can stream unbounded partial frames → OOM. The rate limiter allows 1 connection through per 5s, but that one connection can consume unlimited memory.

Fix: Add a size guard (e.g., 256KB or 1MB):

if (sb.Length > 256 * 1024) return null;

---

🟡 MODERATE — EnsureServerPassword calls ConnectionSettings.Load()/Save() — test isolation violation (3/4 models)
FiestaService.cs:~615-633 + FiestaPairingTests.cs

Per codebase convention: "Tests must NEVER call ConnectionSettings.Save() or ConnectionSettings.Load()." When _bridgeServer.ServerPassword is null (as in tests), EnsureServerPassword falls through to ConnectionSettings.Load() and potentially settings.Save(), writing to the real ~/.polypilot/settings.json.

Fix: Set _bridgeServer.ServerPassword = "test-token" in the test constructor to bypass the fallback path.

---

Clean Areas ✅

• TrySetResult atomic gate — correctly prevents concurrent sends in all paths
• NIC scoring — all RFC 1918 ranges covered, virtual adapter filtering solid
• WsBridgeServer rate limiting — CAS pattern is TOCTOU-safe
• Pairing string encode/decode — URL-safe base64, proper validation
• Secret handling — RandomNumberGenerator, tokens not logged, pairing string blurred in UI
• Test coverage — 7 tests covering roundtrip, send failure, null BridgeUrl, concurrent race, deny ordering

Verdict: ⚠️ Request Changes (minor)

All Round 2 CRITICALs are FIXED. Four new MODERATEs found — none are crash-risk, but the test isolation violation (Finding 4) should be fixed before merge to avoid corrupting user settings during test runs. The other three are defense-in-depth improvements.

Must fix: Test isolation (_bridgeServer.ServerPassword = "test-token" in constructor)
Should fix: ReadSingleMessageAsync size limit, duplicate-request deny delivery
Nice to have: SendComplete TCS for finally-block race

[PureWeen]

PR #322 Re-Review (Round 4) — Multi-Model Consensus (5 models: 2×Opus + Sonnet + Gemini + GPT)

CI Status: ⚠️ No checks reported
Commit reviewed: 2fbc31ad (HEAD of session-20260308-090305 branch)

---

Previous Findings Status (Round 3)

#	Finding	Status
R3-1	HandlePairHandshakeAsync finally closes socket while send in-flight	✅ FIXED — SendComplete TCS on PendingPairRequest; all three sender paths set it in finally; HandleIncomingPairHandshakeAsync awaits it at lines 462 and 484 before returning
R3-2	Duplicate-request deny fire-and-forget	✅ FIXED — isDuplicate flag captured inside lock, deny sent via inline await SendAsync(...) outside lock; no deadlock risk
R3-3	ReadSingleMessageAsync unbounded OOM	✅ FIXED — if (sb.Length > 256 * 1024) return null; guard added
R3-4	EnsureServerPassword test isolation violation	✅ FIXED — _bridgeServer.ServerPassword = "test-token-isolation" in test constructor bypasses ConnectionSettings.Load/Save

All 4 Round 3 findings: FIXED ✅

---

New Finding (Round 4)

🟡 MODERATE — TimeoutException from SendComplete wait escapes catch block, re-enabling socket-close race (2/5 models: Opus-2, GPT)
FiestaService.cs:462

try
{
    await tcs.Task.WaitAsync(expiryCts.Token);   // line 459 — throws OperationCanceledException
    await pending.SendComplete.Task.WaitAsync(TimeSpan.FromSeconds(5));  // line 462 — throws TimeoutException!
}
catch (OperationCanceledException) { ... }  // does NOT catch TimeoutException

Task.WaitAsync(TimeSpan) throws TimeoutException on expiry, not OperationCanceledException. If the approve/deny sender's SendAsync is slow or hung (>5s), the TimeoutException escapes the catch block, propagates through HandleIncomingPairHandshakeAsync's finally (which removes the pending request and fires OnStateChanged), then up to WsBridgeServer's handler which closes the socket — while the sender's SendAsync is still in-flight on that same socket. This is exactly the socket-close race that R3-1 was introduced to prevent.

Contrast with line 484 (the equivalent in the else branch) which correctly guards: try { await pending.SendComplete.Task.WaitAsync(TimeSpan.FromSeconds(5)); } catch { }

Fix: Match the pattern at line 484:

try { await pending.SendComplete.Task.WaitAsync(TimeSpan.FromSeconds(5)); } catch { }

---

Below Consensus (noted for awareness, not blocking)

• Sonnet only: SendComplete.TrySetResult() in timeout-deny finally fires even when SendAsync threw — this is correct by design (SendComplete signals "send attempt done, safe to close socket", not "send succeeded"); not a bug.
• Gemini only: QrScannerService.ScanAsync TOCTOU on _tcs — theoretical; ScanAsync is only called from MAUI UI event handlers (single-threaded); not a real race in practice.
• GPT only: FiestaPairingTests loads real fiesta.json — pre-existing concern; test isolation (SetBaseDirForTesting) covers the polypilot dir but not fiesta.json; separate issue from this PR.

---

Clean Areas ✅ (confirmed by multiple models)

• TCS atomic gate — TrySetResult correctly prevents concurrent sends in all paths
• NIC scoring — all three RFC 1918 ranges covered correctly
• Pairing string encode/decode — URL-safe base64, proper validation
• Secret handling — RandomNumberGenerator, tokens not logged, ApprovePairRequestAsync returns bool for UI branching
• SendComplete lifecycle — set in finally on all three send paths (approve, deny, auto-deny-timeout)
• isDuplicate lock scope — captured inside lock as stack-local bool; inline await correctly outside lock
• JsonDocument disposal — using added in Dashboard.razor and Settings.razor (cc95609)

---

Test Coverage

Tests in FiestaPairingTests.cs (353 lines, 7+ tests) cover:

• ✅ Pairing string roundtrip
• ✅ ApprovePairRequestAsync SendAsync failure path
• ✅ Null BridgeUrl handling
• ✅ Concurrent approve + timeout race (TCS atomic gate)
• ✅ Deny message delivery ordering
• ✅ Test isolation (ServerPassword pre-set)

Missing:

• ❌ SendComplete.WaitAsync timeout path (the new finding above)

---

Verdict: ⚠️ Request Changes (one small fix)

All Round 3 findings are FIXED ✅. One new finding:

Must fix: Wrap line 462 to catch TimeoutException:

// Before (line 462):
await pending.SendComplete.Task.WaitAsync(TimeSpan.FromSeconds(5));

// After:
try { await pending.SendComplete.Task.WaitAsync(TimeSpan.FromSeconds(5)); } catch { }

This is a one-liner that mirrors the identical pattern already in the else-branch at line 484.

[PureWeen]

🤖 Multi-Model Review -- Round 4 Summary

CI: ⚠️ No checks configured
Previous Findings (Round 3): All 4 fixed ✅ (SendComplete TCS, duplicate-deny, ReadSingleMessageAsync OOM, EnsureServerPassword test isolation)

Outstanding Finding (consensus 3/5 models)

Severity	File	Issue
🟡 MODERATE	PolyPilot/Services/FiestaService.cs:462	TimeoutException escapes catch (OperationCanceledException) -- re-enables socket-close race

Details: await pending.SendComplete.Task.WaitAsync(TimeSpan.FromSeconds(5)) at line 462 throws TimeoutException (not OperationCanceledException) in .NET 6+. This escapes the catch (OperationCanceledException) handler, bypassing the graceful cleanup path and re-enabling the socket-close race condition this PR aims to fix. Line 484 already handles this correctly with a bare try { ... } catch { } block.

Suggested fix (one-liner): Wrap line 462 the same way line 484 is wrapped -- replace the specific catch (OperationCanceledException) with a bare catch { }, or add a second catch (TimeoutException) clause with the same body.

// Option A: Match line 484 pattern
try { await pending.SendComplete.Task.WaitAsync(TimeSpan.FromSeconds(5)); } catch { }

// Option B: Add TimeoutException handler
catch (OperationCanceledException) { /* existing cleanup */ }
catch (TimeoutException) { /* same cleanup */ }

Verdict: ⚠️ Request changes -- one-liner fix needed before merge

Review by PR Review Squad (claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex -- 2+ model consensus filter)

[PureWeen]

PR Review Squad -- Automated Review

Previous Findings Status

Finding	Status
Round 4 TimeoutException in HandlePairHandshakeAsync	✅ FIXED

Current Findings

🟡 MODERATE -- Watchdog crash recovery missing CancelTurnEndFallback (5/5 model consensus)
CopilotService.Events.cs:~2171: The watchdog crash recovery path (catch (Exception ex)) clears IsProcessing and all 9 companion fields but does NOT call CancelTurnEndFallback(state). The normal timeout path at line ~2113 does call it. This means a stale TurnEnd→Idle fallback timer can fire after crash recovery and run CompleteResponse against an already force-completed turn. CancelToolHealthCheck(state) is also missing (present in normal timeout path).

Suggested fix: Add CancelTurnEndFallback(state); and CancelToolHealthCheck(state); at the top of the InvokeOnUI lambda in the crash recovery block (~line 2189), matching the normal Case C timeout cleanup pattern.

🟡 MODERATE -- EnsureServerPassword race condition (2/5 consensus)
FiestaService.cs: EnsureServerPassword lacks synchronization. Concurrent calls from ApprovePairRequestAsync and GeneratePairingString can race to generate different passwords. If T1 generates password A and T2 generates password B (saved last), T1 returns A which is now invalid -- causing an immediate pairing auth failure for that user action. Mitigating factor: both callers are currently UI-thread-only, making the race structurally unreachable today, but there is no architectural enforcement.

Suggested fix: Add lock (_stateLock) around the generate-and-save path in EnsureServerPassword, or use Lazy<string> initialization.

Verified Clean

• Round 4 TimeoutException: both WaitAsync(TimeSpan) call sites now catch TimeoutException ✅
• Rate-limiting CAS logic in WsBridgeServer.AcceptLoopAsync ✅ correct (single-threaded accept loop makes CAS failure structurally impossible)
• Path traversal guard in GetFiestaWorkspaceDirectory ✅ safe on both Unix and Windows
• TCS SendComplete coordination in all three send paths ✅ correct
• ReadSingleMessageAsync 256KB guard ✅ safe
• isDuplicate lock scope and inline deny ✅ correct
• posix_spawn detection orthogonality ✅ verified by test

Test Coverage Gap

❌ No test for the watchdog crash recovery path -- specifically that CancelTurnEndFallback is called, and that a subsequent user send after crash recovery is not prematurely completed by a stale timer.

Verdict: ⚠️ Request Changes

Must fix: CancelTurnEndFallback(state) + CancelToolHealthCheck(state) in watchdog crash recovery (violates the processing-state-safety invariant documented in .claude/skills/processing-state-safety/SKILL.md). This is a one-liner fix matching the identical pattern in the Case C timeout path immediately above.
Recommended: Add lock in EnsureServerPassword for architectural safety.

Review by PR Review Squad -- Round 5 (claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex -- 2+ model consensus filter)

[PureWeen]

Multi-Model Consensus Review — Round 8 (4/5 models, final findings)

CI Status: ⚠️ No CI configured

Tests: 2589 passed, 2 PR-specific failures, 1 pre-existing failure

---

Prior Findings Status

#	Prior Finding	Status
1	CancelTurnEndFallback missing from watchdog crash recovery	MOVED TO SEPARATE PR — no longer in diff
2	EnsureServerPassword race condition (no lock)	STILL PRESENT — confirmed by direct code inspection (FiestaService.cs:653)
3	Missing tests for Fiesta pairing code paths	PARTIALLY ADDRESSED — FiestaPairingTests added but 2 tests broken

---

New Findings (4/5 model consensus)

Sev	File:Line	Description
🔴	FiestaService.cs:37 + TestSetup.cs	Test isolation bug — writes to real ~/.polypilot/fiesta.json. _stateFilePath is static string? with ??= initializer — resolves to real user's ~/.polypilot/fiesta.json and is never reset between tests. Constructor calls LoadState() which reads persisted workers from disk. After first test run, disk has 1+ worker, so second test instance loads it and Assert.Single() sees 2 items → ParseAndLinkPairingString_Roundtrip_CorrectWorkerFields fails. Worse: SaveState() in tests overwrites the user's real pairing data. Fix: add SetStateFilePathForTesting(string) to FiestaService and call it from TestSetup.cs's ModuleInitializer (same pattern as CopilotService.SetBaseDirForTesting).
🔴	Settings.razor.css:719	font-size: 0.85em not in allowlist — breaks FontSizingEnforcementTests. .onboarding-list code { font-size: 0.85em; } uses a raw em value not in CssFontSizeAllowlist. Fix: add ("Settings.razor.css", @"^0\.85em$", "Inline code (.onboarding-list code) — standard CSS shrink for monospace in body text") to the allowlist in FontSizingEnforcementTests.cs. (Note: var(--type-headline) = 0.85rem but that's rem, not em — the em here is intentional for inline code scaling with parent.)
🟡	FiestaService.cs:653	EnsureServerPassword race (STILL PRESENT). No lock on check→generate→save path. Two concurrent callers can generate different passwords; last writer wins and first caller's return value is stale. Fix: wrap lines 655–672 in lock (_stateLock) { ... } with a re-check after acquiring the lock.
🟡	FiestaService.cs:515	ApprovePairRequestAsync uses Tailscale IP without IsRunning guard. Line 515: _tailscale?.MagicDnsName ?? _tailscale?.TailscaleIp can return a stale IP when Tailscale is installed-but-not-running (because TailscaleService.ParseStatus sets IPs independently of IsRunning). GeneratePairingString (line 351) correctly guards with _tailscale?.IsRunning == true; ApprovePairRequestAsync does not. Fix: var localIp = (_tailscale?.IsRunning == true ? (_tailscale.MagicDnsName ?? _tailscale.TailscaleIp) : null) ?? GetPrimaryLocalIpAddress() ?? "localhost";

---

Pre-existing failure (not PR-specific)

ServerManagerTests.CheckServerRunning_NoUnobservedTaskException_OnConnectionRefused — port collision, same on main.

---

Recommended Action: ⚠️ Request Changes

Two blocking test failures must be fixed before merge:

1. Add FiestaService.SetStateFilePathForTesting() + wire it up in TestSetup.cs
2. Add Settings.razor.css 0.85em entry to FontSizingEnforcementTests.cs allowlist

The EnsureServerPassword lock and ApprovePairRequestAsync IsRunning guard are non-blocking but should be addressed.

[PureWeen]

Multi-Model Consensus Review — Round 9 (5/5 models, all findings confirmed)

No new commits since Round 8 (latest: 777a4cce). All 4 previous findings remain unaddressed.

---

CI Status: ⚠️ No CI configured

Tests: 2 PR-specific failures (unchanged)

---

Previous Findings — All STILL PRESENT (5/5 model consensus)

#	Sev	File:Line	Status	Description
1	🔴	FiestaService.cs:37 + TestSetup.cs	STILL PRESENT	Static _stateFilePath not test-isolated. TestSetup.cs resets CopilotService, RepoManager, AuditLogService, PromptLibraryService — but NOT FiestaService. Constructor calls LoadState() against real ~/.polypilot/fiesta.json → ParseAndLinkPairingString_Roundtrip_CorrectWorkerFields fails (Assert.Single() sees 2). Also overwrites real user pairing data during test runs. Fix: add internal static void SetStateFilePathForTesting(string path) => _stateFilePath = path; to FiestaService and call it from TestSetup.ModuleInitializer.
2	🔴	Settings.razor.css:719	STILL PRESENT	.onboarding-list code { font-size: 0.85em; } not in CssFontSizeAllowlist. Only Settings.razor.css entry in the list is 2rem. Fix: add ("Settings.razor.css", @"^0\.85em$", "Inline code (.onboarding-list code) — scales with parent text") to the allowlist in FontSizingEnforcementTests.cs.
3	🟡	FiestaService.cs:653	STILL PRESENT	EnsureServerPassword() has no lock on check→generate→save path. Fix: wrap lines 655–672 in lock (_stateLock) { if already set, return; ... }.
4	🟡	FiestaService.cs:515	STILL PRESENT	ApprovePairRequestAsync reads _tailscale?.MagicDnsName ?? _tailscale?.TailscaleIp without IsRunning == true guard. GeneratePairingString (line 351) has this guard. Fix: var localIp = (_tailscale?.IsRunning == true ? (_tailscale.MagicDnsName ?? _tailscale.TailscaleIp) : null) ?? GetPrimaryLocalIpAddress() ?? "localhost";

---

New Findings (1 model only — below consensus threshold, FYI)

Sev	File:Line	Models	Description
🟡	FiestaService.cs:520	1/5	TCS fires TrySetResult(true) before SendAsync — if send throws, caller gets false but TCS is permanently approved; request can never be retried.
🟡	FiestaService.cs:515	1/5	BridgePort read without _bridgeServer.IsRunning guard — worker could receive http://ip:0 if bridge stopped mid-approval. GeneratePairingString already has this guard.

---

Recommended Action: ⚠️ Request Changes

Two blocking test failures remain unfixed (findings #1 and #2). These are small changes (a one-liner test hook + one allowlist entry). The EnsureServerPassword lock (#3) and Tailscale IsRunning guard (#4) are also straightforward and should be fixed in the same pass.

[PureWeen]

Multi-Model Consensus Review — Round 10 (5/5 models)

Latest commit: a55d1b9a (rebased, no new commits address any previous findings)

CI: ⚠️ No CI configured

Tests: 2616 passed, 2 PR-specific failures, 2 pre-existing

---

Previous Findings — All STILL PRESENT (5/5 unanimous)

#	Sev	File:Line	Status
1	🔴	FiestaService.cs:37 + TestSetup.cs	STILL PRESENT — Static _stateFilePath resolves to real ~/.polypilot/fiesta.json, TestSetup.cs has no FiestaService hook. ParseAndLinkPairingString_Roundtrip_CorrectWorkerFields fails with Assert.Single() seeing 2 items.
2	🔴	Settings.razor.css:719	STILL PRESENT — font-size: 0.85em not in CssFontSizeAllowlist → FontSizingEnforcementTests fails.
3	🟡	FiestaService.cs:~653	STILL PRESENT — EnsureServerPassword() no lock on check→generate→save path.
4	🟡	FiestaService.cs:~515	STILL PRESENT — ApprovePairRequestAsync uses Tailscale IP without IsRunning == true guard.

---

New Findings (2+ model consensus)

Sev	File:Line	Models	Description
🟡	FiestaService.cs:58-75	2/5	GetPolyPilotBaseDir() is a private duplicate of CopilotService's path logic. Even if FiestaService.SetStateFilePathForTesting() were added for finding #1, it would still bypass the CopilotService.BaseDir override used by all other services. The fix is to replace the private GetPolyPilotBaseDir() with a delegate to CopilotService.BaseDir.
🟡	FiestaService.cs:~660,671	2/5	EnsureServerPassword() calls ConnectionSettings.Load() / settings.Save() internally. Project invariant (documented in testing guidelines) states tests must NEVER call ConnectionSettings.Save() or ConnectionSettings.Load(). Any test path reaching the password fallback branch will corrupt real ~/.polypilot/settings.json. The test constructor pre-sets _bridgeServer.ServerPassword as a workaround, but this is fragile — future tests that don't know this convention will corrupt real data.

---

Confirmed PR-specific test failures (from dotnet test):

Failed FiestaPairingTests.ParseAndLinkPairingString_Roundtrip_CorrectWorkerFields
  Assert.Single() Failure: collection had 2 items
  [FiestaLinkedWorker BridgeUrl=http://192.168.1.50:4322, ...] [FiestaLinkedWorker BridgeUrl=http://10.4.0.124:4322, ...]

Failed FontSizingEnforcementTests.AllCssFiles_FontSizes_UseTypeScaleOrAllowlisted  
  Found 1 CSS font-size value not in allowlist: Settings.razor.css:719 — font-size: 0.85em

---

Required fixes before merge:

1. FiestaService.cs: Replace private GetPolyPilotBaseDir() with (fixes structural isolation root cause)
2. TestSetup.cs: Add FiestaService.SetStateFilePathForTesting(Path.Combine(TestBaseDir, "fiesta.json")) to ModuleInitializer
3. Settings.razor.css:719: Add ("Settings.razor.css", @"^0\.85em$", "Inline code (.onboarding-list code) — scales with parent") to CssFontSizeAllowlist in FontSizingEnforcementTests.cs
4. FiestaService.cs:653: Wrap generate path in lock (_stateLock) with re-check under lock
5. FiestaService.cs:515: Add _tailscale?.IsRunning == true guard for Tailscale IP selection

Verdict: ⚠️ Request Changes

10 rounds of review, 0 findings fixed. Two test failures block merge. All 5 fixes are small and self-contained.