Fix three discovery defects found in review

A Proxmox number written as a string took the whole run down. Its perl
backend quotes numeric fields inconsistently — "vmid":"100" and
"maxdisk":"512110190592" both turn up across versions — and the
TryGetInt32 family does not return false for a string, it throws. The
kind is now checked first and a quoted number parsed, which is what the
caller meant either way. Before this, one such field produced
"Unexpected error occurred" plus a stack trace on the CLI, and MCP
forwarded the BCL's "requires an element of type 'Number'" as though the
user had done something wrong.

An ssh:// or npipe:// DOCKER_HOST crashed. `docker context` sets the
first for a remote host and the second is the Windows default, so both
are ordinary values to find in the environment. HttpClient accepts either
URI and only throws NotSupportedException on the first request, which was
past the command's catch list. They are now refused where the client is
built, through the UriFormatException both front ends already turn into
"not a usable Docker endpoint" — one phrasing for a bad endpoint rather
than two — and the message says how to forward the socket instead.

A remote engine with no IPv4 address had its services recorded at this
machine's address. Services are recorded at their host's address and the
inventory holds IPv4 only, so when the endpoint resolves to IPv6 alone the
address is genuinely unknown; substituting whatever machine ran the
command gave every service a confident, wrong address that then flowed
into the ansible, ssh and hosts exports. There is no fallback now, and the
run stops with an explanation.

Each fix has tests that fail without it: quoted numbers across the guest
list, both unreachable endpoint schemes, and an engine answering on an
IPv6-only socket.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

RackPeek.Domain/Discovery/DockerApiClient.cs

@@ -98,12 +98,31 @@ public sealed class DockerApiClient : IDockerClient, IDisposable {
             return (UnixSocketClient(path), dockerHost);
         }
 
+        // Anything else is rejected here rather than at send time. HttpClient accepts an
+        // ssh:// or npipe:// URI happily and only throws NotSupportedException on the
+        // first request — which is not in the caller's catch list, so a DOCKER_HOST that
+        // `docker context` set up quite normally crashed with a stack trace instead of
+        // saying what was wrong.
+        if (!StartsWithScheme(dockerHost, "tcp://")
+            && !StartsWithScheme(dockerHost, "http://")
+            && !StartsWithScheme(dockerHost, "https://"))
+            // A UriFormatException on purpose: both front ends already turn that into
+            // "not a usable Docker endpoint", so there is one phrasing for a bad endpoint
+            // rather than two.
+            throw new UriFormatException(
+                "Use a unix socket (unix:///var/run/docker.sock) or a TCP endpoint "
+                + "(tcp://host:2375). For an ssh:// context, forward the socket first — "
+                + "ssh -L 2375:/var/run/docker.sock user@host — and point --docker-host at that.");
+
         // tcp:// is the scheme people have in DOCKER_HOST, but it is plain HTTP on the wire.
         var uri = new Uri(dockerHost.Replace("tcp://", "http://", StringComparison.OrdinalIgnoreCase));
 
         return (new HttpClient { BaseAddress = uri }, dockerHost);
     }
 
+    private static bool StartsWithScheme(string value, string scheme) =>
+        value.StartsWith(scheme, StringComparison.OrdinalIgnoreCase);
+
     private static HttpClient UnixSocketClient(string socketPath) {
         var handler = new SocketsHttpHandler {
             ConnectCallback = async (_, cancellationToken) => {

RackPeek.Domain/Discovery/ProxmoxModels.cs

@@ -462,15 +462,39 @@ public static class ProxmoxResponseParser {
             ? value.GetString()
             : null;
 
-    private static int? GetInt(JsonElement element, string name) =>
-        element.TryGetProperty(name, out JsonElement value) && value.TryGetInt32(out var result)
-            ? result
-            : null;
+    /// <summary>
+    ///     A number Proxmox may have written as a string.
+    ///     <para>
+    ///         Its perl backend quotes numeric fields inconsistently — <c>"vmid":"100"</c>
+    ///         and <c>"maxdisk":"512110190592"</c> both turn up across versions. The
+    ///         <c>TryGetInt32</c> family does not return false for a string, it throws, so
+    ///         reading one unguarded took the whole run down with a stack trace. The kind
+    ///         is checked first and a quoted number parsed, which is what the caller meant
+    ///         either way.
+    ///     </para>
+    /// </summary>
+    private static int? GetInt(JsonElement element, string name) {
+        if (!element.TryGetProperty(name, out JsonElement value))
+            return null;
 
-    private static long? GetLong(JsonElement element, string name) =>
-        element.TryGetProperty(name, out JsonElement value) && value.TryGetInt64(out var result)
-            ? result
-            : null;
+        return value.ValueKind switch {
+            JsonValueKind.Number when value.TryGetInt32(out var number) => number,
+            JsonValueKind.String when int.TryParse(value.GetString(), out var parsed) => parsed,
+            _ => null
+        };
+    }
+
+    /// <inheritdoc cref="GetInt" />
+    private static long? GetLong(JsonElement element, string name) {
+        if (!element.TryGetProperty(name, out JsonElement value))
+            return null;
+
+        return value.ValueKind switch {
+            JsonValueKind.Number when value.TryGetInt64(out var number) => number,
+            JsonValueKind.String when long.TryParse(value.GetString(), out var parsed) => parsed,
+            _ => null
+        };
+    }
 
     /// <summary>
     ///     Addresses a QEMU guest reports through its guest agent

RackPeek.Mcp/Tools/DiscoveryTools.cs

@@ -1,3 +1,4 @@
+using System.ComponentModel.DataAnnotations;
 using System.ComponentModel;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
@@ -79,9 +80,17 @@ public sealed class DiscoveryTools(IServiceProvider services) {
                 ? host.MachineId ?? host.Hostname
                 : engine?.Id ?? client.Endpoint;
 
+            // No fallback to this machine's address: it is not the remote engine's, and
+            // stamping it on would give every service a confidently wrong one.
             var serviceIp = client.IsLocal
                 ? host.Ip
-                : await DockerApiClient.ResolveIpv4Async(client.RemoteHost!, cancellationToken) ?? host.Ip;
+                : await DockerApiClient.ResolveIpv4Async(client.RemoteHost!, cancellationToken);
+
+            if (string.IsNullOrWhiteSpace(serviceIp))
+                throw new ValidationException(
+                    $"Could not determine an IPv4 address for {client.Endpoint}. Services are "
+                    + "recorded at their host's address and the inventory holds IPv4 only; "
+                    + "dial the engine by address instead, e.g. tcp://192.0.2.10:2375.");
 
             List<Service> found = DockerServiceMapper.ToResources(containers, seed, effectiveHost, serviceIp);
 

Shared.Rcl/Commands/Discovery/DiscoverDockerCommand.cs

@@ -86,10 +86,22 @@ public sealed class DiscoverDockerCommand(IEnumerable<ISystemProbe> probes)
             : engine?.Id ?? client.Endpoint;
 
         // Published ports live on the engine host, so a remote service's address is the
-        // endpoint the user dialled — the local probe's address is only the last resort.
+        // endpoint the user dialled. There is deliberately no fallback: this machine's
+        // own address is not the remote engine's, and stamping it on would put a
+        // confidently wrong address on every service — one that then flows into the
+        // ansible, ssh and hosts exports.
         var serviceIp = client.IsLocal
             ? host.Ip
-            : await DockerApiClient.ResolveIpv4Async(client.RemoteHost!, cancellationToken) ?? host.Ip;
+            : await DockerApiClient.ResolveIpv4Async(client.RemoteHost!, cancellationToken);
+
+        if (string.IsNullOrWhiteSpace(serviceIp)) {
+            AnsiConsole.MarkupLine(
+                $"[red]Could not determine an IPv4 address for {Markup.Escape(client.Endpoint)}.[/] "
+                + "Services are recorded at their host's address, and the inventory holds IPv4 only. "
+                + "Dial the engine by address instead, e.g. --docker-host tcp://192.0.2.10:2375");
+
+            return 1;
+        }
 
         List<Service> services = DockerServiceMapper.ToResources(containers, seed, hostName, serviceIp);
 

Tests.Discovery/ProxmoxQuotedNumberTests.cs

@@ -0,0 +1,63 @@
+using RackPeek.Domain.Discovery;
+
+namespace Tests.Discovery;
+
+/// <summary>
+///     Proxmox's perl backend quotes numeric fields inconsistently — <c>"vmid":"100"</c>
+///     and <c>"maxdisk":"512110190592"</c> both turn up across versions. The
+///     <c>TryGetInt32</c> family does not return false for a string, it throws, so a
+///     single quoted number used to take the whole run down with a stack trace: the CLI
+///     printed "Unexpected error occurred" and MCP forwarded the BCL's
+///     "requires an element of type 'Number'" as if it were the user's fault.
+/// </summary>
+public class ProxmoxQuotedNumberTests {
+    [Fact]
+    public void A_guest_whose_numbers_are_quoted_is_read_rather_than_throwing() {
+        var json = """
+                   {"data":[{"vmid":"100","name":"quoted-guest","cpus":"4",
+                   "maxmem":"4294967296","maxdisk":"512110190592","status":"running"}]}
+                   """;
+
+        ProxmoxGuest guest = Assert.Single(ProxmoxResponseParser.ParseGuests(json, "pve01", "vm"));
+
+        Assert.Equal(100, guest.VmId);
+        Assert.Equal("quoted-guest", guest.Name);
+        Assert.Equal(4, guest.Cores);
+        Assert.Equal(4294967296, guest.MemoryBytes);
+        Assert.Equal(512110190592, guest.DiskBytes);
+    }
+
+    [Fact]
+    public void Plain_numbers_still_read_the_same_way() {
+        var json = """
+                   {"data":[{"vmid":101,"name":"plain-guest","cpus":2,
+                   "maxmem":2147483648,"maxdisk":34359738368,"status":"running"}]}
+                   """;
+
+        ProxmoxGuest guest = Assert.Single(ProxmoxResponseParser.ParseGuests(json, "pve01", "vm"));
+
+        Assert.Equal(101, guest.VmId);
+        Assert.Equal(2, guest.Cores);
+        Assert.Equal(2147483648, guest.MemoryBytes);
+    }
+
+    [Fact]
+    public void A_field_that_is_neither_a_number_nor_a_numeric_string_is_simply_absent() {
+        // Guessing at "N/A" would be worse than leaving the field empty, and it must
+        // still not throw.
+        var json = """{"data":[{"vmid":102,"name":"odd-guest","cpus":"N/A","maxmem":null}]}""";
+
+        ProxmoxGuest guest = Assert.Single(ProxmoxResponseParser.ParseGuests(json, "pve01", "vm"));
+
+        Assert.Equal(0, guest.Cores);
+        Assert.Equal(0, guest.MemoryBytes);
+    }
+
+    [Fact]
+    public void A_guest_whose_vmid_is_quoted_is_still_identified() {
+        // vmid is the guest's identity; dropping it would silently lose the guest.
+        var json = """{"data":[{"vmid":"103","name":"id-guest"}]}""";
+
+        Assert.Equal(103, Assert.Single(ProxmoxResponseParser.ParseGuests(json, "pve01", "vm")).VmId);
+    }
+}

Tests.Discovery/RemoteDockerDiscoveryTests.cs

@@ -179,4 +179,33 @@ public class RemoteDockerDiscoveryTests {
 
         return host;
     }
+
+    // -- endpoints this cannot reach ---------------------------------------------------
+
+    [Theory]
+    // `docker context` sets this for a remote host, so it is a perfectly normal value to
+    // find in DOCKER_HOST.
+    [InlineData("ssh://user@nas")]
+    // The Windows default.
+    [InlineData("npipe:////./pipe/docker_engine")]
+    [InlineData("gibberish://nowhere")]
+    public void An_endpoint_scheme_that_cannot_be_dialled_is_refused_with_advice(string endpoint) {
+        // HttpClient accepts these URIs happily and only throws NotSupportedException on
+        // the first request — which the caller does not catch, so discovery used to die
+        // with a stack trace instead of saying what was wrong.
+        UriFormatException error = Assert.Throws<UriFormatException>(() => new DockerApiClient(endpoint));
+
+        Assert.Contains("ssh -L", error.Message);
+    }
+
+    [Theory]
+    [InlineData("tcp://192.0.2.10:2375")]
+    [InlineData("http://192.0.2.10:2375")]
+    [InlineData("unix:///var/run/docker.sock")]
+    [InlineData("unix:///run/user/1000/podman/podman.sock")]
+    public void The_endpoints_that_do_work_are_untouched(string endpoint) {
+        using var client = new DockerApiClient(endpoint);
+
+        Assert.Equal(endpoint, client.Endpoint);
+    }
 }

Tests.Mcp/DiscoveryToolTests.cs

@@ -193,4 +193,28 @@ public class DiscoveryToolTests {
         Assert.Contains("Could not read", error);
         Assert.Contains("http://127.0.0.1:1", error);
     }
+
+    [Fact]
+    public async Task An_engine_with_no_ipv4_address_is_refused_rather_than_given_this_machines() {
+        // Services are recorded at their host's address. When the engine answers but has
+        // no IPv4 — the inventory holds IPv4 only — the address is genuinely unknown, and
+        // the code used to substitute the address of whatever machine ran the command.
+        // Every service then carried a confident, wrong address that flowed on into the
+        // ansible, ssh and hosts exports.
+        if (!System.Net.Sockets.Socket.OSSupportsIPv6)
+            return; // no loopback to bind; nothing to prove here on this host
+
+        await using FakeHttpServer engine = await FakeHttpServer.StartDockerEngineAsync(true);
+        using var api = new McpFixture();
+        await using McpClient client = await api.ConnectAsync();
+
+        Exception error = await Assert.ThrowsAnyAsync<Exception>(() =>
+            client.CallOkAsync<DiscoveryResult>("discover_docker", new Dictionary<string, object?> {
+                ["dockerHost"] = $"tcp://{engine.Host}"
+            }));
+
+        Assert.Contains("IPv4", error.Message);
+        // Nothing was recorded at a borrowed address.
+        Assert.DoesNotContain("jellyfin", api.StoredYaml);
+    }
 }

Tests.Mcp/FakeHttpServer.cs

@@ -21,10 +21,13 @@ internal sealed class FakeHttpServer : IAsyncDisposable {
 
     public string Host => new Uri(BaseUrl).Authority;
 
-    public static async Task<FakeHttpServer> StartAsync(Action<WebApplication> map) {
+    public static async Task<FakeHttpServer> StartAsync(Action<WebApplication> map, bool ipv6 = false) {
         WebApplicationBuilder builder = WebApplication.CreateBuilder();
         builder.Logging.ClearProviders();
-        builder.WebHost.UseUrls("http://127.0.0.1:0");
+
+        // An IPv6-only endpoint is the one case where an engine answers but has no IPv4
+        // address to record its services at.
+        builder.WebHost.UseUrls(ipv6 ? "http://[::1]:0" : "http://127.0.0.1:0");
 
         WebApplication app = builder.Build();
         map(app);
@@ -34,13 +37,13 @@ internal sealed class FakeHttpServer : IAsyncDisposable {
     }
 
     /// <summary>A fake Docker Engine API with the shared captured fixtures.</summary>
-    public static Task<FakeHttpServer> StartDockerEngineAsync() =>
+    public static Task<FakeHttpServer> StartDockerEngineAsync(bool ipv6 = false) =>
         StartAsync(app => {
             app.MapGet("/containers/json", () => Results.Content(
                 TestData.Fixture("docker-containers.json"), "application/json"));
             app.MapGet("/info", () => Results.Content(
                 TestData.Fixture("docker-info.json"), "application/json"));
-        });
+        }, ipv6);
 
     /// <summary>
     ///     A fake Proxmox VE API. Both fixture nodes answer with the same guest lists,

Tests/EndToEnd/DiscoveryTests/DiscoverDockerEndpointTests.cs

@@ -0,0 +1,33 @@
+using Tests.EndToEnd.Infra;
+using Xunit.Abstractions;
+
+namespace Tests.EndToEnd.DiscoveryTests;
+
+/// <summary>
+///     `rpk discover docker` against endpoints it cannot reach. Every case here fails
+///     before any container is listed, so these tests never talk to a daemon.
+/// </summary>
+[Collection("Yaml CLI tests")]
+public class DiscoverDockerEndpointTests(TempYamlCliFixture fs, ITestOutputHelper outputHelper)
+    : IClassFixture<TempYamlCliFixture> {
+    private async Task<string> ExecuteAsync(params string[] args) =>
+        await YamlCliTestHost.RunAsync(args, fs.Root, outputHelper, "config.yaml");
+
+    [Theory]
+    [InlineData("ssh://user@nas")]
+    [InlineData("npipe:////./pipe/docker_engine")]
+    public async Task an_endpoint_that_cannot_be_dialled_says_so_instead_of_crashing(string endpoint) {
+        // `docker context` sets ssh:// for a remote host and npipe:// is the Windows
+        // default, so both are ordinary values to find in DOCKER_HOST. They used to
+        // reach HttpClient, which accepts the URI and throws NotSupportedException on
+        // the first request — past the command's catch list, so the user got
+        // "Unexpected error occurred" and a stack trace.
+        var output = await ExecuteAsync("discover", "docker", "--docker-host", endpoint);
+
+        Assert.DoesNotContain("Unexpected error", output);
+        Assert.DoesNotContain("at RackPeek.", output);
+
+        // And the message says what to do about it.
+        Assert.Contains("ssh -L", output);
+    }
+}