[WIP] Feature : add a headless client suit in Maple2.TestClient#656
Open
zbian99 wants to merge 3 commits intoMS2Community:masterfrom
Open
[WIP] Feature : add a headless client suit in Maple2.TestClient#656zbian99 wants to merge 3 commits intoMS2Community:masterfrom
zbian99 wants to merge 3 commits intoMS2Community:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new Maple2.TestClient project (headless test client) with low-level Maple network layer, login and game clients, protocol packet builders, demo program, solution/project entries, and documentation; also adjusts Session constants visibility and removes a duplicate FIELD_KEY in GameSession. Changes
Sequence DiagramssequenceDiagram
participant App as TestClient App
participant Login as LoginClient
participant LoginSvr as LoginServer
participant Game as GameClient
participant GameSvr as GameServer
App->>Login: ConnectAsync()
Login->>LoginSvr: TCP + Version handshake
LoginSvr-->>Login: RequestVersion
Login->>LoginSvr: ResponseVersion
App->>Login: LoginAsync(username,password)
Login->>LoginSvr: Login request
LoginSvr-->>Login: LoginResponse + CharacterList
Login-->>App: LoginResult
App->>Login: SelectCharacterAsync(id)
Login->>LoginSvr: SelectCharacter
LoginSvr-->>Login: LoginToGame(token, ip, port, mapId)
Login-->>App: GameServerInfo
App->>Game: ConnectAsync(serverInfo, accountId, token)
Game->>GameSvr: TCP + Version handshake
GameSvr-->>Game: RequestVersion
Game->>GameSvr: ResponseVersion
Game->>GameSvr: ResponseKey
GameSvr-->>Game: FieldData (map, pos, objectId)
Game->>GameSvr: ResponseFieldEnter
GameSvr-->>Game: FieldAddUser / FieldNpcs
opt Combat flow
App->>Game: SpawnNpcAsync(npcId)
Game->>GameSvr: GM spawn command
GameSvr-->>Game: FieldAddUser (npc)
App->>Game: CastSkillAsync(skillId)
Game->>GameSvr: SkillUse(skillUid)
GameSvr-->>Game: SkillUse broadcast
App->>Game: AttackTargetAsync(skillUid, targetId)
Game->>GameSvr: SkillAttackTarget
GameSvr-->>Game: SkillDamage broadcast
end
App->>Game: StayAliveAsync(ct)
Game->>GameSvr: Heartbeat/TimeSync loop
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
Maple2.TestClient/Clients/GameClient.cs (2)
22-22:nextSkillUidinitialized to 1, first allocated UID is 2.
Interlocked.Incrementreturns the post-increment value, so the first skill UID will be2. Initialize to0if you want UIDs starting at1.Suggested fix
- private long nextSkillUid = 1; + private long nextSkillUid = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Clients/GameClient.cs` at line 22, The field nextSkillUid is initialized to 1 but Interlocked.Increment returns the value after increment, so the first allocated UID will be 2; change the initialization of nextSkillUid (the private long nextSkillUid field) to 0 so that the first Interlocked.Increment call yields 1, or alternatively adjust the allocation call to use Interlocked.Increment(ref nextSkillUid) - 1 if you intentionally want to keep the initial value; update the declaration accordingly to ensure UIDs start at 1.
135-135: Remove placeholder comments.
PLACEHOLDER_GAMECLIENT_ATTACK(line 135) andPLACEHOLDER_GAMECLIENT_HANDLERS(line 184) appear to be development scaffolding artifacts. Clean these up before merge.Also applies to: 184-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Clients/GameClient.cs` at line 135, Remove the development placeholder comments "PLACEHOLDER_GAMECLIENT_ATTACK" and "PLACEHOLDER_GAMECLIENT_HANDLERS" from GameClient.cs; locate the placeholder tokens in the file (the exact strings) and delete them or replace them with the intended implementation (e.g., real attack logic or handler registration) so no scaffolding markers remain, and ensure no dead references or TODO markers remain that would break compilation or confuse reviewers.Maple2.sln (1)
40-41: Inconsistent project type GUID.All other projects use the SDK-style GUID
{9A19103F-16F7-4668-BE54-9A1E7A4F7556}, but this entry uses the legacy C# GUID{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}. Both work, but the inconsistency suggests this was added by a different VS version or manually. Consider aligning to9A19103F-...for consistency.Suggested fix
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Maple2.TestClient", "Maple2.TestClient\Maple2.TestClient.csproj", "{DCE05B96-A01B-4FD7-9003-B0E45A12C72D}" +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Maple2.TestClient", "Maple2.TestClient\Maple2.TestClient.csproj", "{DCE05B96-A01B-4FD7-9003-B0E45A12C72D}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.sln` around lines 40 - 41, The Project entry for Maple2.TestClient uses the legacy C# project type GUID "{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}"; update that GUID to the SDK-style GUID "{9A19103F-16F7-4668-BE54-9A1E7A4F7556}" so the Project("{...}") line for "Maple2.TestClient" matches the other SDK-style projects and maintains consistency across the solution.Maple2.TestClient/Program.cs (1)
14-35: CLI argument parsing lacks error handling for malformed input.
int.Parse/short.Parseon Lines 15, 26, 29, 32 will throw an unhelpfulFormatExceptionif the user provides non-numeric values. Consider wrapping inint.TryParseor catching the exception with a user-friendly message.Example for port parsing
-int port = args.Length > 1 ? int.Parse(args[1]) : 20001; +int port = args.Length > 1 && int.TryParse(args[1], out int p) ? p : 20001;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Program.cs` around lines 14 - 35, The CLI parsing currently uses int.Parse/short.Parse (for port, npcId, skillId, skillLevel) and will throw FormatException on malformed input; replace those with TryParse variants (int.TryParse/short.TryParse) or wrap parsing in try/catch to provide clear user-friendly error messages and graceful exit. Specifically, update the initial port parse and each switch case that assigns npcId, skillId, and skillLevel to validate the parsed value, log or Console.WriteLine which argument was invalid (e.g., "--npc", "--skill", "--skill-level", or port) and return non-zero status instead of letting an exception propagate; ensure npcId remains nullable when parse fails and ensure types (int vs short) are correctly handled in the validation.Maple2.TestClient/Network/MapleClient.cs (3)
198-222: One-shot waiter silently suppresses the persistent handler for the same opcode.When a waiter is registered and consumes the packet (line 209
return), the persistentOnhandler for that opcode is skipped. If higher-level code registers both a persistent handler (e.g., for logging) and occasionally callsWaitForPacketAsyncon the same opcode, the handler will silently miss those packets. Consider whether the persistent handler should always run regardless of waiter consumption, or document this mutual-exclusion explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Network/MapleClient.cs` around lines 198 - 222, DispatchPacket currently returns as soon as a one-shot waiter successfully consumes a packet, which prevents the persistent handler from running; change the flow in DispatchPacket so that even if a waiter consumes the packet, the persistent handler (handlers.TryGetValue -> handler(raw)) still executes (i.e., remove or avoid the early return inside the waiters block) or alternatively invoke the persistent handler before dequeuing waiters; update the logic around waiters.TryGetValue and handler invocation in DispatchPacket to ensure both waiter completion and handler(raw) are executed for the same opcode, and keep error handling (Logger.Error) for handler exceptions.
233-247: Dispose check-then-set is not atomic, but impact is benign.Two concurrent
Dispose()calls can both pass theif (disposed) return;guard before either setsdisposed = true. In this case the double-close operations are idempotent so it won't break, but if you want correctness you could useInterlocked.Exchange:♻️ Proposed fix
public void Dispose() { - if (disposed) return; - disposed = true; + if (Interlocked.Exchange(ref Unsafe.As<bool, int>(ref disposed), 1) == 1) return;Or more simply, switch
disposedto anintfield and use:-private volatile bool disposed; +private int disposed; public void Dispose() { - if (disposed) return; - disposed = true; + if (Interlocked.Exchange(ref disposed, 1) == 1) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Network/MapleClient.cs` around lines 233 - 247, The Dispose() method's check/set of the disposed flag is not atomic; replace the boolean race by using an atomic exchange to ensure only one thread proceeds with cleanup: change the disposed field to an int (e.g., disposedFlag) or keep the bool but use Interlocked.Exchange semantics, then in Dispose() call Interlocked.Exchange(ref disposedFlag, 1) (or compare-exchange) and return immediately if the previous value indicated disposal; leave the rest of Dispose() (stream/tcpClient close, recvThread.Join, and draining waiters) unchanged so cleanup only runs once.
167-196: Repeated full-buffer copies on every network read.
accumulator.ToArray()(line 170) plus theremainingcopy (line 192) allocate and copy the entire accumulated buffer on each read. For a test client this is tolerable, but if you ever increase throughput, consider usingSystem.IO.Pipelinesor a ring-buffer to avoid per-read allocations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Network/MapleClient.cs` around lines 167 - 196, ProcessAccumulatedData currently calls accumulator.ToArray() and copies remaining bytes, causing full-buffer allocations; change to use the MemoryStream internal buffer (accumulator.GetBuffer()) and accumulator.Length to build the ReadOnlySequence without ToArray, call recvCipher.TryDecrypt(sequence, out PoolByteReader packet) as before, and when bytes are consumed use Buffer.BlockCopy on the internal buffer to shift the remaining bytes to the front then SetLength on accumulator (and reset Position if necessary) instead of creating a new byte[]; keep using packet.Dispose() and DispatchPacket(raw) but avoid the initial ToArray/remaining allocation to prevent per-read copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.TestClient/Clients/GameClient.cs`:
- Around line 93-111: SpawnNpcAsync currently uses the one-shot waiter
WaitForPacketAsync(SendOp.FieldAddNpc), which consumes the packet before the
persistent handler in RegisterFieldNpcHandlers can run and populate FieldNpcs;
fix by either (A) removing the WaitForPacketAsync usage for SendOp.FieldAddNpc
and instead rely on the existing persistent handler flow (refactor SpawnNpcAsync
to send the /npc command and await a different confirmation or a shared event),
or (B) keep the waiter but after receiving the packet manually add the created
NpcInfo into the same FieldNpcs dictionary the persistent handler would have
used (ensure you use the same keys/structure and any locking used by
RegisterFieldNpcHandlers), referencing SpawnNpcAsync, WaitForPacketAsync,
SendOp.FieldAddNpc, RegisterFieldNpcHandlers, FieldNpcs and
MapleClient.DispatchPacket to locate the relevant code.
In `@Maple2.TestClient/Clients/LoginClient.cs`:
- Around line 103-105: The CancellationTokenSource instance named "cts" created
in the LoginClient method is not disposed (it implements IDisposable and holds a
timer); wrap its lifetime so it's disposed after use — e.g., use a using
statement or call cts.Dispose() after awaiting charListDone.Task — and keep the
registration of cts.Token.Register(() => charListDone.TrySetCanceled()) intact
but ensure the registration is unregistered/disposed if you remove the CTS
before the token fires; update the code around the cts variable to ensure proper
disposal while preserving the await on charListDone.Task.
- Around line 121-130: The ParseCharacterId method corrupts the reader when
multiple characters are present; add a fail-fast guard at the caller side before
looping (the loop that iterates "count" and repeatedly calls ParseCharacterId)
to detect count > 1 and throw a clear NotSupportedException (or return an error)
stating "multi-character accounts not supported" so ParseCharacterId is only
invoked for single-character accounts, or alternatively change ParseCharacterId
to accept a count parameter and throw when count > 1; reference the
ParseCharacterId method and the caller loop that iterates count to locate where
to add this guard.
In `@Maple2.TestClient/Network/MapleClient.cs`:
- Around line 79-82: The call to recvCipher.TryDecrypt(new
ReadOnlySequence<byte>(rawHandshake), out PoolByteReader _) discards the
returned PoolByteReader, leaking its pooled buffer; change this to capture the
out parameter (e.g., out var handshakeReader or out PoolByteReader
handshakeReader) and ensure it is disposed after use (use a using / try/finally)
so the pooled buffer is returned, mirroring how packet.Dispose() is used later
for other readers.
- Around line 79-82: The call to recvCipher.TryDecrypt(new
ReadOnlySequence<byte>(rawHandshake), out PoolByteReader _) in MapleClient.cs
discards a PoolByteReader that implements IDisposable and must be returned to
the pool; fix by capturing the out parameter (e.g., declare a PoolByteReader
variable from TryDecrypt) and either wrap its usage in a using block or
explicitly call Dispose() after use so the pooled buffer is returned (ensure
this change is applied to the TryDecrypt call site where IV sync is performed).
- Around line 112-125: WaitForPacketAsync creates a CancellationTokenSource
(cts) for each call but never disposes it; modify the method so the cts and its
registration are disposed when the TaskCompletionSource (tcs.Task) completes
(either by success, exception, or timeout). Specifically, keep the existing cts
and Token.Register call but capture the registration, then attach a continuation
to tcs.Task that disposes the registration and calls cts.Dispose() (and also
ensures tcs.TrySetCanceled/Result race is safe); reference the
WaitForPacketAsync method, the local variables cts, tcs, and the waiters queue
for placement.
In `@Maple2.TestClient/Program.cs`:
- Around line 69-70: The Log.Information call is printing the session token
(gameServer.Token) in plaintext; change the log to omit or mask the token before
logging by either removing Token from the format string or replacing
gameServer.Token with a masked value (e.g., show only last 4 chars or fixed
placeholder) and update the call site (the Log.Information invocation that
currently references gameServer.Address, gameServer.Port, gameServer.Token,
gameServer.MapId) so the template and arguments no longer expose the full token.
In `@Maple2.TestClient/Protocol/ClientPacket.cs`:
- Around line 141-160: SkillAttackTarget currently indexes targetObjectIds[i] up
to targetCount without validating the array size, causing an
IndexOutOfRangeException; in SkillAttackTarget validate that targetObjectIds is
not null and that targetObjectIds.Length >= targetCount (or if intended, clamp
targetCount to targetObjectIds.Length) before the for-loop, and throw an
ArgumentException with a clear message if the sizes mismatch (or adjust
targetCount to Math.Min(targetCount, targetObjectIds.Length)) so the loop that
writes targetObjectIds[i] is safe.
---
Nitpick comments:
In `@Maple2.sln`:
- Around line 40-41: The Project entry for Maple2.TestClient uses the legacy C#
project type GUID "{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}"; update that GUID to
the SDK-style GUID "{9A19103F-16F7-4668-BE54-9A1E7A4F7556}" so the
Project("{...}") line for "Maple2.TestClient" matches the other SDK-style
projects and maintains consistency across the solution.
In `@Maple2.TestClient/Clients/GameClient.cs`:
- Line 22: The field nextSkillUid is initialized to 1 but Interlocked.Increment
returns the value after increment, so the first allocated UID will be 2; change
the initialization of nextSkillUid (the private long nextSkillUid field) to 0 so
that the first Interlocked.Increment call yields 1, or alternatively adjust the
allocation call to use Interlocked.Increment(ref nextSkillUid) - 1 if you
intentionally want to keep the initial value; update the declaration accordingly
to ensure UIDs start at 1.
- Line 135: Remove the development placeholder comments
"PLACEHOLDER_GAMECLIENT_ATTACK" and "PLACEHOLDER_GAMECLIENT_HANDLERS" from
GameClient.cs; locate the placeholder tokens in the file (the exact strings) and
delete them or replace them with the intended implementation (e.g., real attack
logic or handler registration) so no scaffolding markers remain, and ensure no
dead references or TODO markers remain that would break compilation or confuse
reviewers.
In `@Maple2.TestClient/Network/MapleClient.cs`:
- Around line 198-222: DispatchPacket currently returns as soon as a one-shot
waiter successfully consumes a packet, which prevents the persistent handler
from running; change the flow in DispatchPacket so that even if a waiter
consumes the packet, the persistent handler (handlers.TryGetValue ->
handler(raw)) still executes (i.e., remove or avoid the early return inside the
waiters block) or alternatively invoke the persistent handler before dequeuing
waiters; update the logic around waiters.TryGetValue and handler invocation in
DispatchPacket to ensure both waiter completion and handler(raw) are executed
for the same opcode, and keep error handling (Logger.Error) for handler
exceptions.
- Around line 233-247: The Dispose() method's check/set of the disposed flag is
not atomic; replace the boolean race by using an atomic exchange to ensure only
one thread proceeds with cleanup: change the disposed field to an int (e.g.,
disposedFlag) or keep the bool but use Interlocked.Exchange semantics, then in
Dispose() call Interlocked.Exchange(ref disposedFlag, 1) (or compare-exchange)
and return immediately if the previous value indicated disposal; leave the rest
of Dispose() (stream/tcpClient close, recvThread.Join, and draining waiters)
unchanged so cleanup only runs once.
- Around line 167-196: ProcessAccumulatedData currently calls
accumulator.ToArray() and copies remaining bytes, causing full-buffer
allocations; change to use the MemoryStream internal buffer
(accumulator.GetBuffer()) and accumulator.Length to build the ReadOnlySequence
without ToArray, call recvCipher.TryDecrypt(sequence, out PoolByteReader packet)
as before, and when bytes are consumed use Buffer.BlockCopy on the internal
buffer to shift the remaining bytes to the front then SetLength on accumulator
(and reset Position if necessary) instead of creating a new byte[]; keep using
packet.Dispose() and DispatchPacket(raw) but avoid the initial ToArray/remaining
allocation to prevent per-read copies.
In `@Maple2.TestClient/Program.cs`:
- Around line 14-35: The CLI parsing currently uses int.Parse/short.Parse (for
port, npcId, skillId, skillLevel) and will throw FormatException on malformed
input; replace those with TryParse variants (int.TryParse/short.TryParse) or
wrap parsing in try/catch to provide clear user-friendly error messages and
graceful exit. Specifically, update the initial port parse and each switch case
that assigns npcId, skillId, and skillLevel to validate the parsed value, log or
Console.WriteLine which argument was invalid (e.g., "--npc", "--skill",
"--skill-level", or port) and return non-zero status instead of letting an
exception propagate; ensure npcId remains nullable when parse fails and ensure
types (int vs short) are correctly handled in the validation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Maple2.TestClient/Clients/GameClient.cs (1)
135-135: Remove leftover placeholder comments.Lines 135 and 184 contain
// PLACEHOLDER_GAMECLIENT_ATTACKand// PLACEHOLDER_GAMECLIENT_HANDLERS— these are template artifacts that should be removed before merging.♻️ Proposed fix
- // PLACEHOLDER_GAMECLIENT_ATTACK - /// <summary> /// Attack a target NPC...private void RegisterTimeSyncHandler() { - // PLACEHOLDER_GAMECLIENT_HANDLERS - client.On(SendOp.ResponseTimeSync, raw => {Also applies to: 184-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Clients/GameClient.cs` at line 135, In the GameClient class remove the leftover template comment markers "// PLACEHOLDER_GAMECLIENT_ATTACK" and "// PLACEHOLDER_GAMECLIENT_HANDLERS" (they are stray artifacts in Maple2.TestClient/Clients/GameClient.cs inside the GameClient class) so the file contains no placeholder comments; simply delete those two comment lines and keep surrounding code/spacing intact.Maple2.TestClient/Clients/LoginClient.cs (1)
89-92:constValis read but its value is never used; should be a discard or documented.The read is needed to advance the
ByteReader, but the unnamed integer should be explicitly discarded or explained.♻️ Proposed fix
- int constVal = reader.ReadInt(); + reader.ReadInt(); // unknown constant in login result packet🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Clients/LoginClient.cs` around lines 89 - 92, The integer read into constVal is intentionally only to advance the ByteReader but is never used; replace the unused variable in the LoginClient read sequence by discarding the value (e.g., use a discard '_' assignment or rename to a clearly commented unused variable) so the intent is explicit—locate the block where reader.ReadByte(), reader.ReadInt(), reader.ReadUnicodeString(), reader.ReadLong() are called (around the login state parsing) and change the int constVal = reader.ReadInt(); to an explicit discard or add a brief comment documenting why the value is read but unused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.TestClient/Clients/GameClient.cs`:
- Around line 141-155: The one-shot waiter in AttackTargetAsync
(client.WaitForPacketAsync(SendOp.SkillDamage, ...)) consumes the SkillDamage
packet and prevents the persistent RegisterSkillDamageHandler from
seeing/logging it; change the waiter to a non-consuming approach so the
persistent handler still runs — either call a non-consuming overload (e.g.,
WaitForPacketAsync(SendOp.SkillDamage, timeout, consume:false) if available) or
implement a temporary observer that sets a TaskCompletionSource when a
SkillDamage arrives but does not remove/consume the packet (register alongside
RegisterSkillDamageHandler or use a Peek-style API) so DispatchPacket and the
persistent handler continue to receive the packet. Ensure AttackTargetAsync
still awaits the TCS with the same timeout behavior and leaves the persistent
RegisterSkillDamageHandler intact.
---
Duplicate comments:
In `@Maple2.TestClient/Clients/GameClient.cs`:
- Around line 93-111: SpawnNpcAsync currently consumes the FieldAddNpc packet
via WaitForPacketAsync and parses an NpcInfo but never updates the persistent
FieldNpcs collection, so the persistent handler in RegisterFieldNpcHandlers
never sees it; after you construct the NpcInfo (variable info) and before
returning, add or upsert it into FieldNpcs using the same keying
logic/RegisterFieldNpcHandlers convention (e.g., FieldNpcs[objectId] = info or
thread-safe equivalent), and guard against duplicates if necessary so the
persistent state stays consistent.
In `@Maple2.TestClient/Clients/LoginClient.cs`:
- Around line 103-105: The CancellationTokenSource instance "cts" created before
registering to charListDone is not disposed; wrap its lifetime so the internal
timer is released — either change to a using declaration for "cts" (using var
cts = new CancellationTokenSource(TimeSpan.FromSeconds(10))) or ensure you call
cts.Dispose() in a finally block after awaiting charListDone.Task; keep the
Register call and await unchanged (references: variable cts, charListDone,
CancellationTokenSource).
- Around line 121-130: ParseCharacterId currently consumes the variable-length
name (reader.ReadUnicodeString()) which leaves the ByteReader mid-entry and
corrupts subsequent reads for multi-character accounts; fix it by removing the
ReadUnicodeString() call so ParseCharacterId only reads accountId
(reader.ReadLong()) and characterId (reader.ReadLong()) and returns characterId
without advancing past the variable-length fields, or alternatively implement a
proper skip routine that consumes the entire character entry; update
ParseCharacterId (and any callers) to either not read the name or to throw/guard
when multiple characters are present so the reader position is not silently
corrupted.
In `@Maple2.TestClient/Network/MapleClient.cs`:
- Around line 79-82: The PoolByteReader created by recvCipher.TryDecrypt is
being discarded and not disposed, leaking its pooled buffer; change the call to
capture the out parameter (e.g., PoolByteReader reader) and ensure
reader.Dispose() is called (preferably via a using or try/finally) after
decryption completes; locate the call to recvCipher.TryDecrypt in the handshake
code that builds rawHandshake (uses HANDSHAKE_HEADER_SIZE and payloadLength) and
replace the out-discarded PoolByteReader _ with a properly disposed variable.
- Around line 112-125: WaitForPacketAsync currently creates a
CancellationTokenSource (cts) per call and never disposes it; ensure cts and its
registration are disposed when the TaskCompletionSource (tcs) completes to avoid
timer leaks. After creating cts and registering the timeout callback (the
existing cts.Token.Register call), capture the returned
CancellationTokenRegistration, then attach a continuation to tcs.Task (e.g.,
ContinueWith or Task.ContinueWith) that disposes both the registration and the
cts (reg.Dispose(); cts.Dispose()) once the tcs completes (success, fault, or
cancellation); keep using the same tcs, queue, and waiters logic but add this
cleanup so timers are released promptly.
---
Nitpick comments:
In `@Maple2.TestClient/Clients/GameClient.cs`:
- Line 135: In the GameClient class remove the leftover template comment markers
"// PLACEHOLDER_GAMECLIENT_ATTACK" and "// PLACEHOLDER_GAMECLIENT_HANDLERS"
(they are stray artifacts in Maple2.TestClient/Clients/GameClient.cs inside the
GameClient class) so the file contains no placeholder comments; simply delete
those two comment lines and keep surrounding code/spacing intact.
In `@Maple2.TestClient/Clients/LoginClient.cs`:
- Around line 89-92: The integer read into constVal is intentionally only to
advance the ByteReader but is never used; replace the unused variable in the
LoginClient read sequence by discarding the value (e.g., use a discard '_'
assignment or rename to a clearly commented unused variable) so the intent is
explicit—locate the block where reader.ReadByte(), reader.ReadInt(),
reader.ReadUnicodeString(), reader.ReadLong() are called (around the login state
parsing) and change the int constVal = reader.ReadInt(); to an explicit discard
or add a brief comment documenting why the value is read but unused.
zbian99
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
Maple2.TestClient/Clients/GameClient.cs (2)
136-136: Remove placeholder comments.
PLACEHOLDER_GAMECLIENT_ATTACK(line 136) andPLACEHOLDER_GAMECLIENT_HANDLERS(line 185) are development artifacts that should be cleaned up before merge.Also applies to: 185-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Clients/GameClient.cs` at line 136, Remove the leftover development placeholder comments named PLACEHOLDER_GAMECLIENT_ATTACK and PLACEHOLDER_GAMECLIENT_HANDLERS from GameClient.cs; locate the placeholder markers (the exact tokens "PLACEHOLDER_GAMECLIENT_ATTACK" and "PLACEHOLDER_GAMECLIENT_HANDLERS") and delete them, or replace them with the intended implementation or a concise TODO comment if work remains, ensuring no stray placeholder markers remain in the file.
236-250:Disposeis not thread-safe — concurrent calls can race.
disposedis checked and set without synchronization. Two threads callingDispose()simultaneously could both pass theif (disposed) returncheck. Consider usingInterlocked.Exchangefor the flag.Suggested fix
Note: This same pattern applies to
MapleClient.Dispose()as well.+ private int _disposed; // 0 = not disposed, 1 = disposed public void Dispose() { - if (disposed) return; - disposed = true; + if (Interlocked.Exchange(ref _disposed, 1) != 0) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Clients/GameClient.cs` around lines 236 - 250, The Dispose method is racy because the boolean disposed flag is checked/updated without synchronization; change the pattern to use an atomic operation (e.g., Interlocked.Exchange) against an int-backed flag (e.g., int disposed = 0) so only the first caller proceeds; in Dispose(), perform if (Interlocked.Exchange(ref disposed, 1) == 1) return and then call client.Dispose(); apply the same fix to MapleClient.Dispose() to ensure client.Dispose is only invoked once and the disposed flag update is atomic.Maple2.TestClient/Program.cs (1)
118-123: MissingLog.CloseAndFlush()before exit.Serilog may buffer log entries. Without
CloseAndFlush(), the final messages (including error details) may be lost on exit.Suggested fix
Log.Information("=== TestClient finished ==="); } catch (Exception ex) { Log.Error(ex, "TestClient error"); +} finally { + await Log.CloseAndFlushAsync(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Program.cs` around lines 118 - 123, The program currently logs final messages but never calls Serilog's Log.CloseAndFlush(), which can cause buffered entries to be lost; update Program (around the try/catch that awaits gameClient.StayAliveAsync) to ensure Log.CloseAndFlush() is invoked on all exit paths—add a finally block after the try/catch (or call CloseAndFlush() in both the normal completion path and inside the catch before rethrow/exit) so that the "=== TestClient finished ===" and any exception logs are flushed.Maple2.TestClient/Network/MapleClient.cs (3)
201-225: One-shot waiters prevent persistent handlers from firing for the same packet.
DispatchPacketreturns immediately after a one-shot waiter consumes a packet (line 212), so the persistent handler for the same opcode never runs. This is a known design tradeoff already mitigated inSpawnNpcAsync(manualFieldNpcsadd). Consider adding a code comment here documenting this behavior for future maintainers.Suggested documentation
// Check one-shot waiters first + // NOTE: If a one-shot waiter consumes the packet, the persistent handler + // for this opcode will NOT fire. Callers that need both must manually + // replicate the persistent handler's side effects (see SpawnNpcAsync). if (waiters.TryGetValue(opcode, out var queue)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Network/MapleClient.cs` around lines 201 - 225, DispatchPacket currently returns as soon as a one-shot waiter (waiters queue) successfully consumes a packet, which prevents persistent handlers in handlers from running for that opcode; add a concise inline comment in DispatchPacket right above the return (and mention the related SpawnNpcAsync/FieldNpcs mitigation) documenting that one-shot waiters take precedence and intentionally short-circuit persistent handlers so future maintainers understand the design tradeoff and where SpawnNpcAsync manually re-invokes handler logic.
170-198:accumulator.ToArray()copies the entire buffer on every call.
ProcessAccumulatedDataallocates a full copy of the accumulated data on each invocation. For a test client this is acceptable, but if this code is reused for higher-throughput scenarios, consider usingaccumulator.GetBuffer()withaccumulator.Lengthor aPipe/PipeReaderbased approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Network/MapleClient.cs` around lines 170 - 198, ProcessAccumulatedData currently calls accumulator.ToArray() which copies the entire buffer each invocation; replace this to avoid allocations by using accumulator.GetBuffer() together with accumulator.Length (or switch to a Pipe/PipeReader) so you operate on the actual used span without copying. Update the code around ProcessAccumulatedData where data/sequence is built (currently using accumulator.ToArray() and new ReadOnlySequence<byte>(data)) to create a ReadOnlySequence over the buffer slice (using GetBuffer() and length) or feed the PipeReader; keep the loop using recvCipher.TryDecrypt and handling PoolByteReader/DispatchPacket the same, and adjust the logic that trims the accumulator (totalConsumed, remaining, accumulator.SetLength/Write) to work with the buffer/length approach. Ensure you still dispose packet and only shift/write the remaining bytes based on totalConsumed and the original accumulator.Length.
236-250:Disposehas a check-then-set race ondisposed.
volatileensures visibility but not atomicity of theif (disposed) return; disposed = true;sequence. Concurrent callers can both pass the check. UseInterlocked.CompareExchangefor a safe single-entry guard.Suggested fix
- private volatile bool disposed; + private int _disposed; // 0 = active, 1 = disposed public void Dispose() { - if (disposed) return; - disposed = true; + if (Interlocked.CompareExchange(ref _disposed, 1, 0) != 0) return;Note: Other reads of
disposed(e.g., inReceiveLoop,Send) should then check_disposed != 0or useVolatile.Read(ref _disposed) != 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Network/MapleClient.cs` around lines 236 - 250, The Dispose method uses a non-atomic check-then-set on the boolean disposed causing a race; replace the bool with an int (e.g., _disposed) and atomically set it using Interlocked.CompareExchange in Dispose (if Interlocked.CompareExchange(ref _disposed, 1, 0) != 0 return) so only one caller proceeds to close stream/tcpClient, Join recvThread, and cancel waiters; also update other reads (ReceiveLoop, Send, etc.) to test the flag as _disposed != 0 or use Volatile.Read(ref _disposed) != 0 for visibility.Maple2.TestClient/Protocol/ClientPacket.cs (1)
23-29: VERSION constant is duplicated across files.
ResponseVersion()hardcodes12, andMapleClient.csalso definesprivate const uint VERSION = 12. If the version changes, both must be updated independently. Consider extracting a shared constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Protocol/ClientPacket.cs` around lines 23 - 29, ResponseVersion() currently hardcodes the protocol version (12) while MapleClient defines a separate private const uint VERSION = 12; extract a single shared constant (e.g., public const uint VERSION) into a common location accessible to both (for example a new or existing protocol/constants class) and replace the literal 12 in ClientPacket.ResponseVersion() with that shared constant; also update MapleClient to reference the same shared constant (remove its private duplicate) so both use the identical symbol (e.g., ProtocolConstants.VERSION or ClientPacket.VERSION).Maple2.TestClient/Clients/LoginClient.cs (2)
93-96: Unused variableconstVal.
constValis read from the packet but never referenced. If it's intentionally discarded, use a discard to signal intent.Suggested fix
byte loginState = reader.ReadByte(); - int constVal = reader.ReadInt(); + _ = reader.ReadInt(); // constant value (unused) string banReason = reader.ReadUnicodeString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Clients/LoginClient.cs` around lines 93 - 96, The code reads an int into an unused variable constVal in LoginClient.cs (byte loginState = reader.ReadByte(); int constVal = reader.ReadInt(); ...); replace the unused local with a deliberate discard to signal intent by changing the read to "_ = reader.ReadInt();" (or otherwise remove the assignment and call reader.ReadInt() if side-effects are needed) so the value is not stored but the packet position is advanced.
57-117: Variablerawon line 89 shadows the lambda parameterrawon line 61.Both the lambda and the outer scope use
rawas a variable name. While C# allows this (the lambda'srawis a parameter), it's confusing to read. Consider renaming one of them (e.g.,loginRawfor line 89 orpacketRawfor the lambda).Suggested rename
- byte[] raw = await loginResultTask; - var reader = new ByteReader(raw, 0); + byte[] loginRaw = await loginResultTask; + var reader = new ByteReader(loginRaw, 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Clients/LoginClient.cs` around lines 57 - 117, The lambda parameter named raw in the client.On(SendOp.CharacterList, raw => { ... }) handler is shadowed by the outer variable raw assigned from await loginResultTask; rename one to avoid confusion — e.g., rename the outer variable raw (used with ByteReader after awaiting loginResultTask) to loginRaw (and update the ByteReader construction and subsequent reads), or rename the lambda parameter to packetRaw (and update all reads inside that handler) so all references are unambiguous (affecting the client.On handler and the login result read path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.TestClient/Clients/GameClient.cs`:
- Around line 196-207: RegisterFieldAddUserHandler currently assumes the first
FieldAddUser packet is the local player and sets ObjectId from it; instead,
parse and validate a unique identifier in the packet (e.g., accountId or
characterId) before assigning ObjectId, or only set ObjectId when receiving an
explicit “owner/login” packet from the server; locate the lambda registered to
client.On(SendOp.FieldAddUser) in RegisterFieldAddUserHandler and change the
logic to read the additional identifier from the ByteReader and compare it to
the local known identifier (or defer assignment until a definitive owner packet
arrives) rather than using the first boolean-first flag.
In `@Maple2.TestClient/Program.cs`:
- Around line 14-17: The CLI numeric parsing uses int.Parse/short.Parse (e.g.,
parsing port into variable "port" in Program.Main and the other parses at lines
referenced) which can throw FormatException outside the try block; replace these
with TryParse patterns (int.TryParse/short.TryParse) and validate the result
before proceeding, printing a clear, user-friendly error and exiting or falling
back to a default when parsing fails; ensure the parsing logic for "port" and
the other parsed variables is moved into the same try/validation flow used by
Program.Main so non-numeric input is handled gracefully rather than crashing.
---
Duplicate comments:
In `@Maple2.TestClient/Clients/GameClient.cs`:
- Around line 142-156: The one-shot waiter created by WaitForPacketAsync in
AttackTargetAsync can complete before the persistent debug handler
(RegisterSkillDamageHandler) runs, causing lost log output; fix by ensuring the
persistent handler is registered before sending the SkillAttackTarget or by
changing the packet dispatch order so persistent handlers are invoked before
completing any one-shot waiters in DispatchPacket/WaitForPacketAsync — locate
AttackTargetAsync and either (a) call client.RegisterSkillDamageHandler(...) (or
the existing persistent handler registration) prior to creating/awaiting
damageTask and sending the packet, or (b) modify
DispatchPacket/WaitForPacketAsync to invoke persistent handlers first and only
then signal one-shot waiters so the persistent debug handler always runs.
In `@Maple2.TestClient/Network/MapleClient.cs`:
- Around line 113-128: The code now disposes the CancellationTokenSource when
the TaskCompletionSource's Task completes using tcs.Task.ContinueWith(_ =>
cts.Dispose(), TaskContinuationOptions.ExecuteSynchronously), so no further
change is required; leave the WaitForPacketAsync implementation as-is (keep the
TaskCompletionSource<byte[]> tcs, the CancellationTokenSource cts with timeout,
the Register callback that sets a TimeoutException on tcs, and the ContinueWith
disposal to ensure cts is disposed when tcs.Task completes).
In `@Maple2.TestClient/Program.cs`:
- Around line 69-70: The log call using Log.Information that previously included
the session token should not log sensitive tokens; update the Log.Information
invocation (the call that formats "Game server: {Address}:{Port},
MapId={MapId}") to ensure the session token is not included anywhere in its
format string or argument list and remove any references to a token variable so
only Address, Port and MapId are logged.
In `@Maple2.TestClient/Protocol/ClientPacket.cs`:
- Around line 141-163: No change required: the added guard in SkillAttackTarget
(method SkillAttackTarget) correctly validates targetObjectIds.Length against
targetCount and the SpawnNpcAsync change adding FieldNpcs resolves the handler
conflict; leave ByteWriter SkillAttackTarget and SpawnNpcAsync as implemented
(no further fixes needed).
---
Nitpick comments:
In `@Maple2.TestClient/Clients/GameClient.cs`:
- Line 136: Remove the leftover development placeholder comments named
PLACEHOLDER_GAMECLIENT_ATTACK and PLACEHOLDER_GAMECLIENT_HANDLERS from
GameClient.cs; locate the placeholder markers (the exact tokens
"PLACEHOLDER_GAMECLIENT_ATTACK" and "PLACEHOLDER_GAMECLIENT_HANDLERS") and
delete them, or replace them with the intended implementation or a concise TODO
comment if work remains, ensuring no stray placeholder markers remain in the
file.
- Around line 236-250: The Dispose method is racy because the boolean disposed
flag is checked/updated without synchronization; change the pattern to use an
atomic operation (e.g., Interlocked.Exchange) against an int-backed flag (e.g.,
int disposed = 0) so only the first caller proceeds; in Dispose(), perform if
(Interlocked.Exchange(ref disposed, 1) == 1) return and then call
client.Dispose(); apply the same fix to MapleClient.Dispose() to ensure
client.Dispose is only invoked once and the disposed flag update is atomic.
In `@Maple2.TestClient/Clients/LoginClient.cs`:
- Around line 93-96: The code reads an int into an unused variable constVal in
LoginClient.cs (byte loginState = reader.ReadByte(); int constVal =
reader.ReadInt(); ...); replace the unused local with a deliberate discard to
signal intent by changing the read to "_ = reader.ReadInt();" (or otherwise
remove the assignment and call reader.ReadInt() if side-effects are needed) so
the value is not stored but the packet position is advanced.
- Around line 57-117: The lambda parameter named raw in the
client.On(SendOp.CharacterList, raw => { ... }) handler is shadowed by the outer
variable raw assigned from await loginResultTask; rename one to avoid confusion
— e.g., rename the outer variable raw (used with ByteReader after awaiting
loginResultTask) to loginRaw (and update the ByteReader construction and
subsequent reads), or rename the lambda parameter to packetRaw (and update all
reads inside that handler) so all references are unambiguous (affecting the
client.On handler and the login result read path).
In `@Maple2.TestClient/Network/MapleClient.cs`:
- Around line 201-225: DispatchPacket currently returns as soon as a one-shot
waiter (waiters queue) successfully consumes a packet, which prevents persistent
handlers in handlers from running for that opcode; add a concise inline comment
in DispatchPacket right above the return (and mention the related
SpawnNpcAsync/FieldNpcs mitigation) documenting that one-shot waiters take
precedence and intentionally short-circuit persistent handlers so future
maintainers understand the design tradeoff and where SpawnNpcAsync manually
re-invokes handler logic.
- Around line 170-198: ProcessAccumulatedData currently calls
accumulator.ToArray() which copies the entire buffer each invocation; replace
this to avoid allocations by using accumulator.GetBuffer() together with
accumulator.Length (or switch to a Pipe/PipeReader) so you operate on the actual
used span without copying. Update the code around ProcessAccumulatedData where
data/sequence is built (currently using accumulator.ToArray() and new
ReadOnlySequence<byte>(data)) to create a ReadOnlySequence over the buffer slice
(using GetBuffer() and length) or feed the PipeReader; keep the loop using
recvCipher.TryDecrypt and handling PoolByteReader/DispatchPacket the same, and
adjust the logic that trims the accumulator (totalConsumed, remaining,
accumulator.SetLength/Write) to work with the buffer/length approach. Ensure you
still dispose packet and only shift/write the remaining bytes based on
totalConsumed and the original accumulator.Length.
- Around line 236-250: The Dispose method uses a non-atomic check-then-set on
the boolean disposed causing a race; replace the bool with an int (e.g.,
_disposed) and atomically set it using Interlocked.CompareExchange in Dispose
(if Interlocked.CompareExchange(ref _disposed, 1, 0) != 0 return) so only one
caller proceeds to close stream/tcpClient, Join recvThread, and cancel waiters;
also update other reads (ReceiveLoop, Send, etc.) to test the flag as _disposed
!= 0 or use Volatile.Read(ref _disposed) != 0 for visibility.
In `@Maple2.TestClient/Program.cs`:
- Around line 118-123: The program currently logs final messages but never calls
Serilog's Log.CloseAndFlush(), which can cause buffered entries to be lost;
update Program (around the try/catch that awaits gameClient.StayAliveAsync) to
ensure Log.CloseAndFlush() is invoked on all exit paths—add a finally block
after the try/catch (or call CloseAndFlush() in both the normal completion path
and inside the catch before rethrow/exit) so that the "=== TestClient finished
===" and any exception logs are flushed.
In `@Maple2.TestClient/Protocol/ClientPacket.cs`:
- Around line 23-29: ResponseVersion() currently hardcodes the protocol version
(12) while MapleClient defines a separate private const uint VERSION = 12;
extract a single shared constant (e.g., public const uint VERSION) into a common
location accessible to both (for example a new or existing protocol/constants
class) and replace the literal 12 in ClientPacket.ResponseVersion() with that
shared constant; also update MapleClient to reference the same shared constant
(remove its private duplicate) so both use the identical symbol (e.g.,
ProtocolConstants.VERSION or ClientPacket.VERSION).
Comment on lines
+196
to
+207
| private void RegisterFieldAddUserHandler() { | ||
| bool first = true; | ||
| client.On(SendOp.FieldAddUser, raw => { | ||
| var r = new ByteReader(raw, 0); | ||
| r.Read<SendOp>(); | ||
| int objectId = r.ReadInt(); | ||
| if (first) { | ||
| ObjectId = objectId; | ||
| first = false; | ||
| Logger.Information("My ObjectId={ObjectId}", objectId); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
RegisterFieldAddUserHandler assumes the first FieldAddUser packet is for the local client.
If the server broadcasts other players' FieldAddUser before the local client's, ObjectId will be set to the wrong value. Consider validating against a known identifier (e.g., account ID or character ID) if available in the packet, or document this assumption more prominently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.TestClient/Clients/GameClient.cs` around lines 196 - 207,
RegisterFieldAddUserHandler currently assumes the first FieldAddUser packet is
the local player and sets ObjectId from it; instead, parse and validate a unique
identifier in the packet (e.g., accountId or characterId) before assigning
ObjectId, or only set ObjectId when receiving an explicit “owner/login” packet
from the server; locate the lambda registered to client.On(SendOp.FieldAddUser)
in RegisterFieldAddUserHandler and change the logic to read the additional
identifier from the ByteReader and compare it to the local known identifier (or
defer assignment until a definitive owner packet arrives) rather than using the
first boolean-first flag.
Comment on lines
+14
to
+17
| string host = args.Length > 0 ? args[0] : "127.0.0.1"; | ||
| int port = args.Length > 1 ? int.Parse(args[1]) : 20001; | ||
| string username = args.Length > 2 ? args[2] : "testbot"; | ||
| string password = args.Length > 3 ? args[3] : "testbot"; |
Contributor
There was a problem hiding this comment.
int.Parse / short.Parse for CLI args can throw unhandled FormatException.
Lines 15, 26, 29, and 32 use int.Parse/short.Parse outside the try block. Non-numeric input will crash with an unhandled exception instead of a user-friendly error.
Suggested fix for port parsing (apply similar pattern to others)
string host = args.Length > 0 ? args[0] : "127.0.0.1";
-int port = args.Length > 1 ? int.Parse(args[1]) : 20001;
+int port = args.Length > 1 && int.TryParse(args[1], out int parsedPort) ? parsedPort : 20001;
string username = args.Length > 2 ? args[2] : "testbot";
string password = args.Length > 3 ? args[3] : "testbot";📝 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.
Suggested change
| string host = args.Length > 0 ? args[0] : "127.0.0.1"; | |
| int port = args.Length > 1 ? int.Parse(args[1]) : 20001; | |
| string username = args.Length > 2 ? args[2] : "testbot"; | |
| string password = args.Length > 3 ? args[3] : "testbot"; | |
| string host = args.Length > 0 ? args[0] : "127.0.0.1"; | |
| int port = args.Length > 1 && int.TryParse(args[1], out int parsedPort) ? parsedPort : 20001; | |
| string username = args.Length > 2 ? args[2] : "testbot"; | |
| string password = args.Length > 3 ? args[3] : "testbot"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.TestClient/Program.cs` around lines 14 - 17, The CLI numeric parsing
uses int.Parse/short.Parse (e.g., parsing port into variable "port" in
Program.Main and the other parses at lines referenced) which can throw
FormatException outside the try block; replace these with TryParse patterns
(int.TryParse/short.TryParse) and validate the result before proceeding,
printing a clear, user-friendly error and exiting or falling back to a default
when parsing fails; ensure the parsing logic for "port" and the other parsed
variables is moved into the same try/validation flow used by Program.Main so
non-numeric input is handled gracefully rather than crashing.
Contributor
Author
There was a problem hiding this comment.
usage will be rewrite in next refactor
Contributor
There was a problem hiding this comment.
@zbian99, understood! Since you're planning to rewrite the usage in a future refactor, would you like me to create a GitHub issue to track this parsing improvement so it doesn't get lost?
Collaborator
|
I don't like the duplicated packet entries, variables, etc. It's redundant code that needs twice the upkeep. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Maple2.Server.Core/Network/Session.cs (1)
25-25: Consider relocatingFIELD_KEYto the existingMaple2.Server.Core.Constantsnamespace.
Sessionalready importsMaple2.Server.Core.Constants(Line 10). Protocol-level constants likeFIELD_KEYare a reasonable fit for a dedicated constants file rather than being embedded in the abstractSessionclass, which primarily concerns itself with transport/lifecycle concerns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Core/Network/Session.cs` at line 25, Move the protocol constant FIELD_KEY out of the abstract Session class and into the shared constants container in the Maple2.Server.Core.Constants namespace; specifically, remove the public const int FIELD_KEY = 0x1234 declaration from Session and add an equivalent constant (same name and value) to the existing Constants class or file under Maple2.Server.Core.Constants, then update any references to Session.FIELD_KEY to use Constants.FIELD_KEY so Session focuses on transport/lifecycle concerns.Maple2.TestClient/Protocol/ClientPacket.cs (2)
26-28: Replace magic literal12withSession.VERSION.
Session.VERSION(= 12) is already available via the importedMaple2.Server.Core.Networknamespace and is used correctly inResponseFieldEnterforFIELD_KEY. Using the raw literal here makes the two sites diverge silently ifVERSIONever changes.♻️ Proposed fix
- pWriter.Write<uint>(12); // VERSION + pWriter.Write<uint>(Session.VERSION);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Protocol/ClientPacket.cs` around lines 26 - 28, Replace the magic literal 12 in ClientPacket where pWriter.Write<uint>(12) is called with the canonical symbol Session.VERSION to keep version usage consistent; locate the pWriter.Write<uint>(...) call in the ClientPacket class and change it to use Session.VERSION (the same constant used in ResponseFieldEnter for FIELD_KEY) so both sites reference the same source of truth for protocol version.
142-164:targetCountis redundant; derive it fromtargetObjectIds.Length.After the guard,
targetCountmust equaltargetObjectIds.Length(or less, silently dropping entries). Requiring callers to pass both is error-prone; removingtargetCountand computing it from the array eliminates the class of bugs that the guard was added to catch.♻️ Proposed refactor
public static ByteWriter SkillAttackTarget(long skillUid, long targetUid, Vector3 impactPosition, Vector3 direction, byte attackPoint, - byte targetCount, int[] targetObjectIds) { - if (targetObjectIds.Length < targetCount) { - throw new ArgumentException($"targetObjectIds length ({targetObjectIds.Length}) must be >= targetCount ({targetCount})"); - } + int[] targetObjectIds) { + ArgumentNullException.ThrowIfNull(targetObjectIds); + byte targetCount = (byte)targetObjectIds.Length; var pWriter = Of(RecvOp.Skill, 256);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.TestClient/Protocol/ClientPacket.cs` around lines 142 - 164, The method SkillAttackTarget currently takes both targetCount and targetObjectIds which is error-prone; change the signature to remove the targetCount parameter and derive byte computedCount = (byte)targetObjectIds.Length, keep the existing guard only to check for null/empty if desired, replace pWriter.WriteByte(targetCount) with pWriter.WriteByte(computedCount), iterate for (int i = 0; i < computedCount; i++) when writing targetObjectIds[i], and update any callers of SkillAttackTarget to stop passing targetCount (they should just pass the array).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.TestClient/Protocol/ClientPacket.cs`:
- Around line 144-147: Add a null-check for the targetObjectIds parameter before
accessing its Length: in the method that accepts byte targetCount, int[]
targetObjectIds, throw an ArgumentNullException(nameof(targetObjectIds)) (or
ArgumentException) if targetObjectIds is null, then keep the existing length
check (targetObjectIds.Length < targetCount) so the current ArgumentException
remains valid; place the new null check immediately before the existing bounds
check that references targetObjectIds.Length.
---
Nitpick comments:
In `@Maple2.Server.Core/Network/Session.cs`:
- Line 25: Move the protocol constant FIELD_KEY out of the abstract Session
class and into the shared constants container in the
Maple2.Server.Core.Constants namespace; specifically, remove the public const
int FIELD_KEY = 0x1234 declaration from Session and add an equivalent constant
(same name and value) to the existing Constants class or file under
Maple2.Server.Core.Constants, then update any references to Session.FIELD_KEY to
use Constants.FIELD_KEY so Session focuses on transport/lifecycle concerns.
In `@Maple2.TestClient/Protocol/ClientPacket.cs`:
- Around line 26-28: Replace the magic literal 12 in ClientPacket where
pWriter.Write<uint>(12) is called with the canonical symbol Session.VERSION to
keep version usage consistent; locate the pWriter.Write<uint>(...) call in the
ClientPacket class and change it to use Session.VERSION (the same constant used
in ResponseFieldEnter for FIELD_KEY) so both sites reference the same source of
truth for protocol version.
- Around line 142-164: The method SkillAttackTarget currently takes both
targetCount and targetObjectIds which is error-prone; change the signature to
remove the targetCount parameter and derive byte computedCount =
(byte)targetObjectIds.Length, keep the existing guard only to check for
null/empty if desired, replace pWriter.WriteByte(targetCount) with
pWriter.WriteByte(computedCount), iterate for (int i = 0; i < computedCount;
i++) when writing targetObjectIds[i], and update any callers of
SkillAttackTarget to stop passing targetCount (they should just pass the array).
Comment on lines
+144
to
+147
| byte targetCount, int[] targetObjectIds) { | ||
| if (targetObjectIds.Length < targetCount) { | ||
| throw new ArgumentException($"targetObjectIds length ({targetObjectIds.Length}) must be >= targetCount ({targetCount})"); | ||
| } |
Contributor
There was a problem hiding this comment.
targetObjectIds null-check is missing; NullReferenceException before the bounds check.
targetObjectIds.Length on Line 145 will throw a NullReferenceException if null is passed, which occurs before the useful ArgumentException fires.
🛡️ Proposed fix
+ ArgumentNullException.ThrowIfNull(targetObjectIds);
if (targetObjectIds.Length < targetCount) {
throw new ArgumentException($"targetObjectIds length ({targetObjectIds.Length}) must be >= targetCount ({targetCount})");
}📝 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.
Suggested change
| byte targetCount, int[] targetObjectIds) { | |
| if (targetObjectIds.Length < targetCount) { | |
| throw new ArgumentException($"targetObjectIds length ({targetObjectIds.Length}) must be >= targetCount ({targetCount})"); | |
| } | |
| byte targetCount, int[] targetObjectIds) { | |
| ArgumentNullException.ThrowIfNull(targetObjectIds); | |
| if (targetObjectIds.Length < targetCount) { | |
| throw new ArgumentException($"targetObjectIds length ({targetObjectIds.Length}) must be >= targetCount ({targetCount})"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.TestClient/Protocol/ClientPacket.cs` around lines 144 - 147, Add a
null-check for the targetObjectIds parameter before accessing its Length: in the
method that accepts byte targetCount, int[] targetObjectIds, throw an
ArgumentNullException(nameof(targetObjectIds)) (or ArgumentException) if
targetObjectIds is null, then keep the existing length check
(targetObjectIds.Length < targetCount) so the current ArgumentException remains
valid; place the new null check immediately before the existing bounds check
that references targetObjectIds.Length.
Contributor
Author
Thanks for the feedback. I audited the duplication and here's what I found and addressed: Duplicated constants (fixed in c787c70): There were 3 constants duplicated between TestClient and the server: Constant Was duplicated in Now references Packet construction methods (kept as-is): ClientPacket.cs contains 10 packet builder methods (ResponseVersion, Login, ResponseKey, SkillUse, etc.) that mirror what the server handlers read. These are intentionally not shared because: They are the inverse of server handlers — the client writes what the server reads. This is inherent to client-server architecture. |
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
2 participants
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.
Maple2.TestClient
Headless bot client for automated server testing. Simulates the real MapleStory2 client protocol to interact with the server without a game client.
Purpose
Building & Running
Defaults: host=
127.0.0.1, port=20001, username=testbot, password=testbot.Requires a running server stack (World + Login + Game) and an existing character on the account (character creation is not implemented).
Architecture
Dependencies
Maple2.Server.Core— SendOp/RecvOp enums, MapleCipher (via Maple2.PacketLib NuGet), ByteWriter/ByteReaderMaple2.Model— Game enums (Locale, ChatType, etc.)Serilog.Sinks.Console— LoggingProtocol Flow
Key Classes
MapleClient (Network/MapleClient.cs)
Low-level network client handling:
TryDecryptto align with server's cipher stateSendOp-based dispatchWaitForPacketAsync(SendOp)— one-shot async waiter (register BEFORE sending to avoid race conditions)On(SendOp, handler)— persistent packet handler registrationLoginClient (Clients/LoginClient.cs)
High-level login flow:
ConnectAsync()— TCP connect + handshake + version exchangeLoginAsync()— send credentials, parse LoginResult, collect CharacterList (waits for EndList command=4)SelectCharacterAsync()— select character, return GameServerInfo (IP, port, token, mapId)GameClient (Clients/GameClient.cs)
High-level game flow:
ConnectAsync()— auth via ResponseKey, wait for RequestFieldEnter, send ResponseFieldEnterCastSkillAsync()— send Skill.Use, wait for SkillUse broadcast confirmationAttackTargetAsync()— send Skill.Attack.Target, wait for SkillDamage broadcast confirmationSpawnNpcAsync()— send GM/npccommand, wait for FieldAddNpcStayAliveAsync()— respond to TimeSync and Heartbeat until cancelledClientPacket (Protocol/ClientPacket.cs)
Static packet constructors for all client-to-server packets:
ResponseVersion,Login,SelectCharacter,ResponseKeyResponseFieldEnter,RequestTimeSync,ResponseHeartbeatChat,SkillUse,SkillAttackTargetImplementation Notes
recvCiphermust be synced by feeding the raw handshake throughTryDecryptonceWaitForPacketAsyncBEFORE callingSendto avoid race conditions with the receive threadKnown Limitations
/npcspawn commandCLI Arguments
127.0.0.120001testbottestbot--skill <id>--skill-level <n>1--npc <id>Summary by CodeRabbit
New Features
Documentation
Chores