Address review feedback on upgrade detection

- Lowercase an uppercase "V" tag prefix so semver comparison works;
  "V1.2.0" vs "V1.3.0" no longer falls through to undecidable and
  hides a real upgrade.
- Drop the raw provider error from CheckResult.Reason (it is serialized
  into the API response and can leak internal addressing); log it at
  DEBUG and return a fixed string. Same for the CheckAll path.
- Add a defensive default to the comparison switch so an unexpected
  value yields StatusUnknown rather than the least-safe StatusUpToDate.
- Stop reporting network-isolation drift: the registry has no
  network-isolation field, so it fired for every isolated workload
  regardless of the candidate version. Remove the ConfigDrift field
  and the now-unused BoolChange type.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: JAORMX

pkg/workloads/upgrade/checker.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"log/slog"
 
 	regtypes "github.com/stacklok/toolhive-core/registry/types"
 	"github.com/stacklok/toolhive/pkg/registry"
@@ -61,8 +62,13 @@ func (c *Checker) Check(_ context.Context, cfg *runner.RunConfig) (*CheckResult,
 			result.Status = StatusServerNotFound
 			return result, nil
 		}
+		// Keep the detailed provider error out of the result: Reason is
+		// serialized into the HTTP response, and for an unreachable or
+		// misconfigured registry the raw error can carry internal addressing
+		// (e.g. "dial tcp 10.x.x.x:443: ..."). Log it for operators instead.
+		slog.Debug("registry lookup failed", "server", cfg.RegistryServerName, "error", err)
 		result.Status = StatusUnknown
-		result.Reason = fmt.Sprintf("registry lookup failed: %v", err)
+		result.Reason = "registry lookup failed"
 		return result, nil
 	}
 
@@ -86,6 +92,11 @@ func (c *Checker) Check(_ context.Context, cfg *runner.RunConfig) (*CheckResult,
 	case comparisonUndecidable:
 		result.Status = StatusUnknown
 		result.Reason = reason
+	default:
+		// Defensive: a future tagComparison value (or an unset zero value) must
+		// not fall through to the least-safe StatusUpToDate. Treat anything
+		// unexpected as unknown.
+		result.Status = StatusUnknown
 	}
 
 	return result, nil
@@ -105,10 +116,11 @@ func (c *Checker) CheckAll(ctx context.Context, configs []*runner.RunConfig) []*
 		// so the error here is unreachable; encode defensively rather than drop.
 		res, err := c.Check(ctx, cfg)
 		if err != nil {
+			slog.Debug("upgrade check failed", "workload", cfg.Name, "error", err)
 			res = &CheckResult{
 				WorkloadName: cfg.Name,
 				Status:       StatusUnknown,
-				Reason:       fmt.Sprintf("check failed: %v", err),
+				Reason:       "check failed",
 			}
 		}
 		results = append(results, res)
@@ -176,13 +188,6 @@ func computeConfigDrift(cfg *runner.RunConfig, imgMeta *regtypes.ImageMetadata)
 		drift.Transport = &StringChange{From: currentTransport, To: candidateTransport}
 	}
 
-	// Network isolation is a plain boolean on the config; the registry entry has
-	// no explicit network-isolation field, so the candidate posture is the
-	// default (false). Report drift only when the workload currently isolates.
-	if cfg.IsolateNetwork {
-		drift.NetworkIsolation = &BoolChange{From: true, To: false}
-	}
-
 	// Permission profile: compare names. The candidate name is only known when
 	// the registry entry carries a profile with a non-empty Name.
 	candidateProfile := ""
@@ -194,7 +199,7 @@ func computeConfigDrift(cfg *runner.RunConfig, imgMeta *regtypes.ImageMetadata)
 		drift.PermissionProfile = &StringChange{From: currentProfile, To: candidateProfile}
 	}
 
-	if drift.Transport == nil && drift.NetworkIsolation == nil && drift.PermissionProfile == nil {
+	if drift.Transport == nil && drift.PermissionProfile == nil {
 		return nil
 	}
 	return drift

pkg/workloads/upgrade/checker_test.go

@@ -148,7 +148,6 @@ func TestChecker_Check(t *testing.T) {
 				Image:                       "ghcr.io/example/server:1.0.0",
 				RegistryServerName:          testServerName,
 				Transport:                   transporttypes.TransportTypeStdio,
-				IsolateNetwork:              true,
 				PermissionProfileNameOrPath: "none",
 				EnvVars:                     map[string]string{"LOG_LEVEL": "info"},
 				Secrets:                     []string{"mykey,target=API_KEY"},
@@ -182,9 +181,6 @@ func TestChecker_Check(t *testing.T) {
 				require.NotNil(t, res.ConfigDrift.Transport)
 				assert.Equal(t, "stdio", res.ConfigDrift.Transport.From)
 				assert.Equal(t, "streamable-http", res.ConfigDrift.Transport.To)
-				require.NotNil(t, res.ConfigDrift.NetworkIsolation)
-				assert.True(t, res.ConfigDrift.NetworkIsolation.From)
-				assert.False(t, res.ConfigDrift.NetworkIsolation.To)
 				require.NotNil(t, res.ConfigDrift.PermissionProfile)
 				assert.Equal(t, "none", res.ConfigDrift.PermissionProfile.From)
 				assert.Equal(t, "network", res.ConfigDrift.PermissionProfile.To)
@@ -279,16 +275,15 @@ func TestComputeConfigDrift_GracefulDegradation(t *testing.T) {
 		assert.Nil(t, computeConfigDrift(cfg, meta))
 	})
 
-	t.Run("network isolation alone drifts", func(t *testing.T) {
+	t.Run("network isolation alone does not drift", func(t *testing.T) {
 		t.Parallel()
+		// The registry has no network-isolation field, so a workload's own
+		// isolation choice is not registry-driven drift and must not be
+		// reported (it would otherwise fire for every isolated workload).
 		cfg := &runner.RunConfig{IsolateNetwork: true}
 		meta := imageMeta("ghcr.io/example/server:1.0.0")
 		meta.Transport = ""
-		drift := computeConfigDrift(cfg, meta)
-		require.NotNil(t, drift)
-		require.NotNil(t, drift.NetworkIsolation)
-		assert.Nil(t, drift.Transport)
-		assert.Nil(t, drift.PermissionProfile)
+		assert.Nil(t, computeConfigDrift(cfg, meta))
 	})
 }
 

pkg/workloads/upgrade/types.go

@@ -110,10 +110,6 @@ type ConfigDrift struct {
 	// workload's current transport.
 	Transport *StringChange `json:"transport,omitempty"`
 
-	// NetworkIsolation is set when the candidate's network isolation posture
-	// differs from the workload's current setting.
-	NetworkIsolation *BoolChange `json:"network_isolation,omitempty"`
-
 	// PermissionProfile is set when the candidate's permission profile differs
 	// from the workload's current profile.
 	PermissionProfile *StringChange `json:"permission_profile,omitempty"`
@@ -126,13 +122,6 @@ type StringChange struct {
 	To   string `json:"to"`
 }
 
-// BoolChange records a boolean-valued configuration change from the workload's
-// current value to the candidate registry value.
-type BoolChange struct {
-	From bool `json:"from"`
-	To   bool `json:"to"`
-}
-
 // ApplyOptions controls how an upgrade is applied to a workload.
 //
 // It is defined here for use by the Applier (Phase D). No apply logic is

pkg/workloads/upgrade/version.go

@@ -103,15 +103,24 @@ func splitRepoTag(ref string) (repo, tag string, err error) {
 
 // normalizeSemver prepends a "v" to a tag that looks like a bare semantic
 // version (e.g. "1.2.3" -> "v1.2.3") so it can be validated and compared by
-// golang.org/x/mod/semver, which requires the "v" prefix. Tags that already
-// start with "v", or that are not semver-shaped, are returned unchanged.
+// golang.org/x/mod/semver, which requires a lowercase "v" prefix. Tags that
+// already start with a lowercase "v" are returned unchanged; an uppercase "V"
+// prefix is lowercased (semver.IsValid rejects "V1.2.3"). Tags that are not
+// semver-shaped are returned unchanged.
 func normalizeSemver(tag string) string {
 	if tag == "" {
 		return tag
 	}
-	if tag[0] == 'v' || tag[0] == 'V' {
+	if tag[0] == 'v' {
 		return tag
 	}
+	if tag[0] == 'V' {
+		// semver.IsValid only accepts a lowercase "v"; without this an
+		// uppercase-tagged pair (e.g. "V1.2.0" vs "V1.3.0") falls through to
+		// the non-semver path and reports an undecidable comparison, hiding a
+		// real upgrade.
+		return "v" + tag[1:]
+	}
 	if tag[0] >= '0' && tag[0] <= '9' {
 		return "v" + tag
 	}

pkg/workloads/upgrade/version_test.go

@@ -31,6 +31,12 @@ func TestCompareImageTags(t *testing.T) {
 			candidate: "ghcr.io/example/server:v2.0.0",
 			want:      comparisonNewer,
 		},
+		{
+			name:      "candidate newer semver with uppercase V prefix",
+			current:   "ghcr.io/example/server:V1.2.0",
+			candidate: "ghcr.io/example/server:V1.3.0",
+			want:      comparisonNewer,
+		},
 		{
 			name:      "double-digit minor orders numerically not lexically",
 			current:   "ghcr.io/example/server:1.9.0",
@@ -190,7 +196,7 @@ func TestNormalizeSemver(t *testing.T) {
 	}{
 		{name: "bare version gets v prefix", in: "1.2.3", want: "v1.2.3"},
 		{name: "already prefixed unchanged", in: "v1.2.3", want: "v1.2.3"},
-		{name: "uppercase V unchanged", in: "V1.2.3", want: "V1.2.3"},
+		{name: "uppercase V lowercased for semver validation", in: "V1.2.3", want: "v1.2.3"},
 		{name: "non-semver tag unchanged", in: "stable", want: "stable"},
 		{name: "empty unchanged", in: "", want: ""},
 		{name: "numeric prefix gets v", in: "2024.01", want: "v2024.01"},