[Communication] PR5: IADS.cs now supports Mailbox#217
Conversation
|
🧱 Stack PR · Part of stack (9 PRs total) Stack Structure:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the IADS system to use a proxy-based communication model via the Mailbox system, replacing direct method calls with asynchronous messaging for interceptor and target assignments. While the architectural shift is clear, the implementation contains several critical issues: the removal of asset registration logic breaks the hierarchy building process, a missing closing brace in the ReassignTarget method causes a compilation error, and logic regressions in AssignSubInterceptor ignore remaining capacity and target evaluation. Furthermore, redundant registration calls for launchers should be streamlined to improve code clarity.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIADS gains mailbox-based inter-agent coordination for interceptor assignment and target reassignment. Lifecycle methods (Awake/Start/OnDestroy) now initialize an IadsCommsAgent, conditionally subscribe to SimManager events, and establish mailbox subscriptions. Asset/launcher/threat registration is hardened with null-guards and duplicate prevention. New mailbox message routing dispatches AssignTargetRequestMessage and ReassignTargetRequestMessage to handlers that select appropriate launchers and send responses back through the mailbox. Comprehensive test coverage validates three mailbox delivery scenarios using reflection-injected stubs. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 57-64: The loop over SimManager.Instance.Interceptors calls
RegisterNewAsset for every interceptor and then calls RegisterNewLauncher which
also calls RegisterNewAsset for LauncherBase instances, causing duplicate
registration; modify the loop in IADS.cs so that for interceptors that are
LauncherBase you only call RegisterNewLauncher(interceptor) (skip the prior
RegisterNewAsset call), and for all other IInterceptor instances call
RegisterNewAsset(interceptor) as before—identify the code by the foreach over
SimManager.Instance.Interceptors, the RegisterNewAsset method, the
RegisterNewLauncher method, and the LauncherBase type to implement this
conditional change.
- Around line 228-230: The if statement checking closestLauncher.Interceptor is
missing its closing brace; add a closing "}" immediately after the
SendMessage(...) call to properly terminate the if block that contains
SendMessage(new ReassignTargetRequestMessage(_commsAgent,
closestLauncher.Interceptor, target)); ensure indentation is corrected so the
method's existing closing brace still closes the method and not the if block
(look for the if (closestLauncher.Interceptor != null) { ... } around the
SendMessage call).
- Around line 25-29: The two fields 'private IadsCommsAgent _commsAgent;' and
'private bool _mailboxSubscribed = false;' in IADS.cs are indented with 4 spaces
while the file uses 2-space indentation; update their leading indentation to
match the file style (2 spaces) so the declarations for
IadsCommsAgent/_commsAgent and _mailboxSubscribed align with the rest of the
file.
- Around line 232-237: The method SendMessage shadows Unity's
Component.SendMessage; rename this method to SendMailboxMessage and update all
internal call sites that currently call SendMessage to call SendMailboxMessage
instead (preserve the parameter type Message and behavior). Locate the method
body using the existing identifiers (SendMessage, Mailbox.GetOrCreateInstance,
mailbox.Send) and perform the rename, then update any references inside this
class that invoke SendMessage to use SendMailboxMessage so Unity's API is not
shadowed.
- Around line 204-209: Override InterceptorBase.OnMessage to handle
AssignTargetMessage: detect when the incoming message is an AssignTargetMessage,
verify it targets this interceptor (compare message.SubInterceptor or target
owner if present), extract the IHierarchical target from the message and assign
it to the interceptor by calling the existing assign method or setting the
interceptor's target property (e.g., call AssignTarget/SetTarget or set
Target/HierarchicalAgent.Target on this interceptor). After handling, skip base
processing/return so the message is not discarded. Ensure you reference the
AssignTargetMessage type and InterceptorBase.OnMessage in your 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1253d2b1-5db2-473d-bc7b-fb19f92d3961
📒 Files selected for processing (1)
Assets/Scripts/IADS/IADS.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Scripts/IADS/IADS.cs`:
- Line 50: The if-statement using SimManager.Instance in IADS.cs is indented
with 6 spaces instead of the file's 2-space style; locate the line containing
"if (SimManager.Instance != null) {" and adjust its leading whitespace to 2
spaces to match the surrounding code style so the file's indentation is
consistent.
🪄 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: f2676bee-aff4-4164-8fa7-3e6b31fd1e39
📒 Files selected for processing (4)
Assets/Scripts/Agents/Messaging/Mailbox.cs.metaAssets/Scripts/Agents/Messaging/PendingMessage.cs.metaAssets/Scripts/IADS/IADS.csAssets/Scripts/IADS/IadsCommsAgent.cs.meta
Joseph0120
force-pushed
the
joseph/communication_delay_PR4
branch
from
f132eca to
0edbb50
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR5
branch
from
a15710f to
833b973
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 205-217: AssignSubInterceptor currently accesses
subInterceptor.HierarchicalAgent without a null check which can cause crashes
during mailbox/assignment; add a guard at the start of AssignSubInterceptor to
return early if subInterceptor.HierarchicalAgent is null (similar to existing
checks for _commsAgent and subInterceptor.CapacityRemaining), and ensure callers
of launcher.FindNewTarget(subInterceptor.HierarchicalAgent,
subInterceptor.Capacity) only run when HierarchicalAgent is non-null so
FindNewTarget is never invoked with a null agent.
- Line 138: Check for a null threat before dereferencing its HierarchicalAgent:
update the conditional that currently reads use of threat.HierarchicalAgent so
it first ensures threat != null, then threat.HierarchicalAgent != null, and then
checks !_newThreats.Contains(threat.HierarchicalAgent); this prevents a
NullReferenceException when an upstream event passes a null threat (references:
symbol threat, property HierarchicalAgent, collection _newThreats).
- Around line 132-134: The launcher delegate subscriptions are added each time a
launcher is registered causing duplicate calls after resets; before adding
handlers for launcher.OnAssignSubInterceptor and launcher.OnReassignTarget (and
before adding launcher.HierarchicalAgent to _launchers) either unsubscribe
existing handlers or ensure idempotency by performing
"launcher.OnAssignSubInterceptor -= AssignSubInterceptor" and
"launcher.OnReassignTarget -= ReassignTarget" prior to the "+=" calls, and also
remove/unsubscribe these handlers when launchers are unregistered or when the
simulation resets to prevent duplicate routing.
🪄 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: ddef23c7-0440-406d-9963-20e556e3b8e5
📒 Files selected for processing (5)
Assets/Scripts/Agents/Messaging/Mailbox.cs.metaAssets/Scripts/Agents/Messaging/PendingMessage.cs.metaAssets/Scripts/IADS/IADS.csAssets/Scripts/IADS/IadsCommsAgent.cs.metaAssets/Tests/EditMode/IADSMailboxTests.cs
Joseph0120
force-pushed
the
joseph/communication_delay_PR4
branch
from
51491dc to
a2a4c9e
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR5
branch
from
9fa2435 to
83f3218
Compare
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/IADS/IADS.cs (1)
218-236:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing null guard for
subInterceptor.HierarchicalAgent.Line 230 passes
subInterceptor.HierarchicalAgenttoFindNewTarget, but the guard at line 219 doesn't check if it's null. IfHierarchicalAgentis null, this could cause unexpected behavior inFindNewTarget.🛠️ Proposed fix
private void AssignSubInterceptor(IInterceptor subInterceptor) { - if (subInterceptor == null || subInterceptor.CapacityRemaining <= 0) { + if (subInterceptor == null || subInterceptor.HierarchicalAgent == null || + subInterceptor.CapacityRemaining <= 0) { 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/IADS/IADS.cs` around lines 218 - 236, AssignSubInterceptor currently uses subInterceptor.HierarchicalAgent without guarding it; add a null check for subInterceptor.HierarchicalAgent before calling launcher.FindNewTarget (or skip that launcher) to avoid passing null into FindNewTarget. Specifically, in AssignSubInterceptor ensure subInterceptor.HierarchicalAgent != null (either return early or continue the loop) before invoking launcher.FindNewTarget(subInterceptor.HierarchicalAgent, subInterceptor.CapacityRemaining), keeping the existing behavior that still evaluates the returned target with subInterceptor.EvaluateReassignedTarget.
♻️ Duplicate comments (1)
Assets/Scripts/IADS/IADS.cs (1)
58-65: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRedundant
RegisterNewAssetcall for launcher interceptors persists.For
LauncherBaseinterceptors,RegisterNewAssetis called at line 60, thenRegisterNewLauncherat line 62 callsRegisterNewAssetagain at line 132. While harmless due to the duplicate guard inRegisterNewAsset, this is wasteful.♻️ Proposed fix
foreach (var agent in SimManager.Instance.Interceptors) { if (agent is IInterceptor interceptor) { - RegisterNewAsset(interceptor); if (interceptor is LauncherBase) { RegisterNewLauncher(interceptor); + } else { + RegisterNewAsset(interceptor); } } }🤖 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 58 - 65, In the SimManager.Instance.Interceptors loop avoid calling RegisterNewAsset twice for LauncherBase interceptors: detect launcher interceptors (interceptor is LauncherBase) and call RegisterNewLauncher for them, otherwise call RegisterNewAsset; alternatively remove the RegisterNewAsset call inside RegisterNewLauncher and ensure all callers of RegisterNewLauncher still result in asset registration — update the loop around RegisterNewAsset/RegisterNewLauncher and the methods RegisterNewLauncher/RegisterNewAsset (symbols: SimManager.Instance.Interceptors, IInterceptor, LauncherBase, RegisterNewAsset, RegisterNewLauncher) so a launcher asset is only registered once.
🤖 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/IADS/IADS.cs`:
- Around line 260-279: HandleMailboxAssignTargetRequest can pass a null
subInterceptor.HierarchicalAgent into launcher.FindNewTarget; add a null guard
so we don't call FindNewTarget with a null hierarchical agent. Update the
early-return condition in HandleMailboxAssignTargetRequest (or add a check
before the loop) to ensure subInterceptor.HierarchicalAgent != null (matching
the pattern used in AssignSubInterceptor), and if it's null simply
return/continue so FindNewTarget is only invoked with a valid hierarchical
agent.
In `@Assets/Tests/EditMode/IADSMailboxTests.cs`:
- Around line 13-30: The Mailbox singleton isn't being set before IADS.Start may
call TrySubscribeMailbox; after creating the Mailbox component in SetUp,
explicitly set the singleton (e.g., call the existing test helper
SetMailboxInstance(_mailbox) or assign Mailbox.Instance = _mailbox) so
Mailbox.GetOrCreateInstance()/FindFirstObjectByType<Mailbox>() is deterministic
when IADS.Start invokes TrySubscribeMailbox.
---
Outside diff comments:
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 218-236: AssignSubInterceptor currently uses
subInterceptor.HierarchicalAgent without guarding it; add a null check for
subInterceptor.HierarchicalAgent before calling launcher.FindNewTarget (or skip
that launcher) to avoid passing null into FindNewTarget. Specifically, in
AssignSubInterceptor ensure subInterceptor.HierarchicalAgent != null (either
return early or continue the loop) before invoking
launcher.FindNewTarget(subInterceptor.HierarchicalAgent,
subInterceptor.CapacityRemaining), keeping the existing behavior that still
evaluates the returned target with subInterceptor.EvaluateReassignedTarget.
---
Duplicate comments:
In `@Assets/Scripts/IADS/IADS.cs`:
- Around line 58-65: In the SimManager.Instance.Interceptors loop avoid calling
RegisterNewAsset twice for LauncherBase interceptors: detect launcher
interceptors (interceptor is LauncherBase) and call RegisterNewLauncher for
them, otherwise call RegisterNewAsset; alternatively remove the RegisterNewAsset
call inside RegisterNewLauncher and ensure all callers of RegisterNewLauncher
still result in asset registration — update the loop around
RegisterNewAsset/RegisterNewLauncher and the methods
RegisterNewLauncher/RegisterNewAsset (symbols: SimManager.Instance.Interceptors,
IInterceptor, LauncherBase, RegisterNewAsset, RegisterNewLauncher) so a launcher
asset is only registered once.
🪄 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: f5b2d25f-02a8-4770-bb05-c56a0ad1427d
📒 Files selected for processing (2)
Assets/Scripts/IADS/IADS.csAssets/Tests/EditMode/IADSMailboxTests.cs
tryuan99
left a comment
tryuan99
left a comment
There was a problem hiding this comment.
Please check every change in this PR and ensure that it's absolutely necessary. No need for extra checks and overly preventative code that makes readability worse. If a problem happens, it'll be logged and we can fix then :)
| private Mailbox _mailboxInstance; | ||
| private bool _mailboxSubscribed = false; |
| SimManager.Instance.OnNewAsset += RegisterNewAsset; | ||
| SimManager.Instance.OnNewLauncher += RegisterNewLauncher; | ||
| SimManager.Instance.OnNewThreat += RegisterNewThreat; | ||
| if (SimManager.Instance != null) { |
There was a problem hiding this comment.
why do you need this check now?
| SimManager.Instance.OnNewLauncher += RegisterNewLauncher; | ||
| SimManager.Instance.OnNewThreat += RegisterNewThreat; | ||
|
|
||
| // Just in case the simulation started before IADS subscribed to events. |
There was a problem hiding this comment.
will this ever happen? no need to add if it will never happen
| if (!_mailboxSubscribed) { | ||
| TrySubscribeMailbox(); |
There was a problem hiding this comment.
only call it once. if there's race condition, this doesn't fix the problem
2 participants
Modified Files:
IADS.cs -> IADS can now send, receive, handle messages, and subscriptions go through mailbox instead of straight to agents.