[Communication] PR6: InterceptorBase.cs now supports mailbox.#218
Open
Joseph0120 wants to merge 12 commits into
Open
[Communication] PR6: InterceptorBase.cs now supports mailbox.#218Joseph0120 wants to merge 12 commits into
Joseph0120 wants to merge 12 commits into
Conversation
📝 WalkthroughWalkthroughThis change adds a public CommsParent to InterceptorBase and replaces direct OnAssignSubInterceptor/OnReassignTarget invocations with mailbox request/response messaging routed via CommsParent, with fallbacks to original events when CommsParent is null. IADS and CarrierBase now initialize CommsParent for registered/released interceptors. New edit-mode NUnit tests validate mailbox delivery, forwarding to parent, response handling, and unassigned-target queueing. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
This was referenced Apr 1, 2026
|
🧱 Stack PR · Part of stack (9 PRs total) Stack Structure:
|
This was referenced Apr 1, 2026
![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 transitions the InterceptorBase class from an event-driven model to a mailbox-based messaging system for target assignments. Key changes include the introduction of a CommsParent property, message-sending helper methods, and an OnMessage handler to process incoming requests. Review feedback identifies significant logic issues in the AssignSubInterceptor method, specifically regarding incorrect capacity checks and the handling of target reassignment requests, alongside minor syntax errors like double semicolons.
Joseph0120
changed the title
![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
🤖 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/Interceptors/InterceptorBase.cs`:
- Line 287: The call to SendAssignRequestToParent(interceptor) in class
InterceptorBase contains a duplicate semicolon creating an empty statement; edit
the method containing that call (look for SendAssignRequestToParent(interceptor)
in InterceptorBase.cs) and remove the extra trailing semicolon so the line reads
a single statement terminated by one semicolon, then rebuild to ensure no
warnings remain.
- Line 313: Remove the stray duplicate semicolon after the call to
SendAssignRequestToParent(interceptor) in InterceptorBase (the extra ";" should
be deleted so the line reads SendAssignRequestToParent(interceptor);); also scan
the other occurrence of SendAssignRequestToParent calls (the one noted similarly
earlier) and ensure no extra semicolons remain to keep consistent formatting.
- Around line 399-419: The AssignTargetMessage branch in OnMessage calls
EvaluateReassignedTarget(assignTarget.PayloadData.Target) twice causing
side-effects on HierarchicalAgent.Target and incorrect RequestReassignment
logic; change the code to call EvaluateReassignedTarget once, store its boolean
result in a local variable (e.g., accepted) and use that for the conditional
RequestReassignment(this) decision, and also fix the comment typo "recieving" to
"receiving". Ensure you update the OnMessage method's AssignTargetMessage case
and use the stored result rather than re-invoking EvaluateReassignedTarget.
🪄 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: bc71fed2-5750-40d8-952b-6888e10e6003
📒 Files selected for processing (4)
Assets/Scripts/Agents/Messaging/Mailbox.cs.metaAssets/Scripts/Agents/Messaging/PendingMessage.cs.metaAssets/Scripts/IADS/IadsCommsAgent.cs.metaAssets/Scripts/Interceptors/InterceptorBase.cs
Joseph0120
force-pushed
the
joseph/communication_delay_PR6
branch
from
2223dc0 to
9f5782b
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR5
branch
from
a15710f to
833b973
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR6
branch
from
9f5782b to
58c8cf2
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR6
branch
from
00d5c34 to
f321cad
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR5
branch
from
9fa2435 to
83f3218
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR6
branch
from
f321cad to
bf8fe5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 428-433: The AssignTargetResponseMessage handler currently assigns
assignedTarget directly to HierarchicalAgent.Target and thus skips the prior
accept/reject semantics; modify the handler so that when assignedTarget != null
you call EvaluateReassignedTarget on the sub-interceptor (the same evaluation
method used previously, e.g., EvaluateReassignedTarget) exactly once and only
set HierarchicalAgent.Target = assignedTarget if that method returns true;
ensure you reference the same EvaluateReassignedTarget on the interceptor
instance (not calling it twice) and preserve null-handling and the existing
break flow.
In `@Assets/Tests/EditMode/IADSMailboxTests.cs`:
- Around line 355-365: TestLauncherInterceptor duplicates TestInterceptor: both
inherit InterceptorBase, provide InvokeAwakeForTest() and the same IAgent stubs;
consolidate by extracting a single shared test interceptor (e.g., rename to
SharedTestInterceptor) or move one implementation into a shared test utilities
class and have both tests reference that type; update references in
EditMode/IADSMailboxTests.cs to use the shared class and remove the duplicate
TestLauncherInterceptor, ensuring the shared class exposes InvokeAwakeForTest()
and implements
IAgent.CreateTargetModel/IAgent.DestroyTargetModel/IAgent.UpdateTargetModel.
- Around line 195-224: Duplicate helper methods CreateSimManagerStub,
CreateCluster, SetMailboxInstance, SetSimManagerInstance (and SetIadsInstance)
are repeated across test files; consolidate them by moving these static helpers
into a shared test base or utility class (e.g., create MailboxTestBase :
TestBase or a MailboxTestHelper) and update InterceptorBaseMailboxTests and
IADSMailboxTests to inherit/use that class, keeping the same method signatures
so callers (CreateSimManagerStub, CreateCluster, SetMailboxInstance,
SetSimManagerInstance, SetIadsInstance) require no further changes.
- Around line 77-84: Add a brief comment explaining the two-tick message
processing pattern used in this test: note that the first
InvokePrivateMethod(_mailbox, "Update") processes the incoming
AssignTargetRequestMessage (leaving deliveredMessage null) and the second Update
delivers the outgoing AssignTargetResponseMessage (setting deliveredMessage and
its properties). Place the comment near the two Update() calls in
IADSMailboxTests.cs, mirroring the explanatory style used in
InterceptorBaseMailboxTests.cs, referencing deliveredMessage,
AssignTargetRequestMessage, and AssignTargetResponseMessage so future readers
understand the asynchronous two-tick flow.
In `@Assets/Tests/EditMode/InterceptorBaseMailboxTests.cs`:
- Around line 13-29: The test SetUp currently relies on reflection helpers
(SetMailboxInstance, SetSimManagerInstance, SetPrivateField) and uninitialized
objects to mutate Mailbox.Instance, SimManager.Instance and the private
_dummyAgents field; instead, modify the code under test to allow test-friendly
construction/injection (e.g., add internal/test-only constructors or a factory
for SimManager, expose an internal setter or InitializeForTest on Mailbox, and
provide a public method to set dummy agents) and update CreateSimManagerStub to
use those public/internal APIs rather than
FormatterServices.GetUninitializedObject and SetPrivateField; locate references
in this diff to SetUp, CreateSimManagerStub, SetMailboxInstance,
SetSimManagerInstance, SetPrivateField, _simManager and _dummyAgents and replace
reflection hacks with DI/factory or test-initializers so tests create fully
initialized instances without touching private fields or singletons via
reflection.
- Around line 102-106: Add an inline comment above the two consecutive
InvokePrivateMethod(_mailbox, "Update") calls explaining the two-tick message
processing pattern: note that the first Update() processes the incoming request
and enqueues or prepares the response, while the second Update() actually
delivers the response (hence the Assert.IsNull after the first call and
Assert.NotNull after the second). Reference the _mailbox instance and
deliveredMessage variable in the comment so future readers understand why both
Update() invocations are required.
🪄 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: 1131007f-7006-46aa-8652-882d5f176808
📒 Files selected for processing (5)
Assets/Scripts/IADS/IADS.csAssets/Scripts/Interceptors/CarrierBase.csAssets/Scripts/Interceptors/InterceptorBase.csAssets/Tests/EditMode/IADSMailboxTests.csAssets/Tests/EditMode/InterceptorBaseMailboxTests.cs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 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 428-433: The AssignTargetResponseMessage handler currently assigns
assignedTarget directly to HierarchicalAgent.Target and thus skips the prior
accept/reject semantics; modify the handler so that when assignedTarget != null
you call EvaluateReassignedTarget on the sub-interceptor (the same evaluation
method used previously, e.g., EvaluateReassignedTarget) exactly once and only
set HierarchicalAgent.Target = assignedTarget if that method returns true;
ensure you reference the same EvaluateReassignedTarget on the interceptor
instance (not calling it twice) and preserve null-handling and the existing
break flow.
In `@Assets/Tests/EditMode/IADSMailboxTests.cs`:
- Around line 355-365: TestLauncherInterceptor duplicates TestInterceptor: both
inherit InterceptorBase, provide InvokeAwakeForTest() and the same IAgent stubs;
consolidate by extracting a single shared test interceptor (e.g., rename to
SharedTestInterceptor) or move one implementation into a shared test utilities
class and have both tests reference that type; update references in
EditMode/IADSMailboxTests.cs to use the shared class and remove the duplicate
TestLauncherInterceptor, ensuring the shared class exposes InvokeAwakeForTest()
and implements
IAgent.CreateTargetModel/IAgent.DestroyTargetModel/IAgent.UpdateTargetModel.
- Around line 195-224: Duplicate helper methods CreateSimManagerStub,
CreateCluster, SetMailboxInstance, SetSimManagerInstance (and SetIadsInstance)
are repeated across test files; consolidate them by moving these static helpers
into a shared test base or utility class (e.g., create MailboxTestBase :
TestBase or a MailboxTestHelper) and update InterceptorBaseMailboxTests and
IADSMailboxTests to inherit/use that class, keeping the same method signatures
so callers (CreateSimManagerStub, CreateCluster, SetMailboxInstance,
SetSimManagerInstance, SetIadsInstance) require no further changes.
- Around line 77-84: Add a brief comment explaining the two-tick message
processing pattern used in this test: note that the first
InvokePrivateMethod(_mailbox, "Update") processes the incoming
AssignTargetRequestMessage (leaving deliveredMessage null) and the second Update
delivers the outgoing AssignTargetResponseMessage (setting deliveredMessage and
its properties). Place the comment near the two Update() calls in
IADSMailboxTests.cs, mirroring the explanatory style used in
InterceptorBaseMailboxTests.cs, referencing deliveredMessage,
AssignTargetRequestMessage, and AssignTargetResponseMessage so future readers
understand the asynchronous two-tick flow.
In `@Assets/Tests/EditMode/InterceptorBaseMailboxTests.cs`:
- Around line 13-29: The test SetUp currently relies on reflection helpers
(SetMailboxInstance, SetSimManagerInstance, SetPrivateField) and uninitialized
objects to mutate Mailbox.Instance, SimManager.Instance and the private
_dummyAgents field; instead, modify the code under test to allow test-friendly
construction/injection (e.g., add internal/test-only constructors or a factory
for SimManager, expose an internal setter or InitializeForTest on Mailbox, and
provide a public method to set dummy agents) and update CreateSimManagerStub to
use those public/internal APIs rather than
FormatterServices.GetUninitializedObject and SetPrivateField; locate references
in this diff to SetUp, CreateSimManagerStub, SetMailboxInstance,
SetSimManagerInstance, SetPrivateField, _simManager and _dummyAgents and replace
reflection hacks with DI/factory or test-initializers so tests create fully
initialized instances without touching private fields or singletons via
reflection.
- Around line 102-106: Add an inline comment above the two consecutive
InvokePrivateMethod(_mailbox, "Update") calls explaining the two-tick message
processing pattern: note that the first Update() processes the incoming request
and enqueues or prepares the response, while the second Update() actually
delivers the response (hence the Assert.IsNull after the first call and
Assert.NotNull after the second). Reference the _mailbox instance and
deliveredMessage variable in the comment so future readers understand why both
Update() invocations are required.
🪄 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: 1131007f-7006-46aa-8652-882d5f176808
📒 Files selected for processing (5)
Assets/Scripts/IADS/IADS.csAssets/Scripts/Interceptors/CarrierBase.csAssets/Scripts/Interceptors/InterceptorBase.csAssets/Tests/EditMode/IADSMailboxTests.csAssets/Tests/EditMode/InterceptorBaseMailboxTests.cs
🛑 Comments failed to post (6)
Assets/Scripts/Interceptors/InterceptorBase.cs (1)
428-433:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAssignTargetResponseMessage handler bypasses target evaluation.
The
AssignTargetResponseMessagecase directly assigns the target toHierarchicalAgent.Targetwithout callingEvaluateReassignedTarget. This breaks the evaluation semantics that existed in the synchronous event-based flow, where the parent would callsub.EvaluateReassignedTarget(target)and respect the accept/reject decision.In the old flow (IADS.cs:235), the IADS calls
subInterceptor.EvaluateReassignedTarget(target)and only assigns if the sub-interceptor accepts. In the new mailbox flow, the sub-interceptor blindly accepts all assignments, even if the target has lower fractional speed than the current target or other evaluation criteria fail.The previous review comment correctly identified that
EvaluateReassignedTargetwas being called twice, but the fix over-corrected by removing evaluation entirely instead of calling it once and checking the result.🔧 Proposed fix to restore evaluation semantics
case AssignTargetResponseMessage assignTarget: IHierarchical assignedTarget = assignTarget.PayloadData.Target; - if (assignedTarget != null) { - HierarchicalAgent.Target = assignedTarget; + if (assignedTarget != null && !EvaluateReassignedTarget(assignedTarget)) { + // Target was rejected; request a new assignment from parent. + RequestReassignment(this); } break;📝 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.case AssignTargetResponseMessage assignTarget: IHierarchical assignedTarget = assignTarget.PayloadData.Target; if (assignedTarget != null && !EvaluateReassignedTarget(assignedTarget)) { // Target was rejected; request a new assignment from parent. RequestReassignment(this); } break;🤖 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 428 - 433, The AssignTargetResponseMessage handler currently assigns assignedTarget directly to HierarchicalAgent.Target and thus skips the prior accept/reject semantics; modify the handler so that when assignedTarget != null you call EvaluateReassignedTarget on the sub-interceptor (the same evaluation method used previously, e.g., EvaluateReassignedTarget) exactly once and only set HierarchicalAgent.Target = assignedTarget if that method returns true; ensure you reference the same EvaluateReassignedTarget on the interceptor instance (not calling it twice) and preserve null-handling and the existing break flow.Assets/Tests/EditMode/IADSMailboxTests.cs (3)
77-84: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Document the two-tick message processing pattern.
Similar to the tests in
InterceptorBaseMailboxTests.cs, this test invokesUpdate()twice. The first tick processes the incomingAssignTargetRequestMessage, and the second tick delivers the outgoingAssignTargetResponseMessage. Adding brief comments would clarify this asynchronous message flow.📝 Suggested documentation
+ // First update: IADS proxy processes incoming AssignTargetRequest InvokePrivateMethod(_mailbox, "Update"); Assert.IsNull(deliveredMessage); + // Second update: IADS proxy delivers outgoing AssignTargetResponse InvokePrivateMethod(_mailbox, "Update");📝 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.// First update: IADS proxy processes incoming AssignTargetRequest InvokePrivateMethod(_mailbox, "Update"); Assert.IsNull(deliveredMessage); // Second update: IADS proxy delivers outgoing AssignTargetResponse InvokePrivateMethod(_mailbox, "Update"); Assert.NotNull(deliveredMessage); Assert.AreSame(_commsAgent, deliveredMessage.Sender); Assert.AreSame(subInterceptor, deliveredMessage.Receiver); Assert.AreEqual(MessageType.AssignTargetResponse, deliveredMessage.Type);🤖 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/IADSMailboxTests.cs` around lines 77 - 84, Add a brief comment explaining the two-tick message processing pattern used in this test: note that the first InvokePrivateMethod(_mailbox, "Update") processes the incoming AssignTargetRequestMessage (leaving deliveredMessage null) and the second Update delivers the outgoing AssignTargetResponseMessage (setting deliveredMessage and its properties). Place the comment near the two Update() calls in IADSMailboxTests.cs, mirroring the explanatory style used in InterceptorBaseMailboxTests.cs, referencing deliveredMessage, AssignTargetRequestMessage, and AssignTargetResponseMessage so future readers understand the asynchronous two-tick flow.
195-224: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract duplicated helper methods to a shared test base class.
The methods
CreateSimManagerStub,CreateCluster,SetMailboxInstance, andSetSimManagerInstanceare duplicated verbatim inInterceptorBaseMailboxTests.cs. This violates DRY and creates maintenance burden—any bug fix or improvement must be applied in multiple locations.Consider extracting these helpers to a shared test utility class or enhancing the existing
TestBaseparent class.♻️ Suggested refactor
Create a shared
MailboxTestHelperclass or extendTestBase:public class MailboxTestBase : TestBase { protected static SimManager CreateSimManagerStub() { return (SimManager)FormatterServices.GetUninitializedObject(typeof(SimManager)); } protected static HierarchicalBase CreateCluster(params IHierarchical[] targets) { var cluster = new HierarchicalBase(); foreach (IHierarchical target in targets) { cluster.AddSubHierarchical(target); } return cluster; } protected static void SetMailboxInstance(Mailbox mailbox) { FieldInfo instanceField = typeof(Mailbox).GetField( "<Instance>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static); instanceField.SetValue(null, mailbox); } protected static void SetSimManagerInstance(SimManager simManager) { FieldInfo instanceField = typeof(SimManager) .GetField("<Instance>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static); instanceField.SetValue(null, simManager); } }Then both test classes can inherit from
MailboxTestBase.🤖 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/IADSMailboxTests.cs` around lines 195 - 224, Duplicate helper methods CreateSimManagerStub, CreateCluster, SetMailboxInstance, SetSimManagerInstance (and SetIadsInstance) are repeated across test files; consolidate them by moving these static helpers into a shared test base or utility class (e.g., create MailboxTestBase : TestBase or a MailboxTestHelper) and update InterceptorBaseMailboxTests and IADSMailboxTests to inherit/use that class, keeping the same method signatures so callers (CreateSimManagerStub, CreateCluster, SetMailboxInstance, SetSimManagerInstance, SetIadsInstance) require no further changes.
355-365: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consolidate duplicate test interceptor implementations.
TestLauncherInterceptoris identical toTestInterceptorinInterceptorBaseMailboxTests.cs(lines 209-219). Both extendInterceptorBase, exposeInvokeAwakeForTest(), and stub the sameIAgentmethods. Consider extracting this to a shared test utility or reusing one implementation across both test files.🤖 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/IADSMailboxTests.cs` around lines 355 - 365, TestLauncherInterceptor duplicates TestInterceptor: both inherit InterceptorBase, provide InvokeAwakeForTest() and the same IAgent stubs; consolidate by extracting a single shared test interceptor (e.g., rename to SharedTestInterceptor) or move one implementation into a shared test utilities class and have both tests reference that type; update references in EditMode/IADSMailboxTests.cs to use the shared class and remove the duplicate TestLauncherInterceptor, ensuring the shared class exposes InvokeAwakeForTest() and implements IAgent.CreateTargetModel/IAgent.DestroyTargetModel/IAgent.UpdateTargetModel.Assets/Tests/EditMode/InterceptorBaseMailboxTests.cs (2)
13-29: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
Consider reducing reliance on reflection for test setup.
The setup manipulates global singletons (
Mailbox.Instance,SimManager.Instance) and private fields (_dummyAgents) via reflection, and usesFormatterServices.GetUninitializedObjectto bypass normal object construction. This approach is fragile:
- Field/property name changes in production code will break all tests
- Bypassing constructors may hide initialization issues
- Singleton manipulation can create test interdependencies
Consider dependency injection or test-specific factory methods to create properly initialized objects without reflection.
🤖 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/InterceptorBaseMailboxTests.cs` around lines 13 - 29, The test SetUp currently relies on reflection helpers (SetMailboxInstance, SetSimManagerInstance, SetPrivateField) and uninitialized objects to mutate Mailbox.Instance, SimManager.Instance and the private _dummyAgents field; instead, modify the code under test to allow test-friendly construction/injection (e.g., add internal/test-only constructors or a factory for SimManager, expose an internal setter or InitializeForTest on Mailbox, and provide a public method to set dummy agents) and update CreateSimManagerStub to use those public/internal APIs rather than FormatterServices.GetUninitializedObject and SetPrivateField; locate references in this diff to SetUp, CreateSimManagerStub, SetMailboxInstance, SetSimManagerInstance, SetPrivateField, _simManager and _dummyAgents and replace reflection hacks with DI/factory or test-initializers so tests create fully initialized instances without touching private fields or singletons via reflection.
102-106: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Document the two-tick message processing pattern.
The test invokes
Update()twice with an assertion between them. The firstUpdate()processes the incoming request, and the second delivers the response. This two-tick delivery pattern is not obvious and would benefit from inline comments explaining why two update cycles are needed for the mailbox to fully process the request-response flow.📝 Suggested documentation
+ // First update processes the incoming AssignTargetRequest InvokePrivateMethod(_mailbox, "Update"); Assert.IsNull(deliveredMessage); + // Second update delivers the outgoing AssignTargetResponse InvokePrivateMethod(_mailbox, "Update");📝 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.// First update processes the incoming AssignTargetRequest InvokePrivateMethod(_mailbox, "Update"); Assert.IsNull(deliveredMessage); // Second update delivers the outgoing AssignTargetResponse InvokePrivateMethod(_mailbox, "Update"); Assert.NotNull(deliveredMessage);🤖 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/InterceptorBaseMailboxTests.cs` around lines 102 - 106, Add an inline comment above the two consecutive InvokePrivateMethod(_mailbox, "Update") calls explaining the two-tick message processing pattern: note that the first Update() processes the incoming request and enqueues or prepares the response, while the second Update() actually delivers the response (hence the Assert.IsNull after the first call and Assert.NotNull after the second). Reference the _mailbox instance and deliveredMessage variable in the comment so future readers understand why both Update() invocations are required.
Joseph0120
force-pushed
the
joseph/communication_delay_PR6
branch
from
bf8fe5a to
ea4e639
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 124-129: The CommsParent assignment to _commsAgent is happening
before the guard that skips pursuers, corrupting pursuer routing; update the
logic in the registration block so that InterceptorBase.CommsParent is only set
after verifying the asset has a non-null HierarchicalAgent, is not a pursuer
(asset.IsPursuer == false), and is not already registered in _assets — i.e.,
move the interceptorBase.CommsParent = _commsAgent assignment below or inside
the conditional that checks asset?.HierarchicalAgent, asset.IsPursuer, and
_assets.Contains(asset.HierarchicalAgent) (or wrap it with the same boolean
guard) so pursuers keep the CarrierBase-assigned CommsParent.
In `@Assets/Scripts/Interceptors/InterceptorBase.cs`:
- Around line 428-433: The AssignTargetResponseMessage case currently assigns
any non-null assignedTarget to HierarchicalAgent.Target without re-evaluating
it; update the handler to call EvaluateReassignedTarget(assignedTarget) exactly
once and only set HierarchicalAgent.Target = assignedTarget if that call returns
true (preserve the null check first), ensuring you remove any duplicate calls so
the reassignment is validated before overwriting an existing target.
🪄 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: 558673d6-17ab-41b0-88f6-0e47f621d546
📒 Files selected for processing (5)
Assets/Scripts/IADS/IADS.csAssets/Scripts/Interceptors/CarrierBase.csAssets/Scripts/Interceptors/InterceptorBase.csAssets/Tests/EditMode/IADSMailboxTests.csAssets/Tests/EditMode/InterceptorBaseMailboxTests.cs
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)
150-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThrottle repeated assignment requests to prevent mailbox flooding.
Line 151 can trigger
RequestReassignment(this)every physics tick, and Line 312 always enqueues a newAssignTargetRequestMessage. With mailbox latency/drop, this can create large duplicate backlogs and unnecessary parent work.Suggested fix (rate-limit with retry window)
+ private const float _assignRequestRetryPeriod = 0.25f; + private float _nextAssignRequestTime = 0f; + private void RequestReassignment(IInterceptor interceptor) { - if (interceptor.IsReassignable) { - // Request a new target from the parent interceptor. - SendAssignRequestToParent(interceptor); - } + if (!interceptor.IsReassignable) { + return; + } + if (Time.time < _nextAssignRequestTime) { + return; + } + SendAssignRequestToParent(interceptor); + _nextAssignRequestTime = Time.time + _assignRequestRetryPeriod; }Also applies to: 309-313
🤖 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 150 - 152, Prevent spamming RequestReassignment(this) and enqueued AssignTargetRequestMessage by adding a simple rate-limit/cooldown in InterceptorBase: track the last reassignment request time (or a retry window flag) on the interceptor instance (e.g., a private DateTime/float lastRequestTime or bool awaitingRetry) and only call RequestReassignment(this) or enqueue a new AssignTargetRequestMessage when the cooldown has elapsed; reset the flag when a successful assignment acknowledgement arrives or after the window expires. Apply the same guard around the code paths that currently enqueue AssignTargetRequestMessage so HierarchicalAgent.Target checks use the shared cooldown to avoid duplicate mailbox floods.
🤖 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 150-152: Prevent spamming RequestReassignment(this) and enqueued
AssignTargetRequestMessage by adding a simple rate-limit/cooldown in
InterceptorBase: track the last reassignment request time (or a retry window
flag) on the interceptor instance (e.g., a private DateTime/float
lastRequestTime or bool awaitingRetry) and only call RequestReassignment(this)
or enqueue a new AssignTargetRequestMessage when the cooldown has elapsed; reset
the flag when a successful assignment acknowledgement arrives or after the
window expires. Apply the same guard around the code paths that currently
enqueue AssignTargetRequestMessage so HierarchicalAgent.Target checks use the
shared cooldown to avoid duplicate mailbox floods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8144b28d-f2b2-46e5-ae41-155d269ac846
📒 Files selected for processing (5)
Assets/Scripts/IADS/IADS.csAssets/Scripts/Interceptors/InterceptorBase.csAssets/Tests/EditMode/IADSMailboxTests.csAssets/Tests/EditMode/IADSMailboxTests.cs.metaAssets/Tests/EditMode/InterceptorBaseMailboxTests.cs.meta
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
1 participant
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.
Files Modified:
InterceptorBase.cs
Description:
Replaced all ?.Invoke to SendRequest function calls at the bottom.
Added SendRequest Functions: SendAssignRequestToParent(IInterceptor subInterceptor), SendReassignRequestToParent(IHierarchical target), SendAssignTargetToSub(IInterceptor subInterceptor, IHierarchical target)
These SendRequest functions then call on SendMessage(Message message) that sends message into the mailbox.
When recieving messages it handles with OnMessage(Message message). OnMessage "opens/unpack" the messages and read and executes the information contained by the message envelopes.