From ed308087204ab46298b1cd0dbfde78c33a1df9f8 Mon Sep 17 00:00:00 2001 From: Erik Date: Wed, 12 Aug 2026 00:46:42 +0200 Subject: [PATCH] fix(net): FA1 review round -- zero-id tree rejection, monarch clear, 0x001F builder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies both MUST-FIX items and the code-facing SHOULD-FIX items from the dual-lens FA1 review (docs/research/2026-08-12-fa1-review-mechanism.md, docs/research/2026-08-12-fa1-review-blast.md): Mechanism MF-1 / blast SF-2: AllegianceHierarchy::Add @0x005B6E90 wraps its entire body in `if (_id != 0)` -- a record whose own id is zero discards the WHOLE message, for both the monarch and a child record, and this is also what makes treeParent == 0 unconditionally fatal for a non-monarch record. ReadAllegianceProfileBody now rejects CharacterId == 0 on both paths; four new boundary tests in AllegianceProfileVersionGateTests.cs (zero-id monarch, zero-id child, zero treeParent, plus the existing orphan/self-parent/duplicate trio). Mechanism MF-2: added the missing 0x001F AllegianceUpdateRequest builder -- the structural twin of the fellowship 0x00A6 this slice already repaired -- with golden-vector tests for both on/off. Mechanism SF-1 / blast SF-3: UnPack's last act before returning success forces the monarch's MayPassupExperience to false regardless of the wire bit or the HasPackedLevel-absent legacy-compat fallback. Ported at the end of the record loop; the pre-existing HasPackedLevel-absent test moved off the monarch record (which the new clear makes indistinguishable from "the fallback never fired") onto a vassal record, and a new test proves the monarch clear fires even when the wire bit explicitly asks for true. Mechanism SF-2: removed ParseFellowshipDisband's invented body-length validation -- retail's DispatchUI_Disband reads only the opcode and never inspects a trailing body. The parser now always succeeds; the matching test flips from asserting rejection to asserting acceptance. Mechanism SF-3: added the D5 `<<1` shareLoot-shape test at the 0x02C0 FellowshipUpdateFellow site -- previously only pinned at 0x02BE, so a future split of the shared ReadFellow helper could silently reintroduce a bool read on this leg undetected. Mechanism SF-5: renumbered the version-gate comments in ReadAllegianceProfileBody to the true AllegianceVersion enum values (1-11, matching acclient.h's SpokespersonAdded..ApprovedVassal) instead of wire-appearance order, which only reached 10 and silently dropped gate 5 (BannedCharactersAdded, which is real but gates nothing in UnPack -- now called out explicitly). Fixed the stale "lane B §12" citation in SocialActions.cs to the actual master-table row. Blast SF-1: pinned the two retail-faithful but user-visible behavior changes FA1 made to the ALREADY-LIVE `@allegiance info` command -- reversed vassal print order (3-vassal test through FormatAllegianceInfoLines) and malformed-tree silent-drop (test at the GameEventWiring registration layer, which is `if (info is null) return;`). Blast SF-4: fixed a doc comment citing a nonexistent `ConfirmationResponseTests` class; the actual class is `ConfirmationTripleTests`. Blast SF-5: cross-referenced the confirmation-triple discriminator's split representation (ConfirmationType on the response leg only; bare uint on the two inbound legs production actually reads) at both sites, so FA4 inherits a stated decision rather than an unexplained inconsistency. Full Release suite: 13,158 passed / 4 skipped / 0 failed (13,162 total), up from the pre-fix-round 13,149/4/0 (+9 tests this round). Co-Authored-By: Claude Fable 5 --- .../Messages/AllegianceRequests.cs | 29 ++++ .../Messages/ClientCommandResponses.cs | 70 +++++++--- src/AcDream.Core.Net/Messages/GameEvents.cs | 42 +++++- .../Messages/SocialActions.cs | 3 +- .../AllegianceProfileVersionGateTests.cs | 126 +++++++++++++++++- .../Messages/AllegianceRequestsTests.cs | 37 +++++ .../Messages/ClientCommandResponsesTests.cs | 76 +++++++++++ .../Messages/FellowshipEventsTests.cs | 36 ++++- 8 files changed, 385 insertions(+), 34 deletions(-) diff --git a/src/AcDream.Core.Net/Messages/AllegianceRequests.cs b/src/AcDream.Core.Net/Messages/AllegianceRequests.cs index ebb2b5d4..bc952531 100644 --- a/src/AcDream.Core.Net/Messages/AllegianceRequests.cs +++ b/src/AcDream.Core.Net/Messages/AllegianceRequests.cs @@ -37,6 +37,24 @@ public static class AllegianceRequests public const uint SwearOpcode = 0x001Du; public const uint BreakOpcode = 0x001Eu; + /// + /// 0x001F AllegianceUpdateRequest — the allegiance panel's + /// subscribe/unsubscribe toggle, the structural twin of the fellowship + /// 0x00A6 FellowshipUpdateRequest repaired elsewhere in this + /// slice. Retail's CM_Allegiance::Event_UpdateRequest(u32) + /// (@0x006A72BA, lane C §1.2) is NOT a one-shot query — send + /// 1 on gmAllegianceUI::PostInit's tail + /// (@0x004911C6) and on RecvNotice_PlayerDescReceived + /// (@0x00490D59), 1 on OnVisibilityChanged's + /// visible branch (@0x004912DD), 0 on its hidden branch + /// (@0x00491311). ACE's own handler + /// (GameActionAllegianceUpdateRequest.cs:12) reads the value + /// and ignores it — always replying once regardless — but retail + /// servers do not, so this must be sent correctly (lane C §7.1 + /// message #3). + /// + public const uint AllegianceUpdateRequestOpcode = 0x001Fu; + /// Pledge yourself to the given patron. public static byte[] BuildSwear(uint gameActionSequence, uint patronGuid) { @@ -72,6 +90,17 @@ public static class AllegianceRequests return Build(gameActionSequence, BreakOpcode, vassalGuid); } + /// + /// Declare the allegiance panel's visibility/subscription state to the + /// server (0x001F AllegianceUpdateRequest). See + /// for the four retail send + /// sites and their arguments. + /// + public static byte[] BuildAllegianceUpdateRequest(uint gameActionSequence, bool on) + { + return Build(gameActionSequence, AllegianceUpdateRequestOpcode, on ? 1u : 0u); + } + private static byte[] Build(uint seq, uint sub, uint targetGuid) { byte[] body = new byte[16]; diff --git a/src/AcDream.Core.Net/Messages/ClientCommandResponses.cs b/src/AcDream.Core.Net/Messages/ClientCommandResponses.cs index 43e32719..1159f295 100644 --- a/src/AcDream.Core.Net/Messages/ClientCommandResponses.cs +++ b/src/AcDream.Core.Net/Messages/ClientCommandResponses.cs @@ -408,11 +408,24 @@ public static class ClientCommandResponses ushort recordCount = ReadU16(payload, ref pos); ushort oldVersion = ReadU16(payload, ref pos); - // §4.2 gates 1/2: officers (oldVersion >= 6, - // MultipleAllegianceOfficersAdded) vs the legacy single - // spokesperson-id 4-byte skip (1 <= oldVersion < 6). Entries are - // consumed but not surfaced (ACE always sends officers empty — - // lane C §5.1) so every later field still lands correctly. + // §4.2's ELEVEN gates are the eleven non-zero AllegianceVersion + // enum values (acclient.h:2979-2994, SpokespersonAdded=1 through + // ApprovedVassal=11), numbered below by THAT version number — not + // by wire-appearance order, which is a different sequence (the + // officers table at version 6 is read before officer titles at + // version 9 but after the version-1/2/3/4 fields). Gate 5 + // (BannedCharactersAdded) is real — it is one of the eleven + // AllegianceVersion values — but gates NOTHING in UnPack: the ban + // list never rides this blob, so there is no field/read for it + // here (pinned by the negative test + // VersionGate_4to5_BannedCharactersAddedGatesNothing). + + // Gate 1 (SpokespersonAdded, 1 <= oldVersion < 6) vs gate 6 + // (MultipleAllegianceOfficersAdded, oldVersion >= 6): the officers + // PHashTable REPLACES the legacy single spokesperson-id 4-byte + // skip at version 6. Entries are consumed but not surfaced (ACE + // always sends officers empty — lane C §5.1) so every later field + // still lands correctly. if (oldVersion >= 6) { ushort officerCount = ReadU16(payload, ref pos); @@ -428,9 +441,8 @@ public static class ClientCommandResponses _ = ReadU32(payload, ref pos); // old single spokesperson id } - // §4.2 gate 3: officer titles (oldVersion >= 9, - // OfficersTitlesAdded) — PSmartArray: a bare i32 count, - // NOT the PackableHashTable u16/u16 header. + // Gate 9 (OfficersTitlesAdded, oldVersion >= 9) — PSmartArray: + // a bare i32 count, NOT the PackableHashTable u16/u16 header. if (oldVersion >= 9) { int titleCount = unchecked((int)ReadU32(payload, ref pos)); @@ -438,7 +450,7 @@ public static class ClientCommandResponses _ = StringReader.ReadString16L(payload, ref pos); } - // §4.2 gate 4 (PoolsAdded, oldVersion >= 2): four broadcast counters. + // Gate 2 (PoolsAdded, oldVersion >= 2): four broadcast counters. if (oldVersion >= 2) { _ = ReadU32(payload, ref pos); // monarchBroadcastTime @@ -447,7 +459,7 @@ public static class ClientCommandResponses _ = ReadU32(payload, ref pos); // spokesBroadcastsToday } - // §4.2 gate 5 (MotdAdded, oldVersion >= 3). + // Gate 3 (MotdAdded, oldVersion >= 3). string motd = ""; string motdSetBy = ""; if (oldVersion >= 3) @@ -456,12 +468,12 @@ public static class ClientCommandResponses motdSetBy = StringReader.ReadString16L(payload, ref pos); } - // §4.2 gate 6 (ChatRoomIDAdded, oldVersion >= 4). + // Gate 4 (ChatRoomIDAdded, oldVersion >= 4). uint chatRoomId = 0; if (oldVersion >= 4) chatRoomId = ReadU32(payload, ref pos); - // §4.2 gate 7 (Bindstones, oldVersion >= 7): Position = + // Gate 7 (Bindstones, oldVersion >= 7): Position = // cell(u32) + pos(3xfloat) + rotation(4xfloat, W/X/Y/Z) = 32 // bytes. Skipped, not surfaced — see the class doc on // AllegianceProfileBody for why. @@ -471,7 +483,7 @@ public static class ClientCommandResponses _ = ReadU32(payload, ref pos); } - // §4.2 gate 8 (AllegianceName, oldVersion >= 8). + // Gate 8 (AllegianceName, oldVersion >= 8). string allegianceName = ""; uint nameLastSetTime = 0; if (oldVersion >= 8) @@ -480,12 +492,12 @@ public static class ClientCommandResponses nameLastSetTime = ReadU32(payload, ref pos); } - // §4.2 gate 9 (LockedState, oldVersion >= 10). + // Gate 10 (LockedState, oldVersion >= 10). bool isLocked = false; if (oldVersion >= 10) isLocked = ReadU32(payload, ref pos) != 0u; - // §4.2 gate 10 (ApprovedVassal, oldVersion >= 11). + // Gate 11 (ApprovedVassal, oldVersion >= 11). uint approvedVassal = 0; if (oldVersion >= 11) approvedVassal = ReadU32(payload, ref pos); @@ -494,15 +506,22 @@ public static class ClientCommandResponses // wire, never version-gated) followed by (recordCount-1) records // each carrying an explicit treeParent. // AllegianceHierarchy::Add @0x005B6E90 discards the WHOLE message - // if a treeParent is not already in the tree (orphan), equals the - // record's own id (self-parent), or duplicates an id already - // seen — modeled here as a running knownIds set; any failure - // returns null rather than a partial/corrupted tree. + // if: the record's own id is zero (MF-1 — retail's ENTIRE Add + // body is wrapped in `if (_id != 0)`, so a zero id falls straight + // out to `return 0` for BOTH the monarch and a child record — this + // is also what makes treeParent == 0 unconditionally fatal for + // every non-monarch record, since 0 can never be a knownId); a + // treeParent is not already in the tree (orphan); the treeParent + // equals the record's own id (self-parent); or the id duplicates + // one already seen — modeled here as a running knownIds set; any + // failure returns null rather than a partial/corrupted tree. AllegianceMemberRecord? monarch = null; var records = new List(); if (recordCount > 0) { AllegianceMemberRecord monarchRecord = ReadAllegianceData(payload, ref pos, parentGuid: 0u); + if (monarchRecord.CharacterId == 0u) + return null; monarch = monarchRecord; var knownIds = new HashSet { monarchRecord.CharacterId }; @@ -511,7 +530,8 @@ public static class ClientCommandResponses uint parentGuid = ReadU32(payload, ref pos); AllegianceMemberRecord record = ReadAllegianceData(payload, ref pos, parentGuid); - if (!knownIds.Contains(parentGuid) + if (record.CharacterId == 0u + || !knownIds.Contains(parentGuid) || parentGuid == record.CharacterId || knownIds.Contains(record.CharacterId)) { @@ -521,6 +541,16 @@ public static class ClientCommandResponses knownIds.Add(record.CharacterId); records.Add(record); } + + // Retail's LAST act before UnPack returns success + // (@0x005B77A7-0x005B77B3): the monarch can never pass up, + // whatever the wire bit said — force it false regardless of + // ReadAllegianceData's HasPackedLevel-absent legacy-compat + // fallback (SF-1/SF-3). Inert against ACE (AllegianceData.cs + // never sets the bit for a monarch record) but load-bearing + // the moment a record is hand-built or read from a non-ACE + // server. + monarch = monarch.Value with { MayPassupExperience = false }; } return new AllegianceProfileBody( diff --git a/src/AcDream.Core.Net/Messages/GameEvents.cs b/src/AcDream.Core.Net/Messages/GameEvents.cs index d6aacf00..96cf6b37 100644 --- a/src/AcDream.Core.Net/Messages/GameEvents.cs +++ b/src/AcDream.Core.Net/Messages/GameEvents.cs @@ -503,7 +503,14 @@ public static class GameEvents BinaryPrimitives.ReadUInt32LittleEndian(payload.Slice(8)) != 0); } - /// 0x0274 CharacterConfirmationRequest — server-driven modal confirm. + /// + /// 0x0274 CharacterConfirmationRequest — server-driven modal confirm. + /// is a bare uint, not + /// — this is the leg GameplayConfirmationController.HandleRequest + /// actually consumes in production today. See 's + /// doc comment (blast review SF-5) for why the triple currently carries + /// its discriminator two different ways. + /// public readonly record struct CharacterConfirmationRequest( uint Type, uint ContextId, @@ -526,7 +533,10 @@ public static class GameEvents /// /// 0x0276 CharacterConfirmationDone — server cancellation/completion of the /// outstanding confirmation tuple. Retail dispatches the same type/context - /// pair to RecvNotice_AbortConfirmationRequest. + /// pair to RecvNotice_AbortConfirmationRequest. + /// is a bare uint, not — the leg + /// GameplayConfirmationController.HandleDone actually consumes; + /// see 's doc comment (blast review SF-5). /// public readonly record struct CharacterConfirmationDone(uint Type, uint ContextId); @@ -548,6 +558,19 @@ public static class GameEvents /// (lane B §3.15, lane C §1.3). Campaign FA needs exactly /// (1) and (4) /// — D6. + /// + /// + /// FA1 review round (blast SF-5): this enum currently types ONLY the + /// response-side leg () — + /// and + /// , the two legs + /// production actually reads today, remain bare uint. This is a + /// deliberate, not-yet-decided split, not a double parser: FA1 did not + /// touch the inbound legs. Before FA4 wires fellowship (4) and + /// allegiance (1) confirmations, either promote both inbound records + /// to or treat this note as the standing + /// decision that the enum stays response-side only. + /// /// public enum ConfirmationType : uint { @@ -570,7 +593,7 @@ public static class GameEvents /// This record + parser exist to give the triple a complete, TYPED /// representation in Core.Net (the /// enum, not a bare uint) and a round-trip conformance check — - /// see ConfirmationResponseTests for the golden-vector / + /// see ConfirmationTripleTests for the golden-vector / /// round-trip pair against BuildConfirmationResponse. /// public readonly record struct ConfirmationResponse( @@ -742,11 +765,16 @@ public static class GameEvents } /// - /// 0x02BF FellowshipDisband — empty body - /// (DispatchUI_Disband @0x006A5E80 reads only the opcode; ACE - /// writes no body — lane B §3.11). + /// 0x02BF FellowshipDisband — DispatchUI_Disband + /// @0x006A5E80 reads ONLY the opcode and calls straight into the + /// handler; it never inspects, validates, or even looks at a body + /// length. ACE writes no body today (lane B §3.11), but encoding a + /// length check retail itself does not perform would make a future + /// non-empty body (an ACE change, a trailing pad) silently swallow the + /// disband and leave the roster stuck in a fellowship the server + /// already destroyed. Accept unconditionally — this always succeeds. /// - public static bool ParseFellowshipDisband(ReadOnlySpan payload) => payload.Length == 0; + public static bool ParseFellowshipDisband(ReadOnlySpan payload) => true; /// /// 0x01C9 FellowshipFellowUpdateDone — dead in the Sept-2013 diff --git a/src/AcDream.Core.Net/Messages/SocialActions.cs b/src/AcDream.Core.Net/Messages/SocialActions.cs index f7977705..02ef46c8 100644 --- a/src/AcDream.Core.Net/Messages/SocialActions.cs +++ b/src/AcDream.Core.Net/Messages/SocialActions.cs @@ -181,7 +181,8 @@ public static class SocialActions /// show/hide, and ACE gates the whole 0x02C0 member-vitals /// stream on it (Fellowship.cs:723): a client that never sends /// this sees names/levels/max-vitals from 0x02BE but a roster - /// frozen at join time (lane B §4.5, §12). See + /// frozen at join time (lane B §4.5, master-table row 12 — "Panel + /// visibility → update request"). See /// for the real openness /// toggle, which is a separate opcode this builder used to be /// (incorrectly) named for. diff --git a/tests/AcDream.Core.Net.Tests/Messages/AllegianceProfileVersionGateTests.cs b/tests/AcDream.Core.Net.Tests/Messages/AllegianceProfileVersionGateTests.cs index 6c4cfba1..fea1b373 100644 --- a/tests/AcDream.Core.Net.Tests/Messages/AllegianceProfileVersionGateTests.cs +++ b/tests/AcDream.Core.Net.Tests/Messages/AllegianceProfileVersionGateTests.cs @@ -352,6 +352,58 @@ public sealed class AllegianceProfileVersionGateTests Assert.Null(ClientCommandResponses.ParseAllegianceInfoResponse(wire)); } + // MF-1 (mechanism review): AllegianceHierarchy::Add @0x005B6E90 wraps + // its ENTIRE body in `if (_id != 0)` — a record whose own id is zero + // falls straight out to `return 0`, which UnPack turns into "discard + // the whole message", for BOTH the monarch and a child record. This + // is a FOURTH rejection rule distinct from orphan/self-parent/ + // duplicate above, and it is also what makes `treeParent == 0` + // unconditionally fatal for a non-monarch record — 0 can never be a + // knownId once the id==0 monarch case is rejected. + + [Fact] + public void TreeAssembly_ZeroIdMonarch_DiscardsWholeMessage() + { + var records = new List<(uint, uint, bool, string)> + { + (0u /* zero id */, 0u, true, "ZeroIdMonarch"), + }; + byte[] wire = BuildProfileWire(MonarchGuid, 11, records); + + Assert.Null(ClientCommandResponses.ParseAllegianceInfoResponse(wire)); + } + + [Fact] + public void TreeAssembly_ZeroIdChildRecord_DiscardsWholeMessage() + { + var records = new List<(uint, uint, bool, string)> + { + (MonarchGuid, 0u, true, "Monarch"), + (0u /* zero id */, MonarchGuid, true, "ZeroIdChild"), + }; + byte[] wire = BuildProfileWire(MonarchGuid, 11, records); + + Assert.Null(ClientCommandResponses.ParseAllegianceInfoResponse(wire)); + } + + [Fact] + public void TreeAssembly_ZeroTreeParent_OnNonMonarchRecord_DiscardsWholeMessage() + { + // treeParent == 0 for anything but the (implicit) monarch slot can + // never resolve — 0 is never added to knownIds because a zero-id + // record is itself rejected (the two tests above), so this is the + // orphan rule specialized to the zero case retail's own Search + // (arg2 != 0) guard names explicitly. + var records = new List<(uint, uint, bool, string)> + { + (MonarchGuid, 0u, true, "Monarch"), + (0x50000008u, 0u /* treeParent == 0, not the monarch */, true, "ZeroParent"), + }; + byte[] wire = BuildProfileWire(MonarchGuid, 11, records); + + Assert.Null(ClientCommandResponses.ParseAllegianceInfoResponse(wire)); + } + [Fact] public void TreeAssembly_ValidChain_ParentBeforeChild_Succeeds() { @@ -451,10 +503,21 @@ public sealed class AllegianceProfileVersionGateTests { // Lane C §4.1 point 1: when HasPackedLevel (0x8) is absent, retail // sets MayPassupExperience itself regardless of the wire bit. + // + // FA1 review round (SF-1/SF-3): the record under test here MUST be + // a non-monarch record. UnPack's LAST act unconditionally forces + // the monarch's MayPassupExperience back to false regardless of + // how it was computed (see + // ReadAllegianceData_MonarchMayPassupExperience_ForcedFalseRegardlessOfWireBit), + // so putting the legacy-compat fixture on the monarch (as this + // test originally did) could never distinguish "the fallback + // fired" from "the monarch clear fired" — both produce the same + // observed value on that record. A vassal record is untouched by + // the monarch-only clear and isolates the fallback. var w = new AceWireWriter() .Write(MonarchGuid) - .Write((uint)1).Write((uint)0) - .Write((ushort)1).Write((ushort)11) + .Write((uint)2).Write((uint)0) + .Write((ushort)2).Write((ushort)11) .Write((ushort)0).Write((ushort)256) .Write((uint)0) .Write((uint)0).Write((uint)0).Write((uint)0).Write((uint)0) @@ -465,21 +528,76 @@ public sealed class AllegianceProfileVersionGateTests .Write((uint)0) .Write((uint)0) .Write((uint)0) + // monarch — ordinary HasPackedLevel-set record, not under test. .Write(MonarchGuid) .Write((uint)0).Write((uint)0) + .Write((uint)(0x4u | 0x8u)) + .Write((byte)0).Write((byte)0) + .Write((ushort)0) + .Write((uint)0) // level (HasPackedLevel set) + .Write((ushort)0).Write((ushort)0) + .Write((uint)0).Write((uint)0) + .WriteString16L("Monarch") + // vassal — the record under test: HasPackedLevel absent. + .Write(MonarchGuid) // treeParent + .Write(0x50000005u) + .Write((uint)0).Write((uint)0) .Write((uint)0x4u) // HasAllegianceAge only — NO HasPackedLevel, NO MayPassupExperience bit .Write((byte)0).Write((byte)0) .Write((ushort)0) // no level field (HasPackedLevel unset) .Write((ushort)0).Write((ushort)0) .Write((uint)0).Write((uint)0) + .WriteString16L("Vassal"); + + var parsed = ClientCommandResponses.ParseAllegianceInfoResponse(w.ToArray()); + + Assert.NotNull(parsed); + Assert.False(parsed!.Value.Monarch!.Value.MayPassupExperience); // always forced false + Assert.Single(parsed.Value.Records); + Assert.True(parsed.Value.Records[0].MayPassupExperience); // legacy-compat fallback fired + Assert.Equal(0u, parsed.Value.Records[0].Level); // never read — HasPackedLevel unset + } + + // SF-1/SF-3 (review): AllegianceHierarchy::UnPack's LAST act before + // returning success is SetMayPassupExperience(&monarch->_data, 0) — + // the monarch can never pass up, whatever the wire bit says. Build a + // monarch record with the wire bit explicitly SET (0x10) and prove + // the parsed record still comes back false. + [Fact] + public void ReadAllegianceData_MonarchMayPassupExperience_ForcedFalseRegardlessOfWireBit() + { + var w = new AceWireWriter() + .Write(MonarchGuid) // targetGuid + .Write((uint)1).Write((uint)0) // totalMembers, totalVassals + .Write((ushort)1).Write((ushort)11) // recordCount, oldVersion=11 (newest) + .Write((ushort)0).Write((ushort)256) // officers: empty + .Write((uint)0) // officerTitles: empty + .Write((uint)0).Write((uint)0).Write((uint)0).Write((uint)0) // broadcast counters + .WriteString16L("").WriteString16L("") // motd, motdSetBy + .Write((uint)0) // chatRoomID + .Write((uint)0).Write(0f).Write(0f).Write(0f).Write(1f).Write(0f).Write(0f).Write(0f) // bindPoint + .WriteString16L("Alle") // allegianceName + .Write((uint)0) // nameLastSetTime + .Write((uint)0) // isLocked + .Write((uint)0) // approvedVassal + // monarch AllegianceData — HasAllegianceAge | HasPackedLevel | + // MayPassupExperience (0x4|0x8|0x10 = 0x1C), i.e. the wire bit + // asks for MayPassupExperience = true. + .Write(MonarchGuid) + .Write((uint)0).Write((uint)0) + .Write((uint)0x1Cu) + .Write((byte)0).Write((byte)0) + .Write((ushort)1) + .Write((uint)5) // level (HasPackedLevel set) + .Write((ushort)0).Write((ushort)0) + .Write((uint)0).Write((uint)0) .WriteString16L("Monarch"); var parsed = ClientCommandResponses.ParseAllegianceInfoResponse(w.ToArray()); Assert.NotNull(parsed); - Assert.True(parsed!.Value.Monarch!.Value.MayPassupExperience); - Assert.Equal(0u, parsed.Value.Monarch!.Value.Level); // never read — HasPackedLevel unset + Assert.False(parsed!.Value.Monarch!.Value.MayPassupExperience); } // ── 0x0020 AllegianceUpdate shares the same profile reader ───────────── diff --git a/tests/AcDream.Core.Net.Tests/Messages/AllegianceRequestsTests.cs b/tests/AcDream.Core.Net.Tests/Messages/AllegianceRequestsTests.cs index eddc60c4..e71eec06 100644 --- a/tests/AcDream.Core.Net.Tests/Messages/AllegianceRequestsTests.cs +++ b/tests/AcDream.Core.Net.Tests/Messages/AllegianceRequestsTests.cs @@ -88,4 +88,41 @@ public sealed class AllegianceRequestsTests Assert.Equal(AllegianceRequests.BreakOpcode, BinaryPrimitives.ReadUInt32LittleEndian(body.AsSpan(8))); } + + // FA1 review round (mechanism MF-2): the missing 0x001F + // AllegianceUpdateRequest builder — the structural twin of the + // fellowship 0x00A6 this slice already repaired. Golden vector + // hand-derived from lane C §3.2: [u32 0xF7B1][u32 seq][u32 0x1F] + // [u32 on], total 0x10. + [Fact] + public void BuildAllegianceUpdateRequest_GoldenByteVector_On() + { + byte[] body = AllegianceRequests.BuildAllegianceUpdateRequest(gameActionSequence: 4, on: true); + + byte[] expected = + [ + 0xB1, 0xF7, 0x00, 0x00, // envelope 0xF7B1 + 0x04, 0x00, 0x00, 0x00, // seq 4 + 0x1F, 0x00, 0x00, 0x00, // opcode 0x001F + 0x01, 0x00, 0x00, 0x00, // on = 1 + ]; + + Assert.Equal(expected, body); + } + + [Fact] + public void BuildAllegianceUpdateRequest_GoldenByteVector_Off() + { + byte[] body = AllegianceRequests.BuildAllegianceUpdateRequest(gameActionSequence: 7, on: false); + + byte[] expected = + [ + 0xB1, 0xF7, 0x00, 0x00, // envelope 0xF7B1 + 0x07, 0x00, 0x00, 0x00, // seq 7 + 0x1F, 0x00, 0x00, 0x00, // opcode 0x001F + 0x00, 0x00, 0x00, 0x00, // on = 0 + ]; + + Assert.Equal(expected, body); + } } diff --git a/tests/AcDream.Core.Net.Tests/Messages/ClientCommandResponsesTests.cs b/tests/AcDream.Core.Net.Tests/Messages/ClientCommandResponsesTests.cs index b93469d0..44735d16 100644 --- a/tests/AcDream.Core.Net.Tests/Messages/ClientCommandResponsesTests.cs +++ b/tests/AcDream.Core.Net.Tests/Messages/ClientCommandResponsesTests.cs @@ -314,6 +314,82 @@ public sealed class ClientCommandResponsesTests lines); } + // FA1 review round (blast SF-1): FA1's tree-assembly rules flipped + // vassal print order (FindVassals now walks in REVERSE wire order — + // see AllegianceProfileVersionGateTests.TreeAssembly_SiblingOrder_ + // ReversesOnAssembly) at the LIVE @allegiance info surface. The + // pre-existing full-tree test above has exactly ONE vassal, which + // cannot distinguish forward from reverse order — this pins the + // reversal through FormatAllegianceInfoLines itself, not just through + // FindVassals, with three vassals so the ordering is unambiguous. + [Fact] + public void ParseAndFormatAllegianceInfoResponse_ThreeVassals_PrintsInReverseWireOrder() + { + const uint monarchGuid = 0x50000001u; + const uint vassalAGuid = 0x50000010u; + const uint vassalBGuid = 0x50000011u; + const uint vassalCGuid = 0x50000012u; + + byte[] wire = BuildAllegianceWire( + monarchGuid, + new() + { + (monarchGuid, 0u, true, "Monarch"), + (vassalAGuid, monarchGuid, true, "VassalA"), + (vassalBGuid, monarchGuid, true, "VassalB"), + (vassalCGuid, monarchGuid, true, "VassalC"), + }); + + var response = ClientCommandResponses.ParseAllegianceInfoResponse(wire); + Assert.NotNull(response); + + var lines = ClientCommandResponses.FormatAllegianceInfoLines(response.Value).ToArray(); + + Assert.Equal( + new[] + { + "Note: An asterisk (*) indicates that the character is currently online.", + "Allegiance information for Monarch *:", + " Vassals: ", + " VassalC *", + " VassalB *", + " VassalA *", + }, + lines); + } + + // FA1 review round (blast SF-1): the §4.4 tree-assembly rules turned a + // malformed tree (orphan / self-parent / duplicate treeParent) into a + // whole-message discard (ParseAllegianceInfoResponse returns null), + // and GameEventWiring's registration is `if (info is null) return;` — + // so the live @allegiance info command now goes from "prints a + // possibly-garbled roster" to "prints nothing at all" on a malformed + // tree. Pin that silent-drop behavior at the wiring layer that shows + // it to the user. + [Fact] + public void WireAll_AllegianceInfoResponse_MalformedTree_PrintsNothing() + { + var dispatcher = new GameEventDispatcher(); + var chat = new ChatLog(); + GameEventWiring.WireAll(dispatcher, new ClientObjectTable(), new CombatState(), new Spellbook(), chat); + + const uint monarchGuid = 0x50000001u; + byte[] payload = BuildAllegianceWire( + monarchGuid, + new() + { + (monarchGuid, 0u, true, "Monarch"), + (0x50000005u, 0x5000FFFFu /* never-seen parent — orphan */, true, "Orphan"), + }); + + GameEventEnvelope? envelope = GameEventEnvelope.TryParse( + WrapEnvelope(GameEventType.AllegianceInfoResponse, payload)); + Assert.NotNull(envelope); + dispatcher.Dispatch(envelope.Value); + + Assert.Empty(chat.Snapshot()); + } + [Fact] public void FormatAllegianceInfoLines_NoAllegiance_PrintsNothing() { diff --git a/tests/AcDream.Core.Net.Tests/Messages/FellowshipEventsTests.cs b/tests/AcDream.Core.Net.Tests/Messages/FellowshipEventsTests.cs index 88145e61..9b899ca0 100644 --- a/tests/AcDream.Core.Net.Tests/Messages/FellowshipEventsTests.cs +++ b/tests/AcDream.Core.Net.Tests/Messages/FellowshipEventsTests.cs @@ -166,6 +166,33 @@ public sealed class FellowshipEventsTests Assert.Equal(3u, update.Value.UpdateType); } + // D5/mechanism SF-3: the same raw-uint, never-a-bool shareLoot + // requirement pinned for ParseFellowshipFullUpdate above + // (ParseFellowshipFullUpdate_ShareLootIsRawNotBool_D5) must ALSO hold + // at the 0x02C0 site — ReadFellow is shared by both parsers, but + // nothing previously asserted the <<1 incremental-update shape here; + // a future split of ReadFellow could silently reintroduce a bool read + // on this leg without either test catching it. + [Fact] + public void ParseFellowshipUpdateFellow_ShareLootIsRawNotBool_D5() + { + byte[] wire = new AceWireWriter() + .Write(0x50000005u) // guid FIRST + .Write((uint)10).Write((uint)5).Write((uint)3) + .Write((uint)100).Write((uint)80).Write((uint)60) + .Write((uint)90).Write((uint)70).Write((uint)50) + .Write((uint)2) // shareLoot — ACE's incremental-update "<<1" shape + .WriteString16L("Vitals") + .Write((uint)3) // updateType = 3 UpdateVitals + .ToArray(); + + var update = GameEvents.ParseFellowshipUpdateFellow(wire); + + Assert.NotNull(update); + Assert.Equal(2u, update.Value.Member.ShareLoot); + Assert.NotEqual(1u, update.Value.Member.ShareLoot); + } + // ── 0x00A3/0x00A4 S→C ───────────────────────────────────────────────── [Fact] @@ -194,10 +221,15 @@ public sealed class FellowshipEventsTests Assert.True(GameEvents.ParseFellowshipDisband(ReadOnlySpan.Empty)); } + // FA1 review round (mechanism SF-2): retail's DispatchUI_Disband + // @0x006A5E80 reads only the opcode and never inspects a trailing + // body — the pre-fix parser's `payload.Length == 0` check was an + // invented validation retail does not perform. A non-empty body + // (an ACE change, a trailing pad) must still succeed. [Fact] - public void ParseFellowshipDisband_NonEmptyBody_ReturnsFalse() + public void ParseFellowshipDisband_NonEmptyBody_StillReturnsTrue() { - Assert.False(GameEvents.ParseFellowshipDisband(new byte[] { 1 })); + Assert.True(GameEvents.ParseFellowshipDisband(new byte[] { 1 })); } // ── 0x01C9/0x01CA dead events — parse-and-ignore, must never fail ──────