acdream/docs/research/2026-08-12-fa4-review-blast.md

# 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.