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

- access-log: implement the documented Apache CLF fallback. Previously
  the handler used a single parser from sourceProfile (ApacheCombined)
  and the comments + PR body claimed a per-record fallback to
  ApacheCommon that didn't exist. RHEL hosts running default CLF would
  have produced zero parsed records. New parseWithFallback helper +
  sourceFallbackProfile map drive the actual fallback now; Apache is
  the only source using it today.

- access-log: status_min query parameter now accepts an explicit 0 to
  disable the filter. Was previously clamped to a 100 minimum, which
  silently coerced 0 to 100 and broke the "give me all records"
  intent. Default when the param is absent stays 500 (the dashboard's
  primary use case). Lock the new defaults in via two new tests.

- caddy: drop the AdminRunning field. The previous heuristic relied on
  caddy_admin_http_requests_total existing, which Prometheus doesn't
  emit for counters with zero increments. A freshly-started Caddy
  with admin enabled but no admin traffic yet would have falsely read
  "admin disconnected." The real signal is "did the scrape succeed?"
  — which the handler already conveys via 503 before the first
  successful scrape — so the field was redundant on success and
  misleading on cold start.

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

Author: sarg3nt

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

@@ -58,17 +58,28 @@ var defaultLogPaths = map[string]string{
 // "did we try multiple paths?" logic explicit at the call site.
 const apacheFallbackPath = "/var/log/httpd/access_log"
 
-// sourceProfile maps a source identifier to the parser profile the
-// endpoint applies to each line. Apache lines are tried as
-// "combined" first (most operators run that format); ApacheCommon
-// is the fallback handled inside the per-record retry.
+// sourceProfile maps a source identifier to the primary parser
+// profile the endpoint tries first. Apache uniquely also has a
+// fallback profile (CLF without Referer / User-Agent) tried when
+// the primary returns nil — see sourceFallbackProfile and
+// parseWithFallback.
 var sourceProfile = map[string]string{
 	"haproxy": accesslog.ProfileHAProxy,
 	"nginx":   accesslog.ProfileNginxCombined,
 	"apache":  accesslog.ProfileApacheCombined,
 	"caddy":   accesslog.ProfileCaddyJSON,
 }
 
+// sourceFallbackProfile names the second profile tried when the
+// primary returns nil for a line. Today only Apache has one: many
+// RHEL-style installs ship CLF (no Referer / User-Agent) by
+// default, so we try ApacheCombined first (covers the Debian
+// default + custom combined-format ops) and fall back to
+// ApacheCommon. Lines that match neither stay rejected as noise.
+var sourceFallbackProfile = map[string]string{
+	"apache": accesslog.ProfileApacheCommon,
+}
+
 // maxLimit caps the per-request `limit` parameter so a buggy
 // dashboard call can't make the agent shell `tail -n 100000` against
 // a 10 GB log file. 10 000 records matches the existing logs gear's
@@ -205,8 +216,17 @@ func (g *Gear) handleRecent(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "no parser registered for source "+source, http.StatusInternalServerError)
 		return
 	}
-
-	statusMin := parseIntDefault(r.URL.Query().Get("status_min"), 0, 100, 599, 500)
+	// fallback may be nil — most sources have a single profile.
+	// parseWithFallback handles the nil case as "no second try".
+	fallback := accesslog.ProfileByName(sourceFallbackProfile[source])
+
+	// status_min defaults to 500 (the dashboard's primary use case
+	// is 5xx insights) but lets callers pass an explicit 0 to
+	// disable the filter entirely. Min clamp is 0 (not 100) so
+	// "explicitly disable" works; the gating logic compares with
+	// rec.StatusCode < statusMin, which is a no-op when statusMin
+	// is 0.
+	statusMin := parseIntDefault(r.URL.Query().Get("status_min"), 500, 0, 599, 500)
 	limit := parseIntDefault(r.URL.Query().Get("limit"), defaultLimit, 1, maxLimit, defaultLimit)
 	lines := parseIntDefault(r.URL.Query().Get("lines"), defaultLines, 1, maxLines, defaultLines)
 
@@ -230,7 +250,7 @@ func (g *Gear) handleRecent(w http.ResponseWriter, r *http.Request) {
 	// the slice in reverse.
 	matches := make([]accesslog.Record, 0, limit)
 	for i := len(raw) - 1; i >= 0; i-- {
-		rec := parser.Parse(raw[i])
+		rec := parseWithFallback(parser, fallback, raw[i])
 		if rec == nil {
 			continue
 		}
@@ -249,6 +269,22 @@ func (g *Gear) handleRecent(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, resp)
 }
 
+// parseWithFallback tries primary first; if primary returns nil and
+// a fallback parser was registered for this source, it tries the
+// fallback. Returns nil only when both reject the line. The Apache
+// source uses this to handle both combined (default Debian) and
+// CLF (default RHEL) without the caller needing to pre-detect
+// which format the operator's running.
+func parseWithFallback(primary, fallback accesslog.Parser, raw string) *accesslog.Record {
+	if rec := primary.Parse(raw); rec != nil {
+		return rec
+	}
+	if fallback == nil {
+		return nil
+	}
+	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

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

@@ -247,6 +247,122 @@ func TestHandleRecentSurfacesTailFailure(t *testing.T) {
 	}
 }
 
+func TestHandleRecentApacheFallsBackToCommonLogFormat(t *testing.T) {
+	// RHEL-style Apache emits CLF by default (no Referer/UA). Our
+	// primary apache profile is combined; the gear must fall back
+	// to ApacheCommon for any line the combined regex rejects so
+	// the dashboard sees records on those hosts instead of an empty
+	// envelope.
+	g := newTestGear()
+	g.stat = statExisting("/var/log/apache2/access.log")
+	g.tail = staticTail([]string{
+		// CLF line — no trailing quoted fields. ApacheCombined
+		// rejects this; ApacheCommon must catch it.
+		`192.168.1.10 - - [01/Jan/2026:00:00:00 +0000] "GET /clf-route HTTP/1.1" 500 1234`,
+		// Combined line — both profiles would accept; primary wins.
+		`192.168.1.11 - - [01/Jan/2026:00:00:01 +0000] "GET /combined-route HTTP/1.1" 503 100 "-" "curl/8.0"`,
+	})
+
+	r := chi.NewRouter()
+	g.RegisterRoutes(r)
+	req := httptest.NewRequest(http.MethodGet, "/api/v1/access-log/apache/recent?status_min=500", nil)
+	rr := httptest.NewRecorder()
+	r.ServeHTTP(rr, req)
+
+	var resp Response
+	if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if resp.MatchCount != 2 {
+		t.Fatalf("MatchCount = %d, want 2 (combined + CLF fallback)", resp.MatchCount)
+	}
+	// Newest-first ordering: combined line is the second tail
+	// entry (index 1, newer). The records carry their actual
+	// profile so the dashboard can tell which parser matched —
+	// CLF lines come back tagged ProfileApacheCommon, combined as
+	// ProfileApacheCombined.
+	if resp.Records[0].Profile != accesslog.ProfileApacheCombined {
+		t.Errorf("first record profile = %q, want %q (combined wins for that line)", resp.Records[0].Profile, accesslog.ProfileApacheCombined)
+	}
+	if resp.Records[1].Profile != accesslog.ProfileApacheCommon {
+		t.Errorf("second record profile = %q, want %q (CLF fallback)", resp.Records[1].Profile, accesslog.ProfileApacheCommon)
+	}
+}
+
+func TestHandleRecentStatusMinZeroDisablesFilter(t *testing.T) {
+	// Explicit status_min=0 must return EVERY parsed record,
+	// including 2xx. The previous clamp at 100 silently coerced 0
+	// up to 100, which is correct for HTTP statuses but blocked the
+	// "give me everything" use case the dashboard wants for
+	// general-purpose log browsing.
+	g := newTestGear()
+	g.stat = statExisting("/var/log/nginx/access.log")
+	g.tail = staticTail([]string{
+		`1.1.1.1 - - [01/Jan/2026:00:00:00 +0000] "GET /a HTTP/1.1" 200 100 "-" "-"`,
+		`1.1.1.1 - - [01/Jan/2026:00:00:01 +0000] "GET /b HTTP/1.1" 304 0 "-" "-"`,
+		`1.1.1.1 - - [01/Jan/2026:00:00:02 +0000] "GET /c HTTP/1.1" 500 100 "-" "-"`,
+	})
+
+	r := chi.NewRouter()
+	g.RegisterRoutes(r)
+	req := httptest.NewRequest(http.MethodGet, "/api/v1/access-log/nginx/recent?status_min=0", nil)
+	rr := httptest.NewRecorder()
+	r.ServeHTTP(rr, req)
+
+	var resp Response
+	if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if resp.MatchCount != 3 {
+		t.Errorf("MatchCount = %d with status_min=0, want 3 (all records)", resp.MatchCount)
+	}
+}
+
+func TestHandleRecentStatusMinDefaultIs500(t *testing.T) {
+	// No status_min param → defaults to 500 (the dashboard's main
+	// use case). Locks in the documented default so a future tweak
+	// to parseIntDefault doesn't quietly change behaviour for
+	// existing callers.
+	g := newTestGear()
+	g.stat = statExisting("/var/log/nginx/access.log")
+	g.tail = staticTail([]string{
+		`1.1.1.1 - - [01/Jan/2026:00:00:00 +0000] "GET /a HTTP/1.1" 200 100 "-" "-"`,
+		`1.1.1.1 - - [01/Jan/2026:00:00:01 +0000] "GET /b HTTP/1.1" 500 100 "-" "-"`,
+	})
+	r := chi.NewRouter()
+	g.RegisterRoutes(r)
+	req := httptest.NewRequest(http.MethodGet, "/api/v1/access-log/nginx/recent", nil)
+	rr := httptest.NewRecorder()
+	r.ServeHTTP(rr, req)
+
+	var resp Response
+	_ = json.NewDecoder(rr.Body).Decode(&resp)
+	if resp.MatchCount != 1 {
+		t.Errorf("MatchCount without explicit status_min = %d, want 1 (default 500 keeps only the 500-line)", resp.MatchCount)
+	}
+}
+
+func TestParseWithFallbackTriesPrimaryFirst(t *testing.T) {
+	// Direct test of the helper to keep the contract pinned even
+	// if the handler rewires which sources use which fallback.
+	primary := accesslog.NginxCombinedProfile{}
+	fallback := accesslog.ApacheCommonProfile{}
+
+	combinedLine := `1.1.1.1 - - [01/Jan/2026:00:00:00 +0000] "GET /x HTTP/1.1" 200 100 "-" "curl/8.0"`
+	clfLine := `1.1.1.1 - - [01/Jan/2026:00:00:00 +0000] "GET /x HTTP/1.1" 200 100`
+
+	if got := parseWithFallback(primary, fallback, combinedLine); got == nil || got.Profile != accesslog.ProfileNginxCombined {
+		t.Errorf("combined line should match primary; got %+v", got)
+	}
+	if got := parseWithFallback(primary, fallback, clfLine); got == nil || got.Profile != accesslog.ProfileApacheCommon {
+		t.Errorf("CLF line should fall through to fallback; got %+v", got)
+	}
+	// No fallback registered → only primary attempted.
+	if got := parseWithFallback(primary, nil, clfLine); got != nil {
+		t.Errorf("nil fallback should not match CLF line; got %+v", got)
+	}
+}
+
 func TestParseIntDefaultClamps(t *testing.T) {
 	cases := []struct {
 		name                string

gearbox-agent/internal/gears/caddy/collector.go

@@ -37,30 +37,30 @@ type Stats struct {
 	// double-counting.
 	RequestErrorsTotal int64 `json:"request_errors_total"`
 
-	// AdminRunning is 1 when Caddy's admin endpoint reports itself
-	// alive, 0 otherwise. Lets the dashboard surface "Caddy
-	// running but admin disconnected" without a second probe.
-	AdminRunning bool `json:"admin_running"`
-
 	// CollectedAt is when the agent scraped, in RFC3339.
 	CollectedAt string `json:"collected_at"`
 }
 
 // ParsePrometheusOutput extracts a Stats struct from raw Caddy
 // `:2019/metrics` Prometheus output. Exported for unit testing.
+//
+// We deliberately do NOT report a separate "admin reachable"
+// boolean here: the scrape itself goes against the admin endpoint
+// (default `:2019/metrics`), so a Stats record landing in the
+// dashboard's cache already proves admin is reachable. Surfacing it
+// as a field would either always be true (redundant) or depend on
+// request-driven metrics like `caddy_admin_http_requests_total`,
+// which a freshly-started Caddy with admin enabled but no admin
+// traffic yet would not emit — confusing the dashboard with a false
+// "admin disconnected" reading. The 503 response from the handler
+// before the first scrape covers the "not reachable" state.
 func ParsePrometheusOutput(body string) Stats {
 	samples := promtext.Parse(body)
 	stats := Stats{CollectedAt: time.Now().UTC().Format(time.RFC3339)}
 
 	stats.RequestsTotal = int64(promtext.SumByName(samples, "caddy_http_requests_total"))
 	stats.RequestErrorsTotal = int64(promtext.SumByName(samples, "caddy_http_request_errors_total"))
 
-	if s := promtext.FirstByName(samples, "caddy_admin_http_requests_total"); s != nil {
-		// admin_http_requests_total existing at all means the
-		// admin endpoint is up — the metric is only registered
-		// when admin is enabled.
-		stats.AdminRunning = true
-	}
 	return stats
 }
 

gearbox-agent/internal/gears/caddy/collector_test.go

@@ -30,25 +30,21 @@ func TestParsePrometheusOutputSumsCounters(t *testing.T) {
 	want := Stats{
 		RequestsTotal:      59, // 42 + 17
 		RequestErrorsTotal: 3,
-		AdminRunning:       true,
 	}
 	if got != want {
 		t.Errorf("Stats = %+v, want %+v", got, want)
 	}
 }
 
-func TestParsePrometheusOutputHandlesMissingAdminMetric(t *testing.T) {
-	// Admin endpoint disabled (admin off in Caddyfile) — the
-	// `caddy_admin_*` metric won't appear; AdminRunning must be
-	// false even though the rest of the scrape is fine.
-	body := `caddy_http_requests_total{server="srv0"} 7
-`
-	got := ParsePrometheusOutput(body)
-	if got.AdminRunning {
-		t.Error("AdminRunning should be false when admin metric is absent")
-	}
-	if got.RequestsTotal != 7 {
-		t.Errorf("RequestsTotal = %d, want 7", got.RequestsTotal)
+func TestParsePrometheusOutputBackgroundEmptyBody(t *testing.T) {
+	// A scrape that returns 200 with no recognisable Caddy metric
+	// must parse cleanly to zero counters (NOT an error). The
+	// "admin reachable" question is answered by whether a scrape
+	// succeeded at all, which the handler conveys via 503 — not by
+	// any field on the returned Stats.
+	got := ParsePrometheusOutput("")
+	if got.RequestsTotal != 0 || got.RequestErrorsTotal != 0 {
+		t.Errorf("empty body should yield zero-valued counters, got %+v", got)
 	}
 }