Added autologin support for CSRO (ICCGame) & fixed inventory update#17
Conversation
|
Warning Review limit reached
More reviews will be available in 40 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR extends OasisBot to support Chinese game clients by adding startup binary patching, Chinese-specific gateway authentication with ticket generation and RC4 encryption, and modern protocol inventory update parsing with per-field update flags for both Chinese and other modern client variants. ChangesChinese client initialization and authentication
Modern client inventory updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 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
🤖 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 `@Library/RSBot.Core/Components/ClientManager.cs`:
- Around line 169-174: The startup currently proceeds past the Chinese-client
patch window causing unpatched clients; modify ClientManager startup so that
when Game.ClientType == GameClientType.Chinese the code blocks startup until
WaitForChineseClientPatchTargets succeeds and ApplyChineseClientPatch completes,
and if WaitForChineseClientPatchTargets times out or ApplyChineseClientPatch
fails, abort startup (throw or stop further initialization) rather than
continuing. Also wrap any transient module metadata reads used by
WaitForChineseClientPatchTargets (the module enumeration / metadata access it
performs) in a short retry loop with small delays and a bounded overall timeout
to handle module-read races during unpacking, surfacing the final failure to the
startup gate so the process won’t continue with an unpatched client.
In
`@Library/RSBot.Core/Network/Handler/Agent/Inventory/InventoryUpdateItemResponse.cs`:
- Around line 155-156: The packet parsing currently skips consuming the
MagParams payload when ModernItemUpdateFlag.MagParams is set but item.Record is
null, causing subsequent reads to be misaligned; remove the item.Record guard so
that when updateFlags.HasFlag(ModernItemUpdateFlag.MagParams) is true you always
call ReadMagicOptions(packet, item) (or call a null-safe variant of
ReadMagicOptions that consumes the fields even if item.Record is null) to ensure
the wire payload is always consumed and parsing stays in sync.
- Around line 164-165: The modern-path branch in InventoryUpdateItemResponse
currently calls packet.ReadByte() when
updateFlags.HasFlag(ModernItemUpdateFlag.ItemState) and discards the value;
change this to persist the parsed state into the item's State property (e.g.,
set item.State = the byte read) so modern clients do not keep stale state—locate
the conditional using updateFlags.HasFlag(ModernItemUpdateFlag.ItemState),
replace the discarded packet.ReadByte() with reading into a local/temporary and
assigning it to item.State (matching the legacy path behavior).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 463276ad-c99f-4c02-bb8d-eecb5e9eb9cc
📒 Files selected for processing (7)
Library/RSBot.Core/Components/ClientManager.csLibrary/RSBot.Core/Network/Handler/Agent/Inventory/InventoryUpdateItemResponse.csLibrary/RSBot.Core/Objects/Inventory/Item/ModernItemUpdateFlag.csPlugins/RSBot.General/Components/AutoLogin.csPlugins/RSBot.General/Components/ChineseGatewayLogin.csPlugins/RSBot.General/PacketHandler/GatewayLoginRequest.csPlugins/RSBot.General/PacketHandler/GatewayServerListResponse.cs
| if (Game.ClientType == GameClientType.Chinese) | ||
| { | ||
| var sroProcess = System.Diagnostics.Process.GetProcessById((int)pi.dwProcessId); | ||
| WaitForChineseClientPatchTargets(sroProcess, pi.hThread); | ||
| ApplyChineseClientPatch(sroProcess, pi); | ||
| } |
There was a problem hiding this comment.
Make Chinese patch readiness a hard startup gate (and handle module-read races).
Line 169-174 always continues startup even when Line 411-415 times out, so Chinese clients can resume unpatched. Also, Line 406-409 can throw while module metadata is transiently unreadable during unpack, which can abort startup.
Suggested fix
- WaitForChineseClientPatchTargets(sroProcess, pi.hThread);
+ if (!WaitForChineseClientPatchTargets(sroProcess, pi.hThread))
+ {
+ Log.Error("Chinese client patch: targets were not ready before timeout.");
+ return false;
+ }
ApplyChineseClientPatch(sroProcess, pi);
...
- private static void WaitForChineseClientPatchTargets(Process process, IntPtr hThread)
+ private static bool WaitForChineseClientPatchTargets(Process process, IntPtr hThread)
{
const int sliceMs = 8;
const int maxWaitMs = 5000;
...
- if (process.HasExited)
- return;
-
- process.Refresh();
-
- if (ChineseClientPatchTargetsReady(process.Handle, process.MainModule.BaseAddress.ToInt32()))
- return;
+ if (process.HasExited)
+ return false;
+
+ try
+ {
+ process.Refresh();
+ if (ChineseClientPatchTargetsReady(process.Handle, process.MainModule.BaseAddress.ToInt32()))
+ return true;
+ }
+ catch
+ {
+ // Module can be temporarily unreadable while unpacking; keep polling.
+ }
...
if (sw.ElapsedMilliseconds >= maxWaitMs)
{
Log.Warn("Chinese client patch: unpack timeout.");
- return;
+ return false;
}
}
}Also applies to: 390-417
🤖 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 `@Library/RSBot.Core/Components/ClientManager.cs` around lines 169 - 174, The
startup currently proceeds past the Chinese-client patch window causing
unpatched clients; modify ClientManager startup so that when Game.ClientType ==
GameClientType.Chinese the code blocks startup until
WaitForChineseClientPatchTargets succeeds and ApplyChineseClientPatch completes,
and if WaitForChineseClientPatchTargets times out or ApplyChineseClientPatch
fails, abort startup (throw or stop further initialization) rather than
continuing. Also wrap any transient module metadata reads used by
WaitForChineseClientPatchTargets (the module enumeration / metadata access it
performs) in a short retry loop with small delays and a bounded overall timeout
to handle module-read races during unpacking, surfacing the final failure to the
startup gate so the process won’t continue with an unpatched client.
| if (updateFlags.HasFlag(ModernItemUpdateFlag.MagParams) && item.Record != null) | ||
| ReadMagicOptions(packet, item); |
There was a problem hiding this comment.
Always consume MagParams when the flag is set.
Line 155 makes packet consumption depend on item.Record != null, but the magic-option payload is still present on the wire. If that guard fails, every later flagged read in this update shifts onto the wrong offset.
Suggested fix
- if (updateFlags.HasFlag(ModernItemUpdateFlag.MagParams) && item.Record != null)
+ if (updateFlags.HasFlag(ModernItemUpdateFlag.MagParams))
ReadMagicOptions(packet, item);📝 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.
| if (updateFlags.HasFlag(ModernItemUpdateFlag.MagParams) && item.Record != null) | |
| ReadMagicOptions(packet, item); | |
| if (updateFlags.HasFlag(ModernItemUpdateFlag.MagParams)) | |
| ReadMagicOptions(packet, item); |
🤖 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
`@Library/RSBot.Core/Network/Handler/Agent/Inventory/InventoryUpdateItemResponse.cs`
around lines 155 - 156, The packet parsing currently skips consuming the
MagParams payload when ModernItemUpdateFlag.MagParams is set but item.Record is
null, causing subsequent reads to be misaligned; remove the item.Record guard so
that when updateFlags.HasFlag(ModernItemUpdateFlag.MagParams) is true you always
call ReadMagicOptions(packet, item) (or call a null-safe variant of
ReadMagicOptions that consumes the fields even if item.Record is null) to ensure
the wire payload is always consumed and parsing stays in sync.
| if (updateFlags.HasFlag(ModernItemUpdateFlag.ItemState)) | ||
| packet.ReadByte(); |
There was a problem hiding this comment.
Persist the parsed item state instead of discarding it.
The legacy path writes this field into item.State, but the modern path just reads and drops it. That leaves modern clients with stale item state when ItemState is flagged.
Suggested fix
- if (updateFlags.HasFlag(ModernItemUpdateFlag.ItemState))
- packet.ReadByte();
+ if (updateFlags.HasFlag(ModernItemUpdateFlag.ItemState))
+ item.State = (InventoryItemState)packet.ReadByte();📝 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.
| if (updateFlags.HasFlag(ModernItemUpdateFlag.ItemState)) | |
| packet.ReadByte(); | |
| if (updateFlags.HasFlag(ModernItemUpdateFlag.ItemState)) | |
| item.State = (InventoryItemState)packet.ReadByte(); |
🤖 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
`@Library/RSBot.Core/Network/Handler/Agent/Inventory/InventoryUpdateItemResponse.cs`
around lines 164 - 165, The modern-path branch in InventoryUpdateItemResponse
currently calls packet.ReadByte() when
updateFlags.HasFlag(ModernItemUpdateFlag.ItemState) and discards the value;
change this to persist the parsed state into the item's State property (e.g.,
set item.State = the byte read) so modern clients do not keep stale state—locate
the conditional using updateFlags.HasFlag(ModernItemUpdateFlag.ItemState),
replace the discarded packet.ReadByte() with reading into a local/temporary and
assigning it to item.State (matching the legacy path behavior).
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements