fix(agent): address Copilot review on Phase 2 Resources (#112)

Four review findings, all addressed:

1. Probe was mutating g.paths despite the ProbeableGear contract
   that Probe must be side-effect-free. Refactor: extract
   pathsFromDeps as a shared deps→map helper, add
   resolveLogPathWith(src, overrides) so Probe operates on a local
   override map. Initialize is now the single place g.paths gets
   written.

2. Resources was always populated with a "log_sources" key even when
   no readable log files were found, so the JSON envelope's
   `omitempty` tag never fired and the wire format gained a
   permanent (empty-slice) field. Only attach Resources when
   log_sources actually has entries.

3. Manager.ProbeResults returned a shallow copy — callers could
   mutate nested Capabilities/Resources maps and race against
   manager state under load. Added cloneProbeResult that
   duplicates Capabilities as a fresh map and JSON-round-trips
   Resources (so nested slices and sub-maps come back as fresh
   allocations). JSON round-trip is the same cost the capabilities
   endpoint encoder pays anyway, and falls back to a shallow
   Resources copy if marshaling fails.

4. Test comment claimed "decode through json.RawMessage" but the
   test actually decoded straight into map[string]any. Switch the
   test to a two-stage decode: first peel the outer envelope into
   json.RawMessage per gear (catches field-name regressions in the
   wire format), then decode the resources object into a typed
   shape and assert on payload contents. Avoids Go's `any`
   decoding quirks in the wire-format guard.

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

Author: sarg3nt

gearbox-agent/internal/framework/gear/manager.go

@@ -147,17 +147,81 @@ func (m *Manager) isLoaded(name string) bool {
 }
 
 // ProbeResults returns a snapshot of every gear's probe verdict. Useful
-// for the upcoming /api/v1/system/capabilities endpoint and for tests.
+// for the /api/v1/system/capabilities endpoint and for tests.
+//
+// The returned map and every nested Capabilities / Resources map are
+// independent copies — callers can freely mutate them without racing
+// against background re-probes (when those land) or aliasing the
+// manager's internal state. JSON encoding deeply traverses the
+// Resources tree, so the cost stays in O(snapshot) at the request
+// boundary instead of being a hidden mutation hazard for downstream
+// consumers (issue #112 review).
 func (m *Manager) ProbeResults() map[string]ProbeResult {
 	m.mu.RLock()
 	defer m.mu.RUnlock()
 	out := make(map[string]ProbeResult, len(m.probed))
 	for k, v := range m.probed {
-		out[k] = v
+		out[k] = cloneProbeResult(v)
+	}
+	return out
+}
+
+// cloneProbeResult returns a deep copy of r — Capabilities is
+// duplicated as a fresh map[string]string, Resources is duplicated by
+// JSON round-trip because each gear picks its own shape and we can't
+// know the concrete types statically. JSON round-trip is the same
+// cost the /api/v1/system/capabilities encoder pays anyway, so it's
+// the cheapest safe option that handles arbitrary nested data.
+//
+// Falls back to a shallow Resources copy if JSON encoding fails (a
+// gear sneaked an un-marshalable value into the map — should never
+// happen because the same value would crash the capabilities
+// endpoint). Aliasing is the worst case here; better than panicking
+// at a fan-out call site.
+func cloneProbeResult(r ProbeResult) ProbeResult {
+	out := ProbeResult{
+		Status: r.Status,
+		Reason: r.Reason,
+	}
+	if r.Capabilities != nil {
+		out.Capabilities = make(map[string]string, len(r.Capabilities))
+		for k, v := range r.Capabilities {
+			out.Capabilities[k] = v
+		}
+	}
+	if len(r.Resources) > 0 {
+		if cloned, err := cloneResourcesViaJSON(r.Resources); err == nil {
+			out.Resources = cloned
+		} else {
+			// Shallow fallback. See function comment.
+			out.Resources = make(map[string]any, len(r.Resources))
+			for k, v := range r.Resources {
+				out.Resources[k] = v
+			}
+		}
 	}
 	return out
 }
 
+// cloneResourcesViaJSON round-trips the map through encoding/json so
+// nested slices and sub-maps come back as fresh allocations. After
+// decode, every leaf is a JSON-typed value (float64, string, bool,
+// nil) — the same shape downstream consumers see when fetching
+// /api/v1/system/capabilities over the wire, so callers can't
+// accidentally rely on Go-typed values that wouldn't survive the
+// JSON boundary.
+func cloneResourcesViaJSON(in map[string]any) (map[string]any, error) {
+	b, err := json.Marshal(in)
+	if err != nil {
+		return nil, err
+	}
+	var out map[string]any
+	if err := json.Unmarshal(b, &out); err != nil {
+		return nil, err
+	}
+	return out, nil
+}
+
 // logProbeTable renders the visual probe summary to the table writer and
 // logs a structured completion line via slog. The visual table goes to
 // stderr (the systemd journal in production) because slog text handlers

gearbox-agent/internal/framework/gear/manager_probe_test.go

@@ -332,38 +332,53 @@ func TestCapabilitiesEndpointSurfacesResources(t *testing.T) {
 		t.Fatalf("status = %d, want 200", rr.Code)
 	}
 
-	// Decode through json.RawMessage to confirm the field round-trips
-	// without relying on the in-process Go type — the dashboard sees
-	// JSON over the wire, not a Go struct.
-	var raw struct {
+	// Two-stage decode: first peel the outer envelope into
+	// json.RawMessage per gear so we can verify the `resources` key
+	// is present in the raw bytes (catches name-tag regressions like
+	// renaming the field to "Resources" or "extra"). Then decode
+	// the resources object into a typed shape and check the actual
+	// payload contents. Decoding through RawMessage rather than
+	// straight into map[string]any avoids relying on Go's `any`
+	// decoding quirks for the wire-format guard.
+	var envelope struct {
 		Gears map[string]struct {
-			Status    string         `json:"status"`
-			Resources map[string]any `json:"resources"`
+			Status    string          `json:"status"`
+			Resources json.RawMessage `json:"resources"`
 		} `json:"gears"`
 	}
-	if err := json.Unmarshal(rr.Body.Bytes(), &raw); err != nil {
-		t.Fatalf("decode response: %v", err)
+	if err := json.Unmarshal(rr.Body.Bytes(), &envelope); err != nil {
+		t.Fatalf("decode envelope: %v", err)
 	}
-	alpha, ok := raw.Gears["alpha"]
+	alpha, ok := envelope.Gears["alpha"]
 	if !ok {
 		t.Fatalf("alpha missing from response")
 	}
-	logSources, ok := alpha.Resources["log_sources"].([]any)
-	if !ok {
-		t.Fatalf("alpha.Resources[\"log_sources\"] type = %T, want []any (JSON-decoded slice)", alpha.Resources["log_sources"])
+	if len(alpha.Resources) == 0 {
+		t.Fatalf("alpha.resources missing from envelope; got body %s", rr.Body.String())
+	}
+
+	var resources struct {
+		LogSources []struct {
+			Name        string `json:"name"`
+			DisplayName string `json:"display_name"`
+			Path        string `json:"path"`
+		} `json:"log_sources"`
 	}
-	if len(logSources) != 1 {
-		t.Fatalf("alpha.Resources[\"log_sources\"] = %v, want one entry", logSources)
+	if err := json.Unmarshal(alpha.Resources, &resources); err != nil {
+		t.Fatalf("decode alpha.resources: %v", err)
 	}
-	first, ok := logSources[0].(map[string]any)
-	if !ok {
-		t.Fatalf("first log source type = %T, want map[string]any", logSources[0])
+	if len(resources.LogSources) != 1 {
+		t.Fatalf("log_sources = %v, want one entry", resources.LogSources)
+	}
+	got := resources.LogSources[0]
+	if got.Name != "nginx" {
+		t.Errorf("name = %q, want nginx", got.Name)
 	}
-	if first["name"] != "nginx" {
-		t.Errorf("first.name = %v, want nginx", first["name"])
+	if got.DisplayName != "nginx" {
+		t.Errorf("display_name = %q, want nginx", got.DisplayName)
 	}
-	if first["path"] != "/var/log/nginx/access.log" {
-		t.Errorf("first.path = %v, want /var/log/nginx/access.log", first["path"])
+	if got.Path != "/var/log/nginx/access.log" {
+		t.Errorf("path = %q, want /var/log/nginx/access.log", got.Path)
 	}
 }
 

gearbox-agent/internal/gears/accesslog/plugin.go

@@ -162,19 +162,22 @@ var accessLogSourceDisplayName = map[string]string{
 // Each entry is {"name", "display_name", "path"} for one discovered
 // web-server access log. Older dashboards that don't know about
 // Resources continue to read the flat Capabilities map; both views
-// are kept in sync.
+// are kept in sync. When no readable log file is found at all, the
+// Resources field is left unset so the JSON envelope omits it and
+// callers fall back to the flat Capabilities map.
+//
+// Side-effect-free: Probe builds the operator-override map as a
+// local rather than mutating g.paths, so the function honors the
+// ProbeableGear contract that probing must be free of state writes.
+// Initialize re-runs the dep-to-paths copy below to populate g.paths
+// for the handler path that needs it.
 func (g *Gear) Probe(ctx context.Context, deps gear.Dependencies) gear.ProbeResult {
-	g.paths = map[string]string{
-		"haproxy": deps.HAProxyAccessLog,
-		"nginx":   deps.NginxAccessLog,
-		"apache":  deps.ApacheAccessLog,
-		"caddy":   deps.CaddyAccessLog,
-	}
+	overrides := pathsFromDeps(deps)
 
 	caps := map[string]string{}
 	logSources := []map[string]string{}
 	for _, src := range []string{"haproxy", "nginx", "apache", "caddy"} {
-		path := g.resolveLogPath(src)
+		path := g.resolveLogPathWith(src, overrides)
 		if path == "" {
 			continue
 		}
@@ -185,27 +188,40 @@ func (g *Gear) Probe(ctx context.Context, deps gear.Dependencies) gear.ProbeResu
 			"path":         path,
 		})
 	}
-	resources := map[string]any{
-		"log_sources": logSources,
+	// Only attach Resources when we actually have a payload — keeps
+	// the JSON envelope's `resources` field truly omitempty.
+	if len(logSources) > 0 {
+		return gear.ProbeAvailableWithResources(
+			"access-log endpoint registered",
+			caps,
+			map[string]any{"log_sources": logSources},
+		)
 	}
-	return gear.ProbeAvailableWithResources("access-log endpoint registered", caps, resources)
+	return gear.ProbeAvailable("access-log endpoint registered", caps)
 }
 
-// Initialize captures the path overrides for later use by the
-// handler. Probe already populated paths; this re-runs it because
-// Probe runs before Initialize on first boot, and we want explicit
-// re-resolution on Initialize so test harnesses that construct a
-// gear without calling Probe still get usable paths.
-func (g *Gear) Initialize(ctx context.Context, deps gear.Dependencies) error {
-	if err := g.BaseGear.Initialize(ctx, deps); err != nil {
-		return err
-	}
-	g.paths = map[string]string{
+// pathsFromDeps extracts the operator-overrides map from
+// gear.Dependencies. Shared by Probe (local-only, side-effect-free)
+// and Initialize (mutates g.paths so handlers can read it later)
+// so the deps→map mapping has one source of truth.
+func pathsFromDeps(deps gear.Dependencies) map[string]string {
+	return map[string]string{
 		"haproxy": deps.HAProxyAccessLog,
 		"nginx":   deps.NginxAccessLog,
 		"apache":  deps.ApacheAccessLog,
 		"caddy":   deps.CaddyAccessLog,
 	}
+}
+
+// Initialize captures the path overrides for later use by the
+// handler. Probe is side-effect-free (#112 review) so this is the
+// single place g.paths gets written; the handler reads g.paths via
+// resolveLogPath during request handling.
+func (g *Gear) Initialize(ctx context.Context, deps gear.Dependencies) error {
+	if err := g.BaseGear.Initialize(ctx, deps); err != nil {
+		return err
+	}
+	g.paths = pathsFromDeps(deps)
 	return nil
 }
 
@@ -316,12 +332,27 @@ func parseWithFallback(primary, fallback accesslog.Parser, raw string) *accesslo
 	return fallback.Parse(raw)
 }
 
-// resolveLogPath returns the access-log path for src: the operator
-// override if set and readable, the well-known default if readable,
-// or "" when neither exists. Apache gets a second-chance lookup
-// against the RHEL-style path.
+// resolveLogPath returns the access-log path for src using the
+// operator-override map captured in g.paths. Thin wrapper over
+// resolveLogPathWith so existing handler callers keep their
+// `g.resolveLogPath(src)` ergonomics; Probe (side-effect-free)
+// uses resolveLogPathWith directly with a local map.
 func (g *Gear) resolveLogPath(src string) string {
-	if override, ok := g.paths[src]; ok && override != "" {
+	return g.resolveLogPathWith(src, g.paths)
+}
+
+// resolveLogPathWith is the underlying resolver. Takes the
+// operator-override map explicitly so the caller (Probe / handler /
+// test) can avoid touching shared state:
+//
+//   - the operator override if set and readable, OR
+//   - the well-known default if readable, OR
+//   - "" when neither exists.
+//
+// Apache gets a second-chance lookup against the RHEL-style path
+// because Debian and RHEL ship the log under different paths.
+func (g *Gear) resolveLogPathWith(src string, overrides map[string]string) string {
+	if override, ok := overrides[src]; ok && override != "" {
 		// Operator explicitly pointed us at a path — trust them.
 		// If the path isn't readable the endpoint surfaces "tail
 		// failed" rather than silently falling back to a