Merge branch 'refactor/category-drift-guards'

Category drift-guards + load-path consolidation (review findings #3, #5):
collapse handleDataMsg to its live roles and add drift-guard tests across the
tui/domain/keys dispatch planes so a future category that misses a plane fails
a test instead of drifting silently.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: jingle2008

docs/superpowers/plans/2026-06-22-category-drift-guards.md

@@ -0,0 +1,188 @@
+# Category Drift-Guards + Load-Path Consolidation — Design
+
+**Date:** 2026-06-22
+**Status:** Approved (pending implementation)
+**Findings:** Code-quality review #3 (category behavior duplicated across dispatch planes) and #5 (overlapping old/new load-message paths).
+
+## Problem
+
+Per-category behavior is encoded across ~19 dispatch planes in ~14 files (domain
+methods, the columns registry, a dozen TUI maps/switches, CLI `get`, MCP tools).
+Several planes default-fall-through **silently** when a category is missing —
+which is how the original GPUWorkload lazy-load bug shipped. Separately, the TUI
+has a `dataMsg` type-switch whose per-category cases duplicate the typed
+`*LoadedMsg` handlers, and that switch silently omits GPUWorkload.
+
+The chosen scope (explicitly **not** a structural rewrite) is to:
+1. Collapse the duplicated load-path so there is one per-category path (#5).
+2. Add drift-guards across the partial planes so a future category that misses a
+   plane **fails a test** instead of drifting silently (#3, root-cause mitigation).
+
+## Non-goals
+
+- No single "category descriptor" struct driving all planes (the larger #3
+  rewrite). Deferred.
+- No shared CLI/MCP/TUI loader resolver (the medium #3 option). Deferred.
+- No behavior change to any user-facing command. The only production code change
+  is removing dead branches in `handleDataMsg` (#5).
+
+## Section 1 — Collapse the dual load-path (#5)
+
+### Current state
+
+- `dataMsg{Data any, Gen int}` has two **live** production roles:
+  - `dataMsg{Data: *models.Dataset}` — the foundational Init load, produced via
+    `datasetLoadedMsg` → `handleDataMsg` (`model_update.go`).
+  - `dataMsg{}` (nil Data) — a refresh-display signal from `refreshDataCmd`
+    (`reducer_category.go:100`).
+- `handleDataMsg` (`model_reducer.go`) type-switches on `msg.Data` with
+  per-category cases (`[]models.BaseModel`, `map[string][]models.ImportedModel`,
+  `[]models.GPUPool` + `updateGPUPoolState()`, `map[string][]models.GPUNode`,
+  `map[string][]models.DedicatedAICluster`, `models.TenancyOverrideGroup`).
+  These cases are **dead in production**: every per-category load now flows
+  through typed `*LoadedMsg` → `routeListLoadedMsg` → the typed handlers
+  (`handleBaseModelsLoaded`, …). Only tests still drive the `dataMsg`
+  per-category cases. The switch omits GPUWorkload — a latent asymmetry, not a
+  live bug.
+
+### Change
+
+Reduce `handleDataMsg` to its live roles:
+
+```go
+func (m *Model) handleDataMsg(msg dataMsg) tea.Cmd {
+    // gen-guard unchanged
+    if msg.Gen != 0 && msg.Gen != m.gens.msg {
+        m.endTask(true)
+        return nil
+    }
+    if ds, ok := msg.Data.(*models.Dataset); ok {
+        m.dataset = ds
+        m.endTask(true)
+        m.logger.Infow("data loaded", "category", m.category, "pendingTasks", m.pendingTasks)
+    }
+    m.refreshDisplay()
+    return nil
+}
+```
+
+- Removes the dead per-category cases. `updateGPUPoolState()` is unaffected — it
+  is already invoked by the typed `handleGPUPoolsLoaded` path.
+- `dataMsg` keeps both live roles; the per-category responsibility now lives
+  solely in the typed family. The overlap (#5) and GPUWorkload omission are gone.
+
+### Tests touched
+
+- `TestProcessDataAndErrorMsg` (`model_test.go:261-269`) currently drives
+  `handleDataMsg` with per-category `dataMsg` payloads. Repoint these at the
+  typed handlers (`m.handleBaseModelsLoaded(...)`, `m.handleGPUPoolsLoaded(...)`,
+  etc.) so real-path coverage is preserved rather than testing removed branches.
+- `messages_test.go` (`dataMsg{Data: data}`) — adjust if it asserts a removed
+  case; keep the `*models.Dataset` and empty-`dataMsg` assertions.
+- Empty `dataMsg{}` usages (refresh signal) across other tests are unchanged.
+
+### Guard for #5
+
+A test asserting `handleDataMsg` only acts on `*models.Dataset`/refresh — i.e.
+feeding it a per-category payload (e.g. `[]models.GPUPool`) does **not** mutate
+the dataset (that path belongs to the typed handler). This locks in the
+single-path invariant.
+
+## Section 2 — Drift-guard catalog
+
+New `*_drift_test.go` files, one per package, each iterating `domain.Categories`.
+Mirrors the established pattern (`TestRowSources_CoversAllCategories`,
+`internal/columns/registry_test.go`). **No production changes** beyond Section 1:
+the command-constructor switches (`startK8sWatchCmd`, `reloadCategoryCmd`) build
+their `tea.Cmd` lazily, so a test can call them per-category and assert
+handled-vs-nil without triggering any watch or load.
+
+Three guard styles:
+- **Predicate invariant** — plane membership is derived from a domain truth; the
+  guard self-maintains (no list to update).
+- **Behavior-probe** — call the plane's function for each category and assert the
+  expected handled/excluded outcome (for switches that can't be introspected).
+- **Account-for-all** — for genuine per-category UI choices with no domain
+  predicate: a table requires every category to be present or in an explicit
+  exclusion set, so a new category fails until someone makes the decision.
+
+### `internal/domain` — `category_drift_test.go`
+
+- `NeedsKubeConfig`: account-for-all — `want := map[Category]bool{…}` with an
+  entry for every `domain.Categories`; assert `len(want) == len(Categories)` and
+  `want[c] == c.NeedsKubeConfig()` for each. New category → missing entry → fail.
+- `ScopedCategories`/`Parents`: structural invariant — for every category `c` and
+  every `child` in `c.ScopedCategories()`, assert `c` appears in
+  `child.Parents()` (round-trip consistency, no list). Plus an account-for-all
+  scope table asserting each category's `ScopedCategories()` matches an expected
+  map covering all categories.
+- `Aliases`: invariant — every category has ≥1 alias; aliases are unique across
+  categories (no collisions); `ParseCategory(alias) == c` round-trips for every
+  alias of every category.
+
+### `internal/ui/tui` — `category_drift_test.go`
+
+- `lazyLoadedCategories`: predicate — every `c.NeedsKubeConfig()` category is in
+  the set. (Supersedes/absorbs the existing
+  `TestLazyLoadedCategories_CoversKubeBacked`; remove the old one to avoid
+  duplication.)
+- `handlers` map (`reducer_category.go`): predicate — every kube-backed category
+  has an entry.
+- `startK8sWatchCmd`: behavior-probe — returns non-nil `tea.Cmd` for every
+  kube-backed category and nil for non-kube categories.
+- `reloadCategoryCmd`: behavior-probe — non-nil for the same kube-backed set.
+- `statsColumns`: account-for-all — every category present or in an explicit
+  `noStats` exclusion set; assert the union covers all categories.
+- `itemKeyFrom`: behavior-probe — returns a non-nil `ItemKey` for every category,
+  probing with a representative `table.Row`. If a synthetic row proves
+  impractical for some categories, the implementer may instead assert that
+  `itemKeyFrom`'s flat-vs-grouped classification accounts for every category via
+  an account-for-all table; either mechanism must fail when a category is added
+  without handling.
+- `parentScope`: assert consistency with the domain scope graph — every category
+  with `Parents()` is handled by `parentScope` (probe, or account-for-all over
+  the scope-category set if a synthetic row is impractical).
+- `deleteItem` ↔ `confirmDelete` consistency: the set of categories for which
+  `deleteItem` returns non-nil equals the set `confirmDelete` assigns a tier to;
+  assert they agree (guards the finding-#2 coupling from the confirmation work).
+
+### `internal/ui/tui/keys` — `category_drift_test.go`
+
+- `catContext`: account-for-all — every category present or in an explicit
+  `noContextKeys` exclusion set; assert the union covers all categories.
+
+### Already guarded (keep, do not duplicate)
+
+- `columns/registry` (`registry_test.go`), `rowSources`
+  (`TestRowSources_CoversAllCategories`).
+
+## Architecture / isolation
+
+- Each guard test is self-contained in its package's `*_drift_test.go`, reading
+  unexported registries directly (same-package tests) — no new production
+  exports.
+- Explicit exclusion sets (`noStats`, `noContextKeys`, expected-value tables) live
+  beside their guard test, documented with one line on why each category is
+  excluded.
+
+## Error handling
+
+Guards are tests; failures surface at `make ci` / `go test`. No runtime paths
+change. The #5 change removes branches only; the foundational-load and
+refresh-signal paths are preserved exactly.
+
+## Testing
+
+- Section 1: the repointed `TestProcessDataAndErrorMsg`, the new single-path
+  guard, and the existing TUI suite must stay green.
+- Section 2: each new guard must **fail if a category is removed from its plane**
+  — verify by temporarily dropping a known category from one plane and confirming
+  the guard catches it (then revert). This proves the guard actually guards.
+- Full `make ci` green (tests, lint, coverage threshold) on the branch.
+
+## Out of scope / future
+
+- The full category-descriptor refactor (#3 "Full") and the shared loader
+  resolver (#3 "Moderate") remain available as later, larger efforts. These
+  guards make that refactor safer by pinning current cross-plane invariants
+  first.

docs/superpowers/specs/2026-06-22-category-drift-guards-design.md

@@ -0,0 +1,71 @@
+package domain
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Every category's kube-backed status must be a deliberate, listed choice.
+func TestNeedsKubeConfig_EveryCategoryAccountedFor(t *testing.T) {
+	t.Parallel()
+	want := map[Category]bool{
+		Tenant:                          false,
+		LimitDefinition:                 false,
+		ConsolePropertyDefinition:       false,
+		PropertyDefinition:              false,
+		LimitTenancyOverride:            false,
+		ConsolePropertyTenancyOverride:  false,
+		PropertyTenancyOverride:         false,
+		LimitRegionalOverride:           false,
+		ConsolePropertyRegionalOverride: false,
+		PropertyRegionalOverride:        false,
+		BaseModel:                       true,
+		ImportedModel:                   true,
+		ModelArtifact:                   false,
+		Environment:                     false,
+		ServiceTenancy:                  false,
+		GPUPool:                         false,
+		GPUNode:                         true,
+		GPUWorkload:                     true,
+		DedicatedAICluster:              true,
+		Alias:                           false,
+	}
+	require.Len(t, want, len(Categories), "every category must have an expected NeedsKubeConfig value")
+	for _, c := range Categories {
+		exp, ok := want[c]
+		require.Truef(t, ok, "no expected NeedsKubeConfig value for %s", c)
+		assert.Equalf(t, exp, c.NeedsKubeConfig(), "NeedsKubeConfig mismatch for %s", c)
+	}
+}
+
+// Scope graph must be internally consistent: every child lists its parent back.
+func TestScopeGraph_RoundTrips(t *testing.T) {
+	t.Parallel()
+	for _, parent := range Categories {
+		for _, child := range parent.ScopedCategories() {
+			assert.Containsf(t, child.Parents(), parent,
+				"%s is scoped by %s but %s is not in %s.Parents()", child, parent, parent, child)
+		}
+	}
+}
+
+// Aliases must be unique across categories and round-trip through ParseCategory.
+func TestAliases_UniqueAndRoundTrip(t *testing.T) {
+	t.Parallel()
+	seen := map[string]Category{}
+	for _, c := range Categories {
+		aliases := c.Aliases()
+		assert.NotEmptyf(t, aliases, "%s must have at least one alias", c)
+		for _, a := range aliases {
+			if other, dup := seen[a]; dup {
+				t.Errorf("alias %q is shared by %s and %s", a, other, c)
+			}
+			seen[a] = c
+			got, err := ParseCategory(a)
+			require.NoErrorf(t, err, "alias %q does not parse", a)
+			assert.Equalf(t, c, got, "alias %q parses to %s, want %s", a, got, c)
+		}
+	}
+}

internal/domain/category_drift_test.go

@@ -0,0 +1,127 @@
+package tui
+
+import (
+	"context"
+	"testing"
+
+	"github.com/charmbracelet/bubbles/table"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/jingle2008/toolkit/internal/domain"
+	"github.com/jingle2008/toolkit/pkg/models"
+)
+
+// kubeBackedCategories returns every category that loads from a live cluster.
+func kubeBackedCategories() []domain.Category {
+	var out []domain.Category
+	for _, c := range domain.Categories {
+		if c.NeedsKubeConfig() {
+			out = append(out, c)
+		}
+	}
+	return out
+}
+
+// Every kube-backed category must be lazy-loaded, have a load handler, and be
+// reloadable/watchable — the cluster of planes the GPUWorkload bug missed.
+func TestKubeBackedCategories_FullyWired(t *testing.T) {
+	t.Parallel()
+	m := newTestModel(t)
+	for _, c := range kubeBackedCategories() {
+		_, lazy := lazyLoadedCategories[c]
+		assert.Truef(t, lazy, "%s must be in lazyLoadedCategories", c)
+
+		_, handled := categoryHandlers[c]
+		assert.Truef(t, handled, "%s must have a categoryHandlers entry", c)
+
+		assert.NotNilf(t, m.reloadCategoryCmd(c, 1), "%s must be reloadable", c)
+	}
+}
+
+// reloadCategoryCmd returns a command only for kube-backed categories.
+func TestReloadCategoryCmd_OnlyKubeBacked(t *testing.T) {
+	t.Parallel()
+	m := newTestModel(t)
+	for _, c := range domain.Categories {
+		got := m.reloadCategoryCmd(c, 1)
+		if c.NeedsKubeConfig() {
+			assert.NotNilf(t, got, "%s is kube-backed and must reload", c)
+		} else {
+			assert.Nilf(t, got, "%s is not kube-backed and must not reload", c)
+		}
+	}
+}
+
+// startK8sWatchCmd must start a watch for every kube-backed category.
+func TestStartK8sWatchCmd_CoversKubeBacked(t *testing.T) {
+	t.Parallel()
+	ld := &watchableLoader{}
+	for _, c := range kubeBackedCategories() {
+		cmd := startK8sWatchCmd(context.Background(), ld, c, "kc", models.Environment{}, 1)
+		require.NotNilf(t, cmd, "%s watch cmd must be built", c)
+		_, started := cmd().(k8sWatchStartedMsg)
+		assert.Truef(t, started, "%s must produce k8sWatchStartedMsg, got unavailable", c)
+	}
+}
+
+// noStatsCategories are intentionally without aggregate stat columns. A new
+// category must be added here OR to statsColumns — never silently neither.
+// Keep in sync with statsColumns (table_utils.go).
+var noStatsCategories = map[domain.Category]struct{}{
+	domain.Tenant:                          {},
+	domain.LimitDefinition:                 {},
+	domain.ConsolePropertyDefinition:       {},
+	domain.PropertyDefinition:              {},
+	domain.LimitTenancyOverride:            {},
+	domain.ConsolePropertyTenancyOverride:  {},
+	domain.PropertyTenancyOverride:         {},
+	domain.LimitRegionalOverride:           {},
+	domain.ConsolePropertyRegionalOverride: {},
+	domain.PropertyRegionalOverride:        {},
+	domain.BaseModel:                       {},
+	domain.ImportedModel:                   {},
+	domain.ModelArtifact:                   {},
+	domain.Environment:                     {},
+	domain.ServiceTenancy:                  {},
+	domain.Alias:                           {},
+}
+
+// Every category either has stat columns or is explicitly listed as having
+// none — a new category fails until someone decides.
+func TestStatsColumns_EveryCategoryAccountedFor(t *testing.T) {
+	t.Parallel()
+	for _, c := range domain.Categories {
+		_, hasStats := statsColumns[c]
+		_, excluded := noStatsCategories[c]
+		assert.Truef(t, hasStats != excluded,
+			"%s must be in exactly one of statsColumns / noStatsCategories (hasStats=%v excluded=%v)",
+			c, hasStats, excluded)
+	}
+}
+
+// itemKeyFrom must produce a non-nil key for every category so selection works.
+func TestItemKeyFrom_NonNilForEveryCategory(t *testing.T) {
+	t.Parallel()
+	row := table.Row{"a", "b", "c", "d"}
+	for _, c := range domain.Categories {
+		assert.NotNilf(t, itemKeyFrom(c, row), "itemKeyFrom returned nil for %s", c)
+	}
+}
+
+func TestParentScope_ResolvesForScopedCategories(t *testing.T) {
+	t.Parallel()
+	row := table.Row{"a", "b", "c", "d"}
+	for _, c := range domain.Categories {
+		// parentScope resolves a parent only for single-parent categories:
+		// multi-parent categories (the tenancy overrides) can't disambiguate,
+		// and categories with no parent have no scope. Every single-parent
+		// category MUST resolve — a new one whose parent is not handled in
+		// parentScope's switch fails here, surfacing the drift.
+		if len(c.Parents()) != 1 {
+			continue
+		}
+		_, ok := parentScope(c, row)
+		assert.Truef(t, ok, "parentScope must resolve single-parent category %s (parent %s)", c, c.Parents()[0])
+	}
+}