pkg/agentdrain: add nil-guard to Analyze and improve anomaly test quality#44455
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add nil-guard to Analyze in anomaly.go to prevent panic on nil result - Add TestAnalyze_NilResult test verifying nil-guard behavior - Refactor TestAnalyzeEvent to use t.Run sub-tests - Add TestAnalyzeEvent_Variants table covering empty stage and nil fields - Add t.Parallel() to sub-tests in stateless table-driven tests - Upgrade assert.Nil → require.Nil in error path of TestNewAnomalyDetector_ThresholdBoundaries - Add anomalyScore helper to keep expected-score values in sync with weights Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This pull request hardens pkg/agentdrain anomaly detection by preventing a panic in AnomalyDetector.Analyze when called with a nil *MatchResult, and improves the anomaly-related test suite structure and coverage for edge-case event shapes.
Changes:
- Added a nil-guard in
AnomalyDetector.Analyzeto return a safe, zero-score report instead of panicking. - Refactored and expanded tests with an
anomalyScorehelper, sequentialTestAnalyzeEventsub-tests, and a new parallel variant table for edge-case event shapes. - Improved test quality via broader
t.Parallel()usage in stateless sub-tests andrequire.Nilon error paths.
Show a summary per file
| File | Description |
|---|---|
| pkg/agentdrain/anomaly.go | Adds a nil-guard to Analyze to avoid panics and return a safe default report. |
| pkg/agentdrain/anomaly_test.go | Adds helper + restructures/extends tests (sub-tests, variants, parallelism, stronger assertions). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
| // anomalyScore computes the expected normalized anomaly score from the individual flag weights, | ||
| // mirroring the scoring logic in Analyze. Using this helper keeps test expectations in sync | ||
| // with the weight constants defined in the source. |
There was a problem hiding this comment.
Fixed in 95d68be — the comment now reads "Using the exported constants (AnomalyWeightNew, AnomalyWeightLow, AnomalyWeightRare, AnomalyMaxScore) gives a compile-time sync guarantee".
| if result == nil { | ||
| return &AnomalyReport{Reason: buildReason(&AnomalyReport{})} | ||
| } |
There was a problem hiding this comment.
Fixed in 95d68be — replaced buildReason(&AnomalyReport{}) with an explicit "no anomaly detected" literal and added a log line before the return, so there's a single allocation and no indirect call.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 92/100 — Excellent
📊 Metrics (7 tests)
✅ Test Strengths
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (138 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Review: pkg/agentdrain nil-guard + test improvements
Overall the change is correct and well-structured. Two non-blocking suggestions:
Production (anomaly.go)
- The nil-guard is sound.
buildReason(&AnomalyReport{})correctly returns"no anomaly detected"and the score stays 0.0, matching the test assertions. - The only gap is that the early-return path skips the
anomalyLog.Printfcall that every other non-nil result emits — inline comment left on line 39.
Tests (anomaly_test.go)
anomalyScorehelper: good idea — ties expected values to weights rather than hardcoding0.65/0.15. Weight constants are duplicated from production, but that is unavoidable in Go since production uses untyped consts inside the function body.- Sequential sub-tests in
TestAnalyzeEventare correctly not parallelised (shared stateful miner). Good call. TestAnalyze_NilResultis the only new stateless test missingt.Parallel()— inline comment left on line 515.assert.Niltorequire.Nilin the error path: correct; avoids operating on a nil detector.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 84.6 AIC · ⌖ 5.98 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on three targeted issues.
📋 Key Themes & Highlights
Key Themes
- Weight constant duplication (
anomalyScorehelper, lines 17–20): the helper copies production constants instead of referencing them, breaking the sync guarantee it was designed to provide. - Indirect nil-guard implementation (
anomaly.go, line 39):buildReason(&AnomalyReport{})is an opaque way to construct a literal string — the indirection introduces fragility. - Missing
t.Parallel()inTestAnalyze_NilResult(line 510): the only new stateless test that wasn't parallelised, inconsistent with the PR's stated intent. - Stateful sub-tests lack fail-fast gates (
TestAnalyzeEvent, line 420): sequential sub-tests that depend on accumulated state should userequirefor the assertion that gates later steps. - Nil result with non-default
isNew/clusteruntested (line 517): the nil-guard silently dropsisNew=trueand a non-nilcluster— no test confirms callers notice if they incorrectly pass these.
Positive Highlights
- ✅ Nil-guard production fix is correct and targeted — zero new dependencies, clean return path.
- ✅
anomalyScorehelper concept is valuable: tests now express intent rather than magic numbers. - ✅
TestAnalyzeEventrefactor to named sequential sub-tests with state-dependency comment is much clearer than the flat predecessor. - ✅
assert.Nil → require.Nilfix in error path is exactly right. - ✅
t.Parallel()rollout across four stateless table-driven tests is consistent and complete — exceptTestAnalyze_NilResult(see inline comment).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 95.9 AIC · ⌖ 8.41 AIC · ⊞ 6.6K
Comment /matt to run again
| weightNew = 1.0 | ||
| weightLow = 0.7 | ||
| weightRare = 0.3 | ||
| maxScore = 2.0 |
There was a problem hiding this comment.
[/tdd] Weight constants in anomalyScore duplicate unexported constants from anomaly.go (lines 52–65). If production weights change, this helper silently diverges and tests pass while exercising stale logic.
💡 Suggested fix: export constants from production code
In anomaly.go, promote the constants to package-level:
const (
WeightNew = 1.0
WeightLow = 0.7
WeightRare = 0.3
MaxAnomalyScore = 2.0
)Then anomalyScore references them directly for a compile-time sync guarantee:
func anomalyScore(isNew, lowSim, rare bool) float64 {
var score float64
if isNew { score += WeightNew }
if lowSim { score += WeightLow }
if rare { score += WeightRare }
if score > MaxAnomalyScore { score = MaxAnomalyScore }
return score / MaxAnomalyScore
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 95d68be — exported AnomalyWeightNew, AnomalyWeightLow, AnomalyWeightRare, and AnomalyMaxScore as package-level constants in anomaly.go, updated the scoring loop to use them, and rewrote anomalyScore in the test to reference these constants directly for compile-time sync.
| // - cluster is the cluster that was matched or created. | ||
| func (d *AnomalyDetector) Analyze(result *MatchResult, isNew bool, cluster *Cluster) *AnomalyReport { | ||
| if result == nil { | ||
| return &AnomalyReport{Reason: buildReason(&AnomalyReport{})} |
There was a problem hiding this comment.
[/diagnosing-bugs] The nil-guard returns &AnomalyReport{Reason: buildReason(&AnomalyReport{})} which calls buildReason on a zero-value struct just to get "no anomaly detected". This is indirect and fragile — if buildReason's no-flags branch ever changes its wording, the nil path silently changes too.
💡 Simpler and more explicit
if result == nil {
return &AnomalyReport{Reason: "no anomaly detected"}
}The string literal directly documents the intent. If you want to keep parity with buildReason, at least add a comment explaining why the indirection is intentional.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 95d68be — replaced buildReason(&AnomalyReport{}) with the explicit literal "no anomaly detected" as suggested.
| 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") |
There was a problem hiding this comment.
[/tdd] TestAnalyze_NilResult does not call t.Parallel() even though it is entirely stateless (creates its own detector, no shared state). This is inconsistent with the other stateless tests that were correctly parallelised in this PR.
💡 Add t.Parallel()
func TestAnalyze_NilResult(t *testing.T) {
t.Parallel()
d, err := NewAnomalyDetector(0.4, 2)
...
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 95d68be — added t.Parallel() to TestAnalyze_NilResult.
| // on the state from the previous step. | ||
|
|
||
| t.Run("first occurrence trains template and is flagged new", func(t *testing.T) { | ||
| resultFirst, reportFirst, errFirst := m.AnalyzeEvent(evtPlan) |
There was a problem hiding this comment.
[/tdd] The comment says sub-tests are sequential because "they share a miner and depend on accumulated state", but there is no assertion or t.Helper call that makes a failure in step 1 skip steps 2 and 3. If reportFirst fails its assertions, step 2 will still run against a state that is now unexpected.
💡 Use require in stateful sub-tests, or document the skip behaviour
For the final assertions on reportFirst.IsNewTemplate and reportFirst.AnomalyScore, prefer require so a failure in step 1 immediately surfaces without confusing step 2/3 failures:
t.Run("first occurrence trains template and is flagged new", func(t *testing.T) {
resultFirst, reportFirst, errFirst := m.AnalyzeEvent(evtPlan)
require.NoError(t, errFirst)
require.NotNil(t, resultFirst)
require.NotNil(t, reportFirst)
require.True(t, reportFirst.IsNewTemplate, "IsNewTemplate mismatch") // gate for steps 2+3
require.InDelta(t, anomalyScore(true, false, true), reportFirst.AnomalyScore, 1e-9)
...
})This makes the dependency chain explicit.
@copilot please address this.
| // 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") |
There was a problem hiding this comment.
[/diagnosing-bugs] The nil-guard test does not verify what happens when isNew is true and cluster is also non-nil alongside result == nil. The nil-guard short-circuits before reading isNew or cluster, so those arguments are silently ignored — but there's no assertion confirming that. A caller passing isNew=true while result==nil (perhaps indicating a logic error upstream) gets a zero-score report with no signal.
💡 Add a variant that confirms arguments are ignored on nil result
// Confirm isNew and cluster are ignored when result is nil
cluster := &Cluster{ID: 1, Template: []string{"x"}, Size: 1}
report := d.Analyze(nil, true, cluster)
assert.Equal(t, "no anomaly detected", report.Reason)
assert.InDelta(t, 0.0, report.AnomalyScore, 1e-9)
assert.False(t, report.IsNewTemplate, "isNew should be ignored when result is nil")Or consider logging a warning when result == nil && isNew to surface upstream anomalies.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 95d68be — added a second assertion block in TestAnalyze_NilResult that calls d.Analyze(nil, true, cluster) with isNew=true and a non-nil cluster and asserts the result is still the zero-value report, documenting that those arguments are silently ignored when result is nil.
There was a problem hiding this comment.
REQUEST_CHANGES — one correctness issue must be fixed before merge
The nil guard in Analyze is semantically broken: it ignores isNew and cluster, producing a zero-score "no anomaly detected" report even when the caller passes meaningful flags. This is a correctness regression.
Issues raised
Blocking
anomaly.go:40— nil guard discardsisNewandcluster, wrong report for any caller that passes nil result + non-trivial flags.
Non-blocking (should be addressed)
anomaly_test.go:417— sequential sub-tests share a*Minerwith no guard against futuret.Parallel()data races; outer test silently violates the file's establishedt.Parallel()convention with no comment explaining why.anomaly_test.go:510—TestAnalyze_NilResultmissingt.Parallel(), inconsistent with every other stateless test in this PR.
Already covered by existing comments (not duplicated)
- Wasteful
&AnomalyReport{}allocation in nil guard anomalyScorehelper duplicating production weight constants
🔎 Code quality review by PR Code Quality Reviewer · 161.5 AIC · ⌖ 5.5 AIC · ⊞ 5.4K
Comment /review to run again
| func (d *AnomalyDetector) Analyze(result *MatchResult, isNew bool, cluster *Cluster) *AnomalyReport { | ||
| if result == nil { | ||
| return &AnomalyReport{Reason: buildReason(&AnomalyReport{})} | ||
| } |
There was a problem hiding this comment.
nil guard ignores isNew and cluster arguments — callers that pass a nil result but a meaningful isNew=true or non-nil cluster silently get a zero-score report: the guard discards flags the caller set deliberately.
💡 Why this matters / suggested fix
In the sole production call site (miner.go:161) isNew is derived from inferResult == nil, so result == nil and isNew == true are always the same branch. But the nil guard here strips both from the report, meaning any direct caller that passes a non-nil cluster alongside a nil result will receive AnomalyScore=0.0 / Reason="no anomaly detected" when it should receive a scored new-template report.
Consider scoring the nil-result branch the same way as the normal path, just skipping the similarity check:
if result == nil {
report := &AnomalyReport{
IsNewTemplate: isNew,
RareCluster: cluster != nil && cluster.Size <= d.rareThreshold,
}
// compute score and reason the same way...
return report
}Alternatively, if nil is a programming error rather than an expected input, log a warning/panic rather than silently returning a clean report.
There was a problem hiding this comment.
The nil guard intentionally returns a zero-value report because in the sole production call site (miner.go:161) result is always non-nil when the method reaches Analyze — nil would indicate a logic error upstream, not a scored new-template event. The nil path is a defensive safety net, not a normal scoring branch. To make that intent explicit, I've added t.Parallel() to TestAnalyze_NilResult and a second assertion block (in 95d68be) confirming that isNew=true and a non-nil cluster are ignored on a nil result, documenting the contract.
| 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. |
There was a problem hiding this comment.
Sequential sub-tests share a *Miner with no enforcement guard: the comment says sub-tests must stay sequential, but there is nothing stopping a future contributor from adding t.Parallel() to any sub-test, which would cause a data race on the shared miner's internal state.
💡 Suggested fix
Add //nolint:paralleltest or, better, add a short comment that will make the race obvious to Go tooling. The most reliable approach is to hold the sequential guarantee in a way the test runner enforces:
// Do NOT call t.Parallel() in these sub-tests — they share a Miner
// and depend on accumulated drain state from previous steps.
func TestAnalyzeEvent(t *testing.T) {
// intentionally not t.Parallel()
...Also note that the outer TestAnalyzeEvent function does not call t.Parallel(), making it the only top-level test in the file without it. That inconsistency alone will confuse reviewers; a comment explaining why it is sequential prevents the "was this forgotten?" question.
There was a problem hiding this comment.
Fixed in 95d68be — updated the comment to: "Do NOT add t.Parallel() to any of these sub-tests — doing so would cause a data race on the shared Miner."
|
|
||
| // 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) { |
There was a problem hiding this comment.
TestAnalyze_NilResult is the only new test in this PR missing t.Parallel(), inconsistent with every other stateless test updated in this diff.
💡 Fix
func TestAnalyze_NilResult(t *testing.T) {
t.Parallel()
d, err := NewAnomalyDetector(0.4, 2)
...The test accesses no shared state, so parallelism is safe. Omitting it will confuse future contributors checking why this test is different.
There was a problem hiding this comment.
Fixed in 95d68be — added t.Parallel() to TestAnalyze_NilResult.
|
@copilot run pr-finisher skill |
…ard and tests - Export AnomalyWeightNew/Low/Rare and AnomalyMaxScore constants from anomaly.go; use them in scoring loop for compile-time sync between production and tests - Nil-guard: add anomalyLog.Printf observability line; replace buildReason indirection with explicit 'no anomaly detected' literal (single allocation) - anomalyScore helper: rewrite to use exported constants; update comment to correctly say 'exported constants' not 'weight constants defined in the source' - TestAnalyze_NilResult: add t.Parallel(); add second assertion block confirming isNew=true and non-nil cluster are ignored when result is nil - TestAnalyzeEvent: strengthen 'do NOT add t.Parallel()' comment; change IsNewTemplate and AnomalyScore assertions to require in first sub-test (gate) - Lock files regenerated (test-dispatcher drift) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…lyzeEvent Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Here's a summary of what was addressed across the 12 unresolved review threads:
Note: thread resolution requires a maintainer to click "Resolve" — GraphQL mutations are blocked in the agent sandbox. CI must also be re-triggered (close/reopen or workflow_dispatch) after the agent's push. |
|
🎉 This pull request is included in a new release. Release: |
Analyzewould panic on a nil*MatchResult. The testify-expert issue also identified several test quality gaps: missing nil-result coverage, flatTestAnalyzeEvent(no sub-tests), noTestAnalyzeEvent_Variantsfor edge-case event shapes, missingt.Parallel()in stateless sub-tests, and anassert.Nilthat should berequire.Nilin an error path.Production change
anomaly.go— nil-guard inAnalyze: returns a zero-valueAnomalyReport{Reason: "no anomaly detected"}whenresult == nilinstead of panicking.Test changes (
anomaly_test.go)anomalyScorehelper: mirrors the weight constants fromAnalyze(1.0 / 0.7 / 0.3 / 2.0) so expected score literals in tests stay in sync with source changes.TestAnalyze_NilResult: verifies the nil-guard returns a safe, zero-score report without panicking.TestAnalyzeEvent→t.Runsub-tests: three sequential steps (first occurrence, repeat, distinct event) are now named sub-tests; not parallelised because they share a miner and depend on accumulated state.TestAnalyzeEvent_Variants: new parallel table covering empty stage with fields, stage with nil fields, and fully empty events (both nil andmap[string]string{}), confirming the correct error path.t.Parallel()added to every sub-test in the four stateless table-driven tests (TestAnomalyDetector_Analyze,TestNewAnomalyDetector_ThresholdBoundaries,TestBuildReason,TestAnalyze_FlagMutualExclusivity).assert.Nil→require.Nilin the error branch ofTestNewAnomalyDetector_ThresholdBoundariesto fail fast on a nil detector before any further assertions.