Isolate skills client discovery in default-client test (#5417)

TestNewDefaultClient stubbed the TOOLHIVE_API_URL env reader but let
the server-discovery step run against real local state. When a thv or
Desktop server was running, discovery found it and returned its base
URL, so the "falls back to default URL" and "applies options" subtests
saw the discovered URL instead of the default and failed. CI passed
only because no server is running there.

Inject the discovery step into newDefaultClientWithEnv, mirroring the
existing env.Reader injection, and have the subtests stub it. Each
resolution step (env var, discovery, default fallback) is now tested
in isolation, and a new subtest covers the discovered-server path.

Co-authored-by: Dan Barr <6922515+danbarr@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: danbarr

pkg/skills/client/client.go

@@ -83,18 +83,26 @@ func NewClient(baseURL string, opts ...Option) *Client {
 //
 // The context is used for the server discovery health check; it is not stored.
 func NewDefaultClient(ctx context.Context, opts ...Option) *Client {
-	return newDefaultClientWithEnv(ctx, &env.OSReader{}, opts...)
+	return newDefaultClientWithEnv(ctx, &env.OSReader{}, resolveViaDiscovery, opts...)
 }
 
-// newDefaultClientWithEnv is the testable core of NewDefaultClient.
-func newDefaultClientWithEnv(ctx context.Context, envReader env.Reader, opts ...Option) *Client {
+// discoverFunc resolves a running server's base URL and any transport options
+// (e.g. a Unix socket client). It returns an empty base URL when no running
+// server is found. resolveViaDiscovery is the production implementation; tests
+// inject a stub so the discovery step does not read real local state.
+type discoverFunc func(ctx context.Context) (string, []Option)
+
+// newDefaultClientWithEnv is the testable core of NewDefaultClient. The
+// envReader and discover dependencies are injected so each resolution step
+// can be exercised in isolation.
+func newDefaultClientWithEnv(ctx context.Context, envReader env.Reader, discover discoverFunc, opts ...Option) *Client {
 	// 1. Explicit env var override always wins.
 	if base := envReader.Getenv(envAPIURL); base != "" {
 		return NewClient(base, opts...)
 	}
 
 	// 2. Try server discovery.
-	if base, httpOpts := resolveViaDiscovery(ctx); base != "" {
+	if base, httpOpts := discover(ctx); base != "" {
 		// Discovery opts go first so caller-supplied opts can override them
 		// (e.g. a caller-provided WithTimeout replaces the discovery default).
 		merged := make([]Option, 0, len(httpOpts)+len(opts))

pkg/skills/client/client_test.go

@@ -4,6 +4,7 @@
 package client
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"io"
@@ -636,13 +637,27 @@ func TestConnectionError(t *testing.T) {
 func TestNewDefaultClient(t *testing.T) {
 	t.Parallel()
 
+	// noDiscovery stubs server discovery to report no running server, isolating
+	// the test from any real local server (e.g. a running Desktop app).
+	noDiscovery := func(context.Context) (string, []Option) { return "", nil }
+
+	// failDiscovery fails the test if discovery is consulted, asserting that an
+	// earlier resolution step short-circuited.
+	failDiscovery := func(t *testing.T) discoverFunc {
+		t.Helper()
+		return func(context.Context) (string, []Option) {
+			t.Error("discovery should not be called when TOOLHIVE_API_URL is set")
+			return "", nil
+		}
+	}
+
 	t.Run("falls back to default URL when env is empty", func(t *testing.T) {
 		t.Parallel()
 		ctrl := gomock.NewController(t)
 		mockEnv := envmocks.NewMockReader(ctrl)
 		mockEnv.EXPECT().Getenv(envAPIURL).Return("")
 
-		c := newDefaultClientWithEnv(t.Context(), mockEnv)
+		c := newDefaultClientWithEnv(t.Context(), mockEnv, noDiscovery)
 		assert.Equal(t, defaultBaseURL, c.baseURL)
 	})
 
@@ -652,17 +667,30 @@ func TestNewDefaultClient(t *testing.T) {
 		mockEnv := envmocks.NewMockReader(ctrl)
 		mockEnv.EXPECT().Getenv(envAPIURL).Return("http://localhost:9999")
 
-		c := newDefaultClientWithEnv(t.Context(), mockEnv)
+		c := newDefaultClientWithEnv(t.Context(), mockEnv, failDiscovery(t))
 		assert.Equal(t, "http://localhost:9999", c.baseURL)
 	})
 
+	t.Run("uses discovered server when env is empty", func(t *testing.T) {
+		t.Parallel()
+		ctrl := gomock.NewController(t)
+		mockEnv := envmocks.NewMockReader(ctrl)
+		mockEnv.EXPECT().Getenv(envAPIURL).Return("")
+
+		discover := func(context.Context) (string, []Option) {
+			return "http://127.0.0.1:54321", nil
+		}
+		c := newDefaultClientWithEnv(t.Context(), mockEnv, discover)
+		assert.Equal(t, "http://127.0.0.1:54321", c.baseURL)
+	})
+
 	t.Run("applies options", func(t *testing.T) {
 		t.Parallel()
 		ctrl := gomock.NewController(t)
 		mockEnv := envmocks.NewMockReader(ctrl)
 		mockEnv.EXPECT().Getenv(envAPIURL).Return("")
 
-		c := newDefaultClientWithEnv(t.Context(), mockEnv, WithTimeout(5*time.Second))
+		c := newDefaultClientWithEnv(t.Context(), mockEnv, noDiscovery, WithTimeout(5*time.Second))
 		assert.Equal(t, 5*time.Second, c.httpClient.Timeout)
 	})
 }