[Communication] PR5: Route Agents With Mailbox - #235
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe communication layer now delivers queued messages directly to active receivers and explicitly handles nodes without subscribers. Interceptor assignment and reassignment propagation has moved from events to CommsNode messages. IADS handles assignment requests, selects viable launcher targets, and sends responses. Interceptor registration configures message receivers, lifecycle cleanup unsubscribes handlers, and Mailbox latency behavior is covered by an EditMode test. Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/Scripts/Communication/Mailbox.cs (1)
139-143: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve the acknowledged PDR defect before merge.
Line 139 states packet-loss behavior is incorrect whenever
PacketDeliveryRatio != 1.0, so configured lossy links currently have undefined simulation behavior. Define the intended drop/retry semantics and add deterministic coverage for PDR0,1, and an intermediate value.🤖 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 `@Assets/Scripts/Communication/Mailbox.cs` around lines 139 - 143, Fix the packet-loss handling in the mailbox send path around PacketDeliveryRatio so PDR 0 always drops, PDR 1 always delivers, and intermediate values apply the intended deterministic drop/retry semantics before enqueueing into the priority queue. Remove the acknowledged TODO and add deterministic coverage for all three PDR cases, including the expected retry behavior for lossy links.
🤖 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 `@Assets/Scripts/Communication/Mailbox.cs`:
- Around line 93-99: Update the message delivery loop in Mailbox so
_messageBuffer is cleared even when pending.Receiver.Receive(pending.Message)
throws. Ensure cleanup occurs before propagating the exception, preventing
previously processed messages from being redelivered on the next frame.
In `@Assets/Tests/EditMode/MailboxTests.cs`:
- Around line 45-58: Update the Mailbox test setup around the sender and
receiver CommsNode instances to register both nodes with CommsManager before
calling _mailbox.Send. Ensure the corresponding teardown unregisters both nodes,
preserving fixture isolation and preventing CommsManager state from affecting
the test outcome.
---
Outside diff comments:
In `@Assets/Scripts/Communication/Mailbox.cs`:
- Around line 139-143: Fix the packet-loss handling in the mailbox send path
around PacketDeliveryRatio so PDR 0 always drops, PDR 1 always delivers, and
intermediate values apply the intended deterministic drop/retry semantics before
enqueueing into the priority queue. Remove the acknowledged TODO and add
deterministic coverage for all three PDR cases, including the expected retry
behavior for lossy links.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55225490-dca1-4c97-a022-1803cc3879a9
📒 Files selected for processing (7)
Assets/Scripts/Communication/CommsNode.csAssets/Scripts/Communication/Mailbox.csAssets/Scripts/IADS/IADS.csAssets/Scripts/Interceptors/CarrierBase.csAssets/Scripts/Interceptors/IInterceptor.csAssets/Scripts/Interceptors/InterceptorBase.csAssets/Tests/EditMode/MailboxTests.cs
💤 Files with no reviewable changes (1)
- Assets/Scripts/Interceptors/IInterceptor.cs
| foreach (PendingMessage pending in _messageBuffer) { | ||
| if (!IsNodeActive(pending.Receiver)) { | ||
| continue; | ||
| } | ||
| pending.Receiver.Receive(pending.Message); | ||
| } | ||
| _messageBuffer.Clear(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clear the due-message buffer when a receiver throws.
If Receive throws at Line 97, _messageBuffer.Clear() is skipped. The next frame re-delivers every already-buffered message, causing duplicate assignment/reassignment handling.
Proposed fix
- foreach (PendingMessage pending in _messageBuffer) {
- if (!IsNodeActive(pending.Receiver)) {
- continue;
+ try {
+ foreach (PendingMessage pending in _messageBuffer) {
+ if (!IsNodeActive(pending.Receiver)) {
+ continue;
+ }
+ pending.Receiver.Receive(pending.Message);
}
- pending.Receiver.Receive(pending.Message);
+ } finally {
+ _messageBuffer.Clear();
}
- _messageBuffer.Clear();📝 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.
| foreach (PendingMessage pending in _messageBuffer) { | |
| if (!IsNodeActive(pending.Receiver)) { | |
| continue; | |
| } | |
| pending.Receiver.Receive(pending.Message); | |
| } | |
| _messageBuffer.Clear(); | |
| try { | |
| foreach (PendingMessage pending in _messageBuffer) { | |
| if (!IsNodeActive(pending.Receiver)) { | |
| continue; | |
| } | |
| pending.Receiver.Receive(pending.Message); | |
| } | |
| } finally { | |
| _messageBuffer.Clear(); | |
| } |
🤖 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 `@Assets/Scripts/Communication/Mailbox.cs` around lines 93 - 99, Update the
message delivery loop in Mailbox so _messageBuffer is cleared even when
pending.Receiver.Receive(pending.Message) throws. Ensure cleanup occurs before
propagating the exception, preventing previously processed messages from being
redelivered on the next frame.
| var sender = new CommsNode(Configs.AgentType.Iads); | ||
| var receiver = new CommsNode(Configs.AgentType.ShoreBattery); | ||
| int receivedCount = 0; | ||
| Message receivedMessage = null; | ||
| receiver.OnMessageReceived += message => { | ||
| receivedCount++; | ||
| receivedMessage = message; | ||
| }; | ||
|
|
||
| var target = new FixedHierarchical(position: Vector3.one); | ||
| var messageToSend = new AssignTargetResponseMessage(sender, receiver, target); | ||
|
|
||
| SetPrivateProperty(_simManager, "ElapsedTime", 0f); | ||
| _mailbox.Send(messageToSend); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Register the test nodes with CommsManager.
Mailbox.Send drops messages when CommsManager.Instance exists and either node is unregistered. This test currently passes only when no manager exists, so fixture state can turn it into a false pass or failure. Explicitly add both nodes before Send and remove them during teardown.
🤖 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 `@Assets/Tests/EditMode/MailboxTests.cs` around lines 45 - 58, Update the
Mailbox test setup around the sender and receiver CommsNode instances to
register both nodes with CommsManager before calling _mailbox.Send. Ensure the
corresponding teardown unregisters both nodes, preserving fixture isolation and
preventing CommsManager state from affecting the test outcome.
ec703bb to
136b6f0
Compare
b6cb9d4 to
eebe3c2
Compare
1 participant
No description provided.