[P2] feat(security): Implement Suspicion Counter - #591
Conversation
Reviewer's GuideIntroduces a thread-safe ReputationManager to track node reputation scores and a unit test verifying basic deduction behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR adds a thread-safe ReputationManager with tests, a 1D KalmanFilter integrated into the physics agent's temperature smoothing, CI workflow tweaks (go build and Git LFS), an updated security go.mod, two issue-creation scripts, and an optional UNIX-socket IPC path for the guard CLI submit command. ChangesNode Reputation Tracking
Physics Kalman Integration
CI/workflow tweaks
Module file
Local tooling and CLI IPC
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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 |
![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 making the
Reputationmap unexported and only accessing it via methods onReputationManagerso that all access is mutex-protected and callers cannot bypass synchronization. - The
Deductmethod currently allows creating new entries with negative scores and driving scores arbitrarily below zero; if reputations are meant to stay within a bounded range, you may want to initialize missing entries explicitly and clamp to a minimum value. - The test directly mutates
rm.Reputationinstead of going through an API; adding a dedicated initializer or setter method for node reputation would better exercise the public interface and help keep the internal representation flexible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making the `Reputation` map unexported and only accessing it via methods on `ReputationManager` so that all access is mutex-protected and callers cannot bypass synchronization.
- The `Deduct` method currently allows creating new entries with negative scores and driving scores arbitrarily below zero; if reputations are meant to stay within a bounded range, you may want to initialize missing entries explicitly and clamp to a minimum value.
- The test directly mutates `rm.Reputation` instead of going through an API; adding a dedicated initializer or setter method for node reputation would better exercise the public interface and help keep the internal representation flexible.
## Individual Comments
### Comment 1
<location path="phoenix_os/agents/internal/security/reputation.go" line_range="6-8" />
<code_context>
+import "sync"
+
+// ReputationManager tracks the trustworthiness of nodes in the swarm.
+type ReputationManager struct {
+ mu sync.RWMutex
+ Reputation map[string]float64
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Expose the map only via methods to avoid unsynchronized access
Because `Reputation` is exported, callers can bypass the mutex and access the map directly, violating the locking assumptions in `Deduct`/`GetScore` and causing data races. Make the field unexported (e.g. `reputation map[string]float64`) and expose any needed operations via methods that always take the lock.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| type ReputationManager struct { | ||
| mu sync.RWMutex | ||
| Reputation map[string]float64 |
There was a problem hiding this comment.
issue (bug_risk): Expose the map only via methods to avoid unsynchronized access
Because Reputation is exported, callers can bypass the mutex and access the map directly, violating the locking assumptions in Deduct/GetScore and causing data races. Make the field unexported (e.g. reputation map[string]float64) and expose any needed operations via methods that always take the lock.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/security/reputation_test.go`:
- Around line 5-14: The test TestReputationDeduction currently only verifies a
single happy-path deduction; update it to include simulated adversarial runs
that exercise detection and reputation decay logic: create multiple nodes via
NewReputationManager (e.g., "node-1" through "node-N"), simulate repeated
malicious actions by calling Deduct on the offending node(s) and benign actions
on others, then assert via GetScore that malicious nodes drop below detection
thresholds and benign nodes remain above them, and also validate any automated
detection hooks or callbacks your ReputationManager exposes (e.g.,
Detected/Mitigated events or methods) to ensure the detection + deduction flow
behaves as expected under adversarial pressure.
- Around line 11-13: The test currently checks exact equality on the float
variable score (if score != 0.8), which is brittle; change it to a tolerant
comparison using a small epsilon (e.g., math.Abs(score - 0.8) > eps) and fail
the test only when the difference exceeds eps, importing math and keeping the
t.Errorf to show expected and actual values (and optionally the difference) so
the assertion tolerates floating-point rounding.
In `@phoenix_os/agents/internal/security/reputation.go`:
- Around line 7-9: The exported Reputation map allows callers to mutate shared
state without acquiring the mu lock; make the map unexported (rename to
reputation) and replace direct accesses with concurrency-safe accessor/mutator
functions that use mu (e.g., GetReputation(key) float64, SetReputation(key,
value), and/or SnapshotReputations() map[string]float64) and update any callers
to use those methods instead of touching Reputation directly; ensure all reads
use mu.RLock/RUnlock and all writes use mu.Lock/Unlock to preserve the
thread-safety guarantees in the existing methods that reference mu.
- Around line 19-23: Reputation deduction in ReputationManager.Deduct mutates
rm.Reputation without recording the action to the Phoenix Ledger or updating the
SHA-256 hash-chain; change Deduct to build an evidence record (include nodeID,
amount, pre- and post-reputation values, timestamp, and actor), compute its
SHA-256 hash, append the record/hash to the Phoenix Ledger via the project's
ledger API (e.g., Ledger.Append/Record or PhoenixLedger.RecordAction), and only
then apply rm.Reputation[nodeID] -= amount while holding rm.mu so the ledger
entry reflects the exact state transition and advances the hash-chain.
- Around line 19-23: The Deduct method currently allows negative amounts which
will increase a node's reputation; add an input guard in
ReputationManager.Deduct to reject negative deduction values (e.g., if amount <
0 { return } or return an error) before acquiring or modifying rm.Reputation, so
only positive amounts decrease rm.Reputation[nodeID]; reference the Deduct
method and the rm.Reputation map when making the change.
🪄 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: a6166964-5aad-47c7-a9db-25f0223ab8d0
📒 Files selected for processing (2)
phoenix_os/agents/internal/security/reputation.gophoenix_os/agents/internal/security/reputation_test.go
| func TestReputationDeduction(t *testing.T) { | ||
| rm := NewReputationManager() | ||
| rm.Reputation["node-1"] = 1.0 | ||
|
|
||
| rm.Deduct("node-1", 0.2) | ||
| score := rm.GetScore("node-1") | ||
| if score != 0.8 { | ||
| t.Errorf("Expected 0.8, got %f", score) | ||
| } | ||
| } |
There was a problem hiding this comment.
Required adversarial validation is missing from tests.
This test only checks a single happy-path deduction. Issue #143 explicitly requires simulated adversarial runs to validate detection + reputation deduction behavior.
🤖 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/security/reputation_test.go` around lines 5 - 14,
The test TestReputationDeduction currently only verifies a single happy-path
deduction; update it to include simulated adversarial runs that exercise
detection and reputation decay logic: create multiple nodes via
NewReputationManager (e.g., "node-1" through "node-N"), simulate repeated
malicious actions by calling Deduct on the offending node(s) and benign actions
on others, then assert via GetScore that malicious nodes drop below detection
thresholds and benign nodes remain above them, and also validate any automated
detection hooks or callbacks your ReputationManager exposes (e.g.,
Detected/Mitigated events or methods) to ensure the detection + deduction flow
behaves as expected under adversarial pressure.
| if score != 0.8 { | ||
| t.Errorf("Expected 0.8, got %f", score) | ||
| } |
There was a problem hiding this comment.
Use tolerant float comparison in the assertion.
Line 11 uses exact float equality, which is brittle for decimal arithmetic.
Suggested fix
import "testing"
+import "math"
@@
- if score != 0.8 {
+ if math.Abs(score-0.8) > 1e-9 {
t.Errorf("Expected 0.8, got %f", score)
}🤖 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/security/reputation_test.go` around lines 11 - 13,
The test currently checks exact equality on the float variable score (if score
!= 0.8), which is brittle; change it to a tolerant comparison using a small
epsilon (e.g., math.Abs(score - 0.8) > eps) and fail the test only when the
difference exceeds eps, importing math and keeping the t.Errorf to show expected
and actual values (and optionally the difference) so the assertion tolerates
floating-point rounding.
| mu sync.RWMutex | ||
| Reputation map[string]float64 | ||
| } |
There was a problem hiding this comment.
Exported Reputation map bypasses your mutex protection.
Line 8 exposes mutable shared state, so callers can write rm.Reputation[...] without mu, creating races and invalidating the thread-safety contract in Lines 19-23.
Suggested refactor
type ReputationManager struct {
- mu sync.RWMutex
- Reputation map[string]float64
+ mu sync.RWMutex
+ reputation map[string]float64
}
func NewReputationManager() *ReputationManager {
return &ReputationManager{
- Reputation: make(map[string]float64),
+ reputation: make(map[string]float64),
}
}
func (rm *ReputationManager) Deduct(nodeID string, amount float64) {
rm.mu.Lock()
defer rm.mu.Unlock()
- rm.Reputation[nodeID] -= amount
+ rm.reputation[nodeID] -= amount
}
func (rm *ReputationManager) GetScore(nodeID string) float64 {
rm.mu.RLock()
defer rm.mu.RUnlock()
- return rm.Reputation[nodeID]
+ return rm.reputation[nodeID]
}Also applies to: 19-23
🤖 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/security/reputation.go` around lines 7 - 9, The
exported Reputation map allows callers to mutate shared state without acquiring
the mu lock; make the map unexported (rename to reputation) and replace direct
accesses with concurrency-safe accessor/mutator functions that use mu (e.g.,
GetReputation(key) float64, SetReputation(key, value), and/or
SnapshotReputations() map[string]float64) and update any callers to use those
methods instead of touching Reputation directly; ensure all reads use
mu.RLock/RUnlock and all writes use mu.Lock/Unlock to preserve the thread-safety
guarantees in the existing methods that reference mu.
| func (rm *ReputationManager) Deduct(nodeID string, amount float64) { | ||
| rm.mu.Lock() | ||
| defer rm.mu.Unlock() | ||
| rm.Reputation[nodeID] -= amount | ||
| } |
There was a problem hiding this comment.
Reputation actions are not recorded in Phoenix Ledger.
Line 22 mutates reputation with no evidence record or hash-chain update, so this misses the required audit trail.
As per coding guidelines, Every action MUST be recorded in the Phoenix Ledger with a SHA-256 hash-chain for evidence-first actuation.
🤖 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/security/reputation.go` around lines 19 - 23,
Reputation deduction in ReputationManager.Deduct mutates rm.Reputation without
recording the action to the Phoenix Ledger or updating the SHA-256 hash-chain;
change Deduct to build an evidence record (include nodeID, amount, pre- and
post-reputation values, timestamp, and actor), compute its SHA-256 hash, append
the record/hash to the Phoenix Ledger via the project's ledger API (e.g.,
Ledger.Append/Record or PhoenixLedger.RecordAction), and only then apply
rm.Reputation[nodeID] -= amount while holding rm.mu so the ledger entry reflects
the exact state transition and advances the hash-chain.
Reject negative deduction amounts.
At Line 22, passing a negative amount increases a node’s score, which breaks deduction semantics and can be abused.
Suggested fix
func (rm *ReputationManager) Deduct(nodeID string, amount float64) {
+ if amount < 0 {
+ return // or panic/error depending on your error model
+ }
rm.mu.Lock()
defer rm.mu.Unlock()
rm.Reputation[nodeID] -= amount
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (rm *ReputationManager) Deduct(nodeID string, amount float64) { | |
| rm.mu.Lock() | |
| defer rm.mu.Unlock() | |
| rm.Reputation[nodeID] -= amount | |
| } | |
| func (rm *ReputationManager) Deduct(nodeID string, amount float64) { | |
| if amount < 0 { | |
| return // or panic/error depending on your error model | |
| } | |
| rm.mu.Lock() | |
| defer rm.mu.Unlock() | |
| rm.Reputation[nodeID] -= amount | |
| } |
🤖 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/security/reputation.go` around lines 19 - 23, The
Deduct method currently allows negative amounts which will increase a node's
reputation; add an input guard in ReputationManager.Deduct to reject negative
deduction values (e.g., if amount < 0 { return } or return an error) before
acquiring or modifying rm.Reputation, so only positive amounts decrease
rm.Reputation[nodeID]; reference the Deduct method and the rm.Reputation map
when making the change.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e2d2eee97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // ReputationManager tracks the trustworthiness of nodes in the swarm. | ||
| type ReputationManager struct { | ||
| mu sync.RWMutex | ||
| Reputation map[string]float64 |
There was a problem hiding this comment.
The Reputation map is exported, so callers can mutate it without taking rm.mu; this bypasses the lock discipline used by Deduct/GetScore and can trigger data races or concurrent map read and map write panics under concurrent access. Because this manager is explicitly synchronized, exposing raw mutable state defeats the concurrency guarantees and should be replaced with private storage plus setter/getter methods.
Useful? React with 👍 / 👎.
| func (rm *ReputationManager) Deduct(nodeID string, amount float64) { | ||
| rm.mu.Lock() | ||
| defer rm.mu.Unlock() | ||
| rm.Reputation[nodeID] -= amount |
There was a problem hiding this comment.
Deduct is documented to reduce reputation, but passing a negative amount currently increases the score because of -=. If amount comes from noisy telemetry or an unchecked caller, suspicious events can accidentally (or intentionally) boost reputation, which inverts the security signal this component is meant to enforce.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| "--label", label | ||
| ] | ||
| try: | ||
| subprocess.run(cmd, check=True) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| "--label", issue["label"] | ||
| ] | ||
| try: | ||
| subprocess.run(cmd, check=True) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c278297128
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| go 1.25.0 | ||
|
|
||
| replace phoenix/security => ../../../07_security |
There was a problem hiding this comment.
The local replacement path in phoenix_os/agents/internal/security/go.mod is off by one directory level: from this module, ../../../07_security resolves to phoenix_os/07_security, which does not exist in the repo. As soon as this module (or tooling like go mod tidy) needs to resolve phoenix/security, Go will fail with a missing replacement directory error, blocking builds for consumers of this module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
agents/internal/physics/physics.go (1)
92-112:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRe-tune anomaly trigger threshold after Kalman smoothing change (
a.threatTemp > 4.0).
a.threatTempis now updated by a 1D Kalman filter (Q=0.1, R=1.0, P₀=1.0, X₀=0.1): initial gain K≈0.524, but steady-state gain K≈0.27—so the time-to-cross (and fall-through) around 4.0 can shift versus any fixed-weight blend.isAnomalystill uses the hard cut-offa.threatTemp > 4.0, and the current tests only assertIsAnomalyplus a looseThreatTemperaturefloor (e.g., >0.1 / >2.0), with no direct pin on behavior at>4.0; downstream containment/strategy selection baselines may need re-tuning via the adversarial runs from#143.🤖 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 `@agents/internal/physics/physics.go` around lines 92 - 112, The anomaly threshold using a.threatTemp > 4.0 is no longer appropriate after introducing the Kalman smoothing in tempFilter.Update (which changes response/gain), so update the isAnomaly logic (referencing a.threatTemp and tempFilter.Update) to use a retuned threshold or adaptive check: either lower/raise the numeric cutoff based on the Kalman steady-state gain (e.g., compute a calibrated threshold constant or derive it from filter state/variance), and update any tests that assert IsAnomaly/ThreatTemperature expectations to match the new trigger; ensure the change is applied where isAnomaly is computed so downstream strategy selection uses the retuned threshold.tools/guard_runtime_py/cli.py (2)
34-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback after IPC failure risks duplicate submission.
If
sendallsucceeded and the daemon already processed the message but the failure occurred duringrecv(e.g., timeout, connection reset), the code silently falls through todaemon.process_message(msg)and submits the event a second time locally. For a reputation/security event stream this can corrupt accounting.Consider distinguishing pre-send failures (safe to fall back) from post-send failures (must not fall back), or making the fallback opt-in:
♻️ Sketch
- try: - with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as s: - s.connect(sock_path) - s.send(json.dumps(msg).encode("utf-8")) - resp = s.recv(1024) - print(resp.decode("utf-8")) - return - except Exception as e: - print("IPC submit failed:", e) + sent = False + try: + with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as s: + s.settimeout(5.0) + s.connect(sock_path) + s.sendall(json.dumps(msg).encode("utf-8")) + sent = True + resp = s.recv(1024) + print(resp.decode("utf-8")) + return + except Exception as e: + print("IPC submit failed:", e) + if sent: + # Avoid double-submit; remote may have already processed. + return🤖 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 `@tools/guard_runtime_py/cli.py` around lines 34 - 37, The current except block falls through to daemon.process_message(msg) regardless of whether IPC send succeeded, risking duplicate submission if send succeeded but recv/response failed; change the flow to distinguish pre-send vs post-send failures by tracking send success (e.g., set a boolean after a successful socket.sendall) and only call daemon.process_message(msg) when send did not complete, or make the local fallback opt-in via a parameter/flag; update the block around socket.sendall/recv and the call to daemon.process_message(msg) so you only fallback when sendall did not succeed, and include clear variable names (e.g., sent, socket.sendall, socket.recv, daemon.process_message, msg) to locate and implement the fix.
27-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet a socket timeout and use
sendall.Two reliability concerns on the IPC path:
- No timeout is configured, so
connect/send/recvcan block indefinitely if the peer hangs or is slow — a CLI invocation could appear to freeze.s.send(...)is not guaranteed to write the whole payload; usesendallto avoid silent truncation of larger JSON messages.🛡️ Proposed fix
try: with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as s: + s.settimeout(5.0) s.connect(sock_path) - s.send(json.dumps(msg).encode("utf-8")) + s.sendall(json.dumps(msg).encode("utf-8")) resp = s.recv(1024) print(resp.decode("utf-8")) return except Exception as e: print("IPC submit failed:", e)🤖 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 `@tools/guard_runtime_py/cli.py` around lines 27 - 35, The IPC client currently uses socket.socket(...) in the try block (where it calls s.connect(sock_path), s.send(json.dumps(msg)...), and s.recv(1024)) with no timeout and with s.send which may not send the full buffer; update that block to set a socket timeout (e.g., s.settimeout with a small sensible constant) before connect to avoid indefinite blocking, replace s.send(...) with s.sendall(...) to ensure the entire JSON payload is transmitted, and catch socket.timeout (and other socket errors) in the except to surface a clear timeout-specific error for the caller; adjust the error message printed for failures to include the exception context.
🧹 Nitpick comments (4)
agents/internal/physics/physics.go (1)
28-28: 💤 Low valueDocument the Kalman tuning constants.
NewKalmanFilter(0.1, 1.0, 1.0, 0.1)introduces four unlabeled magic numbers at the agent's wiring point. Future maintainers won't know which to nudge when tuning detection sensitivity. A named-constant or a short comment mapping them toQ/R/P/initialis enough.♻️ Suggested clarification
+const ( + tempFilterProcessNoise = 0.1 // Q + tempFilterMeasurementNoise = 1.0 // R + tempFilterInitialCovar = 1.0 // P + tempFilterInitialEstimate = 0.1 // baseline normal temp +) + func NewPhysicsAgent() *Agent { return &Agent{ - threatTemp: 0.1, // baseline normal temp - tempFilter: NewKalmanFilter(0.1, 1.0, 1.0, 0.1), + threatTemp: tempFilterInitialEstimate, + tempFilter: NewKalmanFilter( + tempFilterProcessNoise, + tempFilterMeasurementNoise, + tempFilterInitialCovar, + tempFilterInitialEstimate, + ), } }🤖 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 `@agents/internal/physics/physics.go` at line 28, The Kalman filter constructor call NewKalmanFilter(0.1, 1.0, 1.0, 0.1) in the tempFilter initialization uses four unlabeled magic numbers; replace or annotate them so future maintainers know what to tune: either introduce descriptive named constants (e.g., kfProcessNoiseQ, kfMeasurementNoiseR, kfErrorCovarianceP, kfInitialState) and use those in the NewKalmanFilter call, or add a short inline comment next to the tempFilter assignment mapping the arguments to Q/R/P/initial so it’s clear which parameter controls detection sensitivity.agents/internal/physics/kalman.go (1)
12-19: 💤 Low valueValidate covariance parameters and document semantics.
NewKalmanFiltersilently accepts negativeq,r,pwhich are mathematically invalid (covariances must be non-negative,rstrictly positive for a numerically stable gain). A short godoc on the type/constructor plus a sanity check would prevent misconfiguration at call sites such asNewPhysicsAgent.♻️ Suggested doc + validation
-func NewKalmanFilter(q, r, p, initialValue float64) *KalmanFilter { +// NewKalmanFilter constructs a 1D Kalman filter. +// q: process noise covariance (>= 0) +// r: measurement noise covariance (> 0) +// p: initial estimation error covariance (>= 0) +// initialValue: initial estimate of the state +// The returned filter is not safe for concurrent use; callers must synchronize. +func NewKalmanFilter(q, r, p, initialValue float64) *KalmanFilter { + if q < 0 || r <= 0 || p < 0 { + // Fall back to sane defaults rather than producing NaN/Inf later. + q, r, p = 0, 1, 1 + } return &KalmanFilter{ Q: q, R: r, P: p, X: initialValue, } }🤖 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 `@agents/internal/physics/kalman.go` around lines 12 - 19, Add godoc for the KalmanFilter type and NewKalmanFilter describing that Q and P are non-negative covariances and R must be strictly positive for stable gain; change NewKalmanFilter to validate inputs (require q >= 0, p >= 0, r > 0) and return an error on invalid parameters instead of silently constructing a filter, and update callers (e.g., NewPhysicsAgent) to handle the error; include clear, descriptive error messages mentioning which parameter is invalid.tools/guard_runtime_py/cli.py (1)
26-29: 💤 Low valueMinor TOCTOU between
exists()andconnect().
Path(sock_path).exists()thens.connect(sock_path)is a small TOCTOU window — the socket could be removed/replaced between the two calls. The existingtry/exceptalready covers the failure, so consider dropping theexists()precheck and lettingconnectraise; this also avoids treating a regular file at that path as "available".🤖 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 `@tools/guard_runtime_py/cli.py` around lines 26 - 29, Remove the TOCTOU precheck by dropping the Path(sock_path).exists() call and rely on socket.socket(...).connect(sock_path) to raise on failure; specifically, eliminate the exists() branch around sock_path and keep the existing try/except around s.connect so that connect handles non-existent, removed, or non-socket files (referencing sock_path, Path(...).exists(), socket.socket and s.connect in cli.py).tools/create_adhoc_issues.py (1)
22-34: ⚡ Quick winConsolidate the two issue-creation scripts and narrow the exception scope.
tools/create_adhoc_issues.pyandtools/create_final_issues.pyimplement the same loop, command construction, sleep, and error-handling pattern; only the issue list and item shape (tuple vs. dict) differ. A small shared helper (e.g.,tools/_gh_issue_creator.pyexposingcreate_issues(issues: Iterable[Mapping[str, str]])) would let each script just declare its data and call the helper, which avoids drift if the body suffix, rate-limit, or error format needs to change later.While extracting, also narrow the
except Exceptiontosubprocess.CalledProcessError(which is whatcheck=Trueraises) so unexpected programming errors aren't silently swallowed — this also addresses Ruff BLE001.♻️ Minimal in-place narrowing (without the full extraction)
for title, body, label in issues: cmd = [ "gh", "issue", "create", "--title", title, "--body", body + "\n\n> *This was generated by AI during triage.*", "--label", label ] try: subprocess.run(cmd, check=True) print(f"Created: {title}") time.sleep(1) - except Exception as e: + except (subprocess.CalledProcessError, FileNotFoundError) as e: print(f"Failed: {title} - {e}")Note: Ruff S603 on line 30 is a false positive here —
cmdis constructed from hardcoded literals, not external input.🤖 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 `@tools/create_adhoc_issues.py` around lines 22 - 34, Consolidate duplicated issue-creation logic by extracting the loop that builds cmd, calls subprocess.run(..., check=True), prints results, and sleeps into a shared helper like tools/_gh_issue_creator.py exposing create_issues(issues: Iterable[Mapping[str, str]]), then have both scripts (create_adhoc_issues.py and create_final_issues.py) supply their data and call create_issues; while extracting, narrow the broad except Exception to except subprocess.CalledProcessError to only catch failures from subprocess.run and avoid swallowing programming errors (refer to symbols: issues, cmd, subprocess.run, time.sleep, and subprocess.CalledProcessError).
🤖 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 @.github/workflows/lint_build_docs.yml:
- Around line 49-51: In the if-branch that checks for a Go workspace file (the
block starting with the shell conditional "if [ -f go.work ]; then"), replace
the incorrect build invocation "go build all" with the recursive package pattern
"go build ./..." so the CI builds all workspace packages rather than trying to
build a package literally named "all".
In @.github/workflows/telemetry_gate.yml:
- Around line 13-15: The checkout step currently enables LFS via
actions/checkout@v4 with "lfs: true" but does not disable credential
persistence; update the checkout invocation to include persist-credentials:
false so the GitHub token (and any LFS credential helper) is not written into
.git/config or persisted into the workspace — modify the actions/checkout usage
in the workflow to add persist-credentials: false alongside lfs: true.
In `@agents/internal/physics/kalman.go`:
- Around line 21-36: The Update method can produce NaN/Inf when computing kf.K =
kf.P / (kf.P + kf.R); guard by computing denom := kf.P + kf.R and if denom is
zero or not finite (use math.IsNaN / math.IsInf) set kf.K = 0 (or clamp to a
safe value) and skip the measurement update that would corrupt kf.X; after
computing kf.K also validate it with math.IsNaN/math.IsInf and if invalid set
kf.K = 0 and ensure kf.P remains non-negative (clamp if needed) so the filter
never writes NaN/Inf into kf.X or kf.P.
---
Outside diff comments:
In `@agents/internal/physics/physics.go`:
- Around line 92-112: The anomaly threshold using a.threatTemp > 4.0 is no
longer appropriate after introducing the Kalman smoothing in tempFilter.Update
(which changes response/gain), so update the isAnomaly logic (referencing
a.threatTemp and tempFilter.Update) to use a retuned threshold or adaptive
check: either lower/raise the numeric cutoff based on the Kalman steady-state
gain (e.g., compute a calibrated threshold constant or derive it from filter
state/variance), and update any tests that assert IsAnomaly/ThreatTemperature
expectations to match the new trigger; ensure the change is applied where
isAnomaly is computed so downstream strategy selection uses the retuned
threshold.
In `@tools/guard_runtime_py/cli.py`:
- Around line 34-37: The current except block falls through to
daemon.process_message(msg) regardless of whether IPC send succeeded, risking
duplicate submission if send succeeded but recv/response failed; change the flow
to distinguish pre-send vs post-send failures by tracking send success (e.g.,
set a boolean after a successful socket.sendall) and only call
daemon.process_message(msg) when send did not complete, or make the local
fallback opt-in via a parameter/flag; update the block around
socket.sendall/recv and the call to daemon.process_message(msg) so you only
fallback when sendall did not succeed, and include clear variable names (e.g.,
sent, socket.sendall, socket.recv, daemon.process_message, msg) to locate and
implement the fix.
- Around line 27-35: The IPC client currently uses socket.socket(...) in the try
block (where it calls s.connect(sock_path), s.send(json.dumps(msg)...), and
s.recv(1024)) with no timeout and with s.send which may not send the full
buffer; update that block to set a socket timeout (e.g., s.settimeout with a
small sensible constant) before connect to avoid indefinite blocking, replace
s.send(...) with s.sendall(...) to ensure the entire JSON payload is
transmitted, and catch socket.timeout (and other socket errors) in the except to
surface a clear timeout-specific error for the caller; adjust the error message
printed for failures to include the exception context.
---
Nitpick comments:
In `@agents/internal/physics/kalman.go`:
- Around line 12-19: Add godoc for the KalmanFilter type and NewKalmanFilter
describing that Q and P are non-negative covariances and R must be strictly
positive for stable gain; change NewKalmanFilter to validate inputs (require q
>= 0, p >= 0, r > 0) and return an error on invalid parameters instead of
silently constructing a filter, and update callers (e.g., NewPhysicsAgent) to
handle the error; include clear, descriptive error messages mentioning which
parameter is invalid.
In `@agents/internal/physics/physics.go`:
- Line 28: The Kalman filter constructor call NewKalmanFilter(0.1, 1.0, 1.0,
0.1) in the tempFilter initialization uses four unlabeled magic numbers; replace
or annotate them so future maintainers know what to tune: either introduce
descriptive named constants (e.g., kfProcessNoiseQ, kfMeasurementNoiseR,
kfErrorCovarianceP, kfInitialState) and use those in the NewKalmanFilter call,
or add a short inline comment next to the tempFilter assignment mapping the
arguments to Q/R/P/initial so it’s clear which parameter controls detection
sensitivity.
In `@tools/create_adhoc_issues.py`:
- Around line 22-34: Consolidate duplicated issue-creation logic by extracting
the loop that builds cmd, calls subprocess.run(..., check=True), prints results,
and sleeps into a shared helper like tools/_gh_issue_creator.py exposing
create_issues(issues: Iterable[Mapping[str, str]]), then have both scripts
(create_adhoc_issues.py and create_final_issues.py) supply their data and call
create_issues; while extracting, narrow the broad except Exception to except
subprocess.CalledProcessError to only catch failures from subprocess.run and
avoid swallowing programming errors (refer to symbols: issues, cmd,
subprocess.run, time.sleep, and subprocess.CalledProcessError).
In `@tools/guard_runtime_py/cli.py`:
- Around line 26-29: Remove the TOCTOU precheck by dropping the
Path(sock_path).exists() call and rely on socket.socket(...).connect(sock_path)
to raise on failure; specifically, eliminate the exists() branch around
sock_path and keep the existing try/except around s.connect so that connect
handles non-existent, removed, or non-socket files (referencing sock_path,
Path(...).exists(), socket.socket and s.connect in cli.py).
🪄 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: bcd492f8-fb95-43f7-98d9-3b7507463d51
⛔ Files ignored due to path filters (1)
go.workis excluded by!**/*.work
📒 Files selected for processing (8)
.github/workflows/lint_build_docs.yml.github/workflows/telemetry_gate.ymlagents/internal/physics/kalman.goagents/internal/physics/physics.gophoenix_os/agents/internal/security/go.modtools/create_adhoc_issues.pytools/create_final_issues.pytools/guard_runtime_py/cli.py
✅ Files skipped from review due to trivial changes (1)
- phoenix_os/agents/internal/security/go.mod
| if [ -f go.work ]; then | ||
| echo "Building with go.work..." | ||
| go build ./... | ||
| go build all |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go workspace mode (go.work), what does go build allbuild compared togo build ./...?
💡 Result:
In Go workspace mode (go.work), go build all builds the package pattern named by the import-path pattern all (not “all packages”). If you have no package import path matching all (which is usually the case), it will fail with “no packages to build”. By contrast, go build ./... builds every package under the current directory (recursively), i.e., it expands the ./... package pattern to all matching packages and compiles them (and their dependencies). So, they are not equivalent: ./... is a recursive filesystem-based package pattern, while all is just a literal package-pattern token; workspace mode (go.work) affects how the module set is selected for the build, but it does not make all become “all packages”. [1][2][3]
Citations:
- 1: https://go.dev/doc/articles/go_command
- 2: https://pkg.go.dev/cmd/go
- 3: https://go.dev/doc/tutorial/workspaces
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la .github/workflows || true
sed -n '1,120p' .github/workflows/lint_build_docs.yml | nl -ba | sed -n '35,90p'Repository: fallofpheonix/PheonixOS
Length of output: 497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n .github/workflows/lint_build_docs.yml | sed -n '1,160p'Repository: fallofpheonix/PheonixOS
Length of output: 2446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la go.work || true
[ -f go.work ] && cat -n go.work
echo "---- dirs named all ----"
find . -type d -name all -print | head -n 20
echo "---- any module/package named all (import paths) ----"
rg -n --hidden --no-ignore -S "(/|^)all(/|$)" -g'*.go' . || trueRepository: fallofpheonix/PheonixOS
Length of output: 780
Fix Go workspace build command in CI (go build all)
In the go.work branch of .github/workflows/lint_build_docs.yml, go build all builds the package pattern named all (not “all packages”). This repo doesn’t contain a module/package matching all, so the workflow won’t build the intended set of workspace packages. Use go build ./... instead.
🔧 Proposed fix
if [ -f go.work ]; then
echo "Building with go.work..."
- go build all
+ go build ./...
else🤖 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 @.github/workflows/lint_build_docs.yml around lines 49 - 51, In the if-branch
that checks for a Go workspace file (the block starting with the shell
conditional "if [ -f go.work ]; then"), replace the incorrect build invocation
"go build all" with the recursive package pattern "go build ./..." so the CI
builds all workspace packages rather than trying to build a package literally
named "all".
| - uses: actions/checkout@v4 | ||
| with: | ||
| lfs: true |
There was a problem hiding this comment.
Set persist-credentials: false on the LFS-enabled checkout.
Per the zizmor artipacked hint, actions/checkout leaves the .git/config with a base64-encoded GitHub token by default. With lfs: true, an .lfsconfig/credential helper can persist similarly. Since later steps (upload-artifact) publish files derived from the workspace, explicitly disabling credential persistence reduces the risk of inadvertently exfiltrating the repo token via artifacts.
🛡️ Proposed fix
- uses: actions/checkout@v4
with:
lfs: true
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v4 | |
| with: | |
| lfs: true | |
| - uses: actions/checkout@v4 | |
| with: | |
| lfs: true | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 13-15: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/telemetry_gate.yml around lines 13 - 15, The checkout step
currently enables LFS via actions/checkout@v4 with "lfs: true" but does not
disable credential persistence; update the checkout invocation to include
persist-credentials: false so the GitHub token (and any LFS credential helper)
is not written into .git/config or persisted into the workspace — modify the
actions/checkout usage in the workflow to add persist-credentials: false
alongside lfs: true.
| func (kf *KalmanFilter) Update(measurement float64) float64 { | ||
| // Prediction update | ||
| // X = X | ||
| // P = P + Q | ||
| kf.P = kf.P + kf.Q | ||
|
|
||
| // Measurement update | ||
| // K = P / (P + R) | ||
| kf.K = kf.P / (kf.P + kf.R) | ||
| // X = X + K * (measurement - X) | ||
| kf.X = kf.X + kf.K*(measurement-kf.X) | ||
| // P = (1 - K) * P | ||
| kf.P = (1 - kf.K) * kf.P | ||
|
|
||
| return kf.X | ||
| } |
There was a problem hiding this comment.
Guard against division-by-zero / non-finite gain.
kf.K = kf.P / (kf.P + kf.R) divides by kf.P + kf.R. With the current physics.go init (R=1.0, P=1.0) this is safe, but KalmanFilter is an exported type with no constructor validation; any caller passing R=0 together with a P that decays to ~0 (or supplying P=R=0) will produce NaN/±Inf and silently corrupt kf.X, which then flows into a.threatTemp and the IsAnomaly decision in GetSecurityState.
🛡️ Proposed guard
func (kf *KalmanFilter) Update(measurement float64) float64 {
// Prediction update
// X = X
// P = P + Q
kf.P = kf.P + kf.Q
// Measurement update
- // K = P / (P + R)
- kf.K = kf.P / (kf.P + kf.R)
+ // K = P / (P + R)
+ denom := kf.P + kf.R
+ if denom <= 0 {
+ // Degenerate covariance; skip measurement update to avoid NaN/Inf.
+ return kf.X
+ }
+ kf.K = kf.P / denom
// X = X + K * (measurement - X)
kf.X = kf.X + kf.K*(measurement-kf.X)
// P = (1 - K) * P
kf.P = (1 - kf.K) * kf.P
return kf.X
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (kf *KalmanFilter) Update(measurement float64) float64 { | |
| // Prediction update | |
| // X = X | |
| // P = P + Q | |
| kf.P = kf.P + kf.Q | |
| // Measurement update | |
| // K = P / (P + R) | |
| kf.K = kf.P / (kf.P + kf.R) | |
| // X = X + K * (measurement - X) | |
| kf.X = kf.X + kf.K*(measurement-kf.X) | |
| // P = (1 - K) * P | |
| kf.P = (1 - kf.K) * kf.P | |
| return kf.X | |
| } | |
| func (kf *KalmanFilter) Update(measurement float64) float64 { | |
| // Prediction update | |
| // X = X | |
| // P = P + Q | |
| kf.P = kf.P + kf.Q | |
| // Measurement update | |
| // K = P / (P + R) | |
| denom := kf.P + kf.R | |
| if denom <= 0 { | |
| // Degenerate covariance; skip measurement update to avoid NaN/Inf. | |
| return kf.X | |
| } | |
| kf.K = kf.P / denom | |
| // X = X + K * (measurement - X) | |
| kf.X = kf.X + kf.K*(measurement-kf.X) | |
| // P = (1 - K) * P | |
| kf.P = (1 - kf.K) * kf.P | |
| return kf.X | |
| } |
🤖 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 `@agents/internal/physics/kalman.go` around lines 21 - 36, The Update method
can produce NaN/Inf when computing kf.K = kf.P / (kf.P + kf.R); guard by
computing denom := kf.P + kf.R and if denom is zero or not finite (use
math.IsNaN / math.IsInf) set kf.K = 0 (or clamp to a safe value) and skip the
measurement update that would corrupt kf.X; after computing kf.K also validate
it with math.IsNaN/math.IsInf and if invalid set kf.K = 0 and ensure kf.P
remains non-negative (clamp if needed) so the filter never writes NaN/Inf into
kf.X or kf.P.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b696914779
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [ -f go.work ]; then | ||
| echo "Building with go.work..." | ||
| go build ./... | ||
| go build all |
There was a problem hiding this comment.
Using go build all in the build-go job broadens CI from building this repo’s packages to also building every dependency (including test-only deps) of every workspace module, which makes the pipeline fail on upstream/transitive breakage unrelated to this PR and increases build instability. go help packages defines all that way, so this change materially alters CI semantics versus ./... and can block merges when a dependency’s build regresses.
Useful? React with 👍 / 👎.
Resolves #143. This PR introduces a basic ReputationManager to track and deduct scores from nodes detected performing suspicious activities.
Summary by Sourcery
Introduce a reputation manager to track and adjust node trust scores based on suspicious activity.
New Features:
Tests:
Summary by CodeRabbit
New Features
Chores
Tests