Skip to content

Commit e09943a

Browse files
sarg3ntclaude
andauthored
feat(agent): Resources field on ProbeResult + access-log log_sources (#135)
* feat(agent): Resources field on ProbeResult + access-log log_sources (#112) The dashboard's capability-driven UI has been inferring resource lists (log sources, services, metric sources) from gear-availability flags. That works for binary "is this gear present?" decisions but breaks down for "what concrete sources does this gear expose?" — the Logs page's hardcoded [haproxy, system] fallback was the worst offender. Phase 2 of issue #112 introduces a structured Resources field the agent populates per-gear, the dashboard consumes by name. Agent changes: - ProbeResult gains `Resources map[string]any`, serialized as the `resources` JSON object on CapabilityEntry. Omitted when empty so older dashboards see the unchanged wire shape. - New gear.ProbeAvailableWithResources(reason, capabilities, resources) constructor for gears that want to publish typed resource lists; gears that don't need this keep using ProbeAvailable. - access-log gear now publishes `log_sources` — a slice of {name, display_name, path} for every readable web-server access log it discovers (haproxy / nginx / apache / caddy). The flat `<src>_log` keys in Capabilities stay for backward compat with pre-Phase-2 dashboards. Tests: - TestProbePopulatesLogSourcesResource on the access-log gear asserts the structured shape, name → display_name mapping, and absence of entries for non-readable paths. - TestCapabilitiesEndpointSurfacesResources on the manager asserts Resources round-trips through the JSON envelope at /api/v1/system/capabilities — decoded via map[string]any so we catch any field-name regression on the wire. Dashboard-side consumer lands in a follow-up PR (sibling to this one). Phase 2 of #112. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * 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> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 79d3a3e commit e09943a

5 files changed

Lines changed: 323 additions & 26 deletions

File tree

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

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,8 +246,30 @@ type ProbeResult struct {
246246

247247
// Capabilities is optional detected facts the gear wants to surface
248248
// (e.g. "haproxy_version": "2.8.5", "stats_socket": "/run/haproxy/admin.sock").
249-
// Populated mainly when Status == Available.
249+
// Populated mainly when Status == Available. Flat key/value map —
250+
// use Resources for structured lists.
250251
Capabilities map[string]string
252+
253+
// Resources is optional structured data the gear wants to advertise
254+
// to the dashboard, beyond what fits in the flat Capabilities map.
255+
// The shape is gear-specific; the dashboard reads each gear's
256+
// known keys explicitly. Typical use:
257+
//
258+
// - access-log: "log_sources" → []map[string]string of
259+
// {"name", "display_name", "path"} per discovered web server
260+
// log file. The dashboard's Logs page populates its source
261+
// dropdown from this instead of inferring sources from gear
262+
// availability flags.
263+
//
264+
// Stable across the wire: serialized to JSON as part of the
265+
// /api/v1/system/capabilities response. Gears that omit it stay
266+
// fully backward-compatible — the dashboard treats a missing
267+
// Resources as "fall back to the older capability-flag heuristic".
268+
//
269+
// Issue #112 Phase 2 extension: dashboard consumers in
270+
// gearbox/internal/framework/agent.CapabilityEntry mirror this
271+
// field as map[string]json.RawMessage for typed-per-gear decoding.
272+
Resources map[string]any
251273
}
252274

253275
// IsAvailable reports whether the gear should be loaded.
@@ -261,6 +283,20 @@ func ProbeAvailable(reason string, capabilities map[string]string) ProbeResult {
261283
return ProbeResult{Status: ProbeStatusAvailable, Reason: reason, Capabilities: capabilities}
262284
}
263285

286+
// ProbeAvailableWithResources is ProbeAvailable plus the structured
287+
// Resources field. Gears that need to publish typed resource lists
288+
// (log sources, service catalogs, metric sources, …) to the dashboard
289+
// use this constructor; gears that only need the flat Capabilities map
290+
// continue to use ProbeAvailable. Issue #112 Phase 2 extension.
291+
func ProbeAvailableWithResources(reason string, capabilities map[string]string, resources map[string]any) ProbeResult {
292+
return ProbeResult{
293+
Status: ProbeStatusAvailable,
294+
Reason: reason,
295+
Capabilities: capabilities,
296+
Resources: resources,
297+
}
298+
}
299+
264300
// ProbeNotInstalled returns a result for the case where the software the
265301
// gear manages isn't on this host.
266302
func ProbeNotInstalled(reason string) ProbeResult {

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

Lines changed: 74 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,17 +147,81 @@ func (m *Manager) isLoaded(name string) bool {
147147
}
148148

149149
// ProbeResults returns a snapshot of every gear's probe verdict. Useful
150-
// for the upcoming /api/v1/system/capabilities endpoint and for tests.
150+
// for the /api/v1/system/capabilities endpoint and for tests.
151+
//
152+
// The returned map and every nested Capabilities / Resources map are
153+
// independent copies — callers can freely mutate them without racing
154+
// against background re-probes (when those land) or aliasing the
155+
// manager's internal state. JSON encoding deeply traverses the
156+
// Resources tree, so the cost stays in O(snapshot) at the request
157+
// boundary instead of being a hidden mutation hazard for downstream
158+
// consumers (issue #112 review).
151159
func (m *Manager) ProbeResults() map[string]ProbeResult {
152160
m.mu.RLock()
153161
defer m.mu.RUnlock()
154162
out := make(map[string]ProbeResult, len(m.probed))
155163
for k, v := range m.probed {
156-
out[k] = v
164+
out[k] = cloneProbeResult(v)
157165
}
158166
return out
159167
}
160168

169+
// cloneProbeResult returns a deep copy of r — Capabilities is
170+
// duplicated as a fresh map[string]string, Resources is duplicated by
171+
// JSON round-trip because each gear picks its own shape and we can't
172+
// know the concrete types statically. JSON round-trip is the same
173+
// cost the /api/v1/system/capabilities encoder pays anyway, so it's
174+
// the cheapest safe option that handles arbitrary nested data.
175+
//
176+
// Falls back to a shallow Resources copy if JSON encoding fails (a
177+
// gear sneaked an un-marshalable value into the map — should never
178+
// happen because the same value would crash the capabilities
179+
// endpoint). Aliasing is the worst case here; better than panicking
180+
// at a fan-out call site.
181+
func cloneProbeResult(r ProbeResult) ProbeResult {
182+
out := ProbeResult{
183+
Status: r.Status,
184+
Reason: r.Reason,
185+
}
186+
if r.Capabilities != nil {
187+
out.Capabilities = make(map[string]string, len(r.Capabilities))
188+
for k, v := range r.Capabilities {
189+
out.Capabilities[k] = v
190+
}
191+
}
192+
if len(r.Resources) > 0 {
193+
if cloned, err := cloneResourcesViaJSON(r.Resources); err == nil {
194+
out.Resources = cloned
195+
} else {
196+
// Shallow fallback. See function comment.
197+
out.Resources = make(map[string]any, len(r.Resources))
198+
for k, v := range r.Resources {
199+
out.Resources[k] = v
200+
}
201+
}
202+
}
203+
return out
204+
}
205+
206+
// cloneResourcesViaJSON round-trips the map through encoding/json so
207+
// nested slices and sub-maps come back as fresh allocations. After
208+
// decode, every leaf is a JSON-typed value (float64, string, bool,
209+
// nil) — the same shape downstream consumers see when fetching
210+
// /api/v1/system/capabilities over the wire, so callers can't
211+
// accidentally rely on Go-typed values that wouldn't survive the
212+
// JSON boundary.
213+
func cloneResourcesViaJSON(in map[string]any) (map[string]any, error) {
214+
b, err := json.Marshal(in)
215+
if err != nil {
216+
return nil, err
217+
}
218+
var out map[string]any
219+
if err := json.Unmarshal(b, &out); err != nil {
220+
return nil, err
221+
}
222+
return out, nil
223+
}
224+
161225
// logProbeTable renders the visual probe summary to the table writer and
162226
// logs a structured completion line via slog. The visual table goes to
163227
// stderr (the systemd journal in production) because slog text handlers
@@ -508,6 +572,14 @@ type CapabilityEntry struct {
508572
Status ProbeStatus `json:"status"`
509573
Reason string `json:"reason,omitempty"`
510574
Capabilities map[string]string `json:"capabilities,omitempty"`
575+
576+
// Resources carries structured, gear-specific data the dashboard
577+
// consumes for capability-driven UI. See ProbeResult.Resources for
578+
// the keys each gear publishes (e.g. access-log → "log_sources").
579+
// Omitted from the JSON envelope when empty so older dashboards
580+
// that don't know about Resources see the unchanged shape (issue
581+
// #112 Phase 2 extension).
582+
Resources map[string]any `json:"resources,omitempty"`
511583
}
512584

513585
// CapabilitiesResponse is the envelope for GET /api/v1/system/capabilities.

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

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,91 @@ func TestCapabilitiesEndpointReportsEveryGearWithVerdict(t *testing.T) {
297297
}
298298
}
299299

300+
// TestCapabilitiesEndpointSurfacesResources guards the Resources field
301+
// added in issue #112 Phase 2: gears that publish structured resources
302+
// (log sources, services, metric sources, …) must round-trip them
303+
// through the /api/v1/system/capabilities JSON envelope so the
304+
// dashboard's capability-driven UI can read them directly instead of
305+
// reverse-engineering them from gear-availability flags.
306+
func TestCapabilitiesEndpointSurfacesResources(t *testing.T) {
307+
g := &mockProbeGear{
308+
info: Info{Name: "alpha"},
309+
probeResult: ProbeAvailableWithResources(
310+
"ok",
311+
map[string]string{"version": "1.0"},
312+
map[string]any{
313+
"log_sources": []map[string]string{
314+
{"name": "nginx", "display_name": "nginx", "path": "/var/log/nginx/access.log"},
315+
},
316+
},
317+
),
318+
}
319+
withTestRegistry(t, g)
320+
321+
m, _ := newTestManager(t)
322+
m.ProbeAll(context.Background())
323+
324+
r := chi.NewRouter()
325+
m.RegisterSystemRoutes(r)
326+
327+
req := httptest.NewRequest(http.MethodGet, "/api/v1/system/capabilities", nil)
328+
rr := httptest.NewRecorder()
329+
r.ServeHTTP(rr, req)
330+
331+
if rr.Code != http.StatusOK {
332+
t.Fatalf("status = %d, want 200", rr.Code)
333+
}
334+
335+
// Two-stage decode: first peel the outer envelope into
336+
// json.RawMessage per gear so we can verify the `resources` key
337+
// is present in the raw bytes (catches name-tag regressions like
338+
// renaming the field to "Resources" or "extra"). Then decode
339+
// the resources object into a typed shape and check the actual
340+
// payload contents. Decoding through RawMessage rather than
341+
// straight into map[string]any avoids relying on Go's `any`
342+
// decoding quirks for the wire-format guard.
343+
var envelope struct {
344+
Gears map[string]struct {
345+
Status string `json:"status"`
346+
Resources json.RawMessage `json:"resources"`
347+
} `json:"gears"`
348+
}
349+
if err := json.Unmarshal(rr.Body.Bytes(), &envelope); err != nil {
350+
t.Fatalf("decode envelope: %v", err)
351+
}
352+
alpha, ok := envelope.Gears["alpha"]
353+
if !ok {
354+
t.Fatalf("alpha missing from response")
355+
}
356+
if len(alpha.Resources) == 0 {
357+
t.Fatalf("alpha.resources missing from envelope; got body %s", rr.Body.String())
358+
}
359+
360+
var resources struct {
361+
LogSources []struct {
362+
Name string `json:"name"`
363+
DisplayName string `json:"display_name"`
364+
Path string `json:"path"`
365+
} `json:"log_sources"`
366+
}
367+
if err := json.Unmarshal(alpha.Resources, &resources); err != nil {
368+
t.Fatalf("decode alpha.resources: %v", err)
369+
}
370+
if len(resources.LogSources) != 1 {
371+
t.Fatalf("log_sources = %v, want one entry", resources.LogSources)
372+
}
373+
got := resources.LogSources[0]
374+
if got.Name != "nginx" {
375+
t.Errorf("name = %q, want nginx", got.Name)
376+
}
377+
if got.DisplayName != "nginx" {
378+
t.Errorf("display_name = %q, want nginx", got.DisplayName)
379+
}
380+
if got.Path != "/var/log/nginx/access.log" {
381+
t.Errorf("path = %q, want /var/log/nginx/access.log", got.Path)
382+
}
383+
}
384+
300385
func TestProbeResultsIsCopy(t *testing.T) {
301386
// Returning a copy prevents downstream callers (e.g. the capabilities
302387
// API handler) from accidentally mutating manager state under load.

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

Lines changed: 85 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -139,42 +139,89 @@ func (g *Gear) Info() gear.Info {
139139
}
140140
}
141141

142+
// accessLogSourceDisplayName names the access-log source as the
143+
// dashboard's Logs page renders it in the source picker dropdown.
144+
// Centralized here so the agent stays the single source of truth for
145+
// what each source is called — the dashboard's old hardcoded
146+
// "haproxy" → "HAProxy" mapping (api_logs.go) goes away once
147+
// dashboards consume the Resources field added in issue #112.
148+
var accessLogSourceDisplayName = map[string]string{
149+
"haproxy": "HAProxy",
150+
"nginx": "nginx",
151+
"apache": "Apache",
152+
"caddy": "Caddy",
153+
}
154+
142155
// Probe always reports Available — the gear's only job is to read
143156
// files on demand, which is universally possible. Capabilities map
144157
// records which sources have a readable log path; the dashboard
145158
// uses this to gate the "Error Insights" panel per source.
159+
//
160+
// Resources["log_sources"] is the structured form the dashboard's
161+
// Logs page reads to populate its source picker (issue #112 Phase 2).
162+
// Each entry is {"name", "display_name", "path"} for one discovered
163+
// web-server access log. Older dashboards that don't know about
164+
// Resources continue to read the flat Capabilities map; both views
165+
// are kept in sync. When no readable log file is found at all, the
166+
// Resources field is left unset so the JSON envelope omits it and
167+
// callers fall back to the flat Capabilities map.
168+
//
169+
// Side-effect-free: Probe builds the operator-override map as a
170+
// local rather than mutating g.paths, so the function honors the
171+
// ProbeableGear contract that probing must be free of state writes.
172+
// Initialize re-runs the dep-to-paths copy below to populate g.paths
173+
// for the handler path that needs it.
146174
func (g *Gear) Probe(ctx context.Context, deps gear.Dependencies) gear.ProbeResult {
147-
g.paths = map[string]string{
148-
"haproxy": deps.HAProxyAccessLog,
149-
"nginx": deps.NginxAccessLog,
150-
"apache": deps.ApacheAccessLog,
151-
"caddy": deps.CaddyAccessLog,
152-
}
175+
overrides := pathsFromDeps(deps)
153176

154177
caps := map[string]string{}
178+
logSources := []map[string]string{}
155179
for _, src := range []string{"haproxy", "nginx", "apache", "caddy"} {
156-
if path := g.resolveLogPath(src); path != "" {
157-
caps[src+"_log"] = path
180+
path := g.resolveLogPathWith(src, overrides)
181+
if path == "" {
182+
continue
158183
}
184+
caps[src+"_log"] = path
185+
logSources = append(logSources, map[string]string{
186+
"name": src,
187+
"display_name": accessLogSourceDisplayName[src],
188+
"path": path,
189+
})
190+
}
191+
// Only attach Resources when we actually have a payload — keeps
192+
// the JSON envelope's `resources` field truly omitempty.
193+
if len(logSources) > 0 {
194+
return gear.ProbeAvailableWithResources(
195+
"access-log endpoint registered",
196+
caps,
197+
map[string]any{"log_sources": logSources},
198+
)
159199
}
160200
return gear.ProbeAvailable("access-log endpoint registered", caps)
161201
}
162202

163-
// Initialize captures the path overrides for later use by the
164-
// handler. Probe already populated paths; this re-runs it because
165-
// Probe runs before Initialize on first boot, and we want explicit
166-
// re-resolution on Initialize so test harnesses that construct a
167-
// gear without calling Probe still get usable paths.
168-
func (g *Gear) Initialize(ctx context.Context, deps gear.Dependencies) error {
169-
if err := g.BaseGear.Initialize(ctx, deps); err != nil {
170-
return err
171-
}
172-
g.paths = map[string]string{
203+
// pathsFromDeps extracts the operator-overrides map from
204+
// gear.Dependencies. Shared by Probe (local-only, side-effect-free)
205+
// and Initialize (mutates g.paths so handlers can read it later)
206+
// so the deps→map mapping has one source of truth.
207+
func pathsFromDeps(deps gear.Dependencies) map[string]string {
208+
return map[string]string{
173209
"haproxy": deps.HAProxyAccessLog,
174210
"nginx": deps.NginxAccessLog,
175211
"apache": deps.ApacheAccessLog,
176212
"caddy": deps.CaddyAccessLog,
177213
}
214+
}
215+
216+
// Initialize captures the path overrides for later use by the
217+
// handler. Probe is side-effect-free (#112 review) so this is the
218+
// single place g.paths gets written; the handler reads g.paths via
219+
// resolveLogPath during request handling.
220+
func (g *Gear) Initialize(ctx context.Context, deps gear.Dependencies) error {
221+
if err := g.BaseGear.Initialize(ctx, deps); err != nil {
222+
return err
223+
}
224+
g.paths = pathsFromDeps(deps)
178225
return nil
179226
}
180227

@@ -285,12 +332,27 @@ func parseWithFallback(primary, fallback accesslog.Parser, raw string) *accesslo
285332
return fallback.Parse(raw)
286333
}
287334

288-
// resolveLogPath returns the access-log path for src: the operator
289-
// override if set and readable, the well-known default if readable,
290-
// or "" when neither exists. Apache gets a second-chance lookup
291-
// against the RHEL-style path.
335+
// resolveLogPath returns the access-log path for src using the
336+
// operator-override map captured in g.paths. Thin wrapper over
337+
// resolveLogPathWith so existing handler callers keep their
338+
// `g.resolveLogPath(src)` ergonomics; Probe (side-effect-free)
339+
// uses resolveLogPathWith directly with a local map.
292340
func (g *Gear) resolveLogPath(src string) string {
293-
if override, ok := g.paths[src]; ok && override != "" {
341+
return g.resolveLogPathWith(src, g.paths)
342+
}
343+
344+
// resolveLogPathWith is the underlying resolver. Takes the
345+
// operator-override map explicitly so the caller (Probe / handler /
346+
// test) can avoid touching shared state:
347+
//
348+
// - the operator override if set and readable, OR
349+
// - the well-known default if readable, OR
350+
// - "" when neither exists.
351+
//
352+
// Apache gets a second-chance lookup against the RHEL-style path
353+
// because Debian and RHEL ship the log under different paths.
354+
func (g *Gear) resolveLogPathWith(src string, overrides map[string]string) string {
355+
if override, ok := overrides[src]; ok && override != "" {
294356
// Operator explicitly pointed us at a path — trust them.
295357
// If the path isn't readable the endpoint surfaces "tail
296358
// failed" rather than silently falling back to a

0 commit comments

Comments
 (0)