[Communication] PR6: InterceptorBase.cs now supports mailbox.#218
[Communication] PR6: InterceptorBase.cs now supports mailbox.#218Joseph0120 wants to merge 3 commits intojoseph/communication_delay_PR5from
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds three new Unity metadata files for script assets and refactors Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
🧱 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 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.
| if (subInterceptor.CapacityRemaining <= 0) { | ||
| return; | ||
| } | ||
| if (subInterceptor == null || subInterceptor.Capacity <= 0) { return; } |
There was a problem hiding this comment.
The check has been changed from CapacityRemaining to Capacity. Capacity represents the total potential capacity of the interceptor, whereas CapacityRemaining represents its current availability. If an interceptor is already at full capacity (i.e., CapacityRemaining <= 0), it should not be assigned more work. Using Capacity here could lead to an infinite loop of target requests if the interceptor is full but has a non-zero total capacity.
if (subInterceptor == null || subInterceptor.CapacityRemaining <= 0) { return; }| if (target != null) { | ||
| SendAssignTargetToSub(subInterceptor, target); | ||
| } else { | ||
| SendAssignRequestToParent(subInterceptor); | ||
| } | ||
| // Evaluate the new target and decide whether to continue searching for other targets. | ||
| if (!subInterceptor.EvaluateReassignedTarget(target)) { | ||
| // Propagate the sub-interceptor target assignment to the parent interceptor above. | ||
| OnAssignSubInterceptor?.Invoke(subInterceptor); | ||
| SendReassignRequestToParent(target); | ||
| } |
There was a problem hiding this comment.
The updated logic in AssignSubInterceptor has a few issues:
- If
targetisnull, it sends an assignment request to the parent (line 119) but then proceeds to callSendReassignRequestToParent(null)at line 124 (which is a no-op but redundant). - If a non-null
targetis rejected by the sub-interceptor (line 122), the original behavior of requesting a new target for that sub-interceptor is lost, as it only sends a reassignment request for the target. - There is a mix of synchronous state modification (
subInterceptor.EvaluateReassignedTarget) and asynchronous messaging (SendAssignTargetToSub).
To maintain the original behavior while correctly handling the new mailbox communication, consider this refactoring:
if (target != null) {
SendAssignTargetToSub(subInterceptor, target);
}
// Evaluate the new target and decide whether to continue searching for other targets.
if (!subInterceptor.EvaluateReassignedTarget(target)) {
// Propagate the sub-interceptor target assignment to the parent interceptor above.
SendAssignRequestToParent(subInterceptor);
SendReassignRequestToParent(target);
}|
|
||
| // Request a new target from the parent interceptor. | ||
| OnAssignSubInterceptor?.Invoke(interceptor); | ||
| SendAssignRequestToParent(interceptor);; |
There was a problem hiding this comment.
Detected a double semicolon at the end of the line.
SendAssignRequestToParent(interceptor);| if (interceptor.IsReassignable) { | ||
| // Request a new target from the parent interceptor. | ||
| OnAssignSubInterceptor?.Invoke(interceptor); | ||
| SendAssignRequestToParent(interceptor);; |
There was a problem hiding this comment.
Detected a double semicolon at the end of the line.
SendAssignRequestToParent(interceptor);
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
|
|
||
| // Request a new target from the parent interceptor. | ||
| OnAssignSubInterceptor?.Invoke(interceptor); | ||
| SendAssignRequestToParent(interceptor);; |
There was a problem hiding this comment.
Remove duplicate semicolon.
There's a double semicolon (;;) at the end of this line, which creates an unnecessary empty statement.
Proposed fix
- SendAssignRequestToParent(interceptor);;
+ SendAssignRequestToParent(interceptor);📝 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.
| SendAssignRequestToParent(interceptor);; | |
| SendAssignRequestToParent(interceptor); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Interceptors/InterceptorBase.cs` at 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.
| if (interceptor.IsReassignable) { | ||
| // Request a new target from the parent interceptor. | ||
| OnAssignSubInterceptor?.Invoke(interceptor); | ||
| SendAssignRequestToParent(interceptor);; |
There was a problem hiding this comment.
Remove duplicate semicolon.
Same issue as line 287 - there's a double semicolon.
Proposed fix
- SendAssignRequestToParent(interceptor);;
+ SendAssignRequestToParent(interceptor);📝 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.
| SendAssignRequestToParent(interceptor);; | |
| SendAssignRequestToParent(interceptor); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Interceptors/InterceptorBase.cs` at 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.
| // Execution happens here after recieving and reading message. PayloadData is read and handled. | ||
| protected override void OnMessage(Message message) { | ||
| if (message == null) { | ||
| return; | ||
| } | ||
| switch (message) { | ||
| case AssignSubInterceptorRequestMessage assignRequest: | ||
| AssignSubInterceptor(assignRequest.PayloadData.SubInterceptor); | ||
| break; | ||
| case ReassignTargetRequestMessage reassignRequest: | ||
| ReassignTarget(reassignRequest.PayloadData.Target); | ||
| break; | ||
| case AssignTargetMessage assignTarget: | ||
| EvaluateReassignedTarget(assignTarget.PayloadData.Target); | ||
| // EvaluateReassignedTarget. If it is false then bounce back: send a RequestReassignment upwards. | ||
| if (!EvaluateReassignedTarget(assignTarget.PayloadData.Target)) { | ||
| RequestReassignment(this); | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical bug: EvaluateReassignedTarget is called twice.
In the AssignTargetMessage case, EvaluateReassignedTarget is invoked twice:
- Line 412: Called unconditionally (modifies
HierarchicalAgent.Targetif accepted) - Line 414: Called again in the
ifcondition
The first call may accept and assign the target, causing the second call to always return false (since the target is now current). This leads to unnecessary RequestReassignment calls and potentially incorrect behavior.
Also, minor typo on line 399: "recieving" → "receiving".
Proposed fix
- // Execution happens here after recieving and reading message. PayloadData is read and handled.
+ // Execution happens here after receiving and reading message. PayloadData is read and handled.
protected override void OnMessage(Message message) {
if (message == null) {
return;
}
switch (message) {
case AssignSubInterceptorRequestMessage assignRequest:
AssignSubInterceptor(assignRequest.PayloadData.SubInterceptor);
break;
case ReassignTargetRequestMessage reassignRequest:
ReassignTarget(reassignRequest.PayloadData.Target);
break;
case AssignTargetMessage assignTarget:
- EvaluateReassignedTarget(assignTarget.PayloadData.Target);
- // EvaluateReassignedTarget. If it is false then bounce back: send a RequestReassignment upwards.
- if (!EvaluateReassignedTarget(assignTarget.PayloadData.Target)) {
+ // If target is rejected, bounce back: send a RequestReassignment upwards.
+ if (!EvaluateReassignedTarget(assignTarget.PayloadData.Target)) {
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.
| // Execution happens here after recieving and reading message. PayloadData is read and handled. | |
| protected override void OnMessage(Message message) { | |
| if (message == null) { | |
| return; | |
| } | |
| switch (message) { | |
| case AssignSubInterceptorRequestMessage assignRequest: | |
| AssignSubInterceptor(assignRequest.PayloadData.SubInterceptor); | |
| break; | |
| case ReassignTargetRequestMessage reassignRequest: | |
| ReassignTarget(reassignRequest.PayloadData.Target); | |
| break; | |
| case AssignTargetMessage assignTarget: | |
| EvaluateReassignedTarget(assignTarget.PayloadData.Target); | |
| // EvaluateReassignedTarget. If it is false then bounce back: send a RequestReassignment upwards. | |
| if (!EvaluateReassignedTarget(assignTarget.PayloadData.Target)) { | |
| RequestReassignment(this); | |
| } | |
| break; | |
| } | |
| } | |
| // Execution happens here after receiving and reading message. PayloadData is read and handled. | |
| protected override void OnMessage(Message message) { | |
| if (message == null) { | |
| return; | |
| } | |
| switch (message) { | |
| case AssignSubInterceptorRequestMessage assignRequest: | |
| AssignSubInterceptor(assignRequest.PayloadData.SubInterceptor); | |
| break; | |
| case ReassignTargetRequestMessage reassignRequest: | |
| ReassignTarget(reassignRequest.PayloadData.Target); | |
| break; | |
| case AssignTargetMessage assignTarget: | |
| // If target is rejected, bounce back: send a RequestReassignment upwards. | |
| if (!EvaluateReassignedTarget(assignTarget.PayloadData.Target)) { | |
| RequestReassignment(this); | |
| } | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Interceptors/InterceptorBase.cs` around lines 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.
2223dc0 to
9f5782b
Compare
1 participant
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.