[P2] feat(swarm): Implement Byzantine Swarm Protection#620
Closed
fallofpheonix wants to merge 5 commits into
Closed
[P2] feat(swarm): Implement Byzantine Swarm Protection#620fallofpheonix wants to merge 5 commits into
fallofpheonix wants to merge 5 commits into
Conversation
Reviewer's GuideIntroduces configurable warden thresholds, a MARL agent stability controller, swarm governance with reputation-based proposal validation, and an optimized JSON serialization helper, along with tests and configuration wiring. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 (3)
📒 Files selected for processing (17)
✨ 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 |
![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 4 issues, and left some high level feedback:
- In
optimizer.goyou're importing both the standardencoding/jsonandgithub.com/segmentio/encoding/jsonunder the same name and then callingjson.Marshal, which will not compile as-is; either remove the stdlib import or alias one of the packages (e.g.segjson) and call it explicitly. - The
StabilityControllerseparatesCanActandRecordActionunder different lock scopes, so concurrent callers can all passCanActand then updateActionDebtlater, allowing the total to exceedMaxContainment; consider combining the check and update in a single method or keeping the lock held between the two steps. - The
NewWardenconstructor now requires a concrete path andmainuses a hard-coded relative path (../../config/warden.json), which is brittle across environments; consider accepting anio.Readeror injecting thresholds/config externally so callers can provide configuration without depending on repo-relative paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `optimizer.go` you're importing both the standard `encoding/json` and `github.com/segmentio/encoding/json` under the same name and then calling `json.Marshal`, which will not compile as-is; either remove the stdlib import or alias one of the packages (e.g. `segjson`) and call it explicitly.
- The `StabilityController` separates `CanAct` and `RecordAction` under different lock scopes, so concurrent callers can all pass `CanAct` and then update `ActionDebt` later, allowing the total to exceed `MaxContainment`; consider combining the check and update in a single method or keeping the lock held between the two steps.
- The `NewWarden` constructor now requires a concrete path and `main` uses a hard-coded relative path (`../../config/warden.json`), which is brittle across environments; consider accepting an `io.Reader` or injecting thresholds/config externally so callers can provide configuration without depending on repo-relative paths.
## Individual Comments
### Comment 1
<location path="phoenix_os/warden/src/warden.go" line_range="34-50" />
<code_context>
}
-func NewWarden() *Warden {
+func NewWarden(configPath string) (*Warden, error) {
+ configFile, err := os.ReadFile(configPath)
+ if err != nil {
+ return nil, fmt.Errorf("failed to read config file: %w", err)
+ }
+
+ var config Config
+ if err := json.Unmarshal(configFile, &config); err != nil {
+ return nil, fmt.Errorf("failed to parse config file: %w", err)
+ }
+
return &Warden{
CurrentState: StateSafe,
Throttling: 0.0,
- }
+ Config: config,
+ }, nil
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Validate the loaded thresholds to ensure they are sane and ordered.
Given that `EvaluateSDI` relies on strictly increasing thresholds (Safe < Watch < Suspicious < Critical) and normalized values, a misconfigured JSON (e.g., non-monotonic, equal, zero, or >1 thresholds) could cause incorrect or collapsed state transitions. Please add post-unmarshal validation (e.g., monotonicity check and ensuring all thresholds are in [0,1]) and return an error if the config is invalid.
```suggestion
func NewWarden(configPath string) (*Warden, error) {
configFile, err := os.ReadFile(configPath)
if err != nil {
return nil, fmt.Errorf("failed to read config file: %w", err)
}
var config Config
if err := json.Unmarshal(configFile, &config); err != nil {
return nil, fmt.Errorf("failed to parse config file: %w", err)
}
if err := validateConfig(&config); err != nil {
return nil, fmt.Errorf("invalid config: %w", err)
}
return &Warden{
CurrentState: StateSafe,
Throttling: 0.0,
Config: config,
}, nil
}
func validateConfig(config *Config) error {
t := config.Thresholds
// Ensure each threshold is normalized and non-zero: (0, 1]
if t.Safe <= 0 || t.Safe > 1 {
return fmt.Errorf("threshold 'safe' must be in (0, 1], got %f", t.Safe)
}
if t.Watch <= 0 || t.Watch > 1 {
return fmt.Errorf("threshold 'watch' must be in (0, 1], got %f", t.Watch)
}
if t.Suspicious <= 0 || t.Suspicious > 1 {
return fmt.Errorf("threshold 'suspicious' must be in (0, 1], got %f", t.Suspicious)
}
if t.Critical <= 0 || t.Critical > 1 {
return fmt.Errorf("threshold 'critical' must be in (0, 1], got %f", t.Critical)
}
// Ensure strictly increasing thresholds: Safe < Watch < Suspicious < Critical
if !(t.Safe < t.Watch && t.Watch < t.Suspicious && t.Suspicious < t.Critical) {
return fmt.Errorf(
"thresholds must be strictly increasing (safe < watch < suspicious < critical), got safe=%f, watch=%f, suspicious=%f, critical=%f",
t.Safe, t.Watch, t.Suspicious, t.Critical,
)
}
return nil
}
```
</issue_to_address>
### Comment 2
<location path="phoenix_os/warden/src/warden.go" line_range="97" />
<code_context>
func main() {
fmt.Println("Phoenix Warden starting with Finite-State Controller...")
- warden := NewWarden()
+ warden, err := NewWarden("../../config/warden.json")
+ if err != nil {
+ fmt.Printf("Error initializing Warden: %v\n", err)
</code_context>
<issue_to_address>
**suggestion:** Consider making the config path configurable rather than hardcoded relative paths.
Hardcoding "../../config/warden.json" couples startup to a specific working directory and layout, which is likely to break under different launch setups (systemd, containers, tests). Prefer a configurable source (e.g., flag, env var, or embedded default with override) so startup doesn’t depend on cwd.
</issue_to_address>
### Comment 3
<location path="phoenix_os/warden/src/warden_test.go" line_range="18-23" />
<code_context>
+ "critical": 0.9
+ }
+ }`
+ configFile := "test_warden.json"
+ err := os.WriteFile(configFile, []byte(configContent), 0644)
+ if err != nil {
+ t.Fatalf("Failed to create temp config file: %v", err)
+ }
+ defer os.Remove(configFile)
+
+ w, err := NewWarden(configFile)
</code_context>
<issue_to_address>
**suggestion (testing):** Use t.TempDir and unique paths instead of a fixed filename in the working directory
A fixed filename in the working directory can cause tests to race with each other, break under concurrent runs, or fail on read-only directories. Use t.TempDir() and build the path with filepath.Join so each test gets its own temp file and automatic cleanup, instead of relying on os.Remove.
Suggested implementation:
```golang
// Create a config file in a temporary directory for the test
configContent := `{
"thresholds": {
"safe": 0.3,
"watch": 0.5,
"suspicious": 0.7,
"critical": 0.9
}
}`
dir := t.TempDir()
configFile := filepath.Join(dir, "warden_config.json")
err := os.WriteFile(configFile, []byte(configContent), 0o644)
if err != nil {
t.Fatalf("Failed to create temp config file: %v", err)
}
w, err := NewWarden(configFile)
```
1. Ensure `filepath` is imported at the top of `warden_test.go`:
- Add `path/filepath` to the import block, e.g. `import "path/filepath"`, or to the existing grouped imports if present.
2. If other tests in this file also create temporary config files with fixed names in the working directory, consider updating them similarly to use `t.TempDir()` and `filepath.Join` for consistency and to avoid race conditions.
</issue_to_address>
### Comment 4
<location path="phoenix_os/telemetry/serialization/optimizer_test.go" line_range="12-18" />
<code_context>
+ Value string `json:"value"`
+}
+
+func TestOptimizer(t *testing.T) {
+ data := TestStruct{ID: 1, Value: "test"}
+ encoded, err := OptimizedMarshaler(data)
+ if err != nil {
+ t.Fatalf("Failed to marshal: %v", err)
+ }
+ if string(encoded) == "" {
+ t.Error("Marshaled data is empty")
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen assertions by validating the actual JSON output and error paths
Asserting only that the output is non-empty is too weak. Please:
- Assert the exact JSON output for this struct (or decode into a map if field order is unstable) to verify tags and field mapping.
- Add a negative test using an unsupported type and assert that `OptimizedMarshaler` returns a non-nil error.
This will better verify correctness and error handling of the marshaler.
Suggested implementation:
```golang
import (
"encoding/json"
"testing"
)
```
```golang
func TestOptimizer(t *testing.T) {
data := TestStruct{ID: 1, Value: "test"}
encoded, err := OptimizedMarshaler(data)
if err != nil {
t.Fatalf("Failed to marshal: %v", err)
}
if len(encoded) == 0 {
t.Fatal("Marshaled data is empty")
}
var decoded map[string]interface{}
if err := json.Unmarshal(encoded, &decoded); err != nil {
t.Fatalf("Failed to unmarshal encoded JSON: %v", err)
}
id, ok := decoded["id"].(float64)
if !ok || id != 1 {
t.Fatalf("Unexpected id field: %#v", decoded["id"])
}
value, ok := decoded["value"].(string)
if !ok || value != "test" {
t.Fatalf("Unexpected value field: %#v", decoded["value"])
}
}
func TestOptimizerUnsupportedType(t *testing.T) {
type Unsupported struct {
Ch chan int `json:"ch"`
}
data := Unsupported{Ch: make(chan int)}
encoded, err := OptimizedMarshaler(data)
if err == nil {
t.Fatalf("Expected error for unsupported type, got nil (encoded=%q)", string(encoded))
}
}
```
</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
+34
to
50
| func NewWarden(configPath string) (*Warden, error) { | ||
| configFile, err := os.ReadFile(configPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read config file: %w", err) | ||
| } | ||
|
|
||
| var config Config | ||
| if err := json.Unmarshal(configFile, &config); err != nil { | ||
| return nil, fmt.Errorf("failed to parse config file: %w", err) | ||
| } | ||
|
|
||
| return &Warden{ | ||
| CurrentState: StateSafe, | ||
| Throttling: 0.0, | ||
| } | ||
| Config: config, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Validate the loaded thresholds to ensure they are sane and ordered.
Given that EvaluateSDI relies on strictly increasing thresholds (Safe < Watch < Suspicious < Critical) and normalized values, a misconfigured JSON (e.g., non-monotonic, equal, zero, or >1 thresholds) could cause incorrect or collapsed state transitions. Please add post-unmarshal validation (e.g., monotonicity check and ensuring all thresholds are in [0,1]) and return an error if the config is invalid.
Suggested change
| func NewWarden(configPath string) (*Warden, error) { | |
| configFile, err := os.ReadFile(configPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read config file: %w", err) | |
| } | |
| var config Config | |
| if err := json.Unmarshal(configFile, &config); err != nil { | |
| return nil, fmt.Errorf("failed to parse config file: %w", err) | |
| } | |
| return &Warden{ | |
| CurrentState: StateSafe, | |
| Throttling: 0.0, | |
| } | |
| Config: config, | |
| }, nil | |
| } | |
| func NewWarden(configPath string) (*Warden, error) { | |
| configFile, err := os.ReadFile(configPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read config file: %w", err) | |
| } | |
| var config Config | |
| if err := json.Unmarshal(configFile, &config); err != nil { | |
| return nil, fmt.Errorf("failed to parse config file: %w", err) | |
| } | |
| if err := validateConfig(&config); err != nil { | |
| return nil, fmt.Errorf("invalid config: %w", err) | |
| } | |
| return &Warden{ | |
| CurrentState: StateSafe, | |
| Throttling: 0.0, | |
| Config: config, | |
| }, nil | |
| } | |
| func validateConfig(config *Config) error { | |
| t := config.Thresholds | |
| // Ensure each threshold is normalized and non-zero: (0, 1] | |
| if t.Safe <= 0 || t.Safe > 1 { | |
| return fmt.Errorf("threshold 'safe' must be in (0, 1], got %f", t.Safe) | |
| } | |
| if t.Watch <= 0 || t.Watch > 1 { | |
| return fmt.Errorf("threshold 'watch' must be in (0, 1], got %f", t.Watch) | |
| } | |
| if t.Suspicious <= 0 || t.Suspicious > 1 { | |
| return fmt.Errorf("threshold 'suspicious' must be in (0, 1], got %f", t.Suspicious) | |
| } | |
| if t.Critical <= 0 || t.Critical > 1 { | |
| return fmt.Errorf("threshold 'critical' must be in (0, 1], got %f", t.Critical) | |
| } | |
| // Ensure strictly increasing thresholds: Safe < Watch < Suspicious < Critical | |
| if !(t.Safe < t.Watch && t.Watch < t.Suspicious && t.Suspicious < t.Critical) { | |
| return fmt.Errorf( | |
| "thresholds must be strictly increasing (safe < watch < suspicious < critical), got safe=%f, watch=%f, suspicious=%f, critical=%f", | |
| t.Safe, t.Watch, t.Suspicious, t.Critical, | |
| ) | |
| } | |
| return nil | |
| } |
| func main() { | ||
| fmt.Println("Phoenix Warden starting with Finite-State Controller...") | ||
| warden := NewWarden() | ||
| warden, err := NewWarden("../../config/warden.json") |
There was a problem hiding this comment.
suggestion: Consider making the config path configurable rather than hardcoded relative paths.
Hardcoding "../../config/warden.json" couples startup to a specific working directory and layout, which is likely to break under different launch setups (systemd, containers, tests). Prefer a configurable source (e.g., flag, env var, or embedded default with override) so startup doesn’t depend on cwd.
Comment on lines
+18
to
+23
| configFile := "test_warden.json" | ||
| err := os.WriteFile(configFile, []byte(configContent), 0644) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create temp config file: %v", err) | ||
| } | ||
| defer os.Remove(configFile) |
There was a problem hiding this comment.
suggestion (testing): Use t.TempDir and unique paths instead of a fixed filename in the working directory
A fixed filename in the working directory can cause tests to race with each other, break under concurrent runs, or fail on read-only directories. Use t.TempDir() and build the path with filepath.Join so each test gets its own temp file and automatic cleanup, instead of relying on os.Remove.
Suggested implementation:
// Create a config file in a temporary directory for the test
configContent := `{
"thresholds": {
"safe": 0.3,
"watch": 0.5,
"suspicious": 0.7,
"critical": 0.9
}
}`
dir := t.TempDir()
configFile := filepath.Join(dir, "warden_config.json")
err := os.WriteFile(configFile, []byte(configContent), 0o644)
if err != nil {
t.Fatalf("Failed to create temp config file: %v", err)
}
w, err := NewWarden(configFile)- Ensure
filepathis imported at the top ofwarden_test.go:- Add
path/filepathto the import block, e.g.import "path/filepath", or to the existing grouped imports if present.
- Add
- If other tests in this file also create temporary config files with fixed names in the working directory, consider updating them similarly to use
t.TempDir()andfilepath.Joinfor consistency and to avoid race conditions.
Comment on lines
+12
to
+18
| func TestOptimizer(t *testing.T) { | ||
| data := TestStruct{ID: 1, Value: "test"} | ||
| encoded, err := OptimizedMarshaler(data) | ||
| if err != nil { | ||
| t.Fatalf("Failed to marshal: %v", err) | ||
| } | ||
| if string(encoded) == "" { |
There was a problem hiding this comment.
suggestion (testing): Strengthen assertions by validating the actual JSON output and error paths
Asserting only that the output is non-empty is too weak. Please:
- Assert the exact JSON output for this struct (or decode into a map if field order is unstable) to verify tags and field mapping.
- Add a negative test using an unsupported type and assert that
OptimizedMarshalerreturns a non-nil error.
This will better verify correctness and error handling of the marshaler.
Suggested implementation:
import (
"encoding/json"
"testing"
)func TestOptimizer(t *testing.T) {
data := TestStruct{ID: 1, Value: "test"}
encoded, err := OptimizedMarshaler(data)
if err != nil {
t.Fatalf("Failed to marshal: %v", err)
}
if len(encoded) == 0 {
t.Fatal("Marshaled data is empty")
}
var decoded map[string]interface{}
if err := json.Unmarshal(encoded, &decoded); err != nil {
t.Fatalf("Failed to unmarshal encoded JSON: %v", err)
}
id, ok := decoded["id"].(float64)
if !ok || id != 1 {
t.Fatalf("Unexpected id field: %#v", decoded["id"])
}
value, ok := decoded["value"].(string)
if !ok || value != "test" {
t.Fatalf("Unexpected value field: %#v", decoded["value"])
}
}
func TestOptimizerUnsupportedType(t *testing.T) {
type Unsupported struct {
Ch chan int `json:"ch"`
}
data := Unsupported{Ch: make(chan int)}
encoded, err := OptimizedMarshaler(data)
if err == nil {
t.Fatalf("Expected error for unsupported type, got nil (encoded=%q)", string(encoded))
}
}![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: 291faba52d
ℹ️ 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".
Comment on lines
+27
to
+30
| func (sg *SwarmGovernor) ValidateProposal(nodeReputation float64) bool { | ||
| sg.mu.RLock() | ||
| defer sg.mu.RUnlock() | ||
| return nodeReputation >= sg.Policy.MinReputation |
There was a problem hiding this comment.
ValidateProposal only checks MinReputation and never uses Policy.QuorumSize, so proposals can be accepted even when the consensus set is below the configured quorum. In this commit, QuorumSize is defined and set in tests but never read, which means the advertised Byzantine/quorum protection is not actually enforced.
Useful? React with 👍 / 👎.
Comment on lines
+36
to
+40
| if sc.ActionDebt+cost > sc.MaxContainment { | ||
| return false | ||
| } | ||
|
|
||
| return true |
There was a problem hiding this comment.
CanAct and RecordAction are separate locked calls, so concurrent callers can both pass CanAct before either records debt, then both call RecordAction and exceed MaxContainment. This creates a TOCTOU race that bypasses the containment cap under parallel agent actions.
Useful? React with 👍 / 👎.
| func main() { | ||
| fmt.Println("Phoenix Warden starting with Finite-State Controller...") | ||
| warden := NewWarden() | ||
| warden, err := NewWarden("../../config/warden.json") |
There was a problem hiding this comment.
The hard-coded "../../config/warden.json" is resolved from the process working directory, not from the source file location, so normal launches (for example from repo root or as a service) will often fail to find the config and exit at startup. The committed config lives at config/warden.json, so this default path is brittle and frequently incorrect.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
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
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 #128. Implemented SwarmGovernor to enforce node reputation policies and quorum consensus requirements.
Summary by Sourcery
Introduce governance, stability, and telemetry components to enforce configurable safety policies, while externalizing Warden thresholds to JSON configuration and covering new behavior with unit tests.
New Features:
Tests: