fix: empty SpecialSeparator deserializer crash + widen CModePacket.MorphSkin#446
Merged
erwan-joly merged 5 commits intomasterfrom Apr 24, 2026
Merged
Conversation
**Deserializer — empty SpecialSeparator (non-breaking bug fix)**
Four packet declarations use [PacketIndex(n, SpecialSeparator = "")]
around an UpgradeRareSubPacket: EqPacket.WeaponUpgradeRarePacket /
ArmorUpgradeRarePacket, EquipPacket.WeaponUpgradeRareSubPacket /
ArmorUpgradeRareSubPacket, and InCharacterSubPacket.{Weapon,Armor}
UpgradeRareSubPacket. The serializer handles this correctly (joins
the sub-packet's two fields with no separator — e.g. Upgrade=7,
Rare=5 → "75" on the wire). The deserializer was doing
`matches[currentIndex].Replace("", " ")`, which .NET rejects with
ArgumentException. So every equip / eq / in packet with an
upgrade-rare field failed to parse.
Fix: branch on empty SpecialSeparator and insert a space between
every character via `string.Join(' ', chars)`. Existing separators
(".", "|", etc.) are unchanged.
**CModePacket.MorphSkin byte → short (BREAKING)**
Real captures consistently carry values of 7583/8151/8558/9471/etc.
in the last field, overflowing byte. Widening to short matches the
observed range. Source-level breaking because consumers pinning the
property type to byte stop compiling; runtime conversions widen safely.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 18 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughFixes IPacket deserialization when the special separator is empty by splitting matched tokens into single-character fields; package version bumped to 16.11.0; multiple packet field types and nullability adjusted; two fields added to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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: 1
🧹 Nitpick comments (1)
test/NosCore.Packets.Tests/DeserializerTest.cs (1)
420-441: Add a targetedc_moderegression test forMorphSkin > 255.Nice coverage for the empty-separator bug. I’d also add one test that deserializes a
c_modepayload containing a 4-digitMorphSkin(e.g., 7583) to lock in thebyte→shortfix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.Packets.Tests/DeserializerTest.cs` around lines 420 - 441, Add a focused regression test method in the same test class that uses the Deserializer (same constructor pattern as PacketWithEmptySpecialSeparatorSplitsEachCharIntoField) to Deserialize a c_mode payload containing a 4-digit MorphSkin (e.g., "c_mode 7583 ..."), then assert the resulting packet's MorphSkin property (or equivalent) is stored as a value > 255 (type short) to lock in the byte→short fix; reference the Deserializer.Deserialize call and the MorphSkin property in your assertions to locate the code to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NosCore.Packets/NosCore.Packets.csproj`:
- Line 15: The package version must be bumped to a new major release because
CModePacket.MorphSkin changed its public type from byte to short; update the
<Version> element in the project file (currently <Version>16.11.0</Version>) to
the next major version (e.g. <Version>17.0.0</Version>) so the NuGet package
reflects the breaking API change involving CModePacket.MorphSkin.
---
Nitpick comments:
In `@test/NosCore.Packets.Tests/DeserializerTest.cs`:
- Around line 420-441: Add a focused regression test method in the same test
class that uses the Deserializer (same constructor pattern as
PacketWithEmptySpecialSeparatorSplitsEachCharIntoField) to Deserialize a c_mode
payload containing a 4-digit MorphSkin (e.g., "c_mode 7583 ..."), then assert
the resulting packet's MorphSkin property (or equivalent) is stored as a value >
255 (type short) to lock in the byte→short fix; reference the
Deserializer.Deserialize call and the MorphSkin property in your assertions to
locate the code to update.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 7c79cbab-9be1-4fca-9edd-296ff02a5d82
📒 Files selected for processing (4)
src/NosCore.Packets/Deserializer.cssrc/NosCore.Packets/NosCore.Packets.csprojsrc/NosCore.Packets/ServerPackets/Player/CModePacket.cstest/NosCore.Packets.Tests/DeserializerTest.cs
… -1)
Real trace captures show an empty inventory slot as ivn {type} {slot}.0.0...
but NosCore was generating {slot}.-1.0... because the extension in NosCore
treats a null item-instance as a marker and hard-codes VNum = -1. The -1
sentinel is the Serializer's null-sub-packet output — which makes sense
for a nullable element, but the wire format never uses -1 here.
Tightening the packet contract: List<IvnSubPacket?>? -> List<IvnSubPacket>?
on IvnPacket, InvPacket, StashClientPacket, PStashClientPacket,
FStashClientPacket, FStashAllPacket. Now null entries are a compile error;
callers must produce a concrete sub-packet with VNum = 0 for cleared slots.
StashAllPacket and PStashAllPacket were already non-nullable — this aligns
the rest.
Regression test SerializeIvnPacketWithClearedSlotUsesZeroVNum asserts
"ivn 0 19.0.0.0.0" for a zeroed sub-packet so the wire output is pinned.
This unblocks the NosCore-side fix: ItemInstanceExtension.GenerateIvnSubPacket
returns VNum = 0 (not -1) when the item is null, so the client actually
clears the source slot after mvi / mve.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/NosCore.Packets.Tests/SerializerTest.cs (1)
1236-1249: Optional: add packet validity assertion for clearer diagnostics.Line [1237] onward already verifies wire output; adding
Assert.IsTrue(packet.IsValid, ...)makes failures easier to triage when schema constraints change.💡 Proposed improvement
public void SerializeIvnPacketWithClearedSlotUsesZeroVNum() { var packet = new IvnPacket { Type = PocketType.Equipment, IvnSubPackets = new List<IvnSubPacket> { new() { Slot = 19, VNum = 0, RareAmount = 0, UpgradeDesign = 0, SecondUpgrade = 0 } } }; + Assert.IsTrue(packet.IsValid, string.Join("; ", + packet.ValidationResult.Select(v => $"{string.Join(",", v.MemberNames)}: {v.ErrorMessage}"))); Assert.AreEqual("ivn 0 19.0.0.0.0", Serializer.Serialize(packet)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/NosCore.Packets.Tests/SerializerTest.cs` around lines 1236 - 1249, Add a packet validity assertion before serializing in the SerializeIvnPacketWithClearedSlotUsesZeroVNum test: verify the IvnPacket instance is valid (e.g., Assert.IsTrue(packet.IsValid, "expected packet to be valid before serialization")) so test failures show schema/validation problems earlier; place this check just before the existing Assert.AreEqual that compares Serializer.Serialize(packet).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/NosCore.Packets.Tests/SerializerTest.cs`:
- Around line 1236-1249: Add a packet validity assertion before serializing in
the SerializeIvnPacketWithClearedSlotUsesZeroVNum test: verify the IvnPacket
instance is valid (e.g., Assert.IsTrue(packet.IsValid, "expected packet to be
valid before serialization")) so test failures show schema/validation problems
earlier; place this check just before the existing Assert.AreEqual that compares
Serializer.Serialize(packet).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c36c5ee-52da-4d76-9731-05c2d6fff4ef
📒 Files selected for processing (7)
src/NosCore.Packets/ServerPackets/Inventory/InvPacket.cssrc/NosCore.Packets/ServerPackets/Inventory/IvnPacket.cssrc/NosCore.Packets/ServerPackets/Warehouse/FStashAllPacket.cssrc/NosCore.Packets/ServerPackets/Warehouse/FStashClientPacket.cssrc/NosCore.Packets/ServerPackets/Warehouse/PStashClientPacket.cssrc/NosCore.Packets/ServerPackets/Warehouse/StashClientPacket.cstest/NosCore.Packets.Tests/SerializerTest.cs
Real trace captures of ivn with inventory type 0 (equipment) carry seven dot-separated values — e.g. ivn 0 20.4998.0.0.0.0.0 — but the sub-packet only declared five (Slot, VNum, RareAmount, UpgradeDesign, SecondUpgrade), so NosCore was shipping slot.vnum.rare.upgrade.0 with two fields missing. The client never cleared the source slot after an mvi because the packet it received didn't match the expected shape. Added Unknown5 (byte) and Unknown6 (short) at indices 5 and 6. Names are placeholders — reverse-engineering suggests index 5 may be an IsBound flag and index 6 the rune/option count — but without confirmation we keep them opaque. Tests pin both sides of a move to the exact trace: - SerializeIvnPacketWithClearedSlotUsesZeroVNum → "ivn 0 19.0.0.0.0.0.0" - SerializeIvnPacketWithPopulatedEquipmentSlotMatchesTrace → "ivn 0 20.4998.0.0.0.0.0" Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ch trace
Audit against vanosilla (C:\Users\erwan\Desktop\vanosilla) named the
following fields that NosCore.Packets left as Unknown. Each rename is
backed by a direct vanosilla generator line at the same wire position.
Renames (all source-breaking, wire-identical):
- DropPacket.Unknown (idx 5, byte) -> IsQuest (bool).
vanosilla MapExtensions.cs: $"drop {vnum} {id} {x} {y} {amount} {(isQuest ? 1 : 0)} {owner}"
- InCharacterSubPacket.Unknown (idx 11, byte) -> FairyBooster (bool).
vanosilla CharacterPacketExtension.cs line 733: (IsUsingFairyBooster() ? 1 : 0)
- InCharacterSubPacket.Morph (idx 12) -> FairyMorph. Old name was wrong;
vanosilla line 734 writes Fairy.GameItem.Morph here, not the character's.
- InCharacterSubPacket.Unknown2 (idx 13, byte) -> ShowInEffect (bool).
vanosilla line 735: (showInEffect ? 0 : 1)
- InCharacterSubPacket.Unknown3 (idx 14, byte) -> Morph. This is the
character's actual Morph (vanosilla line 736: character.Morph).
- ScnPacket.Unknown21 (idx 34, int) -> IsTeamMember (bool).
vanosilla MatePacketExtensions.cs line 254: (mateEntity.IsTeamMember ? 1 : 0)
- ScnPacket.Unknown22 (idx 35, int) -> LevelXp. vanosilla line 255:
_algorithm.GetLevelXp(mateEntity).
InItemSubPacket position fix
The current layout emits {amount} {isQuest} {owner} {0} but the real
trace for "in 9" and vanosilla both end {amount} {isQuest} 0 {owner}:
trace: in 9 1046 6467573 52 154 10 0 0 14643732
The Unknown byte at index 3 was effectively the "0" that NosTale inserted
before ownerId in a protocol revision. NosCore put it at the wrong end.
Fix: move Unknown to index 2, Owner to index 3. Added regression test
SerializeInPacketForItemPutsOwnerAfterUnknownZero pinning the trace.
Not attempted (no vanosilla evidence at that index): ClistPacket,
NInvPacket, AtPacket, EquipmentSubPacket, PinitSubPacket, MlInfoPacket,
BfePacket, ScPStcPacket, EffObPacket, EffTPacket, RsfiPacket, CtlPacket,
StpSubPacket, LevPacket, ScnPacket.Unknown/Unknown2 (vanosilla hardcodes 0),
CInfoPacket.Unknown1 (vanosilla hardcodes a dash), IvnSubPacket.Unknown5/6
(vanosilla knows RunesCount but trace has one extra unidentified field,
so the position of RunesCount is ambiguous without a runes-populated
capture).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Major because this PR now carries several source-breaking changes beyond the original CModePacket.MorphSkin widening: IvnPacket/InvPacket/Stash* move to List<IvnSubPacket> (non-nullable items), InItemSubPacket swaps indices 2 and 3 so Owner sits after the Unknown zero, and the audited rename pass touches DropPacket / InCharacterSubPacket / ScnPacket.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NosCore.Packets/ServerPackets/Mates/ScnPacket.cs (1)
120-124: Add a focusedsc_nregression test for indices 34/35.Since this is a wire-schema semantic change, add a deserialize/serialize round-trip test that pins Line 120 as
0/1and Line 124 with a large value (e.g.,285816) to prevent future index/type drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NosCore.Packets/ServerPackets/Mates/ScnPacket.cs` around lines 120 - 124, Add a focused round-trip regression test for the ScnPacket wire schema that ensures PacketIndex 34/35 don't drift: create a test (e.g., ScnPacket_Index34_35_RoundTrip) that crafts or loads a raw packet with index 34 set to a boolean byte (0 or 1) and index 35 set to the large integer 285816, then deserialize into the ScnPacket type, assert ScnPacket.IsTeamMember equals the pinned boolean and ScnPacket.LevelXp equals 285816, re-serialize the ScnPacket and assert the output bytes match the original raw bytes exactly to lock the schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NosCore.Packets/ServerPackets/Mates/ScnPacket.cs`:
- Around line 120-124: Add a focused round-trip regression test for the
ScnPacket wire schema that ensures PacketIndex 34/35 don't drift: create a test
(e.g., ScnPacket_Index34_35_RoundTrip) that crafts or loads a raw packet with
index 34 set to a boolean byte (0 or 1) and index 35 set to the large integer
285816, then deserialize into the ScnPacket type, assert ScnPacket.IsTeamMember
equals the pinned boolean and ScnPacket.LevelXp equals 285816, re-serialize the
ScnPacket and assert the output bytes match the original raw bytes exactly to
lock the schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b82b4034-f0a6-4a85-a54e-b1ca1f744ba8
📒 Files selected for processing (5)
src/NosCore.Packets/ServerPackets/Entities/DropPacket.cssrc/NosCore.Packets/ServerPackets/Mates/ScnPacket.cssrc/NosCore.Packets/ServerPackets/Visibility/InCharacterSubPacket.cssrc/NosCore.Packets/ServerPackets/Visibility/InItemSubPacket.cstest/NosCore.Packets.Tests/SerializerTest.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/NosCore.Packets.Tests/SerializerTest.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bumps to 17.0.0 — breaking because of the `CModePacket.MorphSkin` field type change.
Deserializer — empty `SpecialSeparator` (non-breaking bug fix)
Four declarations use `[PacketIndex(n, SpecialSeparator = "")]` around a `UpgradeRareSubPacket`:
The serializer handles `SpecialSeparator = ""` correctly: it joins the sub-packet's fields with no separator (e.g. `Upgrade=7, Rare=5` → `"75"` on the wire). The deserializer was reaching `matches[currentIndex].Replace("", " ")`, which .NET rejects with `ArgumentException: The value cannot be an empty string`. Any incoming `equip` / `eq` / `in` with an upgrade-rare field would blow up.
Fix branches on empty separator and inserts a space between every character via `string.Join(' ', chars)`. Existing separators (`.`, `|`, `^`) are untouched.
Regression test added: round-trips `equip 75 35` and asserts `Upgrade=7, Rare=5` / `Upgrade=3, Rare=5`.
`CModePacket.MorphSkin` byte → short (BREAKING)
Real captures consistently carry 4-digit values (e.g. `7583`, `8151`, `8558`, `9471`) in the last field, which overflows `byte`. Widening to `short` matches the observed range and stops the "Value was either too large or too small for an unsigned byte" failures. Source-level breaking — consumers pinning the property type to `byte` stop compiling; at runtime the wider type is always safe.
Test plan
Follow-ups
The packet validator surfaced more schema bugs that are not in this PR:
Summary by CodeRabbit
Bug Fixes
Data/Protocol Changes
Tests
Chores