Refactor agentdrain event processing pipeline - #54977
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors agent event processing to reduce duplicate work, centralize anomaly-detector lifecycle, and improve package organization.
Changes:
- Reuses prepared tokens and consolidates inference, training, and reporting under one lock.
- Validates and refreshes anomaly detectors during construction and snapshot loading.
- Moves template utilities and simplifies latest action-pin lookup.
Show a summary per file
| File | Description |
|---|---|
pkg/agentdrain/template.go |
Houses tokenization and template algebra. |
pkg/agentdrain/persist.go |
Validates and restores detector configuration. |
pkg/agentdrain/miner.go |
Refactors event processing and detector lifecycle. |
pkg/agentdrain/miner_test.go |
Tests threshold validation and snapshot refresh. |
pkg/agentdrain/event.go |
Removes relocated tokenization logic. |
pkg/agentdrain/cluster.go |
Removes relocated template operations. |
pkg/actionpins/references.go |
Uses the dedicated latest-pin helper. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| // AnalyzeEvent performs inference on the event, builds an AnomalyReport, and | ||
| // then calls TrainEvent to update the miner. Returns the match result and report. |
There was a problem hiding this comment.
Updated the AnalyzeEvent doc comment in d1e95c2: it now describes the direct state update (prepare once, then inference + mutation + report under a single write lock) instead of the old TrainEvent call chain.
| if err != nil { | ||
| return nil, fmt.Errorf("agentdrain: NewMiner: %w", err) | ||
| } | ||
| detector, err := NewAnomalyDetector(cfg.SimThreshold, cfg.RareClusterThreshold) |
There was a problem hiding this comment.
Fixed in d1e95c2: NewAnomalyDetector now rejects math.NaN() explicitly (math.IsNaN(simThreshold) || simThreshold < 0 || simThreshold > 1), with new cases in TestNewAnomalyDetector_ThresholdBoundaries and TestNewMinerRejectsInvalidAnomalyThresholds.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review complete for PR #54977: refactor consolidates duplicated tokenization/detector logic into template.go and adds a shared trainTokens/prepare helper, netting -117/+190... actual diff is a cleanup reducing duplication, no new speculative abstractions, unused flexibility, or hand-rolled stdlib reinventions found. Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This refactor is moving in the right direction, but the new anomaly-detector validation is still incomplete: NaN thresholds are accepted, which undercuts the correctness of the new constructor/load-time guardrails.
Blocking theme
NewAnomalyDetectoronly checks< 0and> 1, somath.NaN()passes validation.- Because this PR now eagerly constructs detectors in both
NewMinerandLoadJSON, that bug directly weakens the exact safety improvement this change is trying to introduce.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 10.1 AIC · ⌖ 7.82 AIC · ⊞ 7K
Comment /review to run again
| if err != nil { | ||
| return nil, fmt.Errorf("agentdrain: NewMiner: %w", err) | ||
| } | ||
| detector, err := NewAnomalyDetector(cfg.SimThreshold, cfg.RareClusterThreshold) |
There was a problem hiding this comment.
This threshold validation still accepts math.NaN() because both range comparisons are false, so an invalid persisted or constructed SimThreshold can slip through and make all matching/reporting decisions behave unpredictably.
💡 Reject NaN explicitly
simThreshold < 0 || simThreshold > 1 does not catch NaN in Go. That means this refactor's new constructor-time validation is still porous, and LoadJSON can successfully hydrate a detector with a non-numeric threshold.
A direct guard fixes it:
if math.IsNaN(simThreshold) || simThreshold < 0 || simThreshold > 1 {
return nil, fmt.Errorf("agentdrain: NewAnomalyDetector: simThreshold must be in [0,1], got %g", simThreshold)
}Please add a test that covers math.NaN() on both NewMiner and LoadJSON, because the new validation path is the whole point of this change.
There was a problem hiding this comment.
Applied the suggested guard in d1e95c2 and added NaN coverage to TestNewAnomalyDetector_ThresholdBoundaries and TestNewMinerRejectsInvalidAnomalyThresholds. No LoadJSON NaN case was added because NaN cannot round-trip through JSON — encoding/json refuses to marshal it and there is no NaN literal to unmarshal — so the LoadJSON test uses an out-of-range value (1.1) with a comment explaining why.
There was a problem hiding this comment.
Impeccable Review — distill + extract modes
This refactor moves computeSimilarity, mergeTemplate, extractParams, and Tokenize into a dedicated template.go file, promotes AnomalyDetector to a long-lived Miner field, and collapses the double-lock pattern in AnalyzeEvent into a single write-lock critical section. The structural goals are sound.
Two pre-existing comments flag the blocking issues
- NaN threshold bypass —
NewAnomalyDetectordoes not rejectmath.NaN()because range comparisons with NaN are always false. Constructor-time validation can be silently bypassed. - Stale
AnalyzeEventdoc comment — the comment still says it callsTrainEvent, but the refactor replaced that with a directtrainTokenscall.
Non-blocking observations
prepare()cleanly deduplicates the flatten→mask→tokenize sequence acrossTrainEventandAnalyzeEvent. Good extraction.- The single
m.mu.Lock()inAnalyzeEvent(replacing two separate RLock + Lock acquisitions) is correct —findBestMatchingClusterandtrainTokensare now atomic. No TOCTOU race. trainTokensstage-propagation uses first-write-wins semantics (c.Stageset only when empty). A brief inline comment would improve readability.TestLoadJSONRefreshesAndValidatesAnomalyThresholdsdirectly accessesm.detector.threshold— this tightly couples the test to an unexported field. Consider asserting through observable behavior or a smallThreshold()accessor.
Address the two flagged blocking issues before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.7 AIC · ⌖ 9.1 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on a concurrency concern and two test-quality issues.
📋 Key Themes & Highlights
Key Themes
- Lock scope widening (
miner.goline 158):AnalyzeEventnow holds a write lock over both inference and training. The old code usedRLockfor the read-only inference pass; the new code serialises all callers entirely. For the current single-drain use case this is fine, but the intent should be documented or the two-phase approach restored. - Internal field access in test (
miner_test.goline 72):m.detector.thresholdis unexported. The test is inpackage agentdrain(white-box), so it compiles, but a behaviour-driven assertion would be less brittle. - Missing NaN edge case (
miner_test.goline 43): an existing comment onNewAnomalyDetectornotes thatNaNslips through the range guard; the new table-driven test doesn't cover this. Tokenizeexport (template.goline 13): all sibling helpers (computeSimilarity,mergeTemplate,extractParams) are unexported;Tokenizeis the odd one out — worth confirming whether any external callers remain.
Positive Highlights
- ✅ Eliminating the per-call
NewAnomalyDetectorallocation is a clear correctness win (and a nice perf win). - ✅ The
preparehelper cleanly centralises flatten-mask-tokenize so neitherTrainEventnorAnalyzeEventduplicate it. - ✅
LoadJSONnow validates thresholds before swapping state, so an invalid snapshot file fails safely. - ✅ Moving template algebra into
template.goimproves navigability — the file name makes the contents immediately findable. - ✅ New tests cover both construction-time and load-time validation, which were previously untested paths.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.7 AIC · ⌖ 10.4 AIC · ⊞ 7.6K
Comment /matt to run again
| defer m.mu.Unlock() | ||
| inferResult, _ := m.findBestMatchingCluster(tokens) | ||
| isNew := inferResult == nil | ||
| result := m.trainTokens(tokens, evt.Stage) |
There was a problem hiding this comment.
[/codebase-design] AnalyzeEvent now holds a write lock for both inference and training, where the old code used RLock for the inference phase. This serializes all concurrent callers for the entire operation, eliminating previous read parallelism.
💡 Consider retaining RLock for the read-only inference pass
If concurrent callers matter, the inference pass can stay under RLock since findBestMatchingCluster is read-only:
m.mu.RLock()
inferResult, _ := m.findBestMatchingCluster(tokens)
m.mu.RUnlock()
isNew := inferResult == nil
m.mu.Lock()
defer m.mu.Unlock()
result := m.trainTokens(tokens, evt.Stage)
cluster, _ := m.store.get(result.ClusterID)
report := m.detector.Analyze(result, isNew, cluster)The trade-off: isNew may rarely be stale when two goroutines race on a new cluster — which was also true in the previous implementation. If single-goroutine use is the intended invariant, a comment to that effect would prevent future misuse.
@copilot please address this.
There was a problem hiding this comment.
Kept the single write lock and documented why in d1e95c2. The inference result is now reused for the mutation (applyMatch(tokens, stage, inferResult)), which removes the second cluster search that trainTokens used to perform and makes isNew consistent with the cluster the event is trained into. Splitting the phases across RLock/Lock would reintroduce that duplicate search and a stale-isNew window, so the doc comment now states the rationale explicitly.
| data, err = json.Marshal(snapshot) | ||
| require.NoError(t, err) | ||
| require.NoError(t, m.LoadJSON(data)) | ||
| assert.InDelta(t, 0.8, m.detector.threshold, 0) |
There was a problem hiding this comment.
[/tdd] The test accesses the unexported m.detector.threshold field directly — this couples the test to internal struct layout. A behaviour-driven assertion (e.g., re-running inference after load and checking the resulting Similarity) would be less brittle and would also verify that the detector actually works with the new threshold.
💡 Suggested behavioural assertion
After LoadJSON succeeds, drive a pair of Train calls and assert that the similarity score is computed using the updated threshold rather than reading the private field directly:
// Instead of:
assert.InDelta(t, 0.8, m.detector.threshold, 0)
// Consider:
result1, _ := m.Train("foo bar baz")
result2, _ := m.Train("foo bar qux")
// similarity should reflect the reloaded threshold in cluster matching
assert.NotNil(t, result2)Or expose a SimThreshold() float64 accessor if the field value truly needs to be asserted.
@copilot please address this.
There was a problem hiding this comment.
Leaving the direct field assertion. miner_test.go is in the same package and other tests in the package (for example TestNewAnomalyDetector_ThresholdBoundaries) already assert on detector.threshold directly; the point of this test is specifically that LoadJSON rebuilds the detector from the snapshot config, which a similarity-based assertion would only verify indirectly. Adding a public SimThreshold() accessor just for a test would grow the exported API for no production caller.
| cfg.RareClusterThreshold = -1 | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
[/tdd] TestNewMinerRejectsInvalidAnomalyThresholds doesn't cover math.NaN() for SimThreshold, which the existing comment on NewAnomalyDetector (line 32) notes is still accepted because NaN comparison returns false for both range checks. Adding this case to the table-driven test would lock in the fix explicitly.
💡 Add NaN test case
{
name: "NaN similarity threshold",
mutate: func(cfg *Config) {
cfg.SimThreshold = math.NaN()
},
},Import "math" and add it to the existing table. This also serves as a regression test if NewAnomalyDetector is later tightened to reject NaN.
@copilot please address this.
There was a problem hiding this comment.
Added the NaN case to TestNewMinerRejectsInvalidAnomalyThresholds and to TestNewAnomalyDetector_ThresholdBoundaries, and NewAnomalyDetector now rejects NaN (d1e95c2).
|
|
||
| // Tokenize splits a log line on whitespace and returns the individual tokens. | ||
| func Tokenize(line string) []string { | ||
| return strings.Fields(line) |
There was a problem hiding this comment.
[/codebase-design] prepare is a new unexported method but Tokenize remains exported. Since Tokenize is now only called from prepare (moved from event.go to template.go), it's worth confirming whether external callers still exist — if not, unexport it to keep the surface area minimal.
💡 Quick check
grep -r 'agentdrain\.Tokenize' .If there are no external callers, rename to tokenize (lowercase) to match computeSimilarity, mergeTemplate, and extractParams — all unexported in the same file.
@copilot please address this.
There was a problem hiding this comment.
Tokenize stays exported: it is part of the package's documented public API (pkg/agentdrain/README.md lists it under the utility API) and is covered by TestSpec_PublicAPI_Utility_Tokenize in spec_test.go, which calls it through the external agentdrain package. Unexporting it would be a breaking API change outside the scope of this refactor.
…le lock Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (190 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
|
|
@copilot run pr-finisher skill |
…eEvent Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Miner.AnalyzeEventflattened, masked, and tokenized every event twice, rebuilt an immutable anomaly detector per call, and split inference/training/reporting across separate lock scopes.Event pipeline
Detector lifecycle
AnomalyDetectorinNewMiner.LoadJSON, so invalid persisted thresholds fail before state replacement.Package organization
template.go.GetLatestActionPinByRepo.Coverage
Run: https://github.com/github/gh-aw/actions/runs/32618879655> Generated by 👨🍳 PR Sous Chef · gpt54 · 6.44 AIC · ⌖ 8.16 AIC · ⊞ 9.5K · ◷