From eda8729c9285c0132b05febe79bc35f41389673a Mon Sep 17 00:00:00 2001 From: Erik Date: Wed, 12 Aug 2026 04:57:11 +0200 Subject: [PATCH] docs: FA4 blast-radius review -- APPROVE-WITH-FIXES (1 SHOULD-FIX) All nine blast axes verified clean at the code level. Single fix: the AD-78 register row still reads "35 of 50 rows dimmed" after FA4's D7 flipped four rows to Live (now 31 of 50) -- the class doc and conformance test were updated, the binding register row was not. Plus one minor non-blocking observation on GetMembers' per-vitals-tick allocation profile. Co-Authored-By: Claude Fable 5 --- docs/research/2026-08-12-fa4-review-blast.md | 307 +++++++++++++++++++ 1 file changed, 307 insertions(+) create mode 100644 docs/research/2026-08-12-fa4-review-blast.md diff --git a/docs/research/2026-08-12-fa4-review-blast.md b/docs/research/2026-08-12-fa4-review-blast.md new file mode 100644 index 00000000..0b697240 --- /dev/null +++ b/docs/research/2026-08-12-fa4-review-blast.md @@ -0,0 +1,307 @@ +# Campaign FA slice FA4 — BLAST-RADIUS review + +**Reviewer lens:** the axes the implementer may not have traversed — +enum leaks, existing-consumer regressions, interface-implementer +coverage, generation-seam consistency, confirmation-ordering, dimming +bookkeeping, suite accounting, register/ledger/gate-script/digest +consistency, and the FA2/FA3 live paths' survival. + +**Target commits (branch `claude/latest-commits-cb0c8f`):** +- `357d2032` feat(runtime): fellowship roster enumeration + `Social` selection source +- `5bdd0528` feat(ui): fellowship page fully live +- `38f08314` docs(fa4): register rows AD-80/AD-81, AD-78 addendum, gate script, ledger + +**Verdict: APPROVE-WITH-FIXES (1 SHOULD-FIX, 1 minor observation).** +Every code-level blast axis verified clean. The single fix is a stale +count in a *binding* doc (the divergence register). No MUST-FIX, no +REJECT. + +--- + +## VERDICT DETAIL + +### SHOULD-FIX 1 — the AD-78 register row still says "35 of 50 rows dimmed" (stale post-D7) + +`docs/architecture/retail-divergence-register.md`, the AD-78 row's +file-path column, still reads verbatim: + +> `CharacterOptionsPageController.cs` (35 of 50 rows dimmed — `RowSpec.StoreOnly`, …) + +FA4 D7 flipped four rows (`IgnoreFellowshipRequests`, +`FellowshipAutoAcceptRequests`, `FellowshipShareXP`, +`FellowshipShareLoot`) from `StoreOnly` to `Live`, so the true count is +now **31 of 50 dimmed / 19 live**. The code +(`CharacterOptionsPageController.Groups`), the class-doc derivation table +(`CharacterOptionsPageController.cs` doc comment, updated in `5bdd0528`), +and the conformance test (`CharacterOptionsPageControllerTests`, now +asserts `31`/`19`) were all updated. **The register row was not** — it +carries no D7/FA4 addendum and no updated count (confirmed: the only +`of 50` token in the AD-78 row is `35 of 50`). + +Why this matters and isn't a nit: the register is CLAUDE.md's "single +auditable list of every KNOWN place acdream's runtime behavior can +deviate from retail," and its discipline requires the commit that +*changes* a deviation to update its row in the same commit. AD-78's own +"Risk" column explicitly warns that dim/live classification drift "in the +OPPOSITE direction it was built to prevent" is "a real defect, not a +rendering nit." A wrong count *inside the register row that governs the +convention* is exactly that drift, internal to the register itself. The +`38f08314` commit message asserts "AD-78's derivation table gains its D7 +addendum," but that addendum landed in the class doc (`5bdd0528`), not in +the register row. + +**Fix:** update the AD-78 register row's `CharacterOptionsPageController.cs` +parenthetical to "31 of 50 rows dimmed" (and, optionally, a one-clause D7 +note naming the four un-dimmed ids), mirroring the class-doc change. + +### Observation A (minor, non-blocking) — `GetMembers` is the first allocating member on `IRuntimeFellowshipView`, and the controller re-materializes it per vitals tick + +`RuntimeFellowshipState.FellowshipView.GetMembers()` materializes a fresh +`RuntimeFellowMemberSnapshot[]` under the gate on every call — correct and +race-safe, and the interface doc is honest that it is "a materialized +array (not a lazy iterator)." But it is the first *allocating* member on +that view: `Snapshot` is a value-returned record struct and +`TryGetMember` is an `out`-param, both 0-alloc. On the consumer side, +`SocialFellowshipPageController.RefreshRoster` additionally wraps the +result in `new List<>(_bindings.Members())` (`SocialFellowshipPageController.cs:549`) +and runs on **every revision bump** — and the controller's own doc +(lines 19-36) states the revision bumps on every `0x02C0` vitals refresh. +So while the fellowship panel is open with N members, each incoming +vitals tick costs two bounded (N ≤ 9) allocations on the UI thread. + +This is off the physics/render hot path, panel-gated, and bounded — it +does not threaten the Slice-I/J frame budgets, so it does not block +approval. Noting it because (a) lens 3 asked for the allocation profile +explicitly, and (b) the per-tick `List` copy is avoidable: the diff at +`RefreshRoster` only needs a count + guid-membership comparison, which +can walk the `IEnumerable` directly without materializing a `List` (the +array from `GetMembers` is already a fresh snapshot). A future tidy, not +a fix this slice owes. + +--- + +## VERIFIED CLEAN, per lens + +### Lens 1 — `SelectionChangeSource.Social` enum leak: CLEAN + +- No exhaustive `switch`/pattern-match on `SelectionChangeSource` exists + anywhere in `src/` (grep for `case SelectionChangeSource.` / + `switch … SelectionChangeSource` / `is SelectionChangeSource.` → + zero matches). Adding a value cannot silently mis-route. +- Every `SelectionTransition` consumer either **discards** the transition + (`RuntimeActionState`, `ExternalContainerController`, `PaperdollController`, + `InventoryController`, `SpellbookWindowController`, `ToolbarController`, + `SpellcastingUiController` take `SelectionTransition _`; `SelectedObjectController` + and `VendorUiController` do `_ = transition;`) or reads only + `SelectedObjectId`/`Reason`, never `Source`: + - `RuntimeCombatTargetState.OnSelectionChanged` reads + `SelectedObjectId`/`Reason` only — a `Social`-sourced selection + cannot mis-target combat. + - `AppraisalUiController.HandleSelectionChanged` reacts to any non-null + `SelectedObjectId` (window-visible-gated) — a fellow-row click + behaves like any other world selection, which is the intended + "clicking a fellow selects them in the world" mechanism (`SelectFellow` + → `Select(guid, Social)`). +- Plugin-facing `SelectionChangedEvent` carries no `Source`, so `Social` + is invisible to plugins. No mis-targeting surface. + +### Lens 2 — `UiTemplateListBox.FlushPreservingScroll` vs `Flush` + existing consumers: CLEAN + +- **Additive.** `Flush()`'s body is unchanged (`_viewport?.ClearContent()`); + only its doc comment expanded. `FlushPreservingScroll` is new. +- **Only one caller.** `FlushPreservingScroll` is called solely by + `SocialFellowshipPageController.RebuildRoster`. The Options/Keyboard/Chat + controllers never call `Flush` *or* `FlushPreservingScroll` at all — + their ListBoxes are built once (`AddItemFromTemplateList`/`AddPrebuiltRow`) + and never torn down — so the new method has zero effect on them. Only + Friends/Squelch/Fellowship touch these methods. +- **Shrink-below-offset math is correct.** `FlushPreservingScroll` saves + `Scroll.ScrollY`, calls `ClearContent()` (which zeroes the panel's + `ContentHeight` and `Scroll.SetScrollY(0)` but leaves `Scroll.ContentHeight`/ + `ViewHeight` at their stale values — those are only refreshed in + `LayoutScrollableChildren`), then re-`SetScrollY(saved)` — clamped + against the still-stale extents, so `saved` (already ≤ old `MaxScroll`) + is preserved exactly. On the next `OnDraw`, `LayoutScrollableChildren` + runs *before* `base.OnDraw` and sets `Scroll.ContentHeight = ContentHeight` + (the fresh, possibly-smaller height) then `SetScrollY(ScrollY)`, which + re-clamps to `[0, max(0, freshHeight - view)]`. So a roster that + shrinks below the old offset is clamped to the valid range *before any + row is painted* — the "clamp now, correct at next layout" contract the + doc claims, verified against `UiScrollable.SetScrollY`/`MaxScroll` and + `UiScrollablePanel.LayoutScrollableChildren`. +- **#371 clip/cull untouched.** The straddling-row intersection cull lives + in `LayoutScrollableChildren`; `FlushPreservingScroll` changes only the + offset, not the cull. + +### Lens 3 — `IRuntimeFellowshipView.GetMembers()` implementer/double coverage: CLEAN (see Observation A for allocation) + +- **Single concrete implementer:** `RuntimeFellowshipState`'s nested + `FellowshipView` (`: IRuntimeFellowshipView`, the only `:`-implements in + the tree). It returns a materialized array — never `null`, an **empty + array** when not in a fellowship (test `GetMembers_EmptyWhenNotInAFellowship` + pins this). Build-green proves no other implementer went stale. +- **Test doubles are property-return null, not interface impls:** the two + doubles (`GameplayInputCommandControllerTests`, + `InteractionUiRuntimeSourcesTests`) expose `IRuntimeFellowshipView Fellowship` + as `=> throw`/`=> null!` on the *host* interface — they do not implement + `IRuntimeFellowshipView`, so the new member cannot break them, and the + suite passing shows neither is exercised through `GetMembers`. +- **Production wiring never yields null:** `InteractionRetainedUiComposition` + binds `() => d.Runtime.Fellowship.GetMembers()`, and `GameRuntime.Fellowship` + → `FellowshipOwner.View` is the concrete impl. No headless/bot view + returns null here. + +### Lens 4 — `DeferredGameRuntimeStateCommands.Fellowship*` seam + adapter consistency: CLEAN + +- All seven wrappers route through `Invoke((commands, generation) => commands.Fellowship.X(generation, …))`. + `Invoke` captures `_commands` and `_view.Generation` together under + `_gate` and returns `RuntimeCommandStatus.Inactive` when deactivated or + unbound — the identical generation-race-safe shape as `AddShortcut`/ + `Advance`. A displaced session (reconnect mid-click) cannot receive a + stale-generation fellowship action. +- **No auto-fire.** Commands fire only from explicit button `OnClick` + handlers and `SetPageVisible` transitions. `SetPanelOpen` (`0x00A6`) is + caller-idempotent (only on an actual `_pageVisible` flip) and a no-op + while disconnected (returns `Inactive`). +- **Adapter consistency:** the underlying `IRuntimeFellowshipCommands` + (all seven methods, incl. `SetOpen`/`SetPanelOpen`) predates FA4 — it + was created at FA2 (`369729f0`) and is unchanged in these commits. Both + host adapters (`DirectGameRuntimeCommandAdapter`, + `CurrentGameRuntimeCommandAdapter`) already implement it (build-green; + they call the interface directly, needing no graphical wrapper). FA4 + adds only the App-side `DeferredGameRuntimeStateCommands` façade. + +### Lens 5 — `TryAutoRespondToFellowshipInvite` interception ordering: CLEAN + +- The interception guard is `request.Type == (uint)ConfirmationType.Fellowship(4) && TryAutoRespondToFellowshipInvite(request)`. + The `&&` short-circuits on type, so **any non-fellowship confirmation + never enters** the interceptor and falls through to + `_gameplayConfirmationController?.HandleRequest`. +- `ConfirmationType` enum: `SwearAllegiance = 1`, `AlterSkill = 2`, + `AlterAttribute = 3`, `Fellowship = 4`, `CraftInteraction = 5`, + `Augmentation = 6`, `YesNo = 7`. **Allegiance is type 1, distinct from + 4 — it passes through untouched.** FA5's allegiance confirmations reach + the generic `GameplayConfirmationController` unchanged (that controller + already retains the type-1/type-4 response tuple per its own doc). +- Within a type-4 request: if neither `IgnoreFellowshipRequests` nor + `FellowshipAutoAcceptRequests` is set, `TryAutoRespondToFellowshipInvite` + returns `false` → falls through to the generic dialog exactly as before. + No swallowing. +- **Accept semantics correct:** `SendResponse(type, contextId, autoAccept)`; + the third `bool` is `accepted` (verified at + `GameplayConfirmationController.cs:97`, `_sendResponse(_serverType, _serverContext, accepted)` + where `accepted = GetBoolean(ConfirmationResult)`). So `autoAccept=true` + → accept, ignore-only → decline. The Ignore↔AutoAccept mutual exclusion + (`RuntimeCharacterState.TrySetOption`) guarantees at most one is set. + +### Lens 6 — the four un-dimmed rows + AD-78 conformance count: CLEAN (code); see SHOULD-FIX 1 (register) + +- Exactly four `RowSpec` entries flipped `StoreOnly`→`Live` + (`IgnoreFellowshipRequests`, `FellowshipAutoAcceptRequests`, + `FellowshipShareXP`, `FellowshipShareLoot`); the other two Grouping-section + rows (`IgnoreAllegianceRequests`, `DisplayAllegianceLogonNotifications`) + stay dimmed. Test updated to `31` store-only / `19` live (50 total), + internally consistent. +- **No other dimming test perturbed.** `ConfigOptionsPageController`, + `KeyboardConfigController`, `ChatOptionsPageController` and their tests + are untouched by FA4 (the only match was a doc-comment reference to + ConfigOptions' OP6 precedent). Their dimming tests pass on the post-FA4 + Release binaries (91 passed). + +### Lens 7 — suite accounting +34: RECONCILES EXACTLY + +| Test file | Δ | how counted | +|---|---|---| +| `RuntimeFellowshipStateTests.cs` | +4 | 4 `GetMembers_*` `[Fact]` (commit `357d2032`) | +| `RowTemplateResolverTests.cs` | +3 | 3 `[Fact]` (new file) | +| `UiTemplateListBoxFlushPreservingScrollTests.cs` | +4 | 4 `[Fact]` (new file) | +| `SocialFellowshipPageControllerTests.cs` | +23 | 20 `[Fact]` + 1 `[Theory]`×3 `[InlineData]` = 23 discovered tests (new file) | +| `CharacterOptionsPageControllerTests.cs` | 0 | no `[Fact]`/`[Theory]` added/removed; one existing test's expected counts changed | +| `SocialPanelControllerTests.cs` | 0 | no `[Fact]`/`[Theory]` added/removed; in-place Callbacks-shape updates | +| `SocialPanelLiveMountProbeTests.cs` | 0 | assertions added inside the existing single env-gated `[Fact]` | + +4+3+4+23 = **+34**, matching 13,238 → 13,272. The ledger's per-file +arithmetic is accurate (the "+23" resolves as 20 Facts + 3 Theory +cases — a naïve attribute count of 21 would look off by 2; it is not). + +### Lens 8 — register/ledger/gate-script/digest bookkeeping + +- **Register AD-80/AD-81 well-formed:** both are new rows filed at FA4 + (AD-79 was the prior max, from FA3), correctly incrementing the AD + section header 59 → **61 active rows** (+2). AD-80 correctly frames the + XP-share display gap as ACE-vs-retail (acdream renders the byte-verified + retail table faithfully), AD-81 the two unported text-composition + primitives. Both cite `SocialFellowshipPageController.cs` at the right + members. Well-formed. +- **AD-78 addendum:** landed in the class doc, **NOT** in the register row + → **SHOULD-FIX 1** above. +- **Gate script §FA4 honest:** the `[SOLO]`/`[TWO-CLIENT]` markings are + accurate (create/caption-swap/button-enable/D5 are solo; roster/recruit/ + dismiss/leader-handoff/invite-dialog need a second character and are + marked `[TWO-CLIENT]` with an explicit FA6-bot-gate deferral that says + not to treat an unrun two-client step as a failure). The FA3 + now-stale "INERT" claims in steps 11-12 are corrected in place with + `[FA4 correction]` pointers, not left wrong. Spot-checked step + expectations against code: step 3 (leader-of-solo → Quit/Disband/Open + enabled, Leader/Dismiss disabled) matches `RefreshButtonStates`; step 15 + (closed shows "Open", flips to "Close") matches `RefreshOpenCaption`; + steps 21-22 (100% solo even-split, 0% share-off) match `FormatStatsText`/ + `EvenSplitPercent`. +- **Plan ledger FA4 row accurate:** CODE-COMPLETE with both SHAs, the + reconciled +34, the live-DAT summary, and the four scoped deferrals. + Consistent with the diffs. +- **`project_settings_options_digest.md`** references AD-78 dimming but + states **no numeric dimmed count**, so lens 8's conditional ("updated if + it states a dimmed count") does not apply — no update owed there. + +### Lens 9 — @allegiance info + FA2/FA3 live paths on post-FA4 binaries: GREEN + +Targeted `dotnet test --no-build -c Release` against the current Release +build (verified newer than every source `.cs`, tree clean): + +- App `SocialFellowshipPageController`/`RowTemplateResolver`/`UiTemplateListBoxFlushPreservingScroll`/`CharacterOptionsPageController` — **75 passed** +- App `SocialPanelController`/`SelectionInteractionController`/`SelectedObjectController` — **92 passed** +- App `ClientCommandController` (the live `@allegiance info` surface) — **26 passed** +- Runtime `RuntimeFellowshipState`/`RuntimeAllegianceState`/allegiance-info — **37 passed** +- Runtime `GameEventWiring`/`LiveSessionEventRouter`/`Allegiance` — **35 passed** +- Core.Net `Allegiance`/`GameEventWiring` — **115 passed** +- App `ConfigOptions`/`KeyboardConfig`/`ChatOptions` (unrelated dimming) — **91 passed** + +Zero failures across all targeted runs. + +--- + +## Additional blast surfaces checked, all clean + +- **`SocialRuntimeBindings` grew 4 → 14 params:** only one construction + site (`InteractionRetainedUiComposition.cs:886`), updated. No test + helper constructs it. Build-green. +- **`SocialPanelController.Callbacks` `FellowshipSnapshot:` → `Fellowship:`:** + only production site is `RetailUiRuntime.MountSocialPanel` (updated); + test sites updated. Build-green. +- **`using AcDream.Runtime;` added to `RetailUiRuntime.cs`:** no ambiguous + type surfaced (build-green); the file already fully-qualified the + Runtime types it used. +- **`RowTemplateResolver` extraction (carry-forward 2):** the cache + semantics are byte-identical to the old `MountSocialPanel` inline + closure — caches the imported `ElementInfo` (including a `null` miss, so + a permanently-unresolvable template is not retried), rebuilds a fresh + `UiElement` on every call. Sharing one instance across + Friends/Squelch/Fellowship is safe: distinct template-id keys, no + collision. The revision-latch carry-forward 4 holds — `_lastRosterRevision` + advances in `Tick` before `RefreshRoster`, so a partial resolver failure + waits for the next real membership change (a count mismatch), not a + per-frame retry. +- **Friends/Squelch scrollbar carry-forward 3:** both controllers now + resolve the scrollbar via `listBox.ScrollbarElementId` (DAT `0x72`) + scoped to `pageRoot`, replacing a hardcoded literal — the same fix + Fellowship's own `Bind` uses. Covered by the FA3 fixture tests + (`Friends_/Squelch_ScrollbarModel_IsWiredToListBoxScroll`, + `*_LongRoster_IsReachableViaScrollbar`), green in the SocialPanel run + above. (The installed-DAT-vs-fixture drift risk for `0x72` is the + pre-existing #383, not FA4-introduced.) +- **Cross-surface option writes:** the fellowship checkboxes write through + the same `SetSingleCharacterOptionRuntimeCmd` command bus the Character + tab uses, and `RefreshCheckboxSelections` re-seeds every tick — so the + Ignore↔AutoAccept mutual exclusion (`TrySetOption`) is enforced once, at + the runtime layer, for both surfaces. No double-write divergence.