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 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
38f08314c7
commit
eda8729c92
1 changed files with 307 additions and 0 deletions
307
docs/research/2026-08-12-fa4-review-blast.md
Normal file
307
docs/research/2026-08-12-fa4-review-blast.md
Normal file
|
|
@ -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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue