From 5d247d551867e8ab2e05af618f0f3f3593e1e03c Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 9 Aug 2026 22:24:43 +0200 Subject: [PATCH] =?UTF-8?q?fix(chat):=20CH4=20re-review=20fixes=20?= =?UTF-8?q?=E2=80=94=20dialog-queue=20reentrancy,=20settings=20option-bit?= =?UTF-8?q?=20chokepoint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 --- docs/ISSUES.md | 22 ++++++- .../retail-divergence-register.md | 2 +- docs/plans/2026-08-09-chat-parity-campaign.md | 2 +- .../Net/LiveSessionRuntimeFactory.cs | 45 ++++++++++---- src/AcDream.App/UI/ClientCommandController.cs | 13 ++-- .../UI/Layout/RetailDialogFactory.cs | 10 +++ .../Panels/Chat/RetailClientCommandCatalog.cs | 9 ++- .../Net/LiveSessionCommandRouterTests.cs | 61 +++++++++++++++++++ .../UI/ClientCommandControllerTests.cs | 29 +++++---- .../UI/Layout/RetailDialogFactoryTests.cs | 45 ++++++++++++++ 10 files changed, 204 insertions(+), 34 deletions(-) diff --git a/docs/ISSUES.md b/docs/ISSUES.md index cf4eabf9..11e26acb 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -54,9 +54,25 @@ this command." locally and stays entirely client-side CH4 REJECT-review found acdream had instead been broadcasting the unmatched subcommand text to the Allegiance chat channel — a real chat-visible bug, now fixed (`TryMatchAllegiance` claims ownership -unconditionally and shows retail's own refusal text). The 22 subcommands -themselves still don't execute; only the fallback behavior changed. -Register row: TS-68. Registry doc: +unconditionally and shows retail's own refusal text). + +**Corrected again 2026-08-09 at the CH4 re-review (SHOULD-FIX 3), for +precision:** the nine allegiance subcommands (boot, ban, officer, title, +motd, name, lock, house, chat, broadcast) are NOT refused by retail — +`DoAllegiance`'s dispatcher table EXECUTES each one locally through its +own handler (e.g. `DoAllegianceBoot @ 0x0057D646` is the dispatcher's +call site into `ClientCommunicationSystem::DoAllegianceBoot`, with +`DoAllegianceBan`/`DoAllegianceOfficer`/`DoAllegianceOfficerTitle`/ +`DoMotd`/`DoAllegianceName`/`DoAllegianceLock`/`DoAllegianceHouse` its +siblings in the same table). acdream shows the same unrecognized- +subcommand refusal for all nine because none of those handlers is +ported yet, pending this issue. What matches retail is the OWNERSHIP +RULE — the verb never reaches `DoChannelCommand`/the server regardless +of subcommand — NOT the subcommand's actual behavior, which retail +executes and acdream does not. + +The 22 subcommands themselves still don't execute; only the fallback +behavior changed. Register row: TS-68. Registry doc: `docs/research/2026-08-09-chat-retail-command-registry.md` §2.5/§2.5b. ## #361 — @day / @log / @render pure-local commands recognized in help only, not executed diff --git a/docs/architecture/retail-divergence-register.md b/docs/architecture/retail-divergence-register.md index 4d3bc40e..cf835bd1 100644 --- a/docs/architecture/retail-divergence-register.md +++ b/docs/architecture/retail-divergence-register.md @@ -389,7 +389,7 @@ AP-94..AP-112 for the confirmed retail-UI completion gaps. | TS-64 | **Retail's sound-preference surface is only partly present.** Retail registers eight `[Sound]` keys in `SoundManager::InitPrefs` @ `0x005503F0`; two are unimplemented in acdream. (a) `s_bPlaySoundOnlyWhenActive` (default **1**) is checked against `Device::m_bIsActiveApp` in every entry point and in both `PlaySoundInternal` overloads, so an unfocused retail client is SILENT; acdream keeps playing when the window loses focus. (b) `s_SoundFeatures == 1` forces pan to dead centre; acdream's `RetailSoundMixer.Mix`/`GetPan` take a `panningEnabled` flag with conformance coverage, but no preference is wired behind it, so panning can never be turned off. The three enable bools (`Sound Disabled`, `Ambient Sound Disabled`, `Interface Sound Disabled`) also have no acdream counterpart — note retail's on-disk polarity is inverted relative to its backing variables, so a future reader must not assume the sense. | `src/AcDream.App/Audio/OpenAlAudioEngine.cs` (no focus gate); `src/AcDream.Core/Audio/RetailSoundMixer.cs` (`panningEnabled`, unwired) | Slice A2 kept its blast radius on the mixing model: window-focus state and a preference surface are host plumbing rather than mixing math, and the mixer parameter exists so wiring them later needs no math change. | Alt-tabbed acdream keeps making noise where retail goes quiet; users cannot disable panning or the individual sound classes. | `SoundManager::InitPrefs @ 0x005503F0`; `SoundManager::PlaySoundInternal @ 0x0054FEC0` and `@ 0x00550170`; `docs/research/2026-08-08-audio-retail-soundmanager-core.md` §1 | | TS-65 | **Volume-squared quirk applied on the ambient path only.** Retail multiplies its volume knob twice on several paths: `PlaySoundA(DataID, CPhysicsObj*)` passes `effect_sound_volume` as the `vol` argument and `GetAttenuation` then multiplies by `effect_sound_volume` again, and both `PlayAmbientSound*` entry points pre-multiply by `ambient_sound_volume` before that same second multiply — so those sliders are effectively squared. acdream's `RetailSoundMixer.TryGetAttenuation` applies the knob exactly once (which is what `GetAttenuation` itself does) and the animation-hook path does not pre-multiply. Slice A5 squares the ambient path, where two independent lanes byte-confirmed the double application. | `src/AcDream.Core/Audio/RetailSoundMixer.cs` (`TryGetAttenuation` remarks); `src/AcDream.App/Audio/OpenAlAudioEngine.cs` (`Play3DWave`) | Which `PlaySoundA` overload the animation-hook path reaches was not pinned by the lane-1 decode, and inventing a squaring on an unconfirmed overload would change every hook sound's loudness curve on a guess. Single-multiply is the conservative, decoded-function-exact choice; the open question is cheap to settle with a cdb breakpoint on the two overloads. | At a non-unity effect slider, hook sounds are louder than retail (slider 0.5 gives −6 dB where retail gives −12). At the default slider of 1.0 the two are identical, so this is inert until the user moves the slider. | `SoundManager::PlaySoundA @ 0x00550AF0`/`@ 0x00550B70`/`@ 0x005507A0`; `SoundManager::GetAttenuation @ 0x00550020`; `docs/research/2026-08-08-audio-retail-soundmanager-core.md` §3 D12 | | ~~TS-66~~ | **RETIRED 2026-08-08 (Campaign A listening-gate fix; user-reported).** `seen_outside` interiors now keep the OUTDOOR ambient set: the listener source resolves the per-cell `CEnvCell.seen_outside` bit through the physics cache's `CellPhysics` record (the same #107 field `AdjustPosition` reads) and converts the ENVCELL-local origin through the cell's `WorldTransform` into landblock coordinates before the 3×3 walk centres on it — an outdoor Position's origin is already landblock-local, an envcell's is not, and skipping the conversion would centre the walk on a wrong point by up to a landblock. A cell record not yet resident resolves to silence for that rebuild rather than a wrong walk. Sealed interiors (dungeons) remain silent, which is retail-correct. | retired | — | — | `Ambient` gate per `docs/research/2026-08-08-audio-retail-ambient-authoring.md` §6/§8; `CEnvCell::add_ambient_sounds` (folded `ret`); user listening gate 2026-08-08 ("in retail I get both outside ambient and the ambient from indoors") | -| TS-68 | **Filed 2026-08-09 (Campaign CH slice CH4); corrected 2026-08-09 at the CH4 REJECT-review, Blocker 1.** `@allegiance`/`@all` and `@house`/`@hou` are real retail management-command dispatchers with 12 and 15 subcommands respectively (registry doc §2.5/§2.5b). acdream ports only the subset with simple parameterless/single-field wire shapes (allegiance `info`/`hometown`/`ho`; house `recall`/`re`/`mansion_recall`/`alleg_recall`/`ma`/`abandon`). For `@house`, every other subcommand (open, close, storage, remove, boot, boot_all, remove_all, guest, available, hooks, on, off) still falls through to ACE server-passthrough (which replies "Unknown command") — unchanged from the original filing. **The original filing was WRONG for `@allegiance`/`@all`: retail's own `DoAllegiance` never reaches DoChannelCommand/server-passthrough for an unrecognized subcommand** — it prints "Please see @help Allegiance for more information on how to use this command." locally (`label_57da4b`, 0x0057DA4B) and stays entirely client-side. acdream now matches this: every allegiance subcommand beyond `info`/`hometown`/`ho` (boot, ban, officer, title, name, lock, chat, broadcast, motd) shows the same retail refusal client-side instead of reaching ACE — closing a real bug where the unmatched subcommand text was instead broadcast to the Allegiance chat channel (0x02000000). The standalone `@motd` verb (reached directly, not via `@allegiance motd`) remains a separate, still-open gap. `RetailClientCommandCatalog.TryMatchHouse`/`TryMatchAllegiance` (`src/AcDream.UI.Abstractions/Panels/Chat/RetailClientCommandCatalog.cs`) | Retail would execute these locally (with its own usage/confirmation/refusal text). House's unported subcommands still reach ACE, which does not implement them as chat commands either — no functional loss on a real server, but a user typing e.g. `@house open` gets ACE's generic "Unknown command" instead of retail's real behavior. Allegiance's unported subcommands now correctly stay local with retail's own refusal text instead of reaching ACE at all — matching retail, not merely harmless. | `ClientCommunicationSystem::DoAllegiance @ 0x0057D5A0`; `DoHouse @ 0x00580860`; ACE `GameActionType` opcodes for each subcommand (all exist server-side) | +| TS-68 | **Filed 2026-08-09 (Campaign CH slice CH4); corrected 2026-08-09 at the CH4 REJECT-review, Blocker 1.** `@allegiance`/`@all` and `@house`/`@hou` are real retail management-command dispatchers with 12 and 15 subcommands respectively (registry doc §2.5/§2.5b). acdream ports only the subset with simple parameterless/single-field wire shapes (allegiance `info`/`hometown`/`ho`; house `recall`/`re`/`mansion_recall`/`alleg_recall`/`ma`/`abandon`). For `@house`, every other subcommand (open, close, storage, remove, boot, boot_all, remove_all, guest, available, hooks, on, off) still falls through to ACE server-passthrough (which replies "Unknown command") — unchanged from the original filing. **The original filing was WRONG for `@allegiance`/`@all`: retail's own `DoAllegiance` never reaches DoChannelCommand/server-passthrough for an unrecognized subcommand** — it prints "Please see @help Allegiance for more information on how to use this command." locally (`label_57da4b`, 0x0057DA4B) and stays entirely client-side. **Corrected again 2026-08-09 at the CH4 re-review, SHOULD-FIX 3.** Retail does NOT refuse boot/ban/officer/title/motd/name/lock/house/chat/broadcast — `DoAllegiance`'s dispatcher table EXECUTES each one locally through its own handler (e.g. `DoAllegianceBoot @ 0x0057D646` is the dispatcher's call site into `ClientCommunicationSystem::DoAllegianceBoot`; `DoAllegianceBan`/`DoAllegianceOfficer`/`DoAllegianceOfficerTitle`/`DoMotd`/`DoAllegianceName`/`DoAllegianceLock`/`DoAllegianceHouse` are its siblings in the same table). acdream has none of those nine handlers ported (tracked by issue #360) and instead shows the SAME unrecognized-subcommand refusal ("Please see @help Allegiance...", `label_57da4b`, 0x0057DA4B) for every one of them, pending the #360 port. What matches retail here is the OWNERSHIP RULE — the verb never reaches `DoChannelCommand`/server-passthrough for `@allegiance`/`@all` regardless of subcommand — NOT the subcommand's actual behavior, which retail executes and acdream does not yet. This still closes the real bug the original filing named (the unmatched subcommand text broadcast to the Allegiance chat channel, 0x02000000). The standalone `@motd` verb (reached directly, not via `@allegiance motd`) remains a separate, still-open gap. `RetailClientCommandCatalog.TryMatchHouse`/`TryMatchAllegiance` (`src/AcDream.UI.Abstractions/Panels/Chat/RetailClientCommandCatalog.cs`) | Retail would execute these locally (with its own usage/confirmation/refusal text). House's unported subcommands still reach ACE, which does not implement them as chat commands either — no functional loss on a real server, but a user typing e.g. `@house open` gets ACE's generic "Unknown command" instead of retail's real behavior. Allegiance's unported subcommands correctly stay local (never reach ACE) but show a generic refusal instead of retail's real per-subcommand execution — a user typing e.g. `@allegiance boot Name` gets "Please see @help Allegiance..." instead of retail's real boot confirmation/effect, until #360 ports the nine `DoAllegiance*`/`DoMotd`/`DoAllegianceHouse` handlers. | `ClientCommunicationSystem::DoAllegiance @ 0x0057D5A0`; `DoHouse @ 0x00580860`; ACE `GameActionType` opcodes for each subcommand (all exist server-side) | | TS-69 | **Filed 2026-08-09 (Campaign CH slice CH4).** `@day`, `@log`, and `@render` are registered retail verbs acdream recognizes only in the `/help ` lookup table, not as executable client commands. `@day` needs a sky/time-of-day override hook the renderer doesn't expose; `@log` needs a safely-lifecycled chat-to-file writer (deferred to avoid an unaudited file-handle leak across reconnects); `@render` has no acdream equivalent to retail's `SmartBox::HandleRenderOption` render-option surface. All three fall through to server passthrough. `RetailCommandHelpTable` (`src/AcDream.UI.Abstractions/Panels/Chat/RetailCommandHelpTable.cs`) | A user typing `@day`/`@log`/`@render` gets ACE's "Unknown command" instead of retail's local toggle/file-copy/render-option behavior — cosmetic/QoL only, no gameplay impact | `ClientCommunicationSystem::DoDay @ 0x005706F0`; `DoSetOutput @ 0x0057E4F0`; `DoRenderOption @ 0x0057E120` | | TS-70 | **Filed 2026-08-09 (Campaign CH slice CH4).** The new `@index`/`@clist`/`@on`/`@off`/`@hslist`/`@allegiance info` outbound requests (`ClientCommandRequests.BuildIndexChannels`/`BuildListChannel`/`BuildOnChannel`/`BuildOffChannel`/`BuildListAvailableHouses`/`BuildAllegianceInfoRequest`) send the byte-correct retail wire request, but the corresponding inbound GameEvents (`ChannelIndex 0x0149`, `ChannelList 0x0148`, `AvailableHouses 0x0271`, `AllegianceInfoResponse 0x027C`) are registered in `GameEventType` but have no `GameEventWiring` handler — the server's reply is silently dropped rather than rendered. `src/AcDream.Core.Net/GameEventWiring.cs` | The request reaches ACE correctly (verifiable on the wire / server-side log) but the client shows nothing in response — looks like the command silently failed | ACE `GameEventChannelIndex`/`GameEventChannelList`/`GameEventHouseListAvailable`/`GameEventAllegianceInfoResponse` (`references/ACE/Source/ACE.Server/Network/GameEvent/Events/`) | | TS-67 | **Ambient contributions are computed in-plane.** Retail's `CLandBlock::add_ambient_sounds` @ `0x530310` positions each contributing land cell at its own SW terrain VERTEX, including that vertex's height, and `Ambient::CalcWeight` deliberately includes Z in its distance (where `CalcDir` deliberately excludes it — the two differ on purpose). acdream's gatherer supplies Z = 0 for the offset, so a cell's weight ignores the height difference between the listener and the terrain under that cell. | `src/AcDream.Core/Audio/AmbientSoundGatherer.cs` (`ContributeLandblock`) | Sampling the height needs the landblock's height table threaded into the walk alongside the terrain words; the walk already runs only on a 24 m crossing so the cost is not the obstacle, the extra plumbing at slice end was. The error is bounded by terrain relief inside 120 m and affects the crossfade weight only, never the direction. | On steep ground an ambient reads slightly louder than retail, because the true 3-D distance is longer than the planar one. | `CLandBlock::add_ambient_sounds @ 0x530310`; `Ambient::CalcWeight @ 0x550DD0` | diff --git a/docs/plans/2026-08-09-chat-parity-campaign.md b/docs/plans/2026-08-09-chat-parity-campaign.md index d0873d9c..2b101218 100644 --- a/docs/plans/2026-08-09-chat-parity-campaign.md +++ b/docs/plans/2026-08-09-chat-parity-campaign.md @@ -114,7 +114,7 @@ implementer per slice against a pinned contract (per | CH1 colors | `172c6f9a` | 11,835 passed / 4 skipped / 0 failed | APPROVE-WITH-FIXES; fixed `34d8a3c0` | pending | | CH2 interface text | `77c8296e`, reworked `e0e78883` | 11,916 passed / 4 skipped / 0 failed | REJECT → reworked `e0e78883` → re-review APPROVE-WITH-FIXES → nits `233c30d1` | pending | | CH3 side channels | `614a1e05` | 11,964 passed / 4 skipped / 0 failed | APPROVE-WITH-FIXES; fixed `e07fba57` | pending (connected gate — see handoff below) | -| CH4 commands | `090825e7` | 12,190 passed / 4 skipped / 0 failed | REJECT; fixed `724ef2d3` | pending | +| CH4 commands | `090825e7` | 12,190 passed / 4 skipped / 0 failed | REJECT; fixed `724ef2d3`; re-review APPROVE-WITH-FIXES; closed `` | pending | | CH5 closeout | — | — | — | — | ### CH4 closeout (2026-08-09) diff --git a/src/AcDream.App/Net/LiveSessionRuntimeFactory.cs b/src/AcDream.App/Net/LiveSessionRuntimeFactory.cs index 8c566213..f809f258 100644 --- a/src/AcDream.App/Net/LiveSessionRuntimeFactory.cs +++ b/src/AcDream.App/Net/LiveSessionRuntimeFactory.cs @@ -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; diff --git a/src/AcDream.App/UI/ClientCommandController.cs b/src/AcDream.App/UI/ClientCommandController.cs index c49358b2..d58ee029 100644 --- a/src/AcDream.App/UI/ClientCommandController.cs +++ b/src/AcDream.App/UI/ClientCommandController.cs @@ -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.", diff --git a/src/AcDream.App/UI/Layout/RetailDialogFactory.cs b/src/AcDream.App/UI/Layout/RetailDialogFactory.cs index f4c71c2f..1c2e2a90 100644 --- a/src/AcDream.App/UI/Layout/RetailDialogFactory.cs +++ b/src/AcDream.App/UI/Layout/RetailDialogFactory.cs @@ -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? queue) || queue.First is null) return; diff --git a/src/AcDream.UI.Abstractions/Panels/Chat/RetailClientCommandCatalog.cs b/src/AcDream.UI.Abstractions/Panels/Chat/RetailClientCommandCatalog.cs index 9a56ead5..74faba5e 100644 --- a/src/AcDream.UI.Abstractions/Panels/Chat/RetailClientCommandCatalog.cs +++ b/src/AcDream.UI.Abstractions/Panels/Chat/RetailClientCommandCatalog.cs @@ -723,8 +723,15 @@ public static class RetailClientCommandCatalog /// verb this catalog claims must actually be in the registry, and vice /// versa. /// + // 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 KnownVerbs { get; } = - ByVerb.Keys.Concat(["house", "hou", "allegiance", "all"]).ToArray(); + ByVerb.Keys.Concat(["house", "hou", "allegiance", "all"]) + .ToFrozenSet(StringComparer.OrdinalIgnoreCase); /// /// /help <verb> lookup for a catalog-dispatched command — diff --git a/tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs b/tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs index d85e5e2a..cb0556cd 100644 --- a/tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs +++ b/tests/AcDream.App.Tests/Net/LiveSessionCommandRouterTests.cs @@ -3,6 +3,7 @@ using AcDream.App.Net; using AcDream.App.UI; using AcDream.Core.Chat; using AcDream.Core.Items; +using AcDream.Core.Net.Messages; using AcDream.Core.Social; using AcDream.Runtime.Gameplay; using AcDream.UI.Abstractions; @@ -437,6 +438,66 @@ public sealed class LiveSessionCommandRouterTests Assert.Equal([(0x26u, true)], options); } + // ── CH4 re-review SHOULD-FIX 2 (2026-08-09) ───────────────────────── + // The Settings Chat toggles reach this same SetSingleCharacterOptionRuntimeCmd + // route (RuntimeSettingsController.PublishHearOptionChange -> + // RuntimeSettingsTargets.SetSingleCharacterOption -> + // SetSingleCharacterOptionRuntimeCmd -> here -> bindings.SendSingleCharacterOption) + // as a second entrance alongside @join/@leave. LiveSessionRuntimeFactory now + // wires SendSingleCharacterOption through the SAME local-write chokepoint as + // the @join/@leave binding, so this mirrors + // TurbineChatMembershipGateTests.JoinChannel_SetOptionBit_FlipsGateToAllowed_ + // WithoutFreshPlayerDescription, but drives the write through the router's + // Settings-route command instead of calling RuntimeCharacterOptionsState + // directly. + + [Fact] + public void SettingsRouteSetSingleCharacterOption_FlipsGateWithoutFreshPlayerDescription() + { + var characterState = new RuntimeCharacterState(); + characterState.Options.Replace(characterState.Options.Options1, 0u); // every Options2 bit off — General starts refused + var turbine = new TurbineChatState(); + turbine.OnChannelsReceived( + allegianceRoom: 0x10u, + generalRoom: 0x11u, + tradeRoom: 0x12u, + lfgRoom: 0x13u, + roleplayRoom: 0x14u, + olthoiRoom: 0x15u, + societyRoom: 0x16u, + societyCelestialHandRoom: 0u, + societyEldrytchWebRoom: 0u, + societyRadiantBloodRoom: 0u); + var sent = new List<(uint OptionId, bool Value)>(); + // Mirrors LiveSessionRuntimeFactory.CreateCommandBindings' shared + // SendSingleCharacterOption local function: local write FIRST, then + // the wire send. + LiveSessionCommandRouter router = NewRouter( + characterState: characterState, + sendSingleCharacterOption: (id, value) => + { + characterState.Options.SetOptionBit(id, value); + sent.Add((id, value)); + }); + router.Activate(); + + Assert.Equal( + TurbineChatGateStatus.NotListening, + TurbineChatMembershipGate.Evaluate( + ChatChannelKindLite.General, turbine, characterState.Options, + isOlthoiPlayer: false).Status); + + router.Publish(new SetSingleCharacterOptionRuntimeCmd( + (uint)CharacterOptionId.ListenToGeneralChat, true)); + + Assert.Equal([((uint)CharacterOptionId.ListenToGeneralChat, true)], sent); + Assert.Equal( + TurbineChatGateStatus.Allowed, + TurbineChatMembershipGate.Evaluate( + ChatChannelKindLite.General, turbine, characterState.Options, + isOlthoiPlayer: false).Status); + } + [Fact] public void ShowWeenieErrorFriendsFull_ResolvesThroughAddText_AndLandsInSpewBoxNotChat() { diff --git a/tests/AcDream.App.Tests/UI/ClientCommandControllerTests.cs b/tests/AcDream.App.Tests/UI/ClientCommandControllerTests.cs index 1b3bf159..92febc2a 100644 --- a/tests/AcDream.App.Tests/UI/ClientCommandControllerTests.cs +++ b/tests/AcDream.App.Tests/UI/ClientCommandControllerTests.cs @@ -303,9 +303,12 @@ public sealed class ClientCommandControllerTests // ── CH4 REJECT-review Blocker 2 (2026-08-09) ──────────────────────── // "@house abandon" must run retail's real two-stage confirmation // (DoHouse's abandon branch @0x00580D58 → HouseAbandonDialogCallback_ - // First @0x00580E1A → HouseAbandonDialogCallback_Second @0x0057BE90, + // First, function entry @0x00580240 (0x00580E1A is only DoHouse's load + // site for the callback pointer), whose own stage-2 string site + // @0x005802D8 opens HouseAbandonDialogCallback_Second @0x0057BE90, // the ONLY Event_AbandonHouse() call site @0x0057BF01) before sending // 0x021F — previously it sent immediately with no confirmation at all. + // Citations corrected 2026-08-09, CH4 re-review nit 4. [Fact] public void HouseAbandon_BothStagesAccepted_ShowsBothPromptsThenSendsExactlyOnce() @@ -398,24 +401,26 @@ public sealed class ClientCommandControllerTests controller.Execute(new ExecuteClientCommandCmd(id, arguments)); } - [Fact] - public void ChannelArgumentCommands_KnownTag_SendsWithoutError() + [Theory] + [InlineData(ClientCommandId.ListChannel, "fellowship", "clist:2048")] + [InlineData(ClientCommandId.OnChannel, "admin", "on:2")] + [InlineData(ClientCommandId.OffChannel, "sentinel", "off:512")] + // CH4 re-review nit 5 (2026-08-09): pins "@clist allegiance" to + // RequestChannelList(0x02000000) — retail's real Allegiance channel id + // (RetailChannelTagTable's "allegiance"/"a"/"ab" entries), distinct + // from the small legacy-channel ids the rows above cover. + [InlineData(ClientCommandId.ListChannel, "allegiance", "clist:33554432")] + public void ChannelArgumentCommands_KnownTag_SendsWithoutError( + ClientCommandId id, string tag, string expectedCall) { var calls = new List(); var errors = new List(); var controller = NewController(calls, errors); - Execute(ClientCommandId.ListChannel, "fellowship"); - Execute(ClientCommandId.OnChannel, "admin"); - Execute(ClientCommandId.OffChannel, "sentinel"); + controller.Execute(new ExecuteClientCommandCmd(id, tag)); Assert.Empty(errors); - Assert.Equal( - ["clist:2048", "on:2", "off:512"], - calls); - - void Execute(ClientCommandId id, string arguments) => - controller.Execute(new ExecuteClientCommandCmd(id, arguments)); + Assert.Equal([expectedCall], calls); } [Fact] diff --git a/tests/AcDream.App.Tests/UI/Layout/RetailDialogFactoryTests.cs b/tests/AcDream.App.Tests/UI/Layout/RetailDialogFactoryTests.cs index 8ae2d1d5..16f2ce4d 100644 --- a/tests/AcDream.App.Tests/UI/Layout/RetailDialogFactoryTests.cs +++ b/tests/AcDream.App.Tests/UI/Layout/RetailDialogFactoryTests.cs @@ -255,6 +255,51 @@ public sealed class RetailDialogFactoryTests Assert.Empty(root.Children); } + [Fact] + public void AcceptingChainedReentrantDialogDoesNotThrowAndDrainsQueueAfterChainCompletes() + { + // CH4 re-review should-fix 1: a callback that synchronously reopens a new + // dialog under the SAME queue key (retail's two-stage house-abandon + // confirmation) used to crash OpenNextDialog with a duplicate-key + // ArgumentException whenever a third confirmation was already queued + // behind the chain. See RetailDialogFactory.OpenNextDialog. + var root = new UiRoot { Width = 800f, Height = 600f }; + var layouts = new List(); + var factory = CreateFactory(root, layouts); + var completed = new List(); + + factory.MakeConfirmation("stage1", _ => + { + completed.Add("stage1"); + factory.MakeConfirmation("stage2", _ => completed.Add("stage2")); + }); + factory.MakeConfirmation("queued behind chain", _ => completed.Add("queued")); + + Assert.Equal(1, factory.PendingCount); + + Exception? thrown = Record.Exception(() => Accept(layouts[0])); + + Assert.Null(thrown); + Assert.Equal(["stage1"], completed); + Assert.Equal(2, layouts.Count); + Assert.Equal("stage2", Message(layouts[1])); + Assert.Same(layouts[1].Root, root.Modal); + Assert.Equal(1, factory.PendingCount); + + Accept(layouts[1]); + + Assert.Equal(["stage1", "stage2"], completed); + Assert.Equal(3, layouts.Count); + Assert.Equal("queued behind chain", Message(layouts[2])); + Assert.Same(layouts[2].Root, root.Modal); + Assert.Equal(0, factory.PendingCount); + + Accept(layouts[2]); + + Assert.Equal(["stage1", "stage2", "queued"], completed); + Assert.False(factory.IsOpen); + } + private static RetailDialogFactory CreateFactory( UiRoot root, List layouts)