pkg/agentdrain: add nil-guard to Analyze and improve anomaly test quality

docs/adr/44455-nil-safe-analyze-boundary-and-test-score-helper.md

@@ -0,0 +1,44 @@
+# ADR-44455: Nil-safe `Analyze` boundary and test score helper for anomaly detection
+
+**Date**: 2026-07-09
+**Status**: Draft
+**Deciders**: Unknown (generated from PR #44455 by adr-writer agent)
+
+---
+
+### Context
+
+`AnomalyDetector.Analyze` accepted a `*MatchResult` pointer but had no nil-guard. In production `AnalyzeEvent` calls `Analyze` after a clustering operation that can produce a nil result on certain error paths, causing an unrecovered panic. Additionally, test assertions in `anomaly_test.go` hard-coded magic float literals (e.g., `0.65`, `0.15`) that duplicated the weight constants defined in `Analyze`, making tests brittle when weights change.
+
+### Decision
+
+We will add a nil-guard at the top of `Analyze` that returns a zero-value `AnomalyReport` (score 0, all flags false, reason "no anomaly detected") when the caller passes `nil`. We will also introduce an `anomalyScore` test-only helper that mirrors the weight constants from `Analyze`, so test expectations stay in sync with production scoring logic without duplicating magic literals.
+
+### Alternatives Considered
+
+#### Alternative 1: Panic with a descriptive message on nil input
+
+Treat nil as a programming error — add `if result == nil { panic("Analyze: nil MatchResult") }`. This makes misuse louder, forcing callers to fix the bug rather than silently swallowing it. Rejected because the call site in `AnalyzeEvent` legitimately reaches this path during error handling and a panic there would surface as an unrecoverable runtime failure rather than a handled error.
+
+#### Alternative 2: Change the API to `(*AnomalyReport, error)`
+
+Return `(nil, error)` when `result == nil`, surfacing the anomaly as an explicit error the caller must handle. This is the most type-safe option and makes nil-path observable at compile time. Rejected because it requires changing every call site across the package and is a larger API break than the targeted bug fix warrants; callers currently rely on `Analyze` always returning a non-nil `*AnomalyReport`.
+
+### Consequences
+
+#### Positive
+- Eliminates the production panic on nil `*MatchResult`; the anomaly pipeline continues operating safely.
+- Test expectations are derived from production weight constants via the helper, so a weight change in `Analyze` propagates automatically to test failures rather than silently passing.
+- `t.Parallel()` added to all stateless sub-tests reduces wall-clock test time.
+
+#### Negative
+- A caller that passes nil due to a logic bug gets no feedback — the nil maps silently to "no anomaly detected", which could mask incorrect upstream behavior.
+- The `anomalyScore` helper duplicates the scoring algorithm in test code; if the scoring structure changes significantly (e.g., new flags added) the helper must be updated manually alongside the production code.
+
+#### Neutral
+- `TestAnalyzeEvent` sub-tests are not parallelised because they share a miner with accumulated state; the sequential dependency is now made explicit in comments.
+- `assert.Nil` → `require.Nil` in the error branch of `TestNewAnomalyDetector_ThresholdBoundaries` prevents further assertions on a nil detector after a validation error, which is a test correctness improvement with no production impact.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/agentdrain/anomaly.go

@@ -9,6 +9,15 @@ import (
 
 var anomalyLog = logger.New("agentdrain:anomaly")
 
+// Scoring weights used by Analyze. Exported so tests can reference them directly
+// and stay in sync with production logic at compile time.
+const (
+	AnomalyWeightNew  = 1.0
+	AnomalyWeightLow  = 0.7
+	AnomalyWeightRare = 0.3
+	AnomalyMaxScore   = 2.0
+)
+
 // AnomalyDetector evaluates match results and produces AnomalyReports.
 type AnomalyDetector struct {
 	threshold     float64
@@ -35,6 +44,10 @@ func NewAnomalyDetector(simThreshold float64, rareClusterThreshold int) (*Anomal
 //   - isNew indicates the line created a brand-new cluster.
 //   - cluster is the cluster that was matched or created.
 func (d *AnomalyDetector) Analyze(result *MatchResult, isNew bool, cluster *Cluster) *AnomalyReport {
+	if result == nil {
+		anomalyLog.Printf("Analyze: nil result, returning zero-value report")
+		return &AnomalyReport{Reason: "no anomaly detected"}
+	}
 	report := &AnomalyReport{
 		IsNewTemplate: isNew,
 		// LowSimilarity is mutually exclusive with IsNewTemplate: brand-new templates are
@@ -46,22 +59,21 @@ func (d *AnomalyDetector) Analyze(result *MatchResult, isNew bool, cluster *Clus
 	// Weighted anomaly score.
 	var score float64
 	if report.IsNewTemplate {
-		score += 1.0
+		score += AnomalyWeightNew
 	}
 	if report.LowSimilarity {
-		score += 0.7
+		score += AnomalyWeightLow
 	}
 	if report.RareCluster {
-		score += 0.3
+		score += AnomalyWeightRare
 	}
 	// Normalize to [0, 1].
-	const maxScore = 2.0
-	// Defensive guard: with current mutually exclusive flags the score cannot exceed maxScore,
+	// Defensive guard: with current mutually exclusive flags the score cannot exceed AnomalyMaxScore,
 	// but keep clamping in case future weighting or flag logic changes.
-	if score > maxScore {
-		score = maxScore
+	if score > AnomalyMaxScore {
+		score = AnomalyMaxScore
 	}
-	report.AnomalyScore = score / maxScore
+	report.AnomalyScore = score / AnomalyMaxScore
 
 	report.Reason = buildReason(report)
 	if anomalyLog.Enabled() {

pkg/agentdrain/anomaly_test.go

@@ -9,6 +9,27 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// anomalyScore computes the expected normalized anomaly score from the individual flag weights,
+// mirroring the scoring logic in Analyze. Using the exported constants (AnomalyWeightNew,
+// AnomalyWeightLow, AnomalyWeightRare, AnomalyMaxScore) gives a compile-time sync guarantee:
+// if production weights change, this helper diverges at compile time, not silently at runtime.
+func anomalyScore(isNew, lowSim, rare bool) float64 {
+	var score float64
+	if isNew {
+		score += AnomalyWeightNew
+	}
+	if lowSim {
+		score += AnomalyWeightLow
+	}
+	if rare {
+		score += AnomalyWeightRare
+	}
+	if score > AnomalyMaxScore {
+		score = AnomalyMaxScore
+	}
+	return score / AnomalyMaxScore
+}
+
 func TestAnomalyDetector_Analyze(t *testing.T) {
 	tests := []struct {
 		name              string
@@ -217,6 +238,7 @@ func TestAnomalyDetector_Analyze(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
 			d, err := NewAnomalyDetector(tt.simThreshold, tt.rareThreshold)
 			require.NoError(t, err, "NewAnomalyDetector should succeed with valid thresholds")
 			require.NotNil(t, d, "NewAnomalyDetector should return a non-nil detector")
@@ -272,11 +294,12 @@ func TestNewAnomalyDetector_ThresholdBoundaries(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
 			detector, err := NewAnomalyDetector(tt.simThreshold, tt.rareThreshold)
 			if tt.wantErr != "" {
 				require.Error(t, err, "NewAnomalyDetector should reject invalid thresholds")
 				assert.Contains(t, err.Error(), tt.wantErr, "error should describe invalid threshold")
-				assert.Nil(t, detector, "NewAnomalyDetector should return nil detector on validation error")
+				require.Nil(t, detector, "NewAnomalyDetector should return nil detector on validation error")
 				return
 			}
 
@@ -358,6 +381,7 @@ func TestBuildReason(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
 			r := &AnomalyReport{
 				IsNewTemplate: tt.isNewTemplate,
 				LowSimilarity: tt.lowSimilarity,
@@ -384,32 +408,126 @@ func TestAnalyzeEvent(t *testing.T) {
 		Fields: map[string]string{"status": "ok"},
 	}
 
-	// Step 1: first occurrence trains the template and is flagged as new.
-	resultFirst, reportFirst, errFirst := m.AnalyzeEvent(evtPlan)
-	require.NoError(t, errFirst, "AnalyzeEvent should not fail for first event")
-	require.NotNil(t, resultFirst, "AnalyzeEvent should return a non-nil result")
-	require.NotNil(t, reportFirst, "AnalyzeEvent should return a non-nil report")
-	assert.True(t, reportFirst.IsNewTemplate, "IsNewTemplate mismatch for first event")
-	assert.InDelta(t, 0.65, reportFirst.AnomalyScore, 1e-9, "AnomalyScore mismatch for first event")
-	assert.Equal(t, "new log template discovered; rare cluster (few observations)", reportFirst.Reason, "Reason mismatch for first event")
+	// Sub-tests are sequential: each step mutates the shared miner state and depends
+	// on the state from the previous step. Do NOT add t.Parallel() to any of these
+	// sub-tests — doing so would cause a data race on the shared Miner.
+
+	t.Run("first occurrence trains template and is flagged new", func(t *testing.T) {
+		resultFirst, reportFirst, errFirst := m.AnalyzeEvent(evtPlan)
+		require.NoError(t, errFirst, "AnalyzeEvent should not fail for first event")
+		require.NotNil(t, resultFirst, "AnalyzeEvent should return a non-nil result")
+		require.NotNil(t, reportFirst, "AnalyzeEvent should return a non-nil report")
+		// Both assertions below are gates for steps 2+3: a failure here stops the test
+		// before exercising state that is now invalid.
+		require.True(t, reportFirst.IsNewTemplate, "IsNewTemplate mismatch for first event")
+		require.InDelta(t, anomalyScore(true, false, true), reportFirst.AnomalyScore, 1e-9, "AnomalyScore mismatch for first event")
+		assert.Equal(t, "new log template discovered; rare cluster (few observations)", reportFirst.Reason, "Reason mismatch for first event")
+	})
+
+	t.Run("second identical occurrence reuses trained template", func(t *testing.T) {
+		resultSecond, reportSecond, errSecond := m.AnalyzeEvent(evtPlan)
+		require.NoError(t, errSecond, "AnalyzeEvent should not fail for second identical event")
+		require.NotNil(t, resultSecond, "AnalyzeEvent should return a non-nil result")
+		require.NotNil(t, reportSecond, "AnalyzeEvent should return a non-nil report")
+		assert.False(t, reportSecond.IsNewTemplate, "IsNewTemplate mismatch for second identical event")
+		assert.InDelta(t, anomalyScore(false, false, true), reportSecond.AnomalyScore, 1e-9, "AnomalyScore mismatch for second identical event")
+		assert.Equal(t, "rare cluster (few observations)", reportSecond.Reason, "Reason mismatch for second identical event")
+	})
+
+	t.Run("distinct event creates separate new template", func(t *testing.T) {
+		resultDistinct, reportDistinct, errDistinct := m.AnalyzeEvent(evtFinish)
+		require.NoError(t, errDistinct, "AnalyzeEvent should not fail for distinct event")
+		require.NotNil(t, resultDistinct, "AnalyzeEvent should return a non-nil result")
+		require.NotNil(t, reportDistinct, "AnalyzeEvent should return a non-nil report")
+		assert.True(t, reportDistinct.IsNewTemplate, "IsNewTemplate mismatch for distinct event")
+		assert.InDelta(t, anomalyScore(true, false, true), reportDistinct.AnomalyScore, 1e-9, "AnomalyScore mismatch for distinct event")
+		assert.Equal(t, "new log template discovered; rare cluster (few observations)", reportDistinct.Reason, "Reason mismatch for distinct event")
+	})
+}
+
+// TestAnalyzeEvent_Variants covers edge-case event shapes: empty stage and nil/empty fields.
+func TestAnalyzeEvent_Variants(t *testing.T) {
+	tests := []struct {
+		name       string
+		evt        AgentEvent
+		wantErr    bool
+		wantErrMsg string
+	}{
+		{
+			name: "empty stage with fields succeeds",
+			evt: AgentEvent{
+				Stage:  "",
+				Fields: map[string]string{"action": "start"},
+			},
+		},
+		{
+			name: "stage with nil fields succeeds",
+			evt: AgentEvent{
+				Stage:  "plan",
+				Fields: nil,
+			},
+		},
+		{
+			name:       "empty stage and nil fields returns error",
+			evt:        AgentEvent{Stage: "", Fields: nil},
+			wantErr:    true,
+			wantErrMsg: "empty event after masking",
+		},
+		{
+			name:       "empty stage and empty fields returns error",
+			evt:        AgentEvent{Stage: "", Fields: map[string]string{}},
+			wantErr:    true,
+			wantErrMsg: "empty event after masking",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			cfg := DefaultConfig()
+			m, err := NewMiner(cfg)
+			require.NoError(t, err, "NewMiner should succeed")
+
+			result, report, err := m.AnalyzeEvent(tt.evt)
+			if tt.wantErr {
+				require.Error(t, err, "AnalyzeEvent should return an error")
+				assert.Contains(t, err.Error(), tt.wantErrMsg, "error message mismatch")
+				assert.Nil(t, result, "AnalyzeEvent should return nil result on error")
+				assert.Nil(t, report, "AnalyzeEvent should return nil report on error")
+				return
+			}
+			require.NoError(t, err, "AnalyzeEvent should not return an error")
+			require.NotNil(t, result, "AnalyzeEvent should return a non-nil result")
+			require.NotNil(t, report, "AnalyzeEvent should return a non-nil report")
+		})
+	}
+}
+
+// TestAnalyze_NilResult ensures that Analyze does not panic when result is nil
+// and instead returns a zero-value report with the "no anomaly detected" reason.
+func TestAnalyze_NilResult(t *testing.T) {
+	t.Parallel()
+	d, err := NewAnomalyDetector(0.4, 2)
+	require.NoError(t, err, "NewAnomalyDetector should succeed")
 
-	// Step 2: second identical occurrence reuses the trained template.
-	resultSecond, reportSecond, errSecond := m.AnalyzeEvent(evtPlan)
-	require.NoError(t, errSecond, "AnalyzeEvent should not fail for second identical event")
-	require.NotNil(t, resultSecond, "AnalyzeEvent should return a non-nil result")
-	require.NotNil(t, reportSecond, "AnalyzeEvent should return a non-nil report")
-	assert.False(t, reportSecond.IsNewTemplate, "IsNewTemplate mismatch for second identical event")
-	assert.InDelta(t, 0.15, reportSecond.AnomalyScore, 1e-9, "AnomalyScore mismatch for second identical event")
-	assert.Equal(t, "rare cluster (few observations)", reportSecond.Reason, "Reason mismatch for second identical event")
+	// Nil result must not panic; the nil-guard in Analyze returns a safe report.
+	report := d.Analyze(nil, false, nil)
+	require.NotNil(t, report, "Analyze should return a non-nil report even for nil result")
+	assert.Equal(t, "no anomaly detected", report.Reason, "nil result should produce no-anomaly reason")
+	assert.InDelta(t, 0.0, report.AnomalyScore, 1e-9, "nil result should produce zero anomaly score")
+	assert.False(t, report.IsNewTemplate, "nil result should not set IsNewTemplate")
+	assert.False(t, report.LowSimilarity, "nil result should not set LowSimilarity")
+	assert.False(t, report.RareCluster, "nil result should not set RareCluster")
 
-	// Step 3: a distinct event creates a separate new template.
-	resultDistinct, reportDistinct, errDistinct := m.AnalyzeEvent(evtFinish)
-	require.NoError(t, errDistinct, "AnalyzeEvent should not fail for distinct event")
-	require.NotNil(t, resultDistinct, "AnalyzeEvent should return a non-nil result")
-	require.NotNil(t, reportDistinct, "AnalyzeEvent should return a non-nil report")
-	assert.True(t, reportDistinct.IsNewTemplate, "IsNewTemplate mismatch for distinct event")
-	assert.InDelta(t, 0.65, reportDistinct.AnomalyScore, 1e-9, "AnomalyScore mismatch for distinct event")
-	assert.Equal(t, "new log template discovered; rare cluster (few observations)", reportDistinct.Reason, "Reason mismatch for distinct event")
+	// Confirm that isNew=true and a non-nil cluster are silently ignored when result is nil.
+	// The nil-guard short-circuits before reading those arguments; callers should not rely on
+	// them being honoured when result is absent.
+	cluster := &Cluster{ID: 1, Template: []string{"x"}, Size: 1}
+	reportWithArgs := d.Analyze(nil, true, cluster)
+	require.NotNil(t, reportWithArgs, "Analyze should return a non-nil report for nil result with non-nil args")
+	assert.Equal(t, "no anomaly detected", reportWithArgs.Reason, "isNew and cluster must be ignored when result is nil")
+	assert.InDelta(t, 0.0, reportWithArgs.AnomalyScore, 1e-9, "AnomalyScore must be zero when result is nil")
+	assert.False(t, reportWithArgs.IsNewTemplate, "IsNewTemplate must be false when result is nil")
 }
 
 func TestAnalyze_FlagMutualExclusivity(t *testing.T) {
@@ -453,6 +571,7 @@ func TestAnalyze_FlagMutualExclusivity(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
 			report := d.Analyze(tt.result, tt.isNew, tt.cluster)
 			require.NotNil(t, report, "Analyze should always return a non-nil report")
 			assert.Equal(t, tt.wantLow, report.LowSimilarity, "LowSimilarity mismatch")