refactor(net): #13 Parsed.TrailerTruncated + diag logging

Code-quality review followup on Task 2 (becbde6) — addresses I1 (the
forward-looking concern that Tasks 3-9's inner-catch will leave partial
lists visible to callers with no signal) and M1 (silent inner catch).

Changes:
  - Parsed gains a trailing `bool TrailerTruncated` field. Both
    construction sites pass `false` by default; the trailer try/catch
    flips a local `trailerTruncated` to `true` on FormatException and
    feeds it into the final return.
  - Inner catch logs `pos`/`payload.Length`/exception message under
    ACDREAM_DUMP_VITALS=1, mirroring the outer catch's diagnostic
    pattern.
  - Task 2 test strengthened to assert defaults on Options2 /
    SpellbookFilters / HotbarSpells / DesiredComps / GameplayOptions /
    Equipped + TrailerTruncated=false (M2 followup — gives Tasks 3-9
    a regression guard if they consume into the wrong field).
  - New test `TryParse_TrailerAbsent_LessThan8BytesAfterEnchantments_*`
    documents the contract that <8 bytes after enchantments means the
    trailer is absent (not truncated): TrailerTruncated stays false,
    upstream attribute data survives.
  - Plan updated in lockstep so Tasks 3-11 implementers see the
    `trailerTruncated` local and the new return-arg position.

271/271 AcDream.Core.Net.Tests pass.
This commit is contained in:
Erik 2026-05-10 08:26:08 +02:00
parent becbde60a4
commit 9a0dfe03da
3 changed files with 96 additions and 11 deletions

View file

@ -154,9 +154,17 @@ public readonly record struct Parsed(
uint SpellbookFilters, uint SpellbookFilters,
ReadOnlyMemory<byte> GameplayOptions, ReadOnlyMemory<byte> GameplayOptions,
IReadOnlyList<InventoryEntry> Inventory, IReadOnlyList<InventoryEntry> Inventory,
IReadOnlyList<EquippedEntry> Equipped); IReadOnlyList<EquippedEntry> Equipped,
bool TrailerTruncated);
``` ```
> **Code-review followup (added after Task 2 review):** the trailing
> `TrailerTruncated` flag was added to let callers distinguish a clean
> parse from one where the trailer try/catch swallowed a `FormatException`
> mid-section (Tasks 39 will make this reachable). All construction sites
> pass `TrailerTruncated: false` by default; the trailer try/catch in
> `TryParse` flips a local to `true` on catch.
- [ ] **Step 3: Update `BuildPartial` to fill the new fields with defaults.** - [ ] **Step 3: Update `BuildPartial` to fill the new fields with defaults.**
Replace the body of `BuildPartial` (~line 275) with: Replace the body of `BuildPartial` (~line 275) with:
@ -179,7 +187,8 @@ private static Parsed BuildPartial(
0u, 0u,
ReadOnlyMemory<byte>.Empty, ReadOnlyMemory<byte>.Empty,
System.Array.Empty<InventoryEntry>(), System.Array.Empty<InventoryEntry>(),
System.Array.Empty<EquippedEntry>()); System.Array.Empty<EquippedEntry>(),
TrailerTruncated: false);
} }
``` ```
@ -198,7 +207,8 @@ return new Parsed(
0u, 0u,
ReadOnlyMemory<byte>.Empty, ReadOnlyMemory<byte>.Empty,
System.Array.Empty<InventoryEntry>(), System.Array.Empty<InventoryEntry>(),
System.Array.Empty<EquippedEntry>()); System.Array.Empty<EquippedEntry>(),
TrailerTruncated: false);
``` ```
- [ ] **Step 5: Run the build + existing tests to verify no regressions.** - [ ] **Step 5: Run the build + existing tests to verify no regressions.**
@ -283,6 +293,7 @@ List<(uint, uint)> desiredComps = new();
ReadOnlyMemory<byte> gameplayOptions = ReadOnlyMemory<byte>.Empty; ReadOnlyMemory<byte> gameplayOptions = ReadOnlyMemory<byte>.Empty;
List<InventoryEntry> inventory = new(); List<InventoryEntry> inventory = new();
List<EquippedEntry> equipped = new(); List<EquippedEntry> equipped = new();
bool trailerTruncated = false;
try try
{ {
@ -292,9 +303,14 @@ try
options1 = ReadU32(payload, ref pos); options1 = ReadU32(payload, ref pos);
} }
} }
catch (FormatException) catch (FormatException ex)
{ {
// Trailer corrupted — keep what we have and return. // Trailer corrupted — keep what we have and flag it. Tasks 3-9
// can leave partial lists in scope; TrailerTruncated lets callers
// ignore the trailer when they need all-or-nothing semantics.
trailerTruncated = true;
if (System.Environment.GetEnvironmentVariable("ACDREAM_DUMP_VITALS") == "1")
System.Console.WriteLine($"PlayerDescriptionParser: trailer FormatException at pos={pos}/{payload.Length}: {ex.Message}");
} }
return new Parsed( return new Parsed(
@ -302,9 +318,14 @@ return new Parsed(
bundle, positions, attributes, skills, spells, enchantments, bundle, positions, attributes, skills, spells, enchantments,
optionFlags, options1, options2, optionFlags, options1, options2,
shortcuts, hotbarSpells, desiredComps, spellbookFilters, shortcuts, hotbarSpells, desiredComps, spellbookFilters,
gameplayOptions, inventory, equipped); gameplayOptions, inventory, equipped, trailerTruncated);
``` ```
> **Tasks 39 note:** every `return new Parsed(...)` extension or
> rewrite in subsequent tasks must include `trailerTruncated` as the
> final positional argument, and any new try-blocks that read trailer
> sections should set `trailerTruncated = true;` in their catch.
- [ ] **Step 4: Run the test — expect PASS.** - [ ] **Step 4: Run the test — expect PASS.**
Run: `dotnet test --filter "FullyQualifiedName~TryParse_TrailerOptionFlagsAndOptions1"` Run: `dotnet test --filter "FullyQualifiedName~TryParse_TrailerOptionFlagsAndOptions1"`

View file

@ -227,6 +227,13 @@ public static class PlayerDescriptionParser
uint EquipLocation, uint EquipLocation,
uint Priority); uint Priority);
/// <summary>Result of <see cref="TryParse"/>. Trailer fields
/// (<c>OptionFlags</c> through <c>Equipped</c>) may be partially
/// populated when <see cref="TrailerTruncated"/> is <c>true</c> —
/// the parse degraded gracefully rather than discarding upstream
/// attribute / skill / spell / enchantment data. Callers that
/// require all-or-nothing trailer semantics should ignore the
/// trailer fields when this flag is set.</summary>
public readonly record struct Parsed( public readonly record struct Parsed(
uint WeenieType, uint WeenieType,
DescriptionPropertyFlag PropertyFlags, DescriptionPropertyFlag PropertyFlags,
@ -247,7 +254,8 @@ public static class PlayerDescriptionParser
uint SpellbookFilters, uint SpellbookFilters,
ReadOnlyMemory<byte> GameplayOptions, ReadOnlyMemory<byte> GameplayOptions,
IReadOnlyList<InventoryEntry> Inventory, IReadOnlyList<InventoryEntry> Inventory,
IReadOnlyList<EquippedEntry> Equipped); IReadOnlyList<EquippedEntry> Equipped,
bool TrailerTruncated);
/// <summary> /// <summary>
/// Parse a PlayerDescription payload. The 0xF7B0 envelope has been /// Parse a PlayerDescription payload. The 0xF7B0 envelope has been
@ -325,6 +333,7 @@ public static class PlayerDescriptionParser
ReadOnlyMemory<byte> gameplayOptions = ReadOnlyMemory<byte>.Empty; ReadOnlyMemory<byte> gameplayOptions = ReadOnlyMemory<byte>.Empty;
List<InventoryEntry> inventory = new(); List<InventoryEntry> inventory = new();
List<EquippedEntry> equipped = new(); List<EquippedEntry> equipped = new();
bool trailerTruncated = false;
try try
{ {
@ -334,9 +343,16 @@ public static class PlayerDescriptionParser
options1 = ReadU32(payload, ref pos); options1 = ReadU32(payload, ref pos);
} }
} }
catch (FormatException) catch (FormatException ex)
{ {
// Trailer corrupted — keep what we have and return. // Trailer corrupted — keep what we have and flag it. Once
// Tasks 3-9 add list reads inside this try block, partial
// lists may be visible to callers; TrailerTruncated tells
// them so they can ignore the trailer if they need all-or-
// nothing semantics.
trailerTruncated = true;
if (System.Environment.GetEnvironmentVariable("ACDREAM_DUMP_VITALS") == "1")
System.Console.WriteLine($"PlayerDescriptionParser: trailer FormatException at pos={pos}/{payload.Length}: {ex.Message}");
} }
return new Parsed( return new Parsed(
@ -344,7 +360,7 @@ public static class PlayerDescriptionParser
bundle, positions, attributes, skills, spells, enchantments, bundle, positions, attributes, skills, spells, enchantments,
optionFlags, options1, options2, optionFlags, options1, options2,
shortcuts, hotbarSpells, desiredComps, spellbookFilters, shortcuts, hotbarSpells, desiredComps, spellbookFilters,
gameplayOptions, inventory, equipped); gameplayOptions, inventory, equipped, trailerTruncated);
} }
catch (FormatException ex) catch (FormatException ex)
{ {
@ -373,7 +389,8 @@ public static class PlayerDescriptionParser
0u, 0u,
ReadOnlyMemory<byte>.Empty, ReadOnlyMemory<byte>.Empty,
System.Array.Empty<InventoryEntry>(), System.Array.Empty<InventoryEntry>(),
System.Array.Empty<EquippedEntry>()); System.Array.Empty<EquippedEntry>(),
TrailerTruncated: false);
} }
// ── Attribute block reader ────────────────────────────────────────────── // ── Attribute block reader ──────────────────────────────────────────────

View file

@ -362,5 +362,52 @@ public sealed class PlayerDescriptionParserTests
Assert.Equal(0xDEADBEEFu, parsed.Value.Options1); Assert.Equal(0xDEADBEEFu, parsed.Value.Options1);
Assert.Empty(parsed.Value.Shortcuts); Assert.Empty(parsed.Value.Shortcuts);
Assert.Empty(parsed.Value.Inventory); Assert.Empty(parsed.Value.Inventory);
// Defaults for the trailer fields not yet read (Tasks 3-9 will
// populate them). Asserting them here gives those tasks a
// pre-existing regression guard if they accidentally consume into
// the wrong field's wire bytes.
Assert.Equal(0u, parsed.Value.Options2);
Assert.Equal(0u, parsed.Value.SpellbookFilters);
Assert.Empty(parsed.Value.HotbarSpells);
Assert.Empty(parsed.Value.DesiredComps);
Assert.True(parsed.Value.GameplayOptions.IsEmpty);
Assert.Empty(parsed.Value.Equipped);
Assert.False(parsed.Value.TrailerTruncated);
}
[Fact]
public void TryParse_TrailerAbsent_LessThan8BytesAfterEnchantments_PreservesUpstreamAndDoesNotFlagTruncation()
{
// Fewer than 8 bytes remain after the enchantment block, so the
// trailer header is treated as absent (no read attempted). Upstream
// attribute data must survive; TrailerTruncated stays false because
// the parser never *started* the trailer — it correctly skipped it.
// (Tasks 3-9 will introduce truncation-mid-section cases that flip
// TrailerTruncated to true.)
var sb = new MemoryStream();
using var writer = new BinaryWriter(sb);
writer.Write(0u); // propertyFlags
writer.Write(0x52u); // weenieType
writer.Write(0x201u); // ATTRIBUTE | ENCHANTMENT
writer.Write(1u); // has_health
// Attribute block: only Strength (bit 0).
writer.Write(0x01u);
writer.Write(50u); writer.Write(10u); writer.Write(0u);
// Empty enchantment mask.
writer.Write(0u);
// Truncated trailer: only 4 bytes (would-be option_flags) instead of 8.
writer.Write(0xCAFEu);
var parsed = PlayerDescriptionParser.TryParse(sb.ToArray());
Assert.NotNull(parsed);
// Upstream attribute survived.
Assert.Single(parsed!.Value.Attributes);
Assert.Equal(1u, parsed.Value.Attributes[0].AtType);
// Trailer was absent (< 8 bytes), so no truncation flag and all
// trailer fields stay at their initial defaults.
Assert.False(parsed.Value.TrailerTruncated);
Assert.Equal(PlayerDescriptionParser.CharacterOptionDataFlag.None, parsed.Value.OptionFlags);
Assert.Equal(0u, parsed.Value.Options1);
} }
} }