[P3] feat(marl): Implement MARL Stability - #592
Merged
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 (1)
📒 Files selected for processing (4)
✨ 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 MARL StabilityController to enforce cooldown-based throttling and containment rate limits for agent actions, along with unit tests validating basic behavior. 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 2 issues, and left some high level feedback:
- The separation of
CanActandRecordActionmeans the check and state mutation aren't atomic, so concurrent callers could all passCanActbefore anyRecordActionruns; consider a singleTryRecordAction(cost) boolthat both checks and updates under one lock to enforce limits correctly under concurrency. - All
StabilityControllerfields are exported but mutated only under an internal mutex; making them unexported or providing read-only accessors would better encapsulate state and prevent callers from bypassing the controller's synchronization and invariants. - The test relies on
time.Sleepwith real time, which can introduce flakiness and slowness; consider injecting a clock or using a configurable time source to simulate cooldown expiry deterministically.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The separation of `CanAct` and `RecordAction` means the check and state mutation aren't atomic, so concurrent callers could all pass `CanAct` before any `RecordAction` runs; consider a single `TryRecordAction(cost) bool` that both checks and updates under one lock to enforce limits correctly under concurrency.
- All `StabilityController` fields are exported but mutated only under an internal mutex; making them unexported or providing read-only accessors would better encapsulate state and prevent callers from bypassing the controller's synchronization and invariants.
- The test relies on `time.Sleep` with real time, which can introduce flakiness and slowness; consider injecting a clock or using a configurable time source to simulate cooldown expiry deterministically.
## Individual Comments
### Comment 1
<location path="phoenix_os/agents/internal/game/marl/stability.go" line_range="26-35" />
<code_context>
+}
+
+// CanAct checks if an agent is allowed to perform a containment action.
+func (sc *StabilityController) CanAct(cost float64) bool {
+ sc.mu.Lock()
+ defer sc.mu.Unlock()
+
+ // Check Cooldown
+ if time.Since(sc.LastAction) < sc.Cooldown {
+ return false
+ }
+
+ // Check Containment Rate (Action Debt)
+ if sc.ActionDebt+cost > sc.MaxContainment {
+ return false
+ }
</code_context>
<issue_to_address>
**question (bug_risk):** Consider whether ActionDebt should decay or reset over time to avoid permanent lockout.
Because ActionDebt only increases and is never reduced, once `ActionDebt + cost` exceeds `MaxContainment`, `CanAct` will always return false and the agent can never act again unless the controller state is externally reset. If the goal is a time‑bounded rate limit, consider adding decay, a reset policy, or a moving-window–based limit instead. If the permanent lockout is intentional, documenting or naming it accordingly would help set expectations for callers.
</issue_to_address>
### Comment 2
<location path="phoenix_os/agents/internal/game/marl/stability_test.go" line_range="22-23" />
<code_context>
+ t.Error("Expected action to be throttled due to cooldown")
+ }
+
+ // Wait for cooldown
+ time.Sleep(150 * time.Millisecond)
+
+ // Should pass if under containment limit
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid real-time sleeps to reduce test flakiness and runtime.
This test’s reliance on a 150ms `time.Sleep` makes it brittle on slower or loaded CI machines. Instead, allow `StabilityController` to take an injectable time source so you can advance time in tests without sleeping, or restructure the test to set `LastAction` directly to cover the cooldown boundary deterministically.
Suggested implementation:
```golang
// Simulate cooldown elapsing by moving LastAction back in time
sc.LastAction = sc.LastAction.Add(-150 * time.Millisecond)
// Should pass if under containment limit
```
This change assumes:
1. `StabilityController` has an exported `LastAction time.Time` field.
2. `CanAct` uses `LastAction` and the current time to enforce cooldowns.
If `LastAction` is unexported or named differently, or if cooldown is enforced via another internal field, you will need to:
- Adjust the field name in the test to match the actual struct field (e.g., `sc.lastAction` or `sc.lastActionTime`).
- Ensure the test file is in the same package as `StabilityController` if you need access to unexported fields.
</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
| // Wait for cooldown | ||
| time.Sleep(150 * time.Millisecond) |
There was a problem hiding this comment.
suggestion (testing): Avoid real-time sleeps to reduce test flakiness and runtime.
This test’s reliance on a 150ms time.Sleep makes it brittle on slower or loaded CI machines. Instead, allow StabilityController to take an injectable time source so you can advance time in tests without sleeping, or restructure the test to set LastAction directly to cover the cooldown boundary deterministically.
Suggested implementation:
// Simulate cooldown elapsing by moving LastAction back in time
sc.LastAction = sc.LastAction.Add(-150 * time.Millisecond)
// Should pass if under containment limitThis change assumes:
StabilityControllerhas an exportedLastAction time.Timefield.CanActusesLastActionand the current time to enforce cooldowns.
If LastAction is unexported or named differently, or if cooldown is enforced via another internal field, you will need to:
- Adjust the field name in the test to match the actual struct field (e.g.,
sc.lastActionorsc.lastActionTime). - Ensure the test file is in the same package as
StabilityControllerif you need access to unexported fields.
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 #144. Implemented StabilityController to enforce action debt, cooldown periods, and maximum containment rates.
Summary by Sourcery
Introduce a stability controller for MARL agents to enforce action cooldowns and containment limits.
New Features:
Tests: