diff --git a/docs/superpowers/plans/2026-05-10-issue-13-pd-trailer.md b/docs/superpowers/plans/2026-05-10-issue-13-pd-trailer.md index 1961065..019939c 100644 --- a/docs/superpowers/plans/2026-05-10-issue-13-pd-trailer.md +++ b/docs/superpowers/plans/2026-05-10-issue-13-pd-trailer.md @@ -154,9 +154,17 @@ public readonly record struct Parsed( uint SpellbookFilters, ReadOnlyMemory GameplayOptions, IReadOnlyList Inventory, - IReadOnlyList Equipped); + IReadOnlyList 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 3–9 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.** Replace the body of `BuildPartial` (~line 275) with: @@ -179,7 +187,8 @@ private static Parsed BuildPartial( 0u, ReadOnlyMemory.Empty, System.Array.Empty(), - System.Array.Empty()); + System.Array.Empty(), + TrailerTruncated: false); } ``` @@ -198,7 +207,8 @@ return new Parsed( 0u, ReadOnlyMemory.Empty, System.Array.Empty(), - System.Array.Empty()); + System.Array.Empty(), + TrailerTruncated: false); ``` - [ ] **Step 5: Run the build + existing tests to verify no regressions.** @@ -283,6 +293,7 @@ List<(uint, uint)> desiredComps = new(); ReadOnlyMemory gameplayOptions = ReadOnlyMemory.Empty; List inventory = new(); List equipped = new(); +bool trailerTruncated = false; try { @@ -292,9 +303,14 @@ try 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( @@ -302,9 +318,14 @@ return new Parsed( bundle, positions, attributes, skills, spells, enchantments, optionFlags, options1, options2, shortcuts, hotbarSpells, desiredComps, spellbookFilters, - gameplayOptions, inventory, equipped); + gameplayOptions, inventory, equipped, trailerTruncated); ``` +> **Tasks 3–9 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.** Run: `dotnet test --filter "FullyQualifiedName~TryParse_TrailerOptionFlagsAndOptions1"` diff --git a/src/AcDream.Core.Net/Messages/PlayerDescriptionParser.cs b/src/AcDream.Core.Net/Messages/PlayerDescriptionParser.cs index 209257d..065d1d2 100644 --- a/src/AcDream.Core.Net/Messages/PlayerDescriptionParser.cs +++ b/src/AcDream.Core.Net/Messages/PlayerDescriptionParser.cs @@ -227,6 +227,13 @@ public static class PlayerDescriptionParser uint EquipLocation, uint Priority); + /// Result of . Trailer fields + /// (OptionFlags through Equipped) may be partially + /// populated when is true — + /// 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. public readonly record struct Parsed( uint WeenieType, DescriptionPropertyFlag PropertyFlags, @@ -247,7 +254,8 @@ public static class PlayerDescriptionParser uint SpellbookFilters, ReadOnlyMemory GameplayOptions, IReadOnlyList Inventory, - IReadOnlyList Equipped); + IReadOnlyList Equipped, + bool TrailerTruncated); /// /// Parse a PlayerDescription payload. The 0xF7B0 envelope has been @@ -325,6 +333,7 @@ public static class PlayerDescriptionParser ReadOnlyMemory gameplayOptions = ReadOnlyMemory.Empty; List inventory = new(); List equipped = new(); + bool trailerTruncated = false; try { @@ -334,9 +343,16 @@ public static class PlayerDescriptionParser 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( @@ -344,7 +360,7 @@ public static class PlayerDescriptionParser bundle, positions, attributes, skills, spells, enchantments, optionFlags, options1, options2, shortcuts, hotbarSpells, desiredComps, spellbookFilters, - gameplayOptions, inventory, equipped); + gameplayOptions, inventory, equipped, trailerTruncated); } catch (FormatException ex) { @@ -373,7 +389,8 @@ public static class PlayerDescriptionParser 0u, ReadOnlyMemory.Empty, System.Array.Empty(), - System.Array.Empty()); + System.Array.Empty(), + TrailerTruncated: false); } // ── Attribute block reader ────────────────────────────────────────────── diff --git a/tests/AcDream.Core.Net.Tests/PlayerDescriptionParserTests.cs b/tests/AcDream.Core.Net.Tests/PlayerDescriptionParserTests.cs index 2e2fe75..1c25333 100644 --- a/tests/AcDream.Core.Net.Tests/PlayerDescriptionParserTests.cs +++ b/tests/AcDream.Core.Net.Tests/PlayerDescriptionParserTests.cs @@ -362,5 +362,52 @@ public sealed class PlayerDescriptionParserTests Assert.Equal(0xDEADBEEFu, parsed.Value.Options1); Assert.Empty(parsed.Value.Shortcuts); 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); } }