fix(headless,runtime): OP7 review fixes + docs: OP3 re-review REOPEN (narrow)
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 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
386076af0f
commit
7b60e71b85
11 changed files with 1007 additions and 23 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
430
docs/research/2026-08-11-op3-rereview.md
Normal file
430
docs/research/2026-08-11-op3-rereview.md
Normal file
|
|
@ -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<bool>` → `Func<bool?>`:** 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<UiTabPanel>(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. |
|
||||
Loading…
Add table
Add a link
Reference in a new issue