[P1] feat(telemetry): Implement Kalman-Based Drift Detection#711
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (119)
📝 WalkthroughWalkthroughThis PR adds a one-dimensional Kalman filter to the telemetry math package for state estimation and drift detection. The implementation includes module setup, the ChangesKalman Filter Implementation
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideIntroduces a reputation-weighted quorum mechanism in the arbiter and adds a basic 1D Kalman filter utility with tests to support telemetry drift detection. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider renaming the
phoenix_os/agents/internal/telemetry/mathpackage to something less likely to be confused with the standard librarymathpackage (e.g.,kalmanorfilter), to avoid ambiguous imports and improve readability. - The
KalmanFilterstruct currently exposes all internal fields (Q,R,P,K,X); if external mutation is not required, making these fields unexported and relying on constructor/config methods would better encapsulate the filter’s invariants and reduce misuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming the `phoenix_os/agents/internal/telemetry/math` package to something less likely to be confused with the standard library `math` package (e.g., `kalman` or `filter`), to avoid ambiguous imports and improve readability.
- The `KalmanFilter` struct currently exposes all internal fields (`Q`, `R`, `P`, `K`, `X`); if external mutation is not required, making these fields unexported and relying on constructor/config methods would better encapsulate the filter’s invariants and reduce misuse.
## Individual Comments
### Comment 1
<location path="phoenix_os/agents/internal/telemetry/math/kalman.go" line_range="22-23" />
<code_context>
+ // prediction
+ kf.P = kf.P + kf.Q
+
+ // measurement update
+ kf.K = kf.P / (kf.P + kf.R)
+ kf.X = kf.X + kf.K*(measurement-kf.X)
+ kf.P = (1 - kf.K) * kf.P
</code_context>
<issue_to_address>
**issue:** Guard against zero or near-zero denominator when computing Kalman gain.
If `P + R` can be zero or extremely small (e.g., both initialized to 0), `kf.K = kf.P / (kf.P + kf.R)` risks NaNs or unstable values. Consider enforcing a minimum epsilon for the denominator (or `R`) or short-circuiting in this degenerate case to keep the filter numerically stable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+22
to
+23
| // measurement update | ||
| kf.K = kf.P / (kf.P + kf.R) |
There was a problem hiding this comment.
issue: Guard against zero or near-zero denominator when computing Kalman gain.
If P + R can be zero or extremely small (e.g., both initialized to 0), kf.K = kf.P / (kf.P + kf.R) risks NaNs or unstable values. Consider enforcing a minimum epsilon for the denominator (or R) or short-circuiting in this degenerate case to keep the filter numerically stable.
There was a problem hiding this comment.
Pull request overview
This PR claims to implement Kalman-based drift detection for the Phoenix Monitor, but the actual changes add a standalone 1D Kalman filter under an internal/ telemetry agent path (not importable by the Monitor) and introduce unrelated quorum/reputation logic in the arbiter module.
Changes:
- Added
Node/Arbiterquorum calculation based on reputation inphoenix_os/arbiter. - Replaced arbiter minimax test with quorum tests (reducing minimax coverage).
- Added a new 1D Kalman filter implementation and basic unit test under
phoenix_os/agents/internal/telemetry/math.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| phoenix_os/arbiter/src/arbiter.go | Adds quorum/reputation structs and CalculateQuorum alongside existing minimax demo logic. |
| phoenix_os/arbiter/src/arbiter_test.go | Swaps minimax test for quorum tests; introduces formatting/convention issues. |
| phoenix_os/agents/internal/telemetry/math/kalman.go | Introduces a 1D Kalman filter implementation under an internal/ package. |
| phoenix_os/agents/internal/telemetry/math/kalman_test.go | Adds a minimal drift-tracking test for the new Kalman filter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+15
to
+26
| // Node represents a participant in the swarm | ||
| type Node struct { | ||
| ID string | ||
| Reputation float64 | ||
| Authorized bool | ||
| } | ||
|
|
||
| // Arbiter handles quorum and reputation | ||
| type Arbiter struct { | ||
| Nodes []Node | ||
| QuorumThreshold float64 | ||
| } |
Comment on lines
+28
to
+45
| // CalculateQuorum validates quorum based on reputation | ||
| func (a *Arbiter) CalculateQuorum(votes map[string]bool) bool { | ||
| var totalReputation float64 | ||
| var positiveReputation float64 | ||
| for _, node := range a.Nodes { | ||
| if !node.Authorized { | ||
| continue | ||
| } | ||
| totalReputation += node.Reputation | ||
| if vote, ok := votes[node.ID]; ok && vote { | ||
| positiveReputation += node.Reputation | ||
| } | ||
| } | ||
| if totalReputation == 0 { | ||
| return false | ||
| } | ||
| return (positiveReputation / totalReputation) >= a.QuorumThreshold | ||
| } |
Comment on lines
7
to
15
| func TestCalculateQuorum(t *testing.T) { | ||
| arbiter := Arbiter{ | ||
| Nodes: []Node{ | ||
| {"node1", 1.0, true}, | ||
| {"node2", 2.0, true}, | ||
| {"node3", 1.0, false}, | ||
| }, | ||
| QuorumThreshold: 0.6, | ||
| } |
Comment on lines
+3
to
+5
| import ( | ||
| "testing" | ||
| ) |
Comment on lines
+10
to
+12
| {"node1", 1.0, true}, | ||
| {"node2", 2.0, true}, | ||
| {"node3", 1.0, false}, |
Comment on lines
+1
to
+15
| package math | ||
|
|
||
| // KalmanFilter implements a basic one-dimensional Kalman filter for state estimation. | ||
| type KalmanFilter struct { | ||
| Q float64 // process noise covariance | ||
| R float64 // measurement noise covariance | ||
| P float64 // estimation error covariance | ||
| K float64 // kalman gain | ||
| X float64 // value | ||
| } | ||
|
|
||
| // NewKalmanFilter creates a filter instance. | ||
| func NewKalmanFilter(q, r, p, initialValue float64) *KalmanFilter { | ||
| return &KalmanFilter{Q: q, R: r, P: p, X: initialValue} | ||
| } |
Comment on lines
+17
to
+25
| // Update incorporates a new measurement into the filter state. | ||
| func (kf *KalmanFilter) Update(measurement float64) float64 { | ||
| // prediction | ||
| kf.P = kf.P + kf.Q | ||
|
|
||
| // measurement update | ||
| kf.K = kf.P / (kf.P + kf.R) | ||
| kf.X = kf.X + kf.K*(measurement-kf.X) | ||
| kf.P = (1 - kf.K) * kf.P |
Comment on lines
+3
to
+18
| import "testing" | ||
|
|
||
| func TestKalmanFilter(t *testing.T) { | ||
| kf := NewKalmanFilter(0.01, 0.1, 1.0, 0.0) | ||
|
|
||
| // Simulate gradual drift | ||
| measurements := []float64{1.0, 1.1, 1.2, 1.3} | ||
|
|
||
| last := 0.0 | ||
| for _, m := range measurements { | ||
| last = kf.Update(m) | ||
| } | ||
|
|
||
| if last < 1.0 { | ||
| t.Errorf("Filter failed to track drift, got %f", last) | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
phoenix_os/agents/internal/telemetry/math/kalman_test.go (1)
16-18: ⚡ Quick winStrengthen the drift-tracking assertion.
last < 1.0is too permissive and may miss regressions. Assert tighter behavior (e.g., increasing outputs across updates and closeness to the final measurement within tolerance).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phoenix_os/agents/internal/telemetry/math/kalman_test.go` around lines 16 - 18, The current test only checks `last < 1.0`, which is too weak; update the test that uses the `last` variable to assert that the filter's outputs are monotonically increasing across successive `Update`/prediction steps and that the final `last` value is within a tight tolerance of the expected final measurement (e.g., abs(last - expectedFinal) < tol). Locate the test that updates the Kalman filter and replace the single permissive check with two assertions: one that the sequence of outputs (tracked in a slice or by comparing previous/next `last`) is non-decreasing, and another that the final `last` is within the chosen tolerance of the known final value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phoenix_os/agents/internal/telemetry/math/kalman.go`:
- Around line 13-15: NewKalmanFilter currently accepts q, r, p (and
initialValue) without validation which allows negative/NaN covariances and
produces invalid updates; change NewKalmanFilter to validate inputs (use
math.IsNaN to reject NaN and require q, r, p >= 0 — or > 0 if you prefer strict
positive variances — and reject initialValue if NaN), and return an error when
any parameter is invalid (i.e., change signature to return (*KalmanFilter,
error)); construct and return the KalmanFilter only when all checks pass and
update callers to handle the error from NewKalmanFilter.
- Around line 18-31: The Update method on KalmanFilter mutates kf.p, kf.k and
kf.x but does not emit Phoenix Ledger evidence; modify KalmanFilter.Update to
produce a deterministic evidence record after the state change: serialize the
pre- and post-state (at least kf.x, kf.p, kf.k), the measurement and timestamp
using a stable encoding, compute a SHA-256 hash of that serialization, package
the hash and serialized payload into an evidence entry, and call the Phoenix
ledger API (e.g., PhoenixLedger.AppendEvidence or PhoenixLedger.RecordEvidence)
to persist it and check for errors; ensure the serialization is deterministic
and that errors from the ledger write are logged/returned as appropriate.
---
Nitpick comments:
In `@phoenix_os/agents/internal/telemetry/math/kalman_test.go`:
- Around line 16-18: The current test only checks `last < 1.0`, which is too
weak; update the test that uses the `last` variable to assert that the filter's
outputs are monotonically increasing across successive `Update`/prediction steps
and that the final `last` value is within a tight tolerance of the expected
final measurement (e.g., abs(last - expectedFinal) < tol). Locate the test that
updates the Kalman filter and replace the single permissive check with two
assertions: one that the sequence of outputs (tracked in a slice or by comparing
previous/next `last`) is non-decreasing, and another that the final `last` is
within the chosen tolerance of the known final value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33cd1428-a939-4530-9598-449be636cc18
⛔ Files ignored due to path filters (1)
go.workis excluded by!**/*.work
📒 Files selected for processing (3)
phoenix_os/agents/internal/telemetry/math/go.modphoenix_os/agents/internal/telemetry/math/kalman.gophoenix_os/agents/internal/telemetry/math/kalman_test.go
Comment on lines
+13
to
+15
| func NewKalmanFilter(q, r, p, initialValue float64) *KalmanFilter { | ||
| return &KalmanFilter{q: q, r: r, p: p, x: initialValue} | ||
| } |
There was a problem hiding this comment.
Validate covariance inputs in constructor.
q, r, and p can be passed as negative/NaN, which can make updates numerically invalid. Add validation before constructing the filter (prefer returning an error for invalid parameters).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@phoenix_os/agents/internal/telemetry/math/kalman.go` around lines 13 - 15,
NewKalmanFilter currently accepts q, r, p (and initialValue) without validation
which allows negative/NaN covariances and produces invalid updates; change
NewKalmanFilter to validate inputs (use math.IsNaN to reject NaN and require q,
r, p >= 0 — or > 0 if you prefer strict positive variances — and reject
initialValue if NaN), and return an error when any parameter is invalid (i.e.,
change signature to return (*KalmanFilter, error)); construct and return the
KalmanFilter only when all checks pass and update callers to handle the error
from NewKalmanFilter.
…Go workspace - Remove all nested go.mod files (bus, guard, ledger, monitor, trace, warden, agents, sentinel, nexus, arbiter, kernel, telemetry, common) - Delete speculative directories: agents/, sentinel/, nexus/, arbiter/, kernel/, telemetry/ - Remove obsolete trace/tests/ and common/math/ directories - Unify package declarations in monitor/ (entropy_engine -> monitor) - Update go.work to target single ./phoenix_os module - Run go mod tidy to resolve dependencies - All tests pass, deterministic replay hash verified: cd065910c87ba0c92ba3e267ea6b88249c234abfadacaebde3981581e1f4e252 Closes #706, #580, #623, #632, #627, #629
The old workflow built 14_experiments/telemetry_replay which used a stale import path (phoenix/telemetry/bus/normalizer). The new workflow builds phoenix_os/main.go and verifies deterministic replay by comparing output hashes across two consecutive runs.
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
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.
Resolves #706. Implemented a 1D Kalman filter to improve predictive state estimation and anomaly thresholding in the Phoenix Monitor.
Summary by Sourcery
Introduce reputation-weighted quorum calculation in the arbiter and add a basic 1D Kalman filter utility for telemetry drift tracking.
New Features:
Tests:
Summary by CodeRabbit
New Features
Tests