From 7b60e71b85703289f3c7993baa02ff69b7048c7c Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 03:19:35 +0200 Subject: [PATCH] fix(headless,runtime): OP7 review fixes + docs: OP3 re-review REOPEN (narrow) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TWO work products share this commit (a staged-index collision between the coordinator's docs commit and the OP7 fixer's staged files — content verified complete and coherent; only this message was wrong before the amend): 1. OP7 review fixes (all nine findings from docs/research/2026-08-11-op7-review.md): - M1: HeadlessSessionDescriptor is a record; WithAccount uses 'with' non-destructive record copy, so a future property cannot be silently dropped; direct-CLI regression test proves CharacterOptions survives --user/--password. - M2 root fix: LiveSessionEventRouter skips BOTH Replace and the options notification on a trailer-truncated PlayerDescription — a truncated re-seed can no longer install zeroed words under an armed latch for OP7's automation to flush into 0x01A1. - SF1: schema keys validate as ordinal strings against the allowed names (numeric / comma-combined aliases rejected). SF2: both-true fellowship exclusion rejected at load, naming both keys. SF3: the onLoginCompleteSent observer moved after transit.EndTeleport(). SF4: production-hook coverage for all three LoginComplete sites. SF5: test-script OP7 wire expectation corrected (batched ids ride only the 0x01A1). 2. docs/research/2026-08-11-op3-rereview.md — OP3 re-review verdict REOPEN (narrow): M1 byte-decode independently re-verified (6a 07 at all six sites); residuals R1 (gate script promises a timestamp prefix acdream doesn't render), R2 (null-controller player-mode still refuses), R3 (dormancy pin lacks stimulus) — coordinator fixes follow. Full Release suite at this tree: 12,956 passed / 4 skipped / 0 failed. Co-Authored-By: Claude Fable 5 --- .../2026-08-11-campaign-op-test-script.md | 21 +- docs/research/2026-08-11-op3-rereview.md | 430 ++++++++++++++++++ .../Configuration/HeadlessConfiguration.cs | 8 +- .../HeadlessConfigurationLoader.cs | 52 ++- .../Hosting/HeadlessProcessHost.cs | 15 +- .../Session/LiveSessionEventRouter.cs | 13 +- .../RuntimeLiveEntitySessionController.cs | 9 +- .../HeadlessConfigurationLoaderTests.cs | 76 ++++ .../HeadlessSessionHostTests.cs | 175 ++++++- .../Session/LiveSessionEventRouterTests.cs | 132 ++++++ ...RuntimeLiveEntitySessionControllerTests.cs | 99 ++++ 11 files changed, 1007 insertions(+), 23 deletions(-) create mode 100644 docs/research/2026-08-11-op3-rereview.md diff --git a/docs/research/2026-08-11-campaign-op-test-script.md b/docs/research/2026-08-11-campaign-op-test-script.md index 78baa518..902848a2 100644 --- a/docs/research/2026-08-11-campaign-op-test-script.md +++ b/docs/research/2026-08-11-campaign-op-test-script.md @@ -228,11 +228,15 @@ persists the sends. `0x24`) — immediately, and ACE's handler additionally joins the Turbine trade-chat room server-side for this session (observable via ACE's own Turbine-chat membership logging if available). -- One `SetSingleCharacterOption (0x0005)` for `SalvageMultiple` (id - `0x22`) followed by exactly one `SetCharacterOptions (0x01A1)` blob - flush — `SalvageMultiple` is batched (dirties the module; OP7's - diff-and-send calls the explicit `SaveOptions` verb once, after every - declared id has been diffed, never interleaved). +- **No** `SetSingleCharacterOption (0x0005)` for `SalvageMultiple` (id + `0x22`) — it is batched, not auto-save, so `TrySetOption` only marks + the module dirty and never sends `0x0005` for it. The single + `SetCharacterOptions (0x01A1)` blob flush is the ONLY wire evidence of + this id changing (OP7's diff-and-send calls the explicit `SaveOptions` + verb once, after every declared id has been diffed, never interleaved + — so this same `0x01A1` also carries whatever the loop already wrote + locally for `IgnoreAllegianceRequests`/`ListenToTradeChat`, even though + those two ALSO got their own immediate `0x0005`). - **No other option bits change.** ACE's stored `CharacterOptions1`/ `CharacterOptions2` for every UNDECLARED id stay exactly what they were before the run. @@ -255,9 +259,10 @@ persists the sends. current value produces nothing" rule — set-character-options-wire.md §3.1/§3.5). 5. **Flip one declared value** in the config (e.g. `SalvageMultiple` back - to `false`) and run again. Expect exactly ONE `0x0005` + - `0x01A1` pair for that id only — the other two, still matching, - produce nothing. + to `false`) and run again. `SalvageMultiple` is batched, so expect + exactly ONE `0x01A1` blob flush and **no** `0x0005` for it — the other + two declared ids, still matching, produce nothing at all (not even the + blob). ### What to report diff --git a/docs/research/2026-08-11-op3-rereview.md b/docs/research/2026-08-11-op3-rereview.md new file mode 100644 index 00000000..2cb8fedd --- /dev/null +++ b/docs/research/2026-08-11-op3-rereview.md @@ -0,0 +1,430 @@ +# Campaign OP slice OP3 — fix-round RE-REVIEW (closure check) + +**Commit under review:** `386076af` ("fix(ui): OP3 review fixes — byte-verified +Magic chat lines, Gameplay/OptionPage shape, mid-air tri-state, shared +geometry") +**Claims closure of:** every finding in +`docs/research/2026-08-11-op3-review-mechanism.md` (M1, S1–S6, N1–N8) and +`docs/research/2026-08-11-op3-review-blast.md` (MUST-FIX 1/2, SHOULD-FIX 1–4, +NOTE 1–8). +**Method:** read-only. No build, no test run, no client launch. Git reads and +a raw byte read of the PDB-paired retail binary only. + +## VERDICT: REOPEN (narrow) + +The *substance* of every finding is closed correctly — including M1, whose +byte-level claim I re-derived independently and confirmed exactly. Three +residuals remain, all cheap, none requiring a design change: + +| ID | Class | One-line | +|---|---|---| +| R1 | **MUST-FIX** | The fix round's own "timestamped, logged" claim is FALSE for acdream (no timestamp prefix is rendered anywhere; no chat-log-file writer exists). It is repeated in the gate script's step 15, so the connected gate now instructs the user to look for something that cannot appear. | +| R2 | SHOULD-FIX | The tri-state `IsGrounded` does not implement the third state it documents: `IsPlayerMode == true` with a null controller still returns `false` → mid-air refusal. Three comments and the commit message assert the opposite — the same comment-vs-code inversion S3 was filed to remove. | +| R3 | SHOULD-FIX | The new dormant-`ActivePageChanged` pin applies no stimulus, so it asserts silence in a vacuum. It passes identically if dormancy is completely broken. | + +Everything else is genuinely closed. Details below, evidence first. + +--- + +## 0. What I verified as CORRECTLY CLOSED + +### 0.1 M1 — INDEPENDENTLY BYTE-VERIFIED, claim exact + +The commit claims `6a 07` at `0x0049E972`/`E9E2`/`EA52`/`EAA4`/`EAF6`/`EB48`. + +**Internal consistency (the check I was asked for).** The mechanism review +recorded the six `AddTextToScroll` call addresses as `0x0049E97C`/`E9EC`/ +`EA5C`/`EAAE`/`EB00`/`EB52`. Every one of the commit's six offsets is exactly +**10 bytes** below its matching call — a uniform delta, and the first one +(`0049e972`) is verbatim the BN line the review quoted. Re-reading +`docs/research/named-retail/acclient_2013_pseudo_c.txt:169512-169640` confirms +all six `*(uint32_t*)((char*)var_2c)[4] = 7;` lines carry precisely those six +addresses. The recorded evidence is complete and self-consistent. + +**Independent decode (beyond what was asked; the binary read is read-only).** +`C:\Users\erikn\Downloads\acclient.exe` — TimeDateStamp `0x52291F34` = +2013-09-06 00:17:56 UTC, the PDB-paired build; ImageBase `0x400000`, `.text` +VA `0x1000` / raw `0x1000`, so VA→file offset is identity minus base. Bytes at +each of the six sites, with six bytes of lead-in: + +``` +6A 00 push 0 ; arg5 windowId = 0 +6A 01 push 1 ; arg4 allowPluginFilter = 1 +8D 57 14 lea edx,[edi+14] +6A 07 push 7 ; arg3 type = 7 <-- THE CLAIM +52 push edx ; arg2 text +E8 16 56 0C 00 call 0x00563F90 ; ClientUISystem::GetUISystem +8B C8 mov ecx,eax ; this +E8 3F 96 FE FF call 0x00487FC0 ; ClientSystem::AddTextToScroll(char const*) +``` + +All six sites are byte-identical in shape (`6A 00 / 6A 01 / 8D {57,4F,47} 14 / +6A 07 / {52,51,50} / call / mov ecx,eax / call`). Both call targets resolve by +name in the pseudo-C: `:368536` `00563f90 ClientUISystem::GetUISystem()` and +`:148528` `00487fc0 ClientSystem::AddTextToScroll(ClientSystem* this @ ecx, +char const* arg2, uint32_t arg3, uint8_t arg4, uint32_t arg5)`. **`arg3` (type) +is 7 at all six sites. Confirmed.** + +**The code change matches the evidence.** `RetailUiRuntime.cs:1945-1946` routes +the six macro lines through the new +`OptionsRuntimeBindings.DisplayMouseTurningMacroLine`, bound at +`InteractionRetainedUiComposition.cs:832-833` to +`AddText(text, RetailLogTextType.Magic)`; `RetailLogTextType.Magic = 0x07` +(`src/AcDream.Core/Chat/RetailLogTextType.cs:33`). The mid-air refusal +(`RetailUiRuntime.cs:1905`) and UA/RA (`RetailUiRuntime.cs:1982` → +`OptionsPanelController.cs:189-192`) both still use `DisplaySystemMessage` = +`ClientLocal`. Exactly the split the review asked for. + +**Route traced to the chat scroll, no truncation.** +`RuntimeCommunicationState.AddText` (`:195-203`): `Trim()`, then +`if (type == ClientLocal) { SpewBox.Enqueue; return; }` — Magic falls through +to `Chat.OnSystemMessage(text, 7)`. `ChatLog.OnSystemMessage` (`:266-293`) +appends a `ChatEntry` with `LogTextType = 7`; `Append` (`:414-418`) trims only +against `_maxEntries` (default **500**, `:42`), so six lines cannot be dropped +— the 4-slot `SpewBoxState.MaxConcurrentItems` truncation that discarded +*Camera Stiffness* and *Camera Adjustment* is gone. The main window's default +type filter is `0xFBFFFFFF` (`ChatWindowState.cs:111`), which clears only bit +`0x1A`; bit 7 passes. The `SystemDedupWindow` short-circuit cannot collapse +the burst — all six strings are distinct +(`MouseTurningSettingsMacro.cs:58-104`). Colour is `#3FBFFF` light blue +(`2026-08-09-chat-retail-color-table.md:151`), matching what the gate script +tells the user to expect. ✅ + +*(The one thing about this route that is NOT true is the "timestamped, logged" +half — see R1.)* + +### 0.2 S1 — Gameplay's slot carries no `AfterApply`; the other three keep theirs + +`OptionsPanelController.cs:138` `_pages.Add(GameplayPageId, new OptionPage +{ AfterApply = null });` and `:139-143` the loop that still gives +Character/Chat/Config `AfterApply = afterApply`. **The seam OP4-6 will bind +survived intact**: `Callbacks.AfterApply` (`:81`) → `RetailUiRuntime.cs:1983-1984` +→ `Publish(new SaveCharacterOptionsRuntimeCmd())`, unchanged, and the three +real pages are constructed from it in the same constructor. Nothing about +OP4-6's wiring path was narrowed. + +Keeping an (empty) `OptionPage` for the Gameplay slot is one of the two fixes +the review itself offered ("or construct it with `AfterApply = null` and +document *why*"). `OnShown()`→`Apply()` over zero rows is then: empty +`foreach`, `AfterApply?.Invoke()` null, `OnOptionChanged?.Invoke()` null — +byte-for-byte the no-op retail's plain `UIElement::OnVisibilityChanged` is. + +**The two tests were rewritten to the right shape.** +`OptionsPanelControllerTests.cs:63-83` `ActivateTabs_SelectsGameplayAsDefault_ +ButNeverFlushesIt` now asserts `gameplayFlushCount == 0` with the controller's +own `AfterApply` wired — a real pin, not a rename. `:143-159` +`GameplayPage_OnShownAndOnHidden_NeverFlush_EvenWhenControllerAfterApplyIsWired` +pins the same property independent of tab framing. The two +`OptionPageModelTests` entries were retargeted at a synthetic empty page and +renamed to say so, with the class doc (`:5-22`) now explicitly stating neither +shape models the Gameplay tab. + +**The model's doc no longer overclaims.** `OptionPageModel.cs:157-172` keeps +the generic empty-page property but bolds "**This is a property of the generic +empty-page shape, not a description of the Gameplay tab**" and cites +`acclient.h:55857` `UIElement_Field`. ✅ + +### 0.3 S2 — `OnOptionChanged` fires at retail's four call sites, Defaults ungated + +`OptionPage.Apply` (`:222-228`) → rows, `AfterApply`, then `OnOptionChanged` +(retail `0x004F2C95`). `Reset` (`:236-241`) → changed rows, then +`OnOptionChanged` (`0x004F2D4A`). `Defaults` (`:246-251`) → all rows, then +`OnOptionChanged` (`0x004F2CE5`). Per-row live edit: `BoolOptionRow. +SetCurrentValue` (`:100-105`) notifies; `RestoreSavedValue`/ +`RestoreDefaultValue` deliberately do not — retail's `Apply(0)` paths, where +the owning verb notifies once itself. Wired by `OptionPage.Register` (`:211`) +through the new `IOptionRow.AttachPageNotify`. This is exactly the shape the +review specified. + +`Defaults` is never gated: the model contains no gate at all, and the +`OnOptionChanged` doc (`:191-203`) records that retail's own override never +fetches `0x100001FE`. `OnShown`/`OnHidden` inherit the notify through +`Apply`/`Reset`, matching retail's `OnVisibilityChanged` → `SaveCurrentValues`/ +`RestoreSavedValues`, both of which end in `OnOptionChanged`. + +**Usable by OP4-6.** The seam is `Action? OnOptionChanged` + the existing +`bool Changed` (`:217`); a page binds +`page.OnOptionChanged = () => { apply.Enabled = page.Changed; reset.Enabled = +page.Changed; }`, which is precisely retail's `GetChildRecursive(0x100001FC/FD)` ++ `SetState(0x01/0x0D)`. The model correctly does NOT own the enabled state +itself. Six new tests pin order (`["afterApply","onOptionChanged"]`), all three +verbs, the empty-page case, the `SetCurrentValue`-only row notify, the +`Restore*` non-notify, and `Register`'s attachment. ✅ + +### 0.4 S4/S5/S6 + blast MUST-FIX 2 — register and script bookkeeping + +- **AD-77** (`retail-divergence-register.md:174`) is scoped client-wide ("every + `Mount*`/`RegisterMainPanel` call site for a `gmPanelUI` sibling … + client-wide, not an Options-specific gap"), classed AD not TS, carries the + five-column shape with retail anchors, and states the inherited-not- + introduced nuance the review asked for. Section count bumped **56 → 57** in + the same hunk (`:65`); TS count correctly *not* bumped (TS-74 amended, not + added). No retired row resurrected in the diff. +- **TS-74** (`:408`) now names all five store-only keys in What/Where + (`Camera_Stiffness`, `Camera_AdjustmentSpeed`, `Camera_AlignToSlope`, + `Input_MouseLookSensitivity`, `Input_InvertMouseLookYAxis`), cites + `CameraTurningSettings.cs` and `CameraPointerInputController.cs` as the two + separate stores, and adds the "chat line quotes an unrelated stored value" + consequence to Risk. Exactly S5's ask. +- **Test script**: step 3 gains the ghosted→enabled + highlight check; step 5 + is corrected to bottom-edge-only with an explicit "this is a correction from + OP3-as-landed" note; steps 13/14 gain the UA/RA legibility eye-item (blast + SHOULD-FIX 4); steps 16/17 add relogin (`0x0005` survived) and relaunch + (five prefs survived), both written so a failure is *discriminating* rather + than ambiguous; "What to report" updated to 17 steps. ✅ + +### 0.5 blast SHOULD-FIX 2 — geometry now matches the cohort exactly + +`RetailUiRuntime.cs:2021-2023` `ResizeX = false, ResizeY = true, +ResizableEdges = ResizeEdges.Bottom`, with `MinWidth`/`MinHeight` removed. I +read every sibling that calls `RegisterMainPanel`: spellbook (`:1337-1339`), +effects (`:1528-1530`), the four indicator-detail panels (`:1751-1753`), +character (`:2266-2268`), inventory (`:2349-2351`) — all identical. No +`ResizeX`/min-size divergence remains. `ContentAnchors` all-four matches +effects and the indicator-detail panels, so it is not a new outlier either. ✅ + +### 0.6 Blast radius OF the fixes — clean on all three axes + +- **Typed message seam:** `OptionsRuntimeBindings` has exactly ONE construction + site (`InteractionRetainedUiComposition.cs:801`) and + `DisplayMouseTurningMacroLine` exactly ONE consumer + (`RetailUiRuntime.cs:1946`). No test constructs the record, so the added + positional parameter breaks nothing. +- **`Func` → `Func`:** one production caller + (`RetailUiRuntime.cs:1899`); the `switch` covers `true`/`false`/`null` + exhaustively. The unrelated `IsGrounded` occurrences across Runtime/Core.Net + are `UpdatePosition.PositionFlags.IsGrounded` — a different symbol entirely. +- **`IOptionRow.AttachPageNotify`:** `BoolOptionRow` is the ONLY implementer + (no test fakes), so widening the interface compiles everywhere. +- **Geometry:** no test pins the old Options geometry. The only Options + geometry in tests is the new `RetailPanelUiControllerTests` helper mount. A + *stale persisted* width from the pre-fix build is also harmless — I traced + it: `RetailWindowLayoutPersistence.Apply:213` → `RetailWindowHandle.ResizeTo:69-70` + → `RetailWindowManager.ResizeTo:184` `if (!frame.ResizeX) width = frame.Width;`. + The saved width is discarded on restore. Verified non-issue. + +### 0.7 Two of the three new pins genuinely pin + +- `RetailPanelUiControllerTests.cs:131-148` uses the REAL + `RetailPanelCatalog.Options` (10) / `.Character` (11) ids against a real + `RetailPanelUiController.RegisterMainPanel`, and asserts the show-hides-the- + other transition in both directions plus `ActivePanelId`. Genuine — it would + fail if `SetPanelVisibility`'s exclusion regressed. +- `RetailDialogFactoryTests.cs:95-110` exercises the queueing end-to-end + (`ActiveCount == 1`, `PendingCount == 1`), which really would break to `2/0` + if the omitted-key overload took a different key. Slightly tautological at + the language level (the default IS `DefaultQueueKey` in the signature, + `RetailDialogFactory.cs:128`), but the behavioural assertion is real. +- The third does not pin — see R3. + +--- + +## 1. RESIDUALS + +### R1 — MUST-FIX: "timestamped, logged" is retail's behaviour, not acdream's, and the gate script now tells the user to look for it + +**The claim, in three places:** + +1. `src/AcDream.App/UI/RetailUiRuntime.cs:190-192` (XML doc on + `DisplayMouseTurningMacroLine`): "Magic routes through + `RuntimeCommunicationState.AddText` to the scrolling chat transcript + (**light blue, timestamped, logged**)". +2. The commit message: "routed at Magic (scrolling chat transcript, light + blue, **timestamped**)". +3. `docs/research/2026-08-11-campaign-op-test-script.md`, step 15: "expect + LIGHT BLUE text in the SCROLLING CHAT TRANSCRIPT (not the transient SpewBox + overlay), **with a timestamp prefix**, same as any other Magic-colored + system line." + +**acdream renders neither.** Evidence: + +- `ChatEntry.Received` exists (`src/AcDream.Core/Chat/ChatLog.cs:457`) but has + **zero consumers** — `git grep "\.Received" -- src/` returns only + `NetClient.cs`/`FragmentAssembler.cs` hits on unrelated `ReceivedBytes`/ + `ReceivedCount` members. Nothing formats it into a line. +- `ChatSettings.ShowTimestamps` + (`src/AcDream.UI.Abstractions/Panels/Settings/ChatSettings.cs:46`) is a + stored preference whose only references are its own declaration, its default + (`:107`), the unrendered ImGui-era `SettingsPanel.cs:415-417`, and + `SettingsStore.cs:196/597`. **No renderer reads it.** +- There is no chat-log-file writer at all: `git grep -ni + "chatlog\.txt|WriteChatLog|LogChatToFile" -- src/` returns nothing, and + register row TS-69 records `@log` as unported precisely because "needs a + safely-lifecycled chat-to-file writer (deferred)". + +**Why MUST.** Item 3 corrupts the acceptance instrument this whole fix round +exists to make meaningful. The blast review's own MUST-FIX rationale was that +M1 "changes what the gate is supposed to observe"; a gate step that asks the +user to confirm a prefix acdream cannot draw will produce either a false +failure report or, worse, a follow-up "fix" for a non-bug. Item 1 is the same +class of defect S1 just retired in this very commit: a doc sentence naming an +acdream type and method while describing retail's mechanism. + +**Fix (documentation only, no code):** in step 15 drop "with a timestamp +prefix"; in `RetailUiRuntime.cs:190-192` either drop "timestamped, logged" or +re-attribute it ("retail additionally timestamps and log-files this type; +acdream does neither — see TS-69"). Nothing about the Magic routing itself +changes. + +### R2 — SHOULD-FIX: the tri-state does not implement its own third state + +`src/AcDream.App/Composition/InteractionRetainedUiComposition.cs:814-816`: + +```csharp +IsGrounded: () => !d.PlayerMode.IsPlayerMode + ? (bool?)null + : d.PlayerController.Controller is { IsAirborne: false }, +``` + +Enumerating the states as asked: + +| State | Expression | `RequestExitToCharacterSelection` | Retail | ✓ | +|---|---|---|---|---| +| player mode, controller live, grounded | `true` | `EndCharacterSession()` | proceed | ✅ | +| player mode, controller live, airborne | `false` | "Cannot log off while in mid-air." | refuse | ✅ | +| not player mode | `null` | silent | silent | ✅ | +| **player mode, controller `null`** | **`false`** | **refusal** | **silent** | ❌ | + +The fourth row is the miss. `Controller is { IsAirborne: false }` is `false` +for a null controller, so it lands in `case false:` (`RetailUiRuntime.cs:1904-1906`) +and emits the refusal. + +**Three comments assert the opposite of the code** — the exact inversion S3 +was filed to remove: + +- `InteractionRetainedUiComposition.cs:807-808`: "with no player object (not in + player mode, **or no live controller yet**) neither the refusal nor the + logoff itself ever fires, i.e. a SILENT no-op, not a refusal." +- `RetailUiRuntime.cs:161-163`: "`null` means 'no player' (acdream: + `!d.PlayerMode.IsPlayerMode || d.PlayerController.Controller is null`)". +- `RetailUiRuntime.cs:1908-1911`: "outside player mode (**or with no live + controller**) there is no player object for `UseTime` to test at all". + +Plus the commit message's own "(or with no live controller)". + +**Reachability is narrow but the two owners are explicitly independent.** +`IsPlayerMode` is App-owned and set true only after the controller is in hand +(`PlayerModeController.cs:370`, after `controller.Position` is read at `:362`), +and `Exit()` clears it at `:166`. But `PlayerModeController.cs:167-169` states +outright: "the movement controller is Runtime-owned — player-mode exit +detaches presentation only; the publication lifecycle (generation reset/ +teardown) owns the controller's retirement." Two owners, two clocks. Any +ordering in which Runtime clears the controller before App clears +`IsPlayerMode` reaches row four. + +**Fix (one expression):** + +```csharp +IsGrounded: () => !d.PlayerMode.IsPlayerMode + || d.PlayerController.Controller is null + ? (bool?)null + : d.PlayerController.Controller is { IsAirborne: false }, +``` + +which is literally what the XML doc already says. + +**On the "no dedicated automated test" flag:** for the `switch` itself, +acceptable — `RetailUiRuntime` is composition-driven and a direct unit test +would need a UiRoot plus DAT layouts, and there are zero test constructions of +`OptionsRuntimeBindings` today. But note what the absence cost here: an +untested *expression* silently failed to implement its own documented +contract. If R2 is fixed, the cheapest real pin is a one-line test over the +lambda shape rather than the runtime — or simply leaving it, given the fix +makes expression and doc agree by construction. + +### R3 — SHOULD-FIX: the dormant-`ActivePageChanged` pin applies no stimulus + +`tests/AcDream.App.Tests/UI/Layout/OP2ReworkBlastRadiusConformanceTests.cs:83-93`: + +```csharp +var host = Assert.IsType(layout.FindElement(0x100000B8u)); +int raised = 0; +host.ActivePageChanged += (_, _) => raised++; + +Assert.False(host.BehaviorActive); +Assert.Equal(0, raised); +``` + +Nothing between the subscription and the assertion does anything. The test +would pass identically if the dormancy model were entirely broken, because no +switch is ever attempted. It adds only the subscription over the test directly +above it (`:61-70`), which already asserts `BehaviorActive == false` and +`ActivePageElementId == 0`. + +The parent's framing is right and the answer is the weaker variant shipped. +Worth being precise about *what* stimulus is correct, because "call `SwitchTo` +and assert silence" would legitimately fail: `UiTabPanel.SwitchTo` has no +dormancy guard and raises unconditionally at its tail +(`src/AcDream.App/UI/UiTabPanel.cs`, `ActivePageChanged?.Invoke(...)` as the +last statement). Dormancy lives entirely in the *click bindings*: +`ActivateTabBehavior` is the only caller of +`RetailTabBinding.SetClick(button, () => SwitchTo(pageId))`, and +`RetailTabBinding.SetClick` (`RetailTabBinding.cs:12-23`) sets +`button.OnClick = action`. + +**Fix:** drive the authored tab button on a dormant host — + +```csharp +var button = layout.FindElement(host.Tabs[0].ButtonElementId); +Assert.Null(((UiButton)button!).OnClick); // never wired -> the real mechanism +Assert.Equal(0, raised); +Assert.Equal(0u, host.ActivePageElementId); +``` + +That fails the moment eager wiring returns, which is the regression AD-73 +exists to prevent. + +--- + +## 2. NOTES (no action required) + +- **N1 — `EmptyPage_WithNoAfterApplyWired_Apply_NeverFlushes` + (`OptionPageModelTests.cs:90-101`) asserts only `Assert.Null(thrown)`.** The + name promises "never flushes"; with `AfterApply` null there is nothing to + count, so it is really a null-safety test. Harmless because the real S1 pin + lives in `OptionsPanelControllerTests.cs:143-159` and does count. Rename or + leave. +- **N2 — plan §5 still open-ended.** + `docs/plans/2026-08-10-options-panel-campaign.md:381` still reads "register + row in OP3 if the review deems it a divergence" with no pointer to AD-77. A + future reader re-litigates a settled ruling. One-line edit whenever that file + is next touched. +- **N3 — `BoolOptionRow.AttachPageNotify` is a single-slot assignment** + (`OptionPageModel.cs:107`). Registering one row into two pages silently drops + the first page's notify. This matches retail's single `UIOption::m_pOCH` + pointer, so it is faithful, not a bug — recorded so OP4-6 does not share a + row instance across pages expecting both to hear it. +- **N4 — Options can now only GROW.** With `MinWidth`/`MinHeight` removed and + no `DatConstraintSource` passed, `RetailWindowFrame.ResolveConstraint:187-201` + falls through to the `fallback` argument, which is the mounted outer extent + (`:161-164`) — i.e. min = the authored 300 x 362 plus chrome inset. That is + identical to all nine siblings, so it is the correct outcome, but gate-script + step 5 says only "resizable from the bottom edge only … remembers its + HEIGHT" without noting the window cannot be dragged *shorter* than it opens. + A tester may report that as a defect. One clause would pre-empt it. +- **N5 — `OptionsPanelController.Pages`' doc (`:104-109`) still describes the + four slots uniformly** ("Every entry exists from construction … only + GameplayPageId's stays permanently empty") without mentioning the + `AfterApply` asymmetry. The constructor comment immediately below covers it. + Cosmetic. +- **N6 — test counts unverified.** The commit's "12,947 passed / 4 skipped / + 0 failed" was not re-run; this re-review is read-only by instruction. + +--- + +## 3. Closure table + +| Finding | Claimed | Actually | +|---|---|---| +| M1 (both lenses) | closed, byte-verified | **CLOSED** — independently re-decoded; `6a 07` at all six sites, `arg3` of `ClientSystem::AddTextToScroll @0x00487FC0`. Route traced to the chat scroll, filter passes, no truncation, no dedupe collapse. R1 is a doc/gate overclaim attached to it, not a routing error. | +| S1 | closed | **CLOSED** — Gameplay slot `AfterApply = null`; other three keep the real seam; both tests rewritten to the right shape; model doc corrected. | +| S2 | closed | **CLOSED** — notify at all three verb tails + `SetCurrentValue` only; Defaults ungated; seam usable by OP4-6. | +| S3 / blast SHOULD-FIX 1 | closed | **PARTIAL → R2.** Three of four states right; `IsPlayerMode && Controller == null` still refuses, and three comments plus the commit message say it does not. | +| S4 / blast MUST-FIX 2 | closed | **CLOSED** — AD-77 filed, client-wide, AD-classed, count 56→57, format correct. | +| S5 | closed | **CLOSED** — TS-74 What/Where names all five keys and both stores. | +| S6 | closed | **CLOSED** — all three script additions present and discriminating (but see R1 on step 15). | +| blast SHOULD-FIX 2 (geometry) | closed | **CLOSED** — matches all nine siblings exactly; stale persisted width provably discarded on restore. | +| blast SHOULD-FIX 3 (three pins) | closed | **2 of 3 CLOSED → R3.** Mutual exclusion and queue key genuinely pin; dormant `ActivePageChanged` does not. | +| blast SHOULD-FIX 4 (UA/RA legibility) | closed | **CLOSED** — eye-item added to steps 13/14 with the honest-fix direction named. | +| blast NOTE 8 (close-button logging) | closed | **CLOSED** — `OptionsPanelController.cs:176-179`, symmetric with `BindButton`. | +| New blast radius from the fixes | — | **CLEAN** — one construction site for the new seam, one caller for the changed signature, one `IOptionRow` implementer, no test pins the old geometry. | diff --git a/src/AcDream.Headless/Configuration/HeadlessConfiguration.cs b/src/AcDream.Headless/Configuration/HeadlessConfiguration.cs index cb157d34..5270258c 100644 --- a/src/AcDream.Headless/Configuration/HeadlessConfiguration.cs +++ b/src/AcDream.Headless/Configuration/HeadlessConfiguration.cs @@ -29,7 +29,13 @@ internal sealed class HeadlessContentDescriptor public string PreparedAssetPath { get; init; } = string.Empty; } -internal sealed class HeadlessSessionDescriptor +// MF-1 (Campaign OP OP7 review fix, 2026-08-11): record, not class — the +// direct-CLI launch path (HeadlessProcessHost.WithAccount) needs a `with` +// expression so adding a future property can't silently drop it from a +// hand-copied clone the way `CharacterOptions` was dropped here (the K3 +// direct-credential launch mode reached HeadlessSessionHost with +// CharacterOptions == null, silently no-op'ing the whole OP7 feature). +internal sealed record HeadlessSessionDescriptor { [JsonRequired] public string Id { get; init; } = string.Empty; diff --git a/src/AcDream.Headless/Configuration/HeadlessConfigurationLoader.cs b/src/AcDream.Headless/Configuration/HeadlessConfigurationLoader.cs index 30243b54..101850c8 100644 --- a/src/AcDream.Headless/Configuration/HeadlessConfigurationLoader.cs +++ b/src/AcDream.Headless/Configuration/HeadlessConfigurationLoader.cs @@ -56,6 +56,26 @@ internal static class HeadlessConfigurationLoader CharacterOptionId.SalvageMultiple, ]; + /// + /// SF-1 (Campaign OP OP7 review fix, 2026-08-11): the exact allow-listed + /// SPELLINGS, checked ordinally against the raw JSON key. Validating + /// against Enum.TryParse's parsed VALUE instead let a key that is + /// not an allow-listed spelling at all reach the wire under a different + /// id — Enum.TryParse on a non-[Flags] enum still accepts a + /// decimal numeric string ("15" parses to 0x0F) and a + /// comma-separated list of member names, OR-combined + /// ("ToggleRun,AutoTarget" = 0x0A | 0x0D = 0x0F) — + /// both alias into , + /// which is itself allow-listed, so the wrong id silently passed. Keying + /// off id.ToString() keeps this set in exact lockstep with + /// with no separate literal list to + /// drift out of sync. + /// + private static readonly HashSet AllowedCharacterOptionNames = + new( + AllowedCharacterOptions.Select(static id => id.ToString()), + StringComparer.Ordinal); + private static readonly JsonSerializerOptions Options = new() { AllowTrailingCommas = false, @@ -207,6 +227,11 @@ internal static class HeadlessConfigurationLoader /// every other type-shape violation this loader lets the deserializer /// reject directly — is /// reserved for semantic validation of already-well-typed values). + /// SF-1 (OP7 review fix, 2026-08-11): validates the raw JSON key STRING + /// ordinally against — never + /// Enum.TryParse's parsed value, which accepts numeric strings and + /// comma-combined member lists that are not allow-listed spellings at + /// all (see that field's own doc). /// private static void ValidateCharacterOptions(HeadlessSessionDescriptor session) { @@ -215,8 +240,7 @@ internal static class HeadlessConfigurationLoader foreach (string name in declared.Keys) { - if (!Enum.TryParse(name, ignoreCase: false, out CharacterOptionId id) - || !AllowedCharacterOptions.Contains(id)) + if (!AllowedCharacterOptionNames.Contains(name)) { throw new HeadlessConfigurationException( $"Session '{session.Id}' characterOptions declares " @@ -224,5 +248,29 @@ internal static class HeadlessConfigurationLoader + "option name."); } } + + // SF-2 (OP7 review fix, 2026-08-11): retail's own OnChanged mutual + // exclusion (RuntimeCharacterState.TrySetOption's MF-2 recursive + // clear) makes IgnoreFellowshipRequests and + // FellowshipAutoAcceptRequests both-true unsatisfiable — turning one + // ON always clears the other. A config declaring both true would + // have the seeder re-diff and re-send on every single connect + // forever, with neither declared value ever actually honoured. + // Reject the contradiction at load, before it can reach the wire. + if (declared.TryGetValue( + nameof(CharacterOptionId.IgnoreFellowshipRequests), out bool ignoreFellowship) + && ignoreFellowship + && declared.TryGetValue( + nameof(CharacterOptionId.FellowshipAutoAcceptRequests), out bool autoAcceptFellowship) + && autoAcceptFellowship) + { + throw new HeadlessConfigurationException( + $"Session '{session.Id}' characterOptions declares both " + + $"'{nameof(CharacterOptionId.IgnoreFellowshipRequests)}' and " + + $"'{nameof(CharacterOptionId.FellowshipAutoAcceptRequests)}' " + + "as true; retail's own mutual exclusion makes that " + + "combination unsatisfiable — turning one on always clears " + + "the other."); + } } } diff --git a/src/AcDream.Headless/Hosting/HeadlessProcessHost.cs b/src/AcDream.Headless/Hosting/HeadlessProcessHost.cs index 9cb0dd73..daf02576 100644 --- a/src/AcDream.Headless/Hosting/HeadlessProcessHost.cs +++ b/src/AcDream.Headless/Hosting/HeadlessProcessHost.cs @@ -143,18 +143,15 @@ internal sealed class HeadlessProcessHost : IDisposable internal HeadlessProcessContentSnapshot? Content => _content?.CaptureSnapshot(); + // MF-1 (Campaign OP OP7 review fix, 2026-08-11): `with` copies every + // record property that this method doesn't explicitly override, so a + // future HeadlessSessionDescriptor property can never be silently + // dropped here the way CharacterOptions previously was by the + // hand-rolled six-of-seven-property object initializer. private static HeadlessSessionDescriptor WithAccount( HeadlessSessionDescriptor source, string account) => - new() - { - Id = source.Id, - Endpoint = source.Endpoint, - Account = account, - Character = source.Character, - Policy = source.Policy, - Credential = source.Credential, - }; + source with { Account = account }; internal Task RunAsync( CancellationToken cancellationToken) diff --git a/src/AcDream.Runtime/Session/LiveSessionEventRouter.cs b/src/AcDream.Runtime/Session/LiveSessionEventRouter.cs index c7af2a14..fba2724f 100644 --- a/src/AcDream.Runtime/Session/LiveSessionEventRouter.cs +++ b/src/AcDream.Runtime/Session/LiveSessionEventRouter.cs @@ -209,8 +209,19 @@ public sealed class LiveSessionEventRouter : ILiveSessionEventRouting onDesiredComponents: null, onCharacterOptions: (options1, options2, trailerTruncated) => { + // MF-2 (Campaign OP OP7 review fix, 2026-08-11): a + // trailer-truncated PlayerDescription's option words are + // the parser's zero placeholders, not server truth (R3, + // above). Installing them AND notifying subscribers let a + // headless seeder diff against zero and flush it into + // 0x01A1 even though HasServerSeed stayed armed from an + // earlier complete seed. Root fix: on truncation, install + // NOTHING and notify NO ONE — the words the caller last + // had (real, seeded) remain current. + if (trailerTruncated) + return; character.Character.Options.Replace( - options1, options2, armServerSeed: !trailerTruncated); + options1, options2, armServerSeed: true); character.OnCharacterOptionsChanged?.Invoke(options1, options2); }, clientTime: character.ClientTime, diff --git a/src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs b/src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs index 30806477..e8064b6a 100644 --- a/src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs +++ b/src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs @@ -875,11 +875,18 @@ public sealed class RuntimeLiveEntitySessionController RuntimeWorldHostAcknowledgementStage.TerminalProjected); _session.SendGameAction(GameActionLoginComplete.Build()); - _onLoginCompleteSent?.Invoke(); transit.EndTeleport(); _log( $"headless: portal complete generation={generation} " + $"cell=0x{destination.CellId:X8}"); + // SF-3 (Campaign OP OP7 review fix, 2026-08-11): invoke AFTER the + // teleport-completion tail, matching the other production call + // site's shape (OnSpawned, above). The observer body is not + // trivial — it runs the full diff, real SendGameActions, and + // event-hub publication to bot policies — so a throw from it must + // not abort transit.EndTeleport() with the retry token already + // discarded. + _onLoginCompleteSent?.Invoke(); } /// diff --git a/tests/AcDream.Headless.Tests/HeadlessConfigurationLoaderTests.cs b/tests/AcDream.Headless.Tests/HeadlessConfigurationLoaderTests.cs index 774d19df..56d16c33 100644 --- a/tests/AcDream.Headless.Tests/HeadlessConfigurationLoaderTests.cs +++ b/tests/AcDream.Headless.Tests/HeadlessConfigurationLoaderTests.cs @@ -89,6 +89,82 @@ public sealed class HeadlessConfigurationLoaderTests StringComparison.Ordinal); } + [Fact] + public void NumericKeyFailsLoadInsteadOfAliasingIntoAnAllowedId() + { + // SF-1 (OP7 review fix, 2026-08-11): Enum.TryParse on a non-[Flags] + // enum accepts a decimal numeric string — "15" parsed to 0x0F + // (FellowshipShareXP), which IS allow-listed, so the old + // parsed-value check let a key that is not an enum-member spelling + // at all silently pass. Validation must reject the STRING. + using TemporaryConfiguration file = TemporaryConfiguration.Create( + ConfigurationWith(Session( + "bot", + "BOT_PASSWORD", + "\"characterOptions\":{\"15\":true}"))); + + HeadlessConfigurationException exception = Assert.Throws< + HeadlessConfigurationException>( + () => HeadlessConfigurationLoader.Load(file.Path)); + + Assert.Contains("15", exception.Message, StringComparison.Ordinal); + } + + [Fact] + public void CommaCombinedKeyFailsLoadInsteadOfOrCombiningIntoAnAllowedId() + { + // SF-1 companion case: Enum.TryParse OR-combines a comma-separated + // member list — "ToggleRun,AutoTarget" (0x0A | 0x0D) parsed to + // 0x0F (FellowshipShareXP), again allow-listed, so a config that + // reads as two movement options would have silently set fellowship + // XP sharing instead. + using TemporaryConfiguration file = TemporaryConfiguration.Create( + ConfigurationWith(Session( + "bot", + "BOT_PASSWORD", + "\"characterOptions\":{\"ToggleRun,AutoTarget\":true}"))); + + HeadlessConfigurationException exception = Assert.Throws< + HeadlessConfigurationException>( + () => HeadlessConfigurationLoader.Load(file.Path)); + + Assert.Contains( + "ToggleRun,AutoTarget", + exception.Message, + StringComparison.Ordinal); + } + + [Fact] + public void BothFellowshipExclusionOptionsTrueFailsLoadNamingBothKeys() + { + // SF-2 (OP7 review fix, 2026-08-11): retail's own OnChanged mutual + // exclusion means IgnoreFellowshipRequests and + // FellowshipAutoAcceptRequests can never both be true at once — + // turning one on always clears the other. A config declaring both + // true would have the seeder oscillate (re-send) on every connect + // forever with neither value ever actually honoured. Reject it at + // load instead. + using TemporaryConfiguration file = TemporaryConfiguration.Create( + ConfigurationWith(Session( + "bot", + "BOT_PASSWORD", + "\"characterOptions\":{\"IgnoreFellowshipRequests\":true," + + "\"FellowshipAutoAcceptRequests\":true}"))); + + HeadlessConfigurationException exception = Assert.Throws< + HeadlessConfigurationException>( + () => HeadlessConfigurationLoader.Load(file.Path)); + + Assert.Contains( + "IgnoreFellowshipRequests", + exception.Message, + StringComparison.Ordinal); + Assert.Contains( + "FellowshipAutoAcceptRequests", + exception.Message, + StringComparison.Ordinal); + } + [Fact] public void NonBoolValueFailsLoad() { diff --git a/tests/AcDream.Headless.Tests/HeadlessSessionHostTests.cs b/tests/AcDream.Headless.Tests/HeadlessSessionHostTests.cs index c823e0cc..f2016f17 100644 --- a/tests/AcDream.Headless.Tests/HeadlessSessionHostTests.cs +++ b/tests/AcDream.Headless.Tests/HeadlessSessionHostTests.cs @@ -133,6 +133,140 @@ public sealed class HeadlessSessionHostTests diagnostics.ToString()); } + [Fact] + public async Task DirectCredentialsPreserveDeclaredCharacterOptions() + { + // MF-1 (Campaign OP OP7 review fix, 2026-08-11): the K3 direct-CLI + // launch mode (--user/--password) rebuilds the configured session + // descriptor via HeadlessProcessHost.WithAccount before constructing + // HeadlessSessionHost. A hand-copied clone that forgets a property + // silently drops that feature for this launch mode with no error — + // exactly what happened to CharacterOptions. Assert the declared + // block survives the direct-credential path by checking the + // constructed session's seeder actually saw it. + var configuration = new HeadlessConfiguration + { + Version = 1, + Sessions = + [ + Descriptor(characterOptions: new Dictionary + { + ["AutoRepeatAttack"] = true, + }), + ], + }; + HeadlessPathSet paths = HeadlessPathSet.Resolve( + new HeadlessPathOverrides()); + using var diagnostics = new StringWriter(); + var operations = new FixtureSessionOperations(); + using var host = new HeadlessProcessHost( + configuration, + paths, + TextReader.Null, + diagnostics, + operations, + directCredentials: new HeadlessDirectCredentials( + "direct-account", + "direct-secret")); + using var cancellation = new CancellationTokenSource(); + cancellation.Cancel(); + + HeadlessExitCode result = + await host.RunAsync(cancellation.Token); + + Assert.Equal(HeadlessExitCode.Success, result); + Assert.Equal("direct-account", operations.LastUser); + Assert.NotNull(host.Session.OptionsSeeder); + Assert.True(host.Session.OptionsSeeder!.HasDeclaredOptions); + } + + [Fact] + public void DirectFirstEntryCompletion_InvokesOnLoginCompleteSentHookThroughProductionWiring() + { + // SF-4 (Campaign OP OP7 review fix, 2026-08-11): the THIRD + // production LoginComplete->seeder hook lives entirely inside + // HeadlessSessionHost.CreateEventRoute's own closure (the direct, + // non-portal first-entry completion callback wired to + // RuntimeFirstEntryDriveController via HeadlessSessionEventRoute's + // localPlayerCompleted parameter) — driving it end-to-end would + // need a real DAT-backed collision fixture this test project does + // not have. Reflection reaches the SAME closure instance + // HeadlessSessionHost actually constructed during Start() (not a + // hand-rolled reconstruction of it) and invokes it exactly as + // RuntimeFirstEntryDriveController would on residence completion, + // then proves the seeder reacted through the real wire — the same + // "declared option actually sends" proof + // HeadlessCharacterOptionsSeederWiringTests uses for the other two + // sites. + var operations = new FixtureSessionOperations(); + using var credential = new HeadlessCredentialSecret( + "fixture", + "password"); + using var host = new HeadlessSessionHost( + Descriptor(characterOptions: new Dictionary + { + ["IgnoreAllegianceRequests"] = true, + }), + credential, + new HeadlessDiagnosticWriter(TextWriter.Null), + operations); + Assert.Equal( + RuntimeSessionStartStatus.Connected, + host.Start().Status); + Assert.NotNull(host.OptionsSeeder); + + WorldSession session = operations.Sessions[^1]; + var sent = new List(); + session.GameActionCapture = body => sent.Add(body); + + object eventRoute = typeof(HeadlessSessionHost) + .GetField( + "_eventRoute", + BindingFlags.NonPublic | BindingFlags.Instance)! + .GetValue(host) + ?? throw new InvalidOperationException( + "HeadlessSessionHost constructed no event route."); + var localPlayerCompleted = + (Action?)typeof(HeadlessSessionEventRoute) + .GetField( + "_localPlayerCompleted", + BindingFlags.NonPublic | BindingFlags.Instance)! + .GetValue(eventRoute); + Assert.NotNull(localPlayerCompleted); + + const uint playerGuid = 0x50000009u; + host.Runtime.PlayerIdentity.ServerGuid = playerGuid; + RuntimeEntityRecord record = host.Runtime.EntityObjects + .RegisterEntity(Spawn(playerGuid)) + .Canonical!; + + // Invoke the PRODUCTION closure directly — proves + // HeadlessSessionHost really wires _optionsSeeder?.NoteLoginCompleteSent() + // into this callback, not merely that some test double does. + localPlayerCompleted!(record); + + Assert.Contains( + sent, + body => body.SequenceEqual(GameActionLoginComplete.Build())); + + // No PlayerDescription has landed yet — HasServerSeed is still + // false, so nothing beyond LoginComplete can have sent. + Assert.DoesNotContain( + sent, + body => ActionOpcode(body) + == SocialActions.SetSingleCharacterOptionOpcode); + + session.GameEvents.Dispatch( + GameEventEnvelope.TryParse( + WrapPlayerDescriptionEnvelope(options1: 0u, options2: 0u))! + .Value); + + Assert.Contains( + sent, + body => ActionOpcode(body) + == SocialActions.SetSingleCharacterOptionOpcode); + } + [Fact] public void DirectFrameUsesSharedRetailOrderAndMovementCadence() { @@ -1832,7 +1966,8 @@ public sealed class HeadlessSessionHostTests private static HeadlessSessionDescriptor Descriptor( HeadlessCredentialProviderKind provider = HeadlessCredentialProviderKind.Environment, - string credentialReference = "BOT_PASSWORD") => new() + string credentialReference = "BOT_PASSWORD", + Dictionary? characterOptions = null) => new() { Id = "bot", Endpoint = new HeadlessEndpointDescriptor @@ -1854,6 +1989,7 @@ public sealed class HeadlessSessionHostTests Provider = provider, Reference = credentialReference, }, + CharacterOptions = characterOptions, }; private static void HydrateGroundedPlayer(GameRuntime runtime) @@ -2458,6 +2594,43 @@ public sealed class HeadlessSessionHostTests BinaryPrimitives.ReadUInt32LittleEndian( body.AsSpan(8, sizeof(uint))); + // SF-4 fixture: minimal PlayerDescription (0x0013) body carrying only + // the CharacterOptions1/2 trailer fields — copied from + // HeadlessCharacterOptionsSeederWiringTests.WrapPlayerDescriptionEnvelope + // (mirrors GameEventWiringTests.WireAll_PlayerDescription_PublishesCharacterOptions's + // fixture layout). + private static byte[] WrapPlayerDescriptionEnvelope( + uint options1, + uint options2) + { + var stream = new MemoryStream(); + using (var writer = new BinaryWriter( + stream, System.Text.Encoding.UTF8, leaveOpen: true)) + { + writer.Write(0u); // property flags + writer.Write(0x52u); // player weenie type + writer.Write(0u); // vector flags + writer.Write(0u); // has health + writer.Write(0x40u); // option flags: CharacterOptions2 + writer.Write(options1); + writer.Write(0u); // legacy hotbar count + writer.Write(0u); // spellbook filters + writer.Write(options2); + writer.Write(0u); // inventory count + writer.Write(0u); // equipped count + } + + byte[] payload = stream.ToArray(); + byte[] body = new byte[GameEventEnvelope.HeaderSize + payload.Length]; + BinaryPrimitives.WriteUInt32LittleEndian(body, GameEventEnvelope.Opcode); + BinaryPrimitives.WriteUInt32LittleEndian(body.AsSpan(4), 0u); + BinaryPrimitives.WriteUInt32LittleEndian(body.AsSpan(8), 0u); + BinaryPrimitives.WriteUInt32LittleEndian( + body.AsSpan(12), (uint)GameEventType.PlayerDescription); + Array.Copy(payload, 0, body, GameEventEnvelope.HeaderSize, payload.Length); + return body; + } + private sealed class FixtureSessionOperations : ILiveSessionOperations { public List Sessions { get; } = []; diff --git a/tests/AcDream.Runtime.Tests/Session/LiveSessionEventRouterTests.cs b/tests/AcDream.Runtime.Tests/Session/LiveSessionEventRouterTests.cs index 6eccabc8..5f5b4693 100644 --- a/tests/AcDream.Runtime.Tests/Session/LiveSessionEventRouterTests.cs +++ b/tests/AcDream.Runtime.Tests/Session/LiveSessionEventRouterTests.cs @@ -290,6 +290,107 @@ public sealed class LiveSessionEventRouterTests router.Dispose(); } + [Fact] + public void PlayerDescription_TrailerTruncatedReSeed_LeavesWordsAndLatchUnchangedAndDoesNotNotify() + { + // MF-2 (Campaign OP OP7 review fix, 2026-08-11): a re-seed whose + // trailer truncates mid-parse must install NOTHING and notify NO + // ONE — the placeholder zero words are not server truth, and the + // prior good seed (real words, HasServerSeed already armed) must + // survive untouched. Previously the router installed the zeroed + // options2 AND fired OnCharacterOptionsChanged unconditionally, + // letting a downstream seeder diff against zero and flush it. + using var session = NewSession(); + var character = new RuntimeCharacterState(); + var observed = new List<(uint Options1, uint Options2)>(); + + var router = new LiveSessionEventRouter( + session, + NoOpEntitySink(), + NoOpEnvironmentSink(), + NewInventoryBindings(), + new LiveCharacterSessionBindings( + new CombatState(), + character, + ResolveSkillFormulaBonus: null, + OnSkillsUpdated: null, + OnConfirmationRequest: null, + OnConfirmationDone: null, + ClientTime: () => 0d, + OnCharacterOptionsChanged: (options1, options2) => + observed.Add((options1, options2))), + NewSocialBindings()); + router.Attach(); + + // Complete seed first — real words, latch arms. + session.GameEvents.Dispatch( + GameEventEnvelope.TryParse( + WrapPlayerDescriptionEnvelope(0x50C4A54Au, 0x00948700u))!.Value); + Assert.Single(observed); + Assert.True(character.Options.HasServerSeed); + Assert.Equal(0x50C4A54Au, character.Options.Options1); + Assert.Equal(0x00948700u, character.Options.Options2); + + // Truncated re-seed — options1 reads early (real-looking value), + // the trailer then throws before options2 is ever read. + session.GameEvents.Dispatch( + GameEventEnvelope.TryParse( + WrapTruncatedPlayerDescriptionEnvelope(0xDEADBEEFu))!.Value); + + // No second notification, words untouched, latch still armed + // (from the earlier GOOD seed, not from this truncated one). + Assert.Single(observed); + Assert.True(character.Options.HasServerSeed); + Assert.Equal(0x50C4A54Au, character.Options.Options1); + Assert.Equal(0x00948700u, character.Options.Options2); + + router.Dispose(); + } + + [Fact] + public void PlayerDescription_TrailerTruncatedFirstSeed_LeavesDefaultsAndNeverArmsLatch() + { + // MF-2 companion case: a truncated PlayerDescription that is the + // FIRST one a session ever sees must leave the client-constructor + // defaults in place and never arm HasServerSeed — a later flush + // stays refused exactly as if no PlayerDescription had arrived. + using var session = NewSession(); + var character = new RuntimeCharacterState(); + var observed = new List<(uint Options1, uint Options2)>(); + + var router = new LiveSessionEventRouter( + session, + NoOpEntitySink(), + NoOpEnvironmentSink(), + NewInventoryBindings(), + new LiveCharacterSessionBindings( + new CombatState(), + character, + ResolveSkillFormulaBonus: null, + OnSkillsUpdated: null, + OnConfirmationRequest: null, + OnConfirmationDone: null, + ClientTime: () => 0d, + OnCharacterOptionsChanged: (options1, options2) => + observed.Add((options1, options2))), + NewSocialBindings()); + router.Attach(); + + uint defaultOptions1 = character.Options.Options1; + uint defaultOptions2 = character.Options.Options2; + + session.GameEvents.Dispatch( + GameEventEnvelope.TryParse( + WrapTruncatedPlayerDescriptionEnvelope(0xDEADBEEFu))!.Value); + + Assert.Empty(observed); + Assert.False(character.Options.HasServerSeed); + Assert.Equal(defaultOptions1, character.Options.Options1); + Assert.Equal(defaultOptions2, character.Options.Options2); + + router.Dispose(); + } + [Fact] public void NestedRouters_DisposeOlderFirstLeavesOnlyNewerRouter() { @@ -705,6 +806,37 @@ public sealed class LiveSessionEventRouterTests return body; } + // MF-2 fixture: a PlayerDescription whose trailer reads options1 (the + // early field) then throws before ever reaching options2 — mirrors + // PlayerDescriptionParserTests' truncated-shortcut-list fixture. An + // unreasonable declared shortcut count trips the parser's own + // FormatException guard immediately after options1 is read, so + // TrailerTruncated comes back true with a real-looking options1 and a + // never-populated (zero) options2 — exactly the shape MF-2 closes. + private static byte[] WrapTruncatedPlayerDescriptionEnvelope(uint options1) + { + var stream = new MemoryStream(); + using (var writer = new BinaryWriter(stream, System.Text.Encoding.UTF8, leaveOpen: true)) + { + writer.Write(0u); // property flags + writer.Write(0x52u); // player weenie type + writer.Write(0u); // vector flags + writer.Write(0u); // has health + writer.Write(0x01u); // option flags: Shortcut + writer.Write(options1); + writer.Write(1_000_000u); // claimed shortcut count — trips the >10_000 guard + } + + byte[] payload = stream.ToArray(); + byte[] body = new byte[GameEventEnvelope.HeaderSize + payload.Length]; + BinaryPrimitives.WriteUInt32LittleEndian(body, GameEventEnvelope.Opcode); + BinaryPrimitives.WriteUInt32LittleEndian(body.AsSpan(4), 0u); + BinaryPrimitives.WriteUInt32LittleEndian(body.AsSpan(8), 0u); + BinaryPrimitives.WriteUInt32LittleEndian(body.AsSpan(12), (uint)GameEventType.PlayerDescription); + Array.Copy(payload, 0, body, GameEventEnvelope.HeaderSize, payload.Length); + return body; + } + private static WorldSession NewSession() => new(new IPEndPoint(IPAddress.Loopback, 9)); diff --git a/tests/AcDream.Runtime.Tests/Session/RuntimeLiveEntitySessionControllerTests.cs b/tests/AcDream.Runtime.Tests/Session/RuntimeLiveEntitySessionControllerTests.cs index 0ee3bd0e..9f80fe45 100644 --- a/tests/AcDream.Runtime.Tests/Session/RuntimeLiveEntitySessionControllerTests.cs +++ b/tests/AcDream.Runtime.Tests/Session/RuntimeLiveEntitySessionControllerTests.cs @@ -194,6 +194,105 @@ public sealed class RuntimeLiveEntitySessionControllerTests Assert.Equal(0x01020001u, runtime.Portal.Snapshot.DestinationCell); } + [Fact] + public void DirectSinkContentLessSpawn_InvokesOnLoginCompleteSentHookExactlyOnce() + { + // SF-4 (Campaign OP OP7 review fix, 2026-08-11): the pre-existing + // wiring tests drove the login-complete half only through + // HeadlessSessionHost.OptionsSeeder's TEST seam, never through this + // controller's own production onLoginCompleteSent call sites — a + // future edit that dropped or misdirected the OnSpawned + // content-less immediate-admission invoke (:199-201) would have + // been invisible to the whole suite. Drives it with a counting + // Action, matching the fix's "the seeder's TryRun latch sees the + // LoginComplete half via the production path" requirement. + using StartedRuntime started = StartRuntime(); + GameRuntime runtime = started.Runtime; + const uint playerGuid = 0x50000004u; + runtime.PlayerIdentity.ServerGuid = playerGuid; + using var session = new WorldSession( + new IPEndPoint(IPAddress.Loopback, 9000), + new FixtureTransport()); + session.GameActionCapture = _ => { }; + int hookInvocations = 0; + var controller = new RuntimeLiveEntitySessionController( + runtime, + session, + onLoginCompleteSent: () => hookInvocations++); + LiveEntitySessionSink sink = controller.CreateSink(); + WorldSession.EntitySpawn spawn = + Spawn(playerGuid, incarnation: 1); + + sink.Spawned(spawn); + Assert.Equal(1, hookInvocations); + + // A repeat Create at the same incarnation must not re-fire the + // hook — mirrors the pre-existing gameActions.Single() assertion + // for the underlying LoginComplete send itself. + sink.Spawned(spawn); + Assert.Equal(1, hookInvocations); + } + + [Fact] + public void DirectSinkPortalCompletion_InvokesOnLoginCompleteSentHookAfterTeleportEnds() + { + // SF-4 companion: covers TryAdvancePortalCompletion's own + // onLoginCompleteSent invoke (the third production hook site lives + // one level up in HeadlessSessionHost.CreateEventRoute's direct + // first-entry callback, exercised at the host level). This also + // doubles as the SF-3 ordering regression test: the hook body reads + // TransitOwner state from INSIDE the callback, so if the invoke + // ever regressed back to firing between the LoginComplete send and + // transit.EndTeleport(), IsSessionIdle would observe false here. + using StartedRuntime started = StartRuntime(); + GameRuntime runtime = started.Runtime; + const uint playerGuid = 0x50000005u; + runtime.PlayerIdentity.ServerGuid = playerGuid; + using var session = new WorldSession( + new IPEndPoint(IPAddress.Loopback, 9000), + new FixtureTransport()); + session.GameActionCapture = _ => { }; + int hookInvocations = 0; + bool? sessionIdleAtSecondHookInvocation = null; + var controller = new RuntimeLiveEntitySessionController( + runtime, + session, + onLoginCompleteSent: () => + { + hookInvocations++; + if (hookInvocations == 2) + { + sessionIdleAtSecondHookInvocation = + runtime.TransitOwner.CaptureOwnership().IsSessionIdle; + } + }); + LiveEntitySessionSink sink = controller.CreateSink(); + WorldSession.EntitySpawn spawn = + Spawn(playerGuid, incarnation: 1); + sink.Spawned(spawn); + Assert.Equal(1, hookInvocations); + + sink.TeleportStarted(1u); + sink.PositionUpdated(new WorldSession.EntityPositionUpdate( + playerGuid, + spawn.Position!.Value with + { + LandblockId = 0x01020001u, + PositionX = 30f, + }, + Velocity: null, + PlacementId: null, + IsGrounded: true, + InstanceSequence: 1, + PositionSequence: 2, + TeleportSequence: 1, + ForcePositionSequence: 0)); + + Assert.Equal(2, hookInvocations); + Assert.True(runtime.Portal.Snapshot.Completed); + Assert.True(sessionIdleAtSecondHookInvocation); + } + [Fact] public void DirectSinkProjectsAcceptedLocalWorldStateThroughOneHostSeam() {