fix(#91): address Copilot review findings on PR #93

- api_capabilities.go: gate /api/{boxID}/capabilities on
  ComponentMetrics + PermissionView. The manifest enumerates installed
  services on the host, enough for fingerprinting in multi-tenant
  deploys — a user without metrics:view shouldn't enumerate the
  software inventory. Mirrors APIMetricsSummaryHandler's gate.
- history.templ: no-HAProxy banner now picks copy from the actual
  capability entry — not_installed / inaccessible / disabled each get
  distinct guidance, and the agent's `reason` is surfaced verbatim.
  Previous copy ("Install HAProxy") pointed at the wrong fix when the
  binary was present but stats unreachable.
- capabilities_cache.go: cache key is now (boxID, agentURL) so an
  operator editing a box's Agent URL gets fresh capabilities on the
  next render rather than stale data until the 5-min TTL expires.
  Invalidate() drops every entry for the boxID regardless of URL.
- haproxy_config.go: invalidate the capabilities cache on box update
  and delete so Agent URL / API key edits take effect immediately.
- api_metrics_insights.go: capability gating uses the sourceHAProxy
  constant instead of a duplicate "haproxy" literal — prevents future
  drift between KPI source IDs and capability lookups.

New tests:
- TestCapabilitiesCacheDifferentAgentURLBypassesCache — same boxID with
  different agent URLs returns each agent's actual verdict, not stale.
- TestCapabilitiesCacheInvalidateDropsAllAgentURLsForBox — Invalidate
  drops every entry for a boxID across all URLs it was fetched against.

Author: sarg3nt

gearbox/internal/framework/agent/capabilities_cache.go

@@ -49,23 +49,40 @@ func (b *BoxCapabilities) Entry(gearName string) (CapabilityEntry, bool) {
 	return e, ok
 }
 
-// CapabilitiesCache memoises agent capability fetches per box. Dashboard
-// pages call into this on every render that needs to decide what to
-// surface; without the cache, that's one synchronous round-trip per
-// render. TTL is short so a recently-restarted agent's new probe table
-// shows up without operator intervention; reconnect events should
-// invalidate explicitly via Invalidate() for an immediate refresh.
+// CapabilitiesCache memoises agent capability fetches per (box, agent
+// URL) pair. Dashboard pages call into this on every render that needs
+// to decide what to surface; without the cache, that's one synchronous
+// round-trip per render. TTL is short so a recently-restarted agent's
+// new probe table shows up without operator intervention; reconnect
+// events should invalidate explicitly via Invalidate() for an immediate
+// refresh.
+//
+// The cache key includes the agent URL (not just the box ID) so that an
+// operator editing a box's Agent URL — or re-pointing the same box ID
+// at a different host — gets fresh capabilities on the next render
+// rather than stale data from the prior agent until the 5-minute TTL
+// expires. The API key is intentionally not part of the key (rotations
+// don't change which gears the host runs; if the new key is wrong the
+// fetch errors and the negative-cache entry expires normally).
 //
 // Negative results (fetch errors, agent dead) are also cached for the
 // TTL window so a flaky or unreachable agent doesn't drag down every
 // page render with a fresh failed call.
 type CapabilitiesCache struct {
 	mu           sync.RWMutex
-	entries      map[string]*cachedCapabilities
+	entries      map[cacheKey]*cachedCapabilities
 	ttl          time.Duration
 	fetchTimeout time.Duration
 }
 
+// cacheKey identifies one cached entry by (boxID, agentURL). Splitting
+// into a struct rather than concatenating strings sidesteps any chance
+// of separator collision in operator-supplied URLs.
+type cacheKey struct {
+	boxID    string
+	agentURL string
+}
+
 type cachedCapabilities struct {
 	caps      *BoxCapabilities // nil when the last fetch errored
 	err       error            // captured on failure for callers that want it
@@ -78,21 +95,27 @@ type cachedCapabilities struct {
 // 3s matches the value the Gears settings page used pre-cache.
 func NewCapabilitiesCache(ttl, fetchTimeout time.Duration) *CapabilitiesCache {
 	return &CapabilitiesCache{
-		entries:      make(map[string]*cachedCapabilities),
+		entries:      make(map[cacheKey]*cachedCapabilities),
 		ttl:          ttl,
 		fetchTimeout: fetchTimeout,
 	}
 }
 
-// Get returns the cached capabilities for boxID. If the entry is missing
-// or older than the TTL, the cache fetches fresh capabilities from
-// agentURL using the supplied API key. On fetch error, returns (nil, err);
-// the error is also cached for the TTL window. Callers should treat
-// (nil, err) as "unknown" and fail open — locking users out of pages
-// because the agent is briefly unreachable is worse than briefly showing
-// gears that may not be available.
+// Get returns the cached capabilities for (boxID, agentURL). If the
+// entry is missing or older than the TTL, the cache fetches fresh
+// capabilities from agentURL using the supplied API key. On fetch error,
+// returns (nil, err); the error is also cached for the TTL window.
+// Callers should treat (nil, err) as "unknown" and fail open — locking
+// users out of pages because the agent is briefly unreachable is worse
+// than briefly showing gears that may not be available.
+//
+// Changing agentURL for the same boxID produces a different cache entry,
+// so config edits take effect on the next call rather than waiting out
+// the TTL. The previous entry expires naturally; no explicit cleanup
+// needed for the rare case of an operator-driven URL change.
 func (c *CapabilitiesCache) Get(boxID, agentURL, apiKey string) (*BoxCapabilities, error) {
-	if entry, ok := c.lookup(boxID); ok {
+	k := cacheKey{boxID: boxID, agentURL: agentURL}
+	if entry, ok := c.lookup(k); ok {
 		return entry.caps, entry.err
 	}
 
@@ -106,7 +129,7 @@ func (c *CapabilitiesCache) Get(boxID, agentURL, apiKey string) (*BoxCapabilitie
 	}
 
 	c.mu.Lock()
-	c.entries[boxID] = &cachedCapabilities{caps: caps, err: err, fetchedAt: now}
+	c.entries[k] = &cachedCapabilities{caps: caps, err: err, fetchedAt: now}
 	c.mu.Unlock()
 
 	return caps, err
@@ -115,10 +138,10 @@ func (c *CapabilitiesCache) Get(boxID, agentURL, apiKey string) (*BoxCapabilitie
 // lookup returns a non-stale entry under the read lock, or (nil, false)
 // if missing/expired. Split out so Get's refresh path can drop the read
 // lock before doing network I/O.
-func (c *CapabilitiesCache) lookup(boxID string) (*cachedCapabilities, bool) {
+func (c *CapabilitiesCache) lookup(k cacheKey) (*cachedCapabilities, bool) {
 	c.mu.RLock()
 	defer c.mu.RUnlock()
-	entry, ok := c.entries[boxID]
+	entry, ok := c.entries[k]
 	if !ok {
 		return nil, false
 	}
@@ -128,20 +151,25 @@ func (c *CapabilitiesCache) lookup(boxID string) (*cachedCapabilities, bool) {
 	return entry, true
 }
 
-// Invalidate drops the cached entry for boxID, forcing a fresh fetch on
-// the next Get. Call this when the box reconnects so a restarted agent's
-// new probe table is picked up immediately rather than at the next TTL
+// Invalidate drops every cached entry for boxID, regardless of the agent
+// URL it was fetched against. Call this when the box reconnects or when
+// the box's config changes (URL or API key edited), so the next Get
+// refetches against the current settings rather than at the next TTL
 // boundary.
 func (c *CapabilitiesCache) Invalidate(boxID string) {
 	c.mu.Lock()
-	delete(c.entries, boxID)
-	c.mu.Unlock()
+	defer c.mu.Unlock()
+	for k := range c.entries {
+		if k.boxID == boxID {
+			delete(c.entries, k)
+		}
+	}
 }
 
 // InvalidateAll drops every cached entry. Useful when global config that
 // affects probing changes (rare) and in tests.
 func (c *CapabilitiesCache) InvalidateAll() {
 	c.mu.Lock()
-	c.entries = make(map[string]*cachedCapabilities)
+	c.entries = make(map[cacheKey]*cachedCapabilities)
 	c.mu.Unlock()
 }

gearbox/internal/framework/agent/capabilities_cache_test.go

@@ -162,6 +162,78 @@ func TestBoxCapabilitiesAccessors(t *testing.T) {
 	}
 }
 
+func TestCapabilitiesCacheDifferentAgentURLBypassesCache(t *testing.T) {
+	srv, hits := newCapabilitiesServer(t, CapabilitiesResponse{
+		Gears: map[string]CapabilityEntry{"haproxy": {Status: "available"}},
+	})
+	defer srv.Close()
+	srv2, hits2 := newCapabilitiesServer(t, CapabilitiesResponse{
+		Gears: map[string]CapabilityEntry{"haproxy": {Status: "not_installed"}},
+	})
+	defer srv2.Close()
+
+	cache := NewCapabilitiesCache(5*time.Minute, 2*time.Second)
+
+	// Fetch against srv, then against srv2 with the same boxID. The
+	// second call must NOT serve cached data from srv — operator edits
+	// to the Agent URL should take effect on the next render.
+	caps1, err := cache.Get("box-1", srv.URL, "test-key")
+	if err != nil || !caps1.IsAvailable("haproxy") {
+		t.Fatalf("first Get: want available, got caps=%+v err=%v", caps1, err)
+	}
+	caps2, err := cache.Get("box-1", srv2.URL, "test-key")
+	if err != nil {
+		t.Fatalf("second Get: %v", err)
+	}
+	if caps2.IsAvailable("haproxy") {
+		t.Error("expected second Get to reflect srv2's not_installed verdict, got available (stale cache)")
+	}
+
+	if hits.Load() != 1 {
+		t.Errorf("srv: expected 1 hit, got %d", hits.Load())
+	}
+	if hits2.Load() != 1 {
+		t.Errorf("srv2: expected 1 hit, got %d", hits2.Load())
+	}
+}
+
+func TestCapabilitiesCacheInvalidateDropsAllAgentURLsForBox(t *testing.T) {
+	srv, hits := newCapabilitiesServer(t, CapabilitiesResponse{
+		Gears: map[string]CapabilityEntry{"haproxy": {Status: "available"}},
+	})
+	defer srv.Close()
+	srv2, hits2 := newCapabilitiesServer(t, CapabilitiesResponse{
+		Gears: map[string]CapabilityEntry{"haproxy": {Status: "available"}},
+	})
+	defer srv2.Close()
+
+	cache := NewCapabilitiesCache(5*time.Minute, 2*time.Second)
+	if _, err := cache.Get("box-1", srv.URL, "k"); err != nil {
+		t.Fatalf("seed srv: %v", err)
+	}
+	if _, err := cache.Get("box-1", srv2.URL, "k"); err != nil {
+		t.Fatalf("seed srv2: %v", err)
+	}
+
+	cache.Invalidate("box-1")
+
+	if _, err := cache.Get("box-1", srv.URL, "k"); err != nil {
+		t.Fatalf("post-invalidate srv: %v", err)
+	}
+	if _, err := cache.Get("box-1", srv2.URL, "k"); err != nil {
+		t.Fatalf("post-invalidate srv2: %v", err)
+	}
+
+	// Each backend should have been hit twice: once for the initial
+	// seed, once after invalidation dropped both entries for box-1.
+	if got := hits.Load(); got != 2 {
+		t.Errorf("srv hits = %d, want 2", got)
+	}
+	if got := hits2.Load(); got != 2 {
+		t.Errorf("srv2 hits = %d, want 2", got)
+	}
+}
+
 func TestCapabilitiesCacheInvalidateAll(t *testing.T) {
 	srv, hits := newCapabilitiesServer(t, CapabilitiesResponse{
 		Gears: map[string]CapabilityEntry{"haproxy": {Status: "available"}},

gearbox/internal/framework/handler/api_capabilities.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/go-chi/chi/v5"
+	"github.com/sarg3nt/gearbox/internal/framework/models"
 )
 
 // APICapabilitiesResponse is the JSON envelope returned by
@@ -43,10 +44,20 @@ type APICapabilityEntry struct {
 // metrics gear's frontend (and any future source-aware UI) to hide
 // cards / KPIs for sources the box can't produce.
 //
+// Gated on ComponentMetrics + PermissionView. The manifest enumerates
+// detected services on the host, which is enough information for
+// fingerprinting in a multi-tenant deploy — a user who can't see
+// metrics has no business knowing whether the host runs nginx vs
+// Apache vs Caddy. Mirrors APIMetricsSummaryHandler's gate.
+//
 // Returns 503 when the box isn't agent-backed or capabilities can't
 // be fetched — callers should fail open (show the full UI) on errors
 // so a flaky agent doesn't lock the user out.
 func (h *Handler) APIBoxCapabilitiesHandler(w http.ResponseWriter, r *http.Request) {
+	if !h.authManager.HasPermission(r, models.ComponentMetrics, models.PermissionView) {
+		http.Error(w, "Forbidden: insufficient permissions to view metrics", http.StatusForbidden)
+		return
+	}
 	boxID := chi.URLParam(r, "boxID")
 	if boxID == "" {
 		http.Error(w, "Server ID required", http.StatusBadRequest)

gearbox/internal/framework/handler/api_metrics_insights.go

@@ -88,7 +88,7 @@ func (h *Handler) APIMetricsSummaryHandler(w http.ResponseWriter, r *http.Reques
 	// down to nothing.
 	haproxyAvailable := true
 	if caps, ok := h.getBoxCapabilities(boxID); ok && caps != nil {
-		if entry, present := caps.Entry("haproxy"); present {
+		if entry, present := caps.Entry(sourceHAProxy); present {
 			haproxyAvailable = entry.IsAvailable()
 		}
 	}

gearbox/internal/framework/handler/haproxy_config.go

@@ -248,6 +248,12 @@ func (h *Handler) HAProxyBoxUpdatePost(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// The Agent URL or API key may have changed; drop any cached
+	// capabilities so the next render fetches against the new endpoint
+	// rather than serving stale data from the previous agent until the
+	// TTL expires.
+	h.invalidateBoxCapabilities(server.BoxID)
+
 	// Log audit
 	h.logAudit(r, user.ID, "haproxy_box_update", fmt.Sprintf("Updated HAProxy box: %s (%s)", server.Name, server.BoxID))
 
@@ -311,6 +317,11 @@ func (h *Handler) HAProxyBoxDeletePost(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Drop cached capabilities — even if the same box ID is recreated
+	// later, it's likely a different host and we shouldn't serve the
+	// previous probe table.
+	h.invalidateBoxCapabilities(server.BoxID)
+
 	// Log audit
 	h.logAudit(r, user.ID, "haproxy_box_delete", fmt.Sprintf("Deleted HAProxy box: %s (%s)", server.Name, server.BoxID))
 

gearbox/internal/framework/templates/pages/history.templ

@@ -68,12 +68,16 @@ templ History(user *models.User, servers []models.BoxConfig) {
 		if len(servers) == 0 {
 			@components.InfoAlert("No servers configured.")
 		} else {
-			<!-- No-HAProxy banner — shown when the active box's agent
-			     reports no HAProxy gear. JS toggles visibility based on
-			     /api/{boxID}/capabilities. -->
+			<!-- HAProxy-unavailable banner — shown when the active box's
+			     agent reports the HAProxy gear is anything other than
+			     'available'. JS in applyCapabilities() picks message
+			     text from the capability entry's status + reason so the
+			     copy matches reality: a missing binary (not_installed)
+			     gets different guidance than an unreachable socket
+			     (inaccessible) or an operator-disabled gear (disabled). -->
 			<div id="no-haproxy-banner" class="hidden mb-4 rounded-lg border border-blue-200 dark:border-blue-900/40 bg-blue-50 dark:bg-blue-900/20 px-4 py-3 text-sm text-blue-900 dark:text-blue-100">
-				<strong class="font-semibold">No HAProxy detected on this host.</strong>
-				Showing host-level metrics only. Install HAProxy or enable the agent's HAProxy gear to see proxy metrics.
+				<strong id="no-haproxy-banner-title" class="font-semibold">HAProxy metrics unavailable on this host.</strong>
+				<span id="no-haproxy-banner-detail">Showing host-level metrics only.</span>
 			</div>
 
 			<!-- KPI summary band — populated by loadKPISummary().
@@ -874,13 +878,14 @@ templ History(user *models.User, servers []models.BoxConfig) {
 		// rather than locking the user out of cards they may need.
 		async function applyCapabilities(serverID) {
 			let haproxyAvailable = true;
+			let haproxyEntry = null;
 			try {
 				const res = await fetch('/api/' + serverID + '/capabilities');
 				if (res.ok) {
 					const data = await res.json();
-					const entry = data && data.gears && data.gears.haproxy;
-					if (entry) {
-						haproxyAvailable = entry.status === 'available';
+					haproxyEntry = data && data.gears && data.gears.haproxy;
+					if (haproxyEntry) {
+						haproxyAvailable = haproxyEntry.status === 'available';
 					}
 				}
 			} catch (err) {
@@ -896,9 +901,52 @@ templ History(user *models.User, servers []models.BoxConfig) {
 			const banner = document.getElementById('no-haproxy-banner');
 			if (banner) {
 				banner.classList.toggle('hidden', haproxyAvailable);
+				if (!haproxyAvailable) {
+					updateNoHAProxyBanner(haproxyEntry);
+				}
 			}
 		}
 
+		// updateNoHAProxyBanner picks user-facing copy that matches the
+		// actual probe verdict — not_installed / inaccessible / disabled
+		// each get different guidance, and the agent's `reason` field is
+		// surfaced verbatim when present so operators see the exact
+		// detection failure.
+		function updateNoHAProxyBanner(entry) {
+			const titleEl = document.getElementById('no-haproxy-banner-title');
+			const detailEl = document.getElementById('no-haproxy-banner-detail');
+			if (!titleEl || !detailEl) return;
+
+			let title = 'HAProxy metrics unavailable on this host.';
+			let detail = 'Showing host-level metrics only.';
+
+			const status = entry && entry.status;
+			const reason = (entry && entry.reason) ? String(entry.reason) : '';
+
+			switch (status) {
+				case 'not_installed':
+					title = 'No HAProxy detected on this host.';
+					detail = 'Install HAProxy or enable the agent\'s HAProxy gear to see proxy metrics. Showing host-level metrics only.';
+					break;
+				case 'inaccessible':
+					title = 'HAProxy is installed but the agent can\'t reach its stats.';
+					detail = (reason ? reason + ' ' : '') + 'Showing host-level metrics only.';
+					break;
+				case 'disabled':
+					title = 'HAProxy gear disabled in agent configuration.';
+					detail = (reason ? reason + ' ' : '') + 'Showing host-level metrics only.';
+					break;
+				default:
+					// status === undefined (agent didn't surface haproxy at
+					// all — older agent or fetch failed). Keep the generic
+					// copy and don't pretend we know more than we do.
+					break;
+			}
+
+			titleEl.textContent = title;
+			detailEl.textContent = ' ' + detail;
+		}
+
 		// hoursToRange maps the hours-select dropdown value (e.g. "0.083",
 		// "24") to the human-readable range token the new /metrics/* APIs
 		// accept ("5m", "24h", …). Anything we don't recognise falls back