fix: address PR #322 review comments

PureWeen · Copilot · PureWeen · commit 937ac4e62971 · 2026-03-12T19:13:30.000-05:00
ApprovePairRequestAsync TCS fix:
- Move tcs.TrySetResult(true) inside the try block; call tcs.TrySetResult(false)
  in the catch block. Previously the TCS was always resolved true even when
  SendAsync threw, causing a silent discrepancy between host ('failed') and
  worker ('approved').

NIC scoring RFC-1918 172.16.0.0/12:
- Add IsRfc1918_172() helper to check the 172.16-31 octet range.
- Update ScoreNetworkInterface() to treat 172.16-31 IPs as private LAN
  (previously only 192.168.x and 10.x were recognized).
- Docker bridge IPs (172.17.x, 172.18.x) are still filtered by name-pattern
  and IsVirtualAdapterIp(), so they are excluded before scoring.

RequestPairAsync null BridgeUrl guard:
- Add explicit check: if resp.BridgeUrl or resp.Token is null/empty, return
  PairRequestResult.Unreachable instead of silently calling LinkWorker with
  null args (which silently no-ops but still returns Approved to the caller).

Tests (FiestaPairingTests.cs):
- ParseAndLinkPairingString_Roundtrip: manually encode pp+ string, call
  ParseAndLinkPairingString, verify Url/Token/Hostname on linked worker.
- ParseAndLinkPairingString_InvalidPrefix / MissingUrl: FormatException cases.
- ApprovePairRequestAsync_SendFails_TcsResultIsFalse: inject a FaultyOpenWebSocket
  (State=Open but SendAsync throws) via reflection; verify TCS resolves false.
- RequestPairAsync_ApprovedWithNullBridgeUrl_ReturnsUnreachable: real HttpListener
  server returns Approved+null BridgeUrl; verify Unreachable + no worker linked.

Co-authored-by: Copilot &lt;223556219+Copilot@users.noreply.github.com&gt;
diff --git a/PolyPilot.Tests/FiestaPairingTests.cs b/PolyPilot.Tests/FiestaPairingTests.cs
@@ -0,0 +1,247 @@
+using System.Net;
+using System.Net.WebSockets;
+using System.Reflection;
+using System.Text;
+using System.Text.Json;
+using Microsoft.Extensions.DependencyInjection;
+using PolyPilot.Models;
+using PolyPilot.Services;
+
+namespace PolyPilot.Tests;
+
+/// <summary>
+/// Tests for Fiesta pairing features: pairing string encode/decode,
+/// ApprovePairRequestAsync TCS behavior on failure, and RequestPairAsync
+/// with a malformed approval response (Approved=true but null BridgeUrl).
+/// </summary>
+public class FiestaPairingTests : IDisposable
+{
+    private readonly WsBridgeServer _bridgeServer;
+    private readonly CopilotService _copilot;
+    private readonly FiestaService _fiesta;
+
+    public FiestaPairingTests()
+    {
+        _bridgeServer = new WsBridgeServer();
+        _copilot = new CopilotService(
+            new StubChatDatabase(),
+            new StubServerManager(),
+            new StubWsBridgeClient(),
+            new RepoManager(),
+            new ServiceCollection().BuildServiceProvider(),
+            new StubDemoService());
+        _fiesta = new FiestaService(_copilot, _bridgeServer);
+    }
+
+    public void Dispose()
+    {
+        _fiesta.Dispose();
+        _bridgeServer.Dispose();
+    }
+
+    // ---- Helpers ----
+
+    private static string BuildPairingString(string url, string token, string hostname)
+    {
+        var payload = new FiestaPairingPayload { Url = url, Token = token, Hostname = hostname };
+        var json = JsonSerializer.Serialize(payload, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase });
+        var b64 = Convert.ToBase64String(Encoding.UTF8.GetBytes(json))
+            .TrimEnd('=')
+            .Replace('+', '-')
+            .Replace('/', '_');
+        return $"pp+{b64}";
+    }
+
+    private static int GetFreePort()
+    {
+        using var l = new System.Net.Sockets.TcpListener(IPAddress.Loopback, 0);
+        l.Start();
+        var port = ((IPEndPoint)l.LocalEndpoint).Port;
+        l.Stop();
+        return port;
+    }
+
+    // ---- Test 1: Pairing string roundtrip ----
+
+    [Fact]
+    public void ParseAndLinkPairingString_Roundtrip_CorrectWorkerFields()
+    {
+        const string url = "http://192.168.1.50:4322";
+        const string token = "test-token-abc123";
+        const string hostname = "devbox-1";
+
+        var pairingString = BuildPairingString(url, token, hostname);
+        Assert.StartsWith("pp+", pairingString);
+
+        var linked = _fiesta.ParseAndLinkPairingString(pairingString);
+
+        Assert.Equal(url, linked.BridgeUrl);
+        Assert.Equal(token, linked.Token);
+        Assert.Equal(hostname, linked.Name);
+        Assert.Single(_fiesta.LinkedWorkers);
+    }
+
+    [Fact]
+    public void ParseAndLinkPairingString_InvalidPrefix_ThrowsFormatException()
+    {
+        Assert.Throws<FormatException>(() => _fiesta.ParseAndLinkPairingString("notvalid"));
+        Assert.Throws<FormatException>(() => _fiesta.ParseAndLinkPairingString("pp+!!!notbase64!!!"));
+    }
+
+    [Fact]
+    public void ParseAndLinkPairingString_MissingUrl_ThrowsFormatException()
+    {
+        // Build a pairing string that's valid base64 but has no URL field
+        var payload = new FiestaPairingPayload { Url = "", Token = "tok", Hostname = "host" };
+        var json = JsonSerializer.Serialize(payload, new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase });
+        var b64 = Convert.ToBase64String(Encoding.UTF8.GetBytes(json)).TrimEnd('=').Replace('+', '-').Replace('/', '_');
+        var s = $"pp+{b64}";
+
+        Assert.Throws<FormatException>(() => _fiesta.ParseAndLinkPairingString(s));
+    }
+
+    // ---- Test 2: ApprovePairRequestAsync TCS result on SendAsync failure ----
+
+    [Fact]
+    public async Task ApprovePairRequestAsync_SendFails_TcsResultIsFalse()
+    {
+        const string requestId = "req-test-001";
+        var tcs = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
+
+        // Inject a pending pair request with a WebSocket that reports Open state
+        // but throws on SendAsync, simulating a race-condition socket failure.
+        var faultySocket = new FaultyOpenWebSocket();
+        var pending = new PendingPairRequest
+        {
+            RequestId = requestId,
+            HostName = "test-host",
+            HostInstanceId = "host-id",
+            RemoteIp = "127.0.0.1",
+            Socket = faultySocket,
+            CompletionSource = tcs,
+            ExpiresAt = DateTime.UtcNow.AddSeconds(60)
+        };
+
+        var dictField = typeof(FiestaService).GetField("_pendingPairRequests", BindingFlags.NonPublic | BindingFlags.Instance)!;
+        var dict = (Dictionary<string, PendingPairRequest>)dictField.GetValue(_fiesta)!;
+        lock (dict) dict[requestId] = pending;
+
+        await _fiesta.ApprovePairRequestAsync(requestId);
+
+        // The TCS should be resolved false because SendAsync threw
+        Assert.True(tcs.Task.IsCompleted);
+        Assert.False(await tcs.Task);
+    }
+
+    [Fact]
+    public async Task ApprovePairRequestAsync_UnknownRequestId_DoesNotThrow()
+    {
+        // Should silently return if the request id is not found
+        await _fiesta.ApprovePairRequestAsync("nonexistent-id");
+    }
+
+    // ---- Test 3: RequestPairAsync with Approved=true but null BridgeUrl ----
+
+    [Fact]
+    public async Task RequestPairAsync_ApprovedWithNullBridgeUrl_ReturnsUnreachable()
+    {
+        var port = GetFreePort();
+        using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(15));
+
+        // Stand up a minimal WebSocket server that responds with Approved=true but no BridgeUrl
+        var serverTask = Task.Run(async () =>
+        {
+            var listener = new HttpListener();
+            listener.Prefixes.Add($"http://127.0.0.1:{port}/");
+            listener.Start();
+            try
+            {
+                var ctx = await listener.GetContextAsync().WaitAsync(cts.Token);
+                if (!ctx.Request.IsWebSocketRequest) { ctx.Response.StatusCode = 400; ctx.Response.Close(); return; }
+
+                var wsCtx = await ctx.AcceptWebSocketAsync(subProtocol: null);
+                var ws = wsCtx.WebSocket;
+
+                // Read (and discard) the pair request
+                var buf = new byte[4096];
+                await ws.ReceiveAsync(new ArraySegment<byte>(buf), cts.Token);
+
+                // Send back Approved=true with no BridgeUrl / Token
+                var response = BridgeMessage.Create(BridgeMessageTypes.FiestaPairResponse,
+                    new FiestaPairResponsePayload
+                    {
+                        RequestId = "req-null-url",
+                        Approved = true,
+                        BridgeUrl = null,
+                        Token = null,
+                        WorkerName = "worker"
+                    });
+                var bytes = Encoding.UTF8.GetBytes(response.Serialize());
+                await ws.SendAsync(new ArraySegment<byte>(bytes), WebSocketMessageType.Text, true, cts.Token);
+
+                // Best-effort close; client may have already closed
+                try { await ws.CloseAsync(WebSocketCloseStatus.NormalClosure, "done", cts.Token); } catch { }
+            }
+            catch (OperationCanceledException) { /* test timed out */ }
+            catch (Exception) { /* ignore server-side cleanup errors */ }
+            finally
+            {
+                listener.Stop();
+            }
+        }, cts.Token);
+
+        // Give the server a moment to bind
+        await Task.Delay(50, cts.Token);
+
+        var worker = new FiestaDiscoveredWorker
+        {
+            InstanceId = "remote-id",
+            Hostname = "remote-box",
+            BridgeUrl = $"http://127.0.0.1:{port}"
+        };
+
+        var countBefore = _fiesta.LinkedWorkers.Count;
+        var result = await _fiesta.RequestPairAsync(worker, cts.Token);
+
+        // An approved response with no BridgeUrl should be treated as Unreachable
+        Assert.Equal(PairRequestResult.Unreachable, result);
+
+        // No new worker should have been linked by this call
+        Assert.Equal(countBefore, _fiesta.LinkedWorkers.Count);
+        Assert.DoesNotContain(_fiesta.LinkedWorkers, w =>
+            string.Equals(w.Hostname, "remote-box", StringComparison.OrdinalIgnoreCase) ||
+            w.BridgeUrl.Contains($"127.0.0.1:{port}"));
+
+        await serverTask;
+    }
+
+    // ---- Helpers: fake WebSocket implementations ----
+
+    /// <summary>
+    /// A WebSocket that passes the State == Open guard but throws on SendAsync,
+    /// simulating a socket that closes between the state check and the write.
+    /// </summary>
+    private sealed class FaultyOpenWebSocket : WebSocket
+    {
+        public override WebSocketState State => WebSocketState.Open;
+        public override WebSocketCloseStatus? CloseStatus => null;
+        public override string? CloseStatusDescription => null;
+        public override string? SubProtocol => null;
+
+        public override void Abort() { }
+
+        public override Task CloseAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken ct)
+            => Task.CompletedTask;
+
+        public override Task CloseOutputAsync(WebSocketCloseStatus closeStatus, string? statusDescription, CancellationToken ct)
+            => Task.CompletedTask;
+
+        public override Task<WebSocketReceiveResult> ReceiveAsync(ArraySegment<byte> buffer, CancellationToken ct)
+            => Task.FromResult(new WebSocketReceiveResult(0, WebSocketMessageType.Close, true));
+
+        public override Task SendAsync(ArraySegment<byte> buffer, WebSocketMessageType messageType, bool endOfMessage, CancellationToken ct)
+            => throw new WebSocketException("Simulated send failure after state check");
+
+        public override void Dispose() { }
+    }
+}
diff --git a/PolyPilot/Services/FiestaService.cs b/PolyPilot/Services/FiestaService.cs
@@ -486,13 +486,13 @@ await SendAsync(pending.Socket, BridgeMessage.Create(
                     Token = token,
                     WorkerName = Environment.MachineName
                 }), CancellationToken.None);
+            tcs.TrySetResult(true);
         }
         catch (Exception ex)
         {
             Console.WriteLine($"[Fiesta] Failed to send pair approval: {ex.Message}");
+            tcs.TrySetResult(false);
         }
-
-        tcs.TrySetResult(true);
     }
 
     public void DenyPairRequest(string requestId)
@@ -556,8 +556,12 @@ await SendAsync(ws, BridgeMessage.Create(
             if (resp == null || !resp.Approved)
                 return PairRequestResult.Denied;
 
+            // Guard: an approval without connection details is a malformed response
+            if (string.IsNullOrWhiteSpace(resp.BridgeUrl) || string.IsNullOrWhiteSpace(resp.Token))
+                return PairRequestResult.Unreachable;
+
             var workerName = !string.IsNullOrWhiteSpace(resp.WorkerName) ? resp.WorkerName : worker.Hostname;
-            LinkWorker(workerName, worker.Hostname, resp.BridgeUrl!, resp.Token!);
+            LinkWorker(workerName, worker.Hostname, resp.BridgeUrl, resp.Token);
             return PairRequestResult.Approved;
         }
         catch (WebSocketException) { return PairRequestResult.Unreachable; }
@@ -1124,12 +1128,20 @@ private static bool IsVirtualAdapterName(string name) =>
     private static bool IsVirtualAdapterIp(string ip) =>
         ip.StartsWith("172.17.", StringComparison.Ordinal) ||  // Docker default bridge
         ip.StartsWith("172.18.", StringComparison.Ordinal);    // Docker custom networks
+    // Note: 172.16-31 is RFC-1918 but IsRfc1918_172 distinguishes real from Docker in scoring
+
+    private static bool IsRfc1918_172(string ip)
+    {
+        var parts = ip.Split('.');
+        return parts.Length >= 2 && int.TryParse(parts[1], out var oct) && oct >= 16 && oct <= 31;
+    }
 
     private static int ScoreNetworkInterface(NetworkInterfaceType type, string ip)
     {
         // Prefer RFC-1918 private ranges (real LAN) vs others
         bool isPrivateLan = ip.StartsWith("192.168.", StringComparison.Ordinal)
-                         || ip.StartsWith("10.", StringComparison.Ordinal);
+                         || ip.StartsWith("10.", StringComparison.Ordinal)
+                         || (ip.StartsWith("172.", StringComparison.Ordinal) && IsRfc1918_172(ip));
 
         return type switch
         {
