diff --git a/docs/adr/44455-nil-safe-analyze-boundary-and-test-score-helper.md b/docs/adr/44455-nil-safe-analyze-boundary-and-test-score-helper.md new file mode 100644 index 00000000000..4ce5d2fbcef --- /dev/null +++ b/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.* diff --git a/pkg/agentdrain/anomaly.go b/pkg/agentdrain/anomaly.go index 8b8d41be904..0d80b79dd42 100644 --- a/pkg/agentdrain/anomaly.go +++ b/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() { diff --git a/pkg/agentdrain/anomaly_test.go b/pkg/agentdrain/anomaly_test.go index 52ae195d835..5628dc76691 100644 --- a/pkg/agentdrain/anomaly_test.go +++ b/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")