Repository navigation
[Communication] PR2: Mailbox Hooked Up With Agents, Added Tests and Examples - #238
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughThe change replaces interceptor assignment and target reassignment callbacks with communication-node messages. IADS and interceptors now dispatch assignment, response, and reassignment messages. Carrier sub-interceptors receive their parent communication node directly. CommsManager forwards messages and reuses existing agent nodes. Mailbox delivery handles immediately due messages and removed receivers. Simulation communication settings and edit-mode mailbox tests were added. 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 💡 1📝 Generate docstrings 💡
🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Assets/Scripts/IADS/IADS.cs (1)
167-188: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing target-termination check and
CommsManager.Instancenull guard inAssignSubInterceptor.Two issues in this method:
- Line 182 calls
subInterceptor.ShouldAcceptReassignedTarget(target)without first checkingtarget.IsTerminated, unlikeInterceptorBase.AssignSubInterceptor's equivalent call, which explicitly checks!target.IsTerminatedfirst.- Line 183-184 calls
CommsManager.Instance.SendMessage(...)without checkingCommsManager.Instancefor null.Both are covered in the consolidated comment with the shared fix.
🤖 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/IADS/IADS.cs` around lines 167 - 188, Update AssignSubInterceptor to accept a reassigned target only when target is non-null and not terminated, matching InterceptorBase.AssignSubInterceptor, and send the AssignTargetResponseMessage only when CommsManager.Instance is non-null.Assets/Scripts/Interceptors/InterceptorBase.cs (2)
1-1: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCentralize the target-termination check inside
ShouldAcceptReassignedTarget.ShouldAcceptReassignedTarget(InterceptorBase.cs) checks onlytarget == null, nottarget.IsTerminated.InterceptorBase.AssignSubInterceptorcompensates locally with an explicit!target.IsTerminatedcheck before calling it, but this compensating check is missing in two other call sites, allowing a target that terminates during message transit — a real possibility now that the mailbox introduces real latency — to be assigned to an interceptor.
Assets/Scripts/Interceptors/InterceptorBase.cs#L111-139: add|| target.IsTerminatedto the early-return condition inShouldAcceptReassignedTarget(Line 115-117), then remove the now-redundant!target.IsTerminatedcheck fromAssignSubInterceptor(Line 149) and rely onEvaluateReassignedTargetcalling the centralized check.Assets/Scripts/IADS/IADS.cs#L167-188: once the check is centralized, the existingsubInterceptor.ShouldAcceptReassignedTarget(target)call at Line 182 is automatically protected without further changes here.🎯 Proposed fix
public bool ShouldAcceptReassignedTarget(IHierarchical target) { // Continue searching for targets if no target was found. - if (target == null) { + if (target == null || target.IsTerminated) { return false; }IHierarchical target = HierarchicalAgent.FindNewTarget(subInterceptor.HierarchicalAgent, subInterceptor.CapacityRemaining); - if (target != null && !target.IsTerminated && - subInterceptor.ShouldAcceptReassignedTarget(target)) { + if (target != null && subInterceptor.ShouldAcceptReassignedTarget(target)) { SendAssignTargetResponse(subInterceptor, target); 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 `@Assets/Scripts/Interceptors/InterceptorBase.cs` at line 1, Update InterceptorBase.ShouldAcceptReassignedTarget to reject targets that are null or terminated by adding the target.IsTerminated condition to its early return. In InterceptorBase.AssignSubInterceptor, remove the redundant !target.IsTerminated check and rely on EvaluateReassignedTarget and the centralized validation; leave the IADS call site unchanged.
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
CommsManager.Instance.SendMessage(...)calls against a null singleton. CallCommsManager.Instance.SendMessage(...)directly fromInterceptorBase.csandIADS.cs; ifCommsManager.Instanceisnullor destroyed beforeMailbox.SendMessagecan evaluate its own null manager check, the member access causes aNullReferenceException. Add an explicitCommsManager.Instance == nullreturn before the.SendMessage(...)inSendAssignTargetRequest,Send AssignTargetResponse,SendReassignTargetRequest, andAssignSubInterceptor.🤖 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/Interceptors/InterceptorBase.cs` at line 1, Add an early return when CommsManager.Instance is null in SendAssignTargetRequest, SendAssignTargetResponse, SendReassignTargetRequest, and AssignSubInterceptor before calling its SendMessage method. Apply the guard in InterceptorBase and IADS while preserving the existing message-sending behavior when the singleton is available.
🤖 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/Interceptors/InterceptorBase.cs`:
- Around line 111-139: Update ShouldAcceptReassignedTarget to reject targets
whose IsTerminated property is true, alongside the existing null check, so every
caller—including EvaluateReassignedTarget—applies the same validation. Remove
the redundant termination guard from AssignSubInterceptor and rely on
ShouldAcceptReassignedTarget as the single acceptance check.
- Around line 79-109: Update SendAssignTargetRequest, SendAssignTargetResponse,
and SendReassignTargetRequest to include a null/fake-null guard for
CommsManager.Instance before calling SendMessage, preserving the existing
argument and target validation checks.
In `@Assets/Scripts/Managers/CommsManager.cs`:
- Around line 14-21: Remove the unused _receivedMessages field and its Add call,
or clear _receivedMessages in ClearNodes alongside _nodes; ensure repeated
simulation runs cannot retain messages indefinitely.
---
Outside diff comments:
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 167-188: Update AssignSubInterceptor to accept a reassigned target
only when target is non-null and not terminated, matching
InterceptorBase.AssignSubInterceptor, and send the AssignTargetResponseMessage
only when CommsManager.Instance is non-null.
In `@Assets/Scripts/Interceptors/InterceptorBase.cs`:
- Line 1: Update InterceptorBase.ShouldAcceptReassignedTarget to reject targets
that are null or terminated by adding the target.IsTerminated condition to its
early return. In InterceptorBase.AssignSubInterceptor, remove the redundant
!target.IsTerminated check and rely on EvaluateReassignedTarget and the
centralized validation; leave the IADS call site unchanged.
- Line 1: Add an early return when CommsManager.Instance is null in
SendAssignTargetRequest, SendAssignTargetResponse, SendReassignTargetRequest,
and AssignSubInterceptor before calling its SendMessage method. Apply the guard
in InterceptorBase and IADS while preserving the existing message-sending
behavior when the singleton is available.
🪄 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 Plus
Run ID: 846b6097-3089-4ef9-9077-286f6b10a950
📒 Files selected for processing (8)
Assets/Scripts/IADS/IADS.csAssets/Scripts/Interceptors/CarrierBase.csAssets/Scripts/Interceptors/IInterceptor.csAssets/Scripts/Interceptors/InterceptorBase.csAssets/Scripts/Managers/CommsManager.csAssets/StreamingAssets/Configs/Simulations/5_swarms_100_ucav.pbtxtAssets/Tests/EditMode/MailboxTests.csAssets/Tests/EditMode/MailboxTests.cs.meta
| _parentCommsNode = parentCommsNode; | ||
| } | ||
|
|
||
| private void SendAssignTargetRequest(IInterceptor subInterceptor) { |
There was a problem hiding this comment.
doesn't the IADS need such functionalities too?
There was a problem hiding this comment.
Why would you need to store parent if IADS is root node?
c77aec4 to
7ae8ef5
Compare
b571747 to
9585d73
Compare
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 (4)
Assets/Scripts/Interceptors/InterceptorBase.cs (2)
1-1: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove the
IsTerminatedcheck intoShouldAcceptReassignedTarget.ShouldAcceptReassignedTargetonly checkstarget == null. Every caller that needs to reject a terminated target currently has to compensate locally, and one caller (IADS.AssignSubInterceptor) doesn't compensate at all. Fixing the check once inShouldAcceptReassignedTargetremoves the duplication and closes the gap forEvaluateReassignedTarget's mailbox-delayed path. This restates a previously raised major issue that remains unresolved.
Assets/Scripts/Interceptors/InterceptorBase.cs#L111-139: add|| target.IsTerminatedto the null check inShouldAcceptReassignedTargetsoEvaluateReassignedTarget(called on delayedAssignTargetResponseMessagedelivery) rejects targets that terminated during message transit.Assets/Scripts/Interceptors/InterceptorBase.cs#L141-157: once the check is centralized, drop the now-redundant!target.IsTerminatedcheck fromAssignSubInterceptorand rely onShouldAcceptReassignedTargetalone.Assets/Scripts/IADS/IADS.cs#L164-171: no local change needed onceShouldAcceptReassignedTargetis fixed; this call site currently has no compensating check and depends entirely on the shared fix.🤖 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/Interceptors/InterceptorBase.cs` at line 1, Update ShouldAcceptReassignedTarget to reject both null and terminated targets, ensuring delayed reassignment validation covers targets that terminate during message transit. In AssignSubInterceptor, remove the redundant local !target.IsTerminated check and rely solely on ShouldAcceptReassignedTarget; leave the IADS call site unchanged.
1-1: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
CommsManager.Instancebefore everySendMessagecall.CommsManager.Instanceis a static property that can be null (or fake-null after teardown) at multiple call sites across both files; none of them check it before dispatching a message. This restates a previously raised major issue that remains unresolved.
Assets/Scripts/Interceptors/InterceptorBase.cs#L86-109: add aCommsManager.Instance == nullcheck (alongside the existingCommsNode/ParentCommsNode/target checks) inSendAssignTargetRequest,SendAssignTargetResponse, andSendReassignTargetRequestbefore callingSendMessage.Assets/Scripts/IADS/IADS.cs#L167-169: add the sameCommsManager.Instance == nullguard before callingSendMessageinAssignSubInterceptor.🤖 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/Interceptors/InterceptorBase.cs` at line 1, Add a CommsManager.Instance null or Unity fake-null guard to the validation checks in InterceptorBase methods SendAssignTargetRequest, SendAssignTargetResponse, and SendReassignTargetRequest before each SendMessage call, and add the same guard in IADS.AssignSubInterceptor. Preserve the existing CommsNode, ParentCommsNode, and target checks while ensuring no SendMessage executes without a valid CommsManager.Instance.Assets/Scripts/IADS/IADS.cs (1)
153-173: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
AssignSubInterceptorshares the missing guards flagged inInterceptorBase.cs.Three related gaps in this method:
CommsManager.Instance.SendMessage(...)(Lines 168-169) has no null-check onCommsManager.Instance, matching the same gap inInterceptorBase.cs.subInterceptor.ShouldAcceptReassignedTarget(target)(Line 167) accepts atargetfromFindNewTargetwithout checkingtarget.IsTerminated;ShouldAcceptReassignedTargetitself doesn't check termination either.- Unlike
InterceptorBase.SendAssignTargetResponse, this method doesn't checksubInterceptor.CommsNode == nullbefore building the message; if null,Mailbox.Send'sContainsNodecheck silently drops the message instead of surfacing a problem.Points 1 and 2 will be captured together with
InterceptorBase.csin cross-file consolidated comments.🤖 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/IADS/IADS.cs` around lines 153 - 173, Update AssignSubInterceptor to reject terminated results from FindNewTarget before calling ShouldAcceptReassignedTarget, and guard both CommsManager.Instance and subInterceptor.CommsNode before constructing or sending AssignTargetResponseMessage. Preserve the existing launcher ordering and reassignment flow for valid targets and communication dependencies.Assets/Scripts/Managers/CommsManager.cs (1)
12-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale comment on
_nodes.The comment says
_nodesis a "Map from agent to the communication node."_nodesis declared asHashSet<CommsNode>, not a map, and it stores nodes only, not agents. Update the comment to describe the actual set of registered communication nodes.📝 Proposed fix
- // Map from agent to the communication node. + // Set of registered communication nodes. private readonly HashSet<CommsNode> _nodes = new HashSet<CommsNode>();🤖 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/Managers/CommsManager.cs` around lines 12 - 13, Update the comment above the _nodes field to describe it as a set of registered communication nodes, removing the inaccurate map and agent terminology. Leave the HashSet<CommsNode> declaration unchanged.
♻️ Duplicate comments (2)
Assets/Scripts/Interceptors/InterceptorBase.cs (2)
111-139: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ShouldAcceptReassignedTargetstill doesn't reject terminated targets.
ShouldAcceptReassignedTargetonly checkstarget == null.EvaluateReassignedTarget(called fromHandleMessageon a mailbox-delayedAssignTargetResponseMessage) relies solely on this check, so a target that terminates during message transit can still be assigned toHierarchicalAgent.Target.AssignSubInterceptor(Line 149) still has to compensate with its own!target.IsTerminatedcheck, confirming the gap belongs inShouldAcceptReassignedTargetitself. This restates a previously raised major issue that remains unresolved, with a shared root cause inIADS.cs'sAssignSubInterceptor. See the consolidated comment for the shared fix.🐛 Proposed fix
public bool ShouldAcceptReassignedTarget(IHierarchical target) { // Continue searching for targets if no target was found. - if (target == null) { + if (target == null || target.IsTerminated) { return false; }🤖 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/Interceptors/InterceptorBase.cs` around lines 111 - 139, Update ShouldAcceptReassignedTarget to return false when the incoming target is null or terminated, before evaluating the current target or fractional speeds. Keep EvaluateReassignedTarget dependent on this shared validation so mailbox-delayed assignments cannot set a terminated target.
86-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
CommsManager.Instanceis still not guarded beforeSendMessage.
SendAssignTargetRequest,SendAssignTargetResponse, andSendReassignTargetRequestall callCommsManager.Instance.SendMessage(...)directly.Instanceis a static property that can be null (or fake-null, per Unity's destroyed-object semantics) after teardown or unusual initialization order. None of the existing guards in these three methods checkCommsManager.Instance. This restates a previously raised major issue that remains unresolved. This finding will be captured together with the equivalent gap inIADS.csin a consolidated comment.🤖 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/Interceptors/InterceptorBase.cs` around lines 86 - 109, Guard CommsManager.Instance before calling SendMessage in SendAssignTargetRequest, SendAssignTargetResponse, and SendReassignTargetRequest, treating Unity destroyed-object values as invalid alongside null. Preserve the existing argument and target validation, and return without sending whenever the manager instance is unavailable.
🤖 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/Interceptors/InterceptorBase.cs`:
- Around line 131-139: Change EvaluateReassignedTarget in InterceptorBase from
public to private, preserving its existing validation and target-assignment
behavior; it should remain callable by HandleMessage while preventing external
callers from bypassing the mailbox response path.
In `@Assets/StreamingAssets/Configs/Simulations/5_swarms_100_ucav.pbtxt`:
- Around line 298-303: Wire the communication configuration’s packet delivery
ratio through the Mailbox.Send path instead of hardcoding Mathf.Clamp01(1),
using config.PacketDeliveryRatio for delivery decisions. Ensure the base
packet_delivery_ratio and the overrides in the link configurations affect
simulation behavior; otherwise explicitly track this limitation as unresolved
for the PR.
In `@Assets/Tests/EditMode/MailboxTests.cs`:
- Around line 37-55: Extend MailboxTests with coverage for delivery to
unregistered or removed receivers, asserting those messages never arrive, and
add a packet-loss test using a packet_delivery_ratio below 1 once that
configuration is wired up. Reuse the existing CommsNode, _commsManager, and
TestMessage setup while preserving the exact-latency test.
---
Outside diff comments:
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 153-173: Update AssignSubInterceptor to reject terminated results
from FindNewTarget before calling ShouldAcceptReassignedTarget, and guard both
CommsManager.Instance and subInterceptor.CommsNode before constructing or
sending AssignTargetResponseMessage. Preserve the existing launcher ordering and
reassignment flow for valid targets and communication dependencies.
In `@Assets/Scripts/Interceptors/InterceptorBase.cs`:
- Line 1: Update ShouldAcceptReassignedTarget to reject both null and terminated
targets, ensuring delayed reassignment validation covers targets that terminate
during message transit. In AssignSubInterceptor, remove the redundant local
!target.IsTerminated check and rely solely on ShouldAcceptReassignedTarget;
leave the IADS call site unchanged.
- Line 1: Add a CommsManager.Instance null or Unity fake-null guard to the
validation checks in InterceptorBase methods SendAssignTargetRequest,
SendAssignTargetResponse, and SendReassignTargetRequest before each SendMessage
call, and add the same guard in IADS.AssignSubInterceptor. Preserve the existing
CommsNode, ParentCommsNode, and target checks while ensuring no SendMessage
executes without a valid CommsManager.Instance.
In `@Assets/Scripts/Managers/CommsManager.cs`:
- Around line 12-13: Update the comment above the _nodes field to describe it as
a set of registered communication nodes, removing the inaccurate map and agent
terminology. Leave the HashSet<CommsNode> declaration unchanged.
---
Duplicate comments:
In `@Assets/Scripts/Interceptors/InterceptorBase.cs`:
- Around line 111-139: Update ShouldAcceptReassignedTarget to return false when
the incoming target is null or terminated, before evaluating the current target
or fractional speeds. Keep EvaluateReassignedTarget dependent on this shared
validation so mailbox-delayed assignments cannot set a terminated target.
- Around line 86-109: Guard CommsManager.Instance before calling SendMessage in
SendAssignTargetRequest, SendAssignTargetResponse, and
SendReassignTargetRequest, treating Unity destroyed-object values as invalid
alongside null. Preserve the existing argument and target validation, and return
without sending whenever the manager instance is unavailable.
🪄 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 Plus
Run ID: 69354c6b-eee5-4c15-987c-42d4be4c7b7c
📒 Files selected for processing (9)
Assets/Scripts/IADS/IADS.csAssets/Scripts/Interceptors/CarrierBase.csAssets/Scripts/Interceptors/IInterceptor.csAssets/Scripts/Interceptors/InterceptorBase.csAssets/Scripts/Managers/CommsManager.csAssets/StreamingAssets/Configs/Simulations/5_swarms_100_ucav.pbtxtAssets/Tests/EditMode/MailboxTests.csAssets/Tests/EditMode/MailboxTests.cs.metaPisterlab_Micromissile.slnx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/Scripts/Interceptors/InterceptorBase.cs (1)
203-223: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound in-flight assignment and reassignment requests.
While
HierarchicalAgent.Targetis missing or all targets are escaping, these conditions remain true acrossFixedUpdatecalls. The code can enqueue the same logical request repeatedly before a mailbox response arrives. Add pending state per interceptor or target, clear it when the request completes, and use a bounded timeout/retry path for delivery failures. (raw.githubusercontent.com)🤖 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/Interceptors/InterceptorBase.cs` around lines 203 - 223, Update the target-assignment flow around RequestReassignment and SendReassignTargetRequest to track pending requests per interceptor or target, suppress duplicate enqueues while a request is in flight, and clear the pending state when the mailbox response completes. Add bounded timeout and retry handling for delivery failures, while preserving reassignment when the target is missing, terminated, or all active targets are escaping.Source: MCP tools
🤖 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.
Outside diff comments:
In `@Assets/Scripts/Interceptors/InterceptorBase.cs`:
- Around line 203-223: Update the target-assignment flow around
RequestReassignment and SendReassignTargetRequest to track pending requests per
interceptor or target, suppress duplicate enqueues while a request is in flight,
and clear the pending state when the mailbox response completes. Add bounded
timeout and retry handling for delivery failures, while preserving reassignment
when the target is missing, terminated, or all active targets are escaping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34a2db87-8a0a-4e4b-8e5b-2311a61301ab
📒 Files selected for processing (2)
Assets/Scripts/Interceptors/InterceptorBase.csAssets/Tests/EditMode/MailboxTests.cs
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/Tests/EditMode/MailboxTests.cs (1)
56-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a receiver removed after queuing, not just never registered.
SendMessage_DoesNotDeliverToUnregisteredReceivernever calls_commsManager.AddNode(receiver). This means the message is dropped atMailbox.Send's existingContainsNodecheck before it is ever queued. It does not exercise the changedDeliverguard at Mailbox.cs Lines 52-54, which specifically protects against a receiver that is removed fromCommsManagerafter a message has already been queued with a futuredeliverAt.Add a test that: registers both
senderandreceiver, sends a message with non-zero latency so it is queued, removes the receiver's node beforeElapsedTimereachesdeliverAt(for example through reflection on the private_nodesfield, sinceCommsManagerhas no publicRemoveNode), then asserts the message is not delivered onFixedUpdate.🧪 Illustrative test outline
[Test] public void SendMessage_DoesNotDeliverToRemovedReceiver() { var sender = new CommsNode(Configs.AgentType.Vessel); var receiver = new CommsNode(Configs.AgentType.Iads); _commsManager.AddNode(sender); _commsManager.AddNode(receiver); Message receivedMessage = null; receiver.OnReceived += message => receivedMessage = message; _commsManager.SendMessage(new TestMessage(sender, receiver)); // Remove the receiver's node before the queued message becomes due. GetPrivateField<HashSet<CommsNode>>(_commsManager, "_nodes").Remove(receiver); SetPrivateProperty(_simManager, "ElapsedTime", 1f); InvokePrivateMethod(_commsManager, "FixedUpdate"); Assert.IsNull(receivedMessage); }🤖 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 56 - 71, Add a separate test near SendMessage_DoesNotDeliverToUnregisteredReceiver that registers both sender and receiver, queues a message with non-zero latency, removes receiver from CommsManager’s private _nodes collection before delivery, then invokes FixedUpdate and asserts the receiver callback was not triggered. Use the existing private-field reflection helper and preserve the current test for a never-registered receiver.
🤖 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/Interceptors/InterceptorBase.cs`:
- Around line 394-408: Guard CommsManager.Instance before every SendMessage
call: update SendAssignTargetRequest, SendAssignTargetResponse, and
SendReassignTargetRequest in Assets/Scripts/Interceptors/InterceptorBase.cs
(lines 394-408), plus AssignSubInterceptor and ReassignTarget in
Assets/Scripts/IADS/IADS.cs (lines 166-168 and 188-189), so each helper skips
sending when the singleton is null or Unity fake-null.
- Around line 90-98: Add pending-request tracking in InterceptorBase around
FixedUpdate and the leaf-target reassignment path near
RequestReassignment(this). Set the flag when sending a reassignment request,
skip subsequent requests while one is outstanding, and clear it when
EvaluateReassignedTarget or the response-handling path evaluates the response or
assigns a target, preserving requests once no request remains pending.
- Around line 265-280: Add an IsTerminated early-return check for
sub-interceptor requesters before target allocation in AssignSubInterceptor
within Assets/Scripts/Interceptors/InterceptorBase.cs (lines 265-280), alongside
the existing null and capacity guard. Apply the same guard in the IADS
implementation within Assets/Scripts/IADS/IADS.cs (lines 152-172), before
computing launcher targets.
In `@Assets/Tests/TestBase.cs`:
- Around line 7-12: Update SetSingleton<T> to store the result of GetProperty
for the public static Instance property, validate that the property was found,
and throw the same descriptive Exception pattern used by GetFieldInfo,
GetMethodInfo, and GetPropertyInfo before calling SetValue.
---
Outside diff comments:
In `@Assets/Tests/EditMode/MailboxTests.cs`:
- Around line 56-71: Add a separate test near
SendMessage_DoesNotDeliverToUnregisteredReceiver that registers both sender and
receiver, queues a message with non-zero latency, removes receiver from
CommsManager’s private _nodes collection before delivery, then invokes
FixedUpdate and asserts the receiver callback was not triggered. Use the
existing private-field reflection helper and preserve the current test for a
never-registered receiver.
🪄 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 Plus
Run ID: 1b098833-1b8f-4712-99ff-d2fec8f51e5f
📒 Files selected for processing (8)
Assets/Scripts/Communication/Mailbox.csAssets/Scripts/IADS/IADS.csAssets/Scripts/Interceptors/CarrierBase.csAssets/Scripts/Interceptors/IInterceptor.csAssets/Scripts/Interceptors/InterceptorBase.csAssets/Scripts/Managers/CommsManager.csAssets/Tests/EditMode/MailboxTests.csAssets/Tests/TestBase.cs
This PR links the mailbox-communication system with Agents - InterceptorBase, IADS, and CarrierBase.
Changes
5_swarms_100_ucavexample.Testing
Added mailbox tests verifying:
**Note
Packet delivery ratio is present (logic is flawed), but packet loss is not enabled yet - "Mailbox" currently uses a delivery ratio of "1" (Everything is delivered).