ALEC-306: Fix LLM clustering engine correctness (post-#157 review) - #167
Closed
joseanesONMS wants to merge 1 commit into
Closed
ALEC-306: Fix LLM clustering engine correctness (post-#157 review)#167joseanesONMS wants to merge 1 commit into
joseanesONMS wants to merge 1 commit into
Conversation
Addresses six review findings against the merged ALEC-301 LLM clustering engine and its topology refresh. engine/llm/LlmClusterEngine: - [P1] Decouple clustering from the RCA 'enabled' flag. readLlmConfig no longer requires 'enabled' (that toggle is Root Cause Analysis); clustering requires only the shared endpoint/model/key. Selecting the engine is the signal to use it — it no longer sits silently inactive under a valid config. - [P1] Enforce and record the shared token budget. Before each tick the engine checks UTC daily/monthly usage against the configured limits (same semantics as the RCA TokenBudget), and records each call's token usage into the shared ALEC_LLM_USAGE store, so clustering both respects and reports into the budget. - [P1] Send topology connectivity. The request now serializes the adjacency between alarm-bearing devices (from the cluster graph) and tags each alarm with its device, so the model can actually group by topology as advertised. - [P2] Bound the request. Beyond MAX_ALARMS (200) the engine clusters the most recent alarms rather than emitting a prompt guaranteed to overflow the model. - [P2] Deduplicate alarm memberships. parseResponse now assigns each alarm to at most one cluster (global + within-group dedup) and drops sub-2-alarm groups, so a repeated ID can't put an alarm in multiple situations or inflate a singleton. datasource/opennms-direct/DirectInventoryDatasource: - [P2] Reconcile deleted edges. The periodic refresh now removes edges ALEC still holds that the EdgeDao poll no longer returns (missed delete callback), instead of leaving their inventory forever. Edge-removal logic extracted to a shared removeEdgeInventory() used by both the delete callback and the refresh. Tests: 34 engine/llm + 20 datasource tests green (new coverage for the budget enforcement, usage recording, topology rendering, membership dedup, >=2 enforcement, and stale-edge reconciliation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UB6PGc2rpPbojnqbTHU5ND
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tracked by ALEC-306. Addresses the six review findings against the merged ALEC-301 LLM clustering engine (#157) — the engine could silently stay inactive under a valid config, bypass the token budget, and lacked the topology it needs.
P1 — correctness
enabledflag.readLlmConfigrequiredenabled— the Root Cause Analysis toggle — so an operator with endpoint/model/key set but RCA off gotnullevery tick. Clustering now requires only the shared connection fields; selecting the engine is the signal to use it.TokenBudget) and writes each call's usage into the sharedALEC_LLM_USAGEstore — so clustering both respects the cap and shows up in the usage dashboard.P2 — robustness
MAX_ALARMS(200) the engine clusters the most-recent alarms instead of emitting a prompt guaranteed to overflow the context window.parseResponseassigns each alarm to at most one cluster (global + within-group) and drops sub-2-alarm groups, so a repeated ID can't span situations or inflate a singleton.EdgeDaono longer returns (missed delete callback) — the exact class of bug the refresh exists to repair. Removal logic factored into a sharedremoveEdgeInventory().Verification
UsageStore(the engine bundle can't depend on features/llm-suggestions).🤖 Generated with Claude Code
https://claude.ai/code/session_01UB6PGc2rpPbojnqbTHU5ND