erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix(net): FA1 review round -- zero-id tree rejection, monarch clear, 0x001F builder

Browse source 
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 <noreply@anthropic.com>
This commit is contained in:
Erik 2026-08-12 00:46:42 +02:00
parent bc693728a6
commit ed30808720
8 changed files with 385 additions and 34 deletions
Download patch file Download diff file Expand all files Collapse all files

29
src/AcDream.Core.Net/Messages/AllegianceRequests.cs
View file

@ -37,6 +37,24 @@ public static class AllegianceRequests
public const uint SwearOpcode = 0x001Du;
public const uint BreakOpcode = 0x001Eu;
/// <summary>
/// <c>0x001F AllegianceUpdateRequest</c> — the allegiance panel's
/// subscribe/unsubscribe toggle, the structural twin of the fellowship
/// <c>0x00A6 FellowshipUpdateRequest</c> repaired elsewhere in this
/// slice. Retail's <c>CM_Allegiance::Event_UpdateRequest(u32)</c>
/// (<c>@0x006A72BA</c>, lane C §1.2) is NOT a one-shot query — send
/// <c>1</c> on <c>gmAllegianceUI::PostInit</c>'s tail
/// (<c>@0x004911C6</c>) and on <c>RecvNotice_PlayerDescReceived</c>
/// (<c>@0x00490D59</c>), <c>1</c> on <c>OnVisibilityChanged</c>'s
/// visible branch (<c>@0x004912DD</c>), <c>0</c> on its hidden branch
/// (<c>@0x00491311</c>). ACE's own handler
/// (<c>GameActionAllegianceUpdateRequest.cs:12</c>) 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).
/// </summary>
public const uint AllegianceUpdateRequestOpcode = 0x001Fu;
/// <summary>Pledge yourself to the given patron.</summary>
public static byte[] BuildSwear(uint gameActionSequence, uint patronGuid)
{
@ -72,6 +90,17 @@ public static class AllegianceRequests
return Build(gameActionSequence, BreakOpcode, vassalGuid);
}
/// <summary>
/// Declare the allegiance panel's visibility/subscription state to the
/// server (<c>0x001F AllegianceUpdateRequest</c>). See
/// <see cref="AllegianceUpdateRequestOpcode"/> for the four retail send
/// sites and their arguments.
/// </summary>
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];

70
src/AcDream.Core.Net/Messages/ClientCommandResponses.cs
View file

@ -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<PString>: a bare i32 count,
// NOT the PackableHashTable u16/u16 header.
// Gate 9 (OfficersTitlesAdded, oldVersion >= 9) — PSmartArray<PString>:
// 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<AllegianceMemberRecord>();
if (recordCount > 0)
{
AllegianceMemberRecord monarchRecord = ReadAllegianceData(payload, ref pos, parentGuid: 0u);
if (monarchRecord.CharacterId == 0u)
return null;
monarch = monarchRecord;
var knownIds = new HashSet<uint> { 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(

42
src/AcDream.Core.Net/Messages/GameEvents.cs
View file

@ -503,7 +503,14 @@ public static class GameEvents
BinaryPrimitives.ReadUInt32LittleEndian(payload.Slice(8)) != 0);
}
/// <summary>0x0274 CharacterConfirmationRequest — server-driven modal confirm.</summary>
/// <summary>
/// 0x0274 CharacterConfirmationRequest — server-driven modal confirm.
/// <see cref="Type"/> is a bare <c>uint</c>, not <see cref="ConfirmationType"/>
/// — this is the leg <c>GameplayConfirmationController.HandleRequest</c>
/// actually consumes in production today. See <see cref="ConfirmationType"/>'s
/// doc comment (blast review SF-5) for why the triple currently carries
/// its discriminator two different ways.
/// </summary>
public readonly record struct CharacterConfirmationRequest(
uint Type,
uint ContextId,
@ -526,7 +533,10 @@ public static class GameEvents
/// <summary>
/// 0x0276 CharacterConfirmationDone — server cancellation/completion of the
/// outstanding confirmation tuple. Retail dispatches the same type/context
/// pair to <c>RecvNotice_AbortConfirmationRequest</c>.
/// pair to <c>RecvNotice_AbortConfirmationRequest</c>. <see cref="Type"/>
/// is a bare <c>uint</c>, not <see cref="ConfirmationType"/> — the leg
/// <c>GameplayConfirmationController.HandleDone</c> actually consumes;
/// see <see cref="ConfirmationType"/>'s doc comment (blast review SF-5).
/// </summary>
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
/// <see cref="SwearAllegiance"/> (1) and <see cref="Fellowship"/> (4)
/// — D6.
///
/// <para>
/// FA1 review round (blast SF-5): this enum currently types ONLY the
/// response-side leg (<see cref="ConfirmationResponse.Type"/>) —
/// <see cref="CharacterConfirmationRequest.Type"/> and
/// <see cref="CharacterConfirmationDone.Type"/>, the two legs
/// production actually reads today, remain bare <c>uint</c>. 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 <see cref="ConfirmationType"/> or treat this note as the standing
/// decision that the enum stays response-side only.
/// </para>
/// </summary>
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 <see cref="ConfirmationType"/>
/// enum, not a bare <c>uint</c>) and a round-trip conformance check —
/// see <c>ConfirmationResponseTests</c> for the golden-vector /
/// see <c>ConfirmationTripleTests</c> for the golden-vector /
/// round-trip pair against <c>BuildConfirmationResponse</c>.
/// </summary>
public readonly record struct ConfirmationResponse(
@ -742,11 +765,16 @@ public static class GameEvents
}
/// <summary>
/// <c>0x02BF FellowshipDisband</c> — empty body
/// (<c>DispatchUI_Disband @0x006A5E80</c> reads only the opcode; ACE
/// writes no body — lane B §3.11).
/// <c>0x02BF FellowshipDisband</c> — <c>DispatchUI_Disband
/// @0x006A5E80</c> 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.
/// </summary>
public static bool ParseFellowshipDisband(ReadOnlySpan<byte> payload) => payload.Length == 0;
public static bool ParseFellowshipDisband(ReadOnlySpan<byte> payload) => true;
/// <summary>
/// <c>0x01C9 FellowshipFellowUpdateDone</c> — dead in the Sept-2013

3
src/AcDream.Core.Net/Messages/SocialActions.cs
View file

@ -181,7 +181,8 @@ public static class SocialActions
/// show/hide, and ACE gates the whole <c>0x02C0</c> member-vitals
/// stream on it (<c>Fellowship.cs:723</c>): a client that never sends
/// this sees names/levels/max-vitals from <c>0x02BE</c> 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
/// <see cref="BuildFellowshipChangeOpenness"/> for the real openness
/// toggle, which is a separate opcode this builder used to be
/// (incorrectly) named for.