fix: drop Force on selfupgrade patch; wire MCP issues event-opt-in + total_matched

Three reviewer findings on PR #658.

CRITICAL — selfupgrade: Force on StrategicMergePatch always fails

K8s apimachinery rejects PatchOptions.Force on non-Apply patches
(apimachinery/pkg/apis/meta/v1/validation: "may not be specified
for non-apply patch"). The previous "fix" set Force=true on a
StrategicMergePatch, so self-upgrade has been 422'ing on every
call since that commit landed — masking itself as IsInvalid →
HTTP 400 in our error path.

Drop Force; keep FieldManager:"helm". The FieldManager alone is
what fixes the original bug (apiserver no longer derives "radar"
from User-Agent as the .image owner), and Force was never
necessary for that. Server-side apply with ApplyPatchType + Force
would also work, but that's a bigger surface change for no
additional win in the single-field-patch case.

Tripwire test inverts: Force MUST be nil now, not non-nil. If a
refactor flips it back the test catches it before the binary
ships.

IMPORTANT — MCP issues: event source silently empty post-default-off

After events became opt-in, the MCP issues tool kept the audit
opt-in loop but didn't add the parallel IncludeEvents wiring.
ComposeWithStats requires BOTH wantSource(event) AND
IncludeEvents, so an MCP caller passing source:"event" got zero
event rows while HTTP /api/issues was honoring it correctly.

Loop now sets IncludeEvents for SourceEvent (mirroring the audit
case) and defaults Since=1h when events are enabled and no window
was given — matches the HTTP handler's behavior. Updated the
source + since jsonschema descriptions to make the opt-in rules
discoverable to LLM callers.

IMPORTANT — MCP issues: total_matched missing from response

HTTP /api/issues returns total_matched (uncapped count) so the
caller can tell "200 returned" from "200 of 1000". The MCP
response dropped this field, leaving agents blind to truncation.
Add total_matched to the response map; field is already on
ComposeStats so no compose-layer change needed.

Author: nadaverell

internal/mcp/tools.go

@@ -337,9 +337,9 @@ type searchInput struct {
 type issuesInput struct {
 	Namespace string `json:"namespace,omitempty" jsonschema:"filter to one namespace"`
 	Severity  string `json:"severity,omitempty" jsonschema:"comma-separated: critical,warning"`
-	Source    string `json:"source,omitempty" jsonschema:"comma-separated: problem,audit,event,condition. audit excluded by default"`
+	Source    string `json:"source,omitempty" jsonschema:"comma-separated: problem,audit,event,condition. Defaults to problem+condition only. Pass 'event' to opt in K8s Warning events (off by default — they flood thousands per cluster and mostly duplicate problem-source rows). Pass 'audit' to opt in best-practice findings (off by default — 50–200 per cluster)."`
 	Kind      string `json:"kind,omitempty" jsonschema:"comma-separated kind filter (e.g. Deployment,Pod)"`
-	Since     string `json:"since,omitempty" jsonschema:"event lookback window, e.g. 15m or 1h"`
+	Since     string `json:"since,omitempty" jsonschema:"event lookback window, e.g. 15m or 1h. Only affects the event source; when events are enabled and since is omitted, defaults to 1h to avoid pulling the full event-cache backlog."`
 	Limit     int    `json:"limit,omitempty" jsonschema:"max issues returned (default 200, max 1000)"`
 	Filter    string `json:"filter,omitempty" jsonschema:"optional CEL boolean expression run against each composed Issue. Bindings: severity, source, kind, group, ns (the namespace — note: use 'ns' not 'namespace' because the latter is a CEL reserved word), name, reason, message, count (int), cluster, last_seen (unix seconds). Examples: 'severity == \"critical\" && count > 5', 'source == \"condition\" && ns.startsWith(\"prod-\")'"`
 }
@@ -1492,19 +1492,32 @@ func handleIssuesTool(_ context.Context, _ *mcp.CallToolRequest, input issuesInp
 		}
 		filters.Since = d
 	}
-	// audit-source opt-in via either source list OR an explicit flag
-	// — honor the source list here since the input doesn't expose a
-	// separate include_audit knob.
+	// Audit + event sources are both opt-in (default off). The
+	// MCP input doesn't surface separate include_* knobs, so the
+	// source list IS the opt-in. Mirror the HTTP handler's
+	// behavior — including the 1h since-default when events are
+	// enabled with no explicit window, so an MCP caller doesn't
+	// silently inherit the full event-cache backlog.
 	for _, s := range filters.Sources {
-		if s == issues.SourceAudit {
+		switch s {
+		case issues.SourceAudit:
 			filters.IncludeAudit = true
-			break
+		case issues.SourceEvent:
+			filters.IncludeEvents = true
 		}
 	}
+	if filters.IncludeEvents && filters.Since == 0 {
+		filters.Since = time.Hour
+	}
 	out, stats := issues.ComposeWithStats(provider, filters)
 	resp := map[string]any{
 		"issues": out,
 		"total":  len(out),
+		// total_matched is the uncapped count — tells the caller
+		// whether the response is windowed or the whole set. Without
+		// it, an MCP agent can't distinguish "200 returned" from
+		// "200 of 1000". Mirrors the HTTP /api/issues response shape.
+		"total_matched": stats.TotalMatched,
 	}
 	if stats.FilterErrors > 0 {
 		resp["filter_errors"] = stats.FilterErrors

internal/server/selfupgrade.go

@@ -16,16 +16,24 @@ import (
 )
 
 // selfUpgradePatchOptions returns the PatchOptions used by the self-upgrade
-// endpoint. Field manager "helm" plus Force is what keeps `.image` ownership
-// stable across self-upgrade and `helm upgrade` cycles. With an empty
-// FieldManager the apiserver derives one from User-Agent → "radar", which
-// then permanently owns .image and breaks every subsequent helm upgrade.
+// endpoint. FieldManager "helm" is what keeps `.image` ownership stable
+// across self-upgrade and `helm upgrade` cycles: with an empty FieldManager
+// the apiserver derives one from User-Agent → "radar", which then
+// permanently owns .image and breaks every subsequent helm upgrade with a
+// server-side-apply conflict.
 //
-// Extracted for tripwire test; if a refactor reverts these values, the test
-// in selfupgrade_test.go fails before the bug ships.
+// We deliberately do NOT set Force here. K8s apimachinery rejects Force on
+// non-Apply patches (apimachinery/pkg/apis/meta/v1/validation:
+// `field.Forbidden("force", "may not be specified for non-apply patch")`)
+// so a StrategicMergePatch with Force=true returns 422 Invalid and the
+// upgrade never runs. If we ever need to reclaim conflicting ownership,
+// the route is to switch the request to ApplyPatchType with a full apply
+// object — not to flip Force back on a strategic merge.
+//
+// Extracted for tripwire test; if a refactor reverts these values, the
+// test in selfupgrade_test.go fails before the bug ships.
 func selfUpgradePatchOptions() metav1.PatchOptions {
-	force := true
-	return metav1.PatchOptions{FieldManager: "helm", Force: &force}
+	return metav1.PatchOptions{FieldManager: "helm"}
 }
 
 // handleSelfUpgrade patches this Radar Deployment's container image so the
@@ -94,8 +102,10 @@ func (s *Server) handleSelfUpgrade(w http.ResponseWriter, r *http.Request) {
 		case apierrors.IsForbidden(err):
 			s.writeError(w, http.StatusForbidden, "SA lacks patch permission on this Deployment (rbac.selfUpgrade=true?)")
 		case apierrors.IsConflict(err):
-			// Force=true reclaims field ownership, but a concurrent helm
-			// upgrade can still race. Retryable on the caller's side.
+			// A concurrent helm upgrade or apply can race the patch.
+			// Retryable on the caller's side. (We could reclaim
+			// ownership via server-side apply, but the StrategicMerge
+			// path keeps the request small and a retry is cheap.)
 			s.writeError(w, http.StatusConflict, "concurrent modification, retry")
 		case apierrors.IsTooManyRequests(err) || apierrors.IsServerTimeout(err):
 			s.writeError(w, http.StatusServiceUnavailable, "apiserver throttled, retry")

internal/server/selfupgrade_test.go

@@ -3,20 +3,19 @@ package server
 import "testing"
 
 // TestSelfUpgradePatchOptions is a tripwire: it pins the PatchOptions used
-// by handleSelfUpgrade. Field manager "helm" + Force=true is what prevents
-// the apiserver from recording "radar" as the owner of .image (derived from
-// User-Agent when FieldManager is empty), which would break every later
-// `helm upgrade` with a server-side-apply conflict. See selfupgrade.go for
-// the full rationale.
+// by handleSelfUpgrade. FieldManager "helm" is what prevents the apiserver
+// from recording "radar" as the owner of .image (derived from User-Agent
+// when FieldManager is empty), which would break every later `helm
+// upgrade` with a server-side-apply conflict. Force MUST stay unset on a
+// StrategicMergePatch — apimachinery rejects it with a 422 Invalid so
+// flipping Force back on regresses self-upgrade to "always fails."
+// See selfupgrade.go for the full rationale.
 func TestSelfUpgradePatchOptions(t *testing.T) {
 	opts := selfUpgradePatchOptions()
 	if opts.FieldManager != "helm" {
 		t.Errorf("FieldManager = %q, want %q", opts.FieldManager, "helm")
 	}
-	if opts.Force == nil {
-		t.Fatal("Force is nil, want *true")
-	}
-	if !*opts.Force {
-		t.Errorf("Force = false, want true")
+	if opts.Force != nil {
+		t.Errorf("Force = %v, want nil (Force is forbidden on StrategicMergePatch and a non-nil value 422s the request)", opts.Force)
 	}
 }