Skip to content

fix: deserialize list-of-subpackets with pipe/dot SpecialSeparator#449

Merged
erwan-joly merged 1 commit intomasterfrom
fix/deserializer-list-subpackets
Apr 24, 2026
Merged

fix: deserialize list-of-subpackets with pipe/dot SpecialSeparator#449
erwan-joly merged 1 commit intomasterfrom
fix/deserializer-list-subpackets

Conversation

@erwan-joly
Copy link
Copy Markdown
Contributor

@erwan-joly erwan-joly commented Apr 24, 2026

Bumps to 17.1.0 — non-breaking bug fix.

What's broken

Lists declared like `ClinitPacket`:

```csharp
[PacketListIndex(0, SpecialSeparator = "|")]
public List<ClinitSubPacket?>? SubPackets { get; set; }
```

…serialise to `clinit 14531|94|0|396|Raizen 3011|99|0|346|Thanos` — list items space-separated, inner fields `|`-separated. The existing serializer test at SerializerTest.cs:1102 locks this exact shape in. But the deserializer couldn't read its own output. Two coupled bugs:

  1. `Deserializer.cs:325` picked `SpecialSeparator` as the list-item separator for IPacket lists, then ran `matches[currentIndex].Split("|")` over the first token — turning `"5130502|99|68|547|Trippybell"` into `length=5`. Looping 5 times while the actual list has 30+ items indexed straight off the end of `matches`.

  2. `Deserializer.cs:364`'s inner field-split loop looked up the next property's `SpecialSeparator`, falling back to `.` when the sub-packet's properties didn't carry their own separator (the normal case for Clinit/Flinit/Finit/Pinit/etc. style sub-packets). `.IndexOf(".")` returned -1 and the `c == -1` branch appended the whole sub-packet once per property — producing `"5130502|99|68|547|Trippybell5130502|99|68|547|Trippybell5130502|99|68|547|Trippybell5130502|99|68|547|Trippybell5130502|99|68|547|Trippybell"` that `long.Parse` rightly rejected.

The fix

  1. For IPacket lists, ignore `SpecialSeparator` as list-item separator — it's the inner-field separator. `ListSeparator` (when set) still wins. Non-IPacket primitive lists keep the existing `SpecialSeparator` / `.` behaviour.

  2. The inner field-split loop falls back to the outer list's `SpecialSeparator` when the sub-packet property doesn't override it.

What this unblocks

Inbound round-trip for any list-of-subpackets where inner fields share a separator: `clinit`, `flinit`, `kdlinit`, `skyinit`, `finit`, `pinit`, `blinit`, `rcblist`, `rcslist`, `ninv`, `mlobjlst`, `tarank`, plus anything new.

Test plan

  • `dotnet test` — 113 / 113 pass, including the new regression test round-tripping a two-entry ClinitPacket.
  • Existing serializer round-trip test for ClinitPacket still passes (serializer output unchanged — only deserializer behaviour changed).

Summary by CodeRabbit

  • Bug Fixes

    • Fixed packet list deserialization to properly handle nested sub-packets with custom separators. Corrected boundary detection during token parsing that previously caused incomplete sub-packet processing.
  • Tests

    • Added test coverage for complex packet list deserialization scenarios with special separators and nested sub-packets without individual property separators.

Lists like ClinitPacket (`[PacketListIndex(0, SpecialSeparator = "|")]`)
carry items space-separated on the wire, with inner fields joined by "|".
The serializer produces this correctly. The deserializer had two bugs:

1. Line 325 picked SpecialSeparator as the list-item separator for IPacket
   lists, then tried to split the first match token by "|" to count items —
   turning "5130502|99|68|547|Trippybell" into length=5 instead of reading
   each whitespace-separated item. This inflated length and indexed past
   the end of the matches array.

2. Line 356's inner field-split loop looked up the next property's
   SpecialSeparator, falling back to "." when the sub-packet's properties
   didn't decorate their own separators. For ClinitSubPacket-style packets
   that fallback never matches the subpacket content, so the whole item
   was appended once per property — yielding "X|..X|..X|..X|..X|.." that
   failed to parse.

Fix 1: for IPacket lists, ignore SpecialSeparator as list-item separator —
it's the inner field separator. ListSeparator (when set) still wins.

Fix 2: the inner field-split loop falls back to the outer list's
SpecialSeparator when the sub-packet's properties don't specify one.

Unlocks deserialisation of clinit / flinit / kdlinit / skyinit / finit /
pinit / blinit / rcblist / rcslist / ninv / mlobjlst / tarank and the
inner-field handling of any future list of sub-packets with shared
separator.

Regression test added on ClinitPacket. Primitive-list handling (stp,
stp2, bpp, bpm, exclist, nstest server list) left unchanged — confirmed
by full suite passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Updates deserialization logic for lists of IPacket to adjust separator selection and add a fallback delimiter for locating property boundaries within sub-packet tokens. Includes version bump and new test coverage for the modified parsing behavior.

Changes

Cohort / File(s) Summary
Deserialization Logic
src/NosCore.Packets/Deserializer.cs
Modified DeserializeList to handle SpecialSeparator/ListSeparator differently for IPacket lists. Adjusts outer separator selection and adds fieldSeparator fallback from SpecialSeparator (defaulting to "."), used when detecting property boundaries in sub-packet tokens to prevent repeated boundary detection failures.
Test Coverage
test/NosCore.Packets.Tests/DeserializerTest.cs
Added test method PacketWithListOfSubPacketsWithoutPerPropertySeparatorsSplitsCorrectly() validating deserialization of list-of-sub-packets with `SpecialSeparator = "
Package Metadata
src/NosCore.Packets/NosCore.Packets.csproj
Version bumped from 17.0.0 to 17.1.0.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • PR #446: Also modifies Deserializer.cs IPacket nested-list parsing to adjust SpecialSeparator handling with fallback behavior for separator edge cases.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely summarizes the main fix: improving deserialization of list-of-subpackets when using pipe/dot SpecialSeparator, which is the primary change across all modified files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/deserializer-list-subpackets

Comment @coderabbitai help to get the list of available commands and usage tips.

@erwan-joly erwan-joly merged commit f0ac6db into master Apr 24, 2026
1 of 3 checks passed
@erwan-joly erwan-joly deleted the fix/deserializer-list-subpackets branch April 24, 2026 08:37
erwan-joly added a commit to NosCoreIO/NosCore.DeveloperTools that referenced this pull request Apr 24, 2026
Picks up NosCoreIO/NosCore.Packets#449 — list-of-sub-packets
deserialization fix (clinit/flinit/kdlinit/skyinit/finit/pinit/blinit
and anything else using a pipe/dot inner-field separator).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant