Skip to content

fix: stop appending tail when sub-packet has fewer fields than schema#450

Merged
erwan-joly merged 1 commit intomasterfrom
fix/deserializer-short-subpacket
Apr 24, 2026
Merged

fix: stop appending tail when sub-packet has fewer fields than schema#450
erwan-joly merged 1 commit intomasterfrom
fix/deserializer-short-subpacket

Conversation

@erwan-joly
Copy link
Copy Markdown
Contributor

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

Bumps to 17.2.0, non-breaking bug fix.

Single-line change in the inner field-split loop plus an early-exit. The loop was iterating for every property of the schema, appending the remaining subpacket on every iteration once the separator was exhausted — producing values like "1 9023 33333" from "1.9023.3" against a 7-field schema, which then failed with Int16 overflow, index-out-of-bounds, or format errors depending on the field type.

Resolves observed failures on inv / sayi / msgi / msgi2 / mlobjlst / infoi / qslot / fish / qstlist / pinit.

Test plan

  • 114/114 green
  • New regression test round-trips inv 1 0.5110.1 1.9023.3 against the seven-field IvnSubPacket and asserts the trailing fields default.

Summary by CodeRabbit

  • Bug Fixes

    • Resolved a deserialization issue where sub-packet list processing would incorrectly continue when wire tokens contained fewer fields than the schema expects, preventing potential data corruption during packet parsing.
  • Chores

    • Package version updated to 17.2.0.

The inner field-split loop ran packSeperators.Count iterations
regardless of whether the wire item had been exhausted. Once the
separator was no longer found (c == -1) on a non-last iteration, it
appended the remaining subpacket on every subsequent iteration —
producing e.g. "1 9023 33333" from "1.9023.3" against a seven-field
IvnSubPacket, which then overflowed RareAmount (short) or raised
index-out-of-bounds / format errors downstream.

Break after the first exhausted separator: emit the tail once and
let the outer DeserializeIPacket leave the rest of the schema's
trailing fields at their default values.

Resolves observed failures on:
  inv 1/2              — "Value was either too large or too small for an Int16."
  sayi / msgi / msgi2  — "Index was outside the bounds of the array."
  mlobjlst / infoi     — "Index was outside the bounds of the array."
  qslot                — "The input string '-1-1' was not in a correct format."
  fish                 — "The input string '0.0.0' was not in a correct format."
  qstlist              — "The input string '39.0.0.0.0...' was not in a correct format."
  pinit                — "Requested value '2|5821712|...' was not found."

Regression test added round-tripping `inv 1 0.5110.1 1.9023.3` against
the seven-field IvnSubPacket.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@erwan-joly erwan-joly merged commit 38df3a5 into master Apr 24, 2026
@erwan-joly erwan-joly deleted the fix/deserializer-short-subpacket branch April 24, 2026 08:55
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6b419ebd-5001-4e0c-be31-f2bb233227dd

📥 Commits

Reviewing files that changed from the base of the PR and between f0ac6db and 8afa4e8.

📒 Files selected for processing (3)
  • src/NosCore.Packets/Deserializer.cs
  • src/NosCore.Packets/NosCore.Packets.csproj
  • test/NosCore.Packets.Tests/DeserializerTest.cs

Walkthrough

The pull request fixes a bug in DeserializeList where iteration continues past available wire separators, potentially appending remaining sub-packet data multiple times. A break statement is added when separators are exhausted, and a test case validates the fix.

Changes

Cohort / File(s) Summary
Deserialization Logic Fix
src/NosCore.Packets/Deserializer.cs
Modifies DeserializeList to stop iterating schema properties when wire tokens lack expected separators; adds break condition when IndexOf returns -1 to prevent duplicate appending of remaining sub-packet data.
Test Coverage
test/NosCore.Packets.Tests/DeserializerTest.cs
Adds PacketWithSubPacketShorterThanSchemaLeavesTrailingFieldsDefault() test to verify correct handling of sub-packets with fewer fields than the schema definition.
Version Update
src/NosCore.Packets/NosCore.Packets.csproj
Bumps NuGet package version from 17.1.0 to 17.2.0.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

✨ 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-short-subpacket

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

erwan-joly added a commit to NosCoreIO/NosCore.DeveloperTools that referenced this pull request Apr 24, 2026
Picks up:
- NosCoreIO/NosCore.Packets#450 (short-subpacket deserialize fix —
  inv/sayi/msgi/msgi2/mlobjlst/infoi/qslot/fish/qstlist/pinit)
- NosCoreIO/NosCore.Packets#451 (TwkPacket setters, PetskiPacket schema,
  QnamliPacket)
- NosCoreIO/NosCore.Packets#452 (direction-pair packets: gidx/npc_req/
  rsfi server-side, mall/npinfo client-side)

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