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