fix(chat): CH4 re-review fixes — dialog-queue reentrancy, settings option-bit chokepoint
Should-fix 1: RetailDialogFactory.CloseDialog's queued branch removed the active DialogInfo, ran DialogDone (whose callback can synchronously open a new dialog under the SAME queue key — the two-stage house-abandon confirmation does exactly this), then called OpenNextDialog, which did an unconditional Dictionary.Add on a key the reentrant dialog had already re-occupied. Retail's HashTable::add tolerates the duplicate; Dictionary throws. OpenNextDialog now returns early when the queue key is already active — the reentrant dialog's own eventual close drains the queue. Should-fix 2: @join/@leave wrote the local RuntimeCharacterOptionsState bit before sending, but the Settings Chat toggles reached a second binding (SendSingleCharacterOption) that only sent the wire message, leaving the Turbine membership gate stale until the next PlayerDescription. LiveSessionRuntimeFactory.CreateCommandBindings now has one shared local function for both entrances. Should-fix 3: corrected TS-68/#360 wording again — retail's DoAllegiance dispatcher table EXECUTES boot/ban/officer/title/motd/name/lock/house/ chat/broadcast locally through their own handlers; acdream shows the unrecognized-subcommand refusal for all nine pending the #360 port. What matches retail is the ownership rule (the verb never reaches DoChannelCommand/the server), not the subcommand behavior itself. Removed the inaccurate "matching retail, not merely harmless" / "now matches this" claims from both the register row and the issue. Nits: corrected the HouseAbandonDialogCallback_First citation (0x00580E1A is DoHouse's load site for the callback pointer, not the function entry — the entry is 0x00580240, with the stage-2 confirmation string built at 0x005802D8) in both ClientCommandController.cs and the mirrored test comment; added an InlineData case pinning "@clist allegiance" to RequestChannelList(0x02000000); converted RetailClientCommandCatalog. KnownVerbs from a plain array to a FrozenSet<string> with StringComparer.OrdinalIgnoreCase, matching the file's other lookup tables. Suite: 12,221 passed / 4 skipped / 0 failed (Release), up from CH4's 12,216/4/0 — net +5 tests, no removals. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
59c053ee47
commit
5d247d5518
10 changed files with 204 additions and 34 deletions
|
|
@ -325,7 +325,31 @@ internal sealed class LiveSessionRuntimeFactory
|
|||
}
|
||||
|
||||
private LiveSessionCommandBindings CreateCommandBindings(
|
||||
WorldSession session) => new(
|
||||
WorldSession session)
|
||||
{
|
||||
// CH4 re-review SHOULD-FIX 2 (2026-08-09): single local-write
|
||||
// chokepoint for retail's SetSingleCharacterOption (0x0005) local
|
||||
// option-bit write, reached by BOTH entrances that can flip a
|
||||
// character option — @join/@leave
|
||||
// (ClientCommandController.Bindings.SetSingleCharacterOption below)
|
||||
// and the Settings Chat toggles (LiveSessionCommandBindings.
|
||||
// SendSingleCharacterOption at the bottom of this method, routed
|
||||
// through RuntimeSettingsController.PublishHearOptionChange ->
|
||||
// RuntimeSettingsTargets.SetSingleCharacterOption ->
|
||||
// SetSingleCharacterOptionRuntimeCmd -> LiveSessionCommandRouter).
|
||||
// Retail's PlayerModule::SetHear*Chat family writes the bit into the
|
||||
// local options copy FIRST, then notifies the server, for both
|
||||
// entrances alike — routing only @join/@leave through the local
|
||||
// write (the prior CH4 fix) left a Settings-route toggle stale in
|
||||
// TurbineChatMembershipGate (which reads _domain.Character.Options)
|
||||
// until the next PlayerDescription happened to arrive.
|
||||
void SendSingleCharacterOption(uint optionId, bool value)
|
||||
{
|
||||
_domain.Character.Options.SetOptionBit(optionId, value);
|
||||
session.SendSetSingleCharacterOption(optionId, value);
|
||||
}
|
||||
|
||||
return new(
|
||||
ClientCommands: new ClientCommandController.Bindings(
|
||||
TeleportToLifestone: session.SendTeleportToLifestone,
|
||||
TeleportToMarketplace: session.SendTeleportToMarketplace,
|
||||
|
|
@ -435,18 +459,16 @@ internal sealed class LiveSessionRuntimeFactory
|
|||
// silent accept is exactly as faithful as a stored-but-unread
|
||||
// value would be, without inventing a consumer.
|
||||
SetChatTitle: _ => { },
|
||||
// CH4 REJECT-review SHOULD-FIX 4 (2026-08-09): only @join/@leave
|
||||
// reach this binding (see ClientCommandController.Execute).
|
||||
// Retail's PlayerModule::SetHear*Chat family writes the bit into
|
||||
// the local options copy FIRST, then notifies the server — match
|
||||
// CH4 re-review SHOULD-FIX 2 (2026-08-09): @join/@leave route
|
||||
// through the same SendSingleCharacterOption local function as
|
||||
// the Settings-route binding below — see the chokepoint comment
|
||||
// at the top of CreateCommandBindings. Retail's
|
||||
// PlayerModule::SetHear*Chat family writes the bit into the
|
||||
// local options copy FIRST, then notifies the server — match
|
||||
// that ordering so TurbineChatMembershipGate (which reads
|
||||
// _domain.Character.Options) stops refusing the newly-joined
|
||||
// room before the next PlayerDescription happens to arrive.
|
||||
SetSingleCharacterOption: (optionId, value) =>
|
||||
{
|
||||
_domain.Character.Options.SetOptionBit(optionId, value);
|
||||
session.SendSetSingleCharacterOption(optionId, value);
|
||||
},
|
||||
SetSingleCharacterOption: SendSingleCharacterOption,
|
||||
AddPlayerPermission: session.SendAddPlayerPermission,
|
||||
RemovePlayerPermission: session.SendRemovePlayerPermission,
|
||||
RequestAvailableHouses: session.SendListAvailableHouses,
|
||||
|
|
@ -497,8 +519,9 @@ internal sealed class LiveSessionRuntimeFactory
|
|||
ModifyGlobalSquelch: session.SendModifyGlobalSquelch,
|
||||
Communication: _domain.Communication,
|
||||
CharacterState: _domain.Character,
|
||||
SendSingleCharacterOption: session.SendSetSingleCharacterOption,
|
||||
SendSingleCharacterOption: SendSingleCharacterOption,
|
||||
Log: _log);
|
||||
}
|
||||
|
||||
private static double ClientTimerNow() =>
|
||||
Stopwatch.GetTimestamp() / (double)Stopwatch.Frequency;
|
||||
|
|
|
|||
|
|
@ -342,15 +342,18 @@ public sealed class ClientCommandController
|
|||
// GameActionHouseAbandon — "@house abandon". Retail's abandon
|
||||
// branch (DoHouse @ 0x00580D58) opens a FIRST confirmation
|
||||
// dialog (DialogFactory::MakeCallbackDialogInCurrentUI →
|
||||
// HouseAbandonDialogCallback_First @0x00580E1A); only on
|
||||
// accept does that callback open a SECOND dialog
|
||||
// (HouseAbandonDialogCallback_Second @0x0057BE90), and only
|
||||
// THAT callback's accept calls Event_AbandonHouse()
|
||||
// HouseAbandonDialogCallback_First, function entry @0x00580240
|
||||
// — 0x00580E1A is only the load site inside DoHouse where the
|
||||
// callback pointer is fetched); only on accept does that
|
||||
// callback open a SECOND dialog (its own stage-2 string site
|
||||
// @0x005802D8 → HouseAbandonDialogCallback_Second @0x0057BE90),
|
||||
// and only THAT callback's accept calls Event_AbandonHouse()
|
||||
// (0x0057BF01 — the ONLY call site). Both strings recovered
|
||||
// verbatim from acclient_2013_pseudo_c.txt (data_7e1460 /
|
||||
// data_7e1370). CH4 REJECT-review Blocker 2 (2026-08-09):
|
||||
// acdream previously sent 0x021F immediately with NO
|
||||
// confirmation at all.
|
||||
// confirmation at all. Citation corrected 2026-08-09, CH4
|
||||
// re-review nit 4.
|
||||
case ClientCommandId.HouseAbandon:
|
||||
_bindings.ShowConfirmation(
|
||||
"Do you really want to abandon your house? Any items in the house (on hooks or in storage) will stay with the house, and you will lose access to them.",
|
||||
|
|
|
|||
|
|
@ -322,6 +322,16 @@ public sealed class RetailDialogFactory : IDisposable
|
|||
|
||||
private void OpenNextDialog(uint queueKey)
|
||||
{
|
||||
// A callback invoked from DialogDone (above, in CloseDialog) may
|
||||
// synchronously make a new dialog under this same queue key before
|
||||
// control returns here — MakeDialog's queued branch will have
|
||||
// already re-occupied _activeQueued[queueKey]. Retail's HashTable::add
|
||||
// tolerates the duplicate; Dictionary.Add does not. Bail out: the
|
||||
// re-entrant dialog's own eventual CloseDialog will drain the
|
||||
// pending queue via its own OpenNextDialog call.
|
||||
if (_activeQueued.ContainsKey(queueKey))
|
||||
return;
|
||||
|
||||
if (!_pending.TryGetValue(queueKey, out LinkedList<DialogInfo>? queue)
|
||||
|| queue.First is null)
|
||||
return;
|
||||
|
|
|
|||
|
|
@ -723,8 +723,15 @@ public static class RetailClientCommandCatalog
|
|||
/// verb this catalog claims must actually be in the registry, and vice
|
||||
/// versa.
|
||||
/// </summary>
|
||||
// CH4 re-review nit 6 (2026-08-09): FrozenSet with an explicit
|
||||
// OrdinalIgnoreCase comparer, matching ByVerb/JoinLeaveTags/HouseTypes
|
||||
// above — callers (ChatCommandRouter.TryDispatchChannelFallback, the
|
||||
// CH4 conformance test) already treat this collection as
|
||||
// case-insensitive; the array was doing that per-call via LINQ's
|
||||
// Contains(item, comparer) overload instead of baking it into the set.
|
||||
public static IReadOnlyCollection<string> KnownVerbs { get; } =
|
||||
ByVerb.Keys.Concat(["house", "hou", "allegiance", "all"]).ToArray();
|
||||
ByVerb.Keys.Concat(["house", "hou", "allegiance", "all"])
|
||||
.ToFrozenSet(StringComparer.OrdinalIgnoreCase);
|
||||
|
||||
/// <summary>
|
||||
/// <c>/help <verb></c> lookup for a catalog-dispatched command —
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue