# Campaign FA slice FA5 — BLAST-RADIUS review **Reviewer lens:** blast radius — the axes the implementer may not have traversed (construction-site fan-out, cross-subsystem toggle interaction, shared-seam collisions, suite-accounting, register/ledger/gate-script bookkeeping, and the NUL-byte class). **Target commits:** `7ed79eaf` (code), `bc29a1db` (register/gate-script/ ledger), `7e394cbf` (NUL-byte repair) on `claude/latest-commits-cb0c8f`. **Date:** 2026-08-12. ## VERDICT: APPROVE-WITH-FIXES One SHOULD-FIX, documentation-only (suite-accounting arithmetic). Every functional axis in scope verified clean; the shipped end-state figure (13,296/4/0) is itself correct — the error is only in the delta narrative. Two NITs (coverage observations). No REJECT-class findings. --- ## SHOULD-FIX 1 — the ledger/commit cite the wrong FA4 baseline and a +11 delta that contradicts their own itemization (actual net is +10) **Where:** `docs/plans/2026-08-11-fellowship-allegiance-campaign.md` FA5 ledger row (line 314) and commit `bc29a1db`'s message; echoed in `7ed79eaf`'s commit message ("up from FA4's 13,285/4/0 baseline"). **The claim:** "Baseline **13,285**/4/0 (13,289 total, FA4's own ledger figure) → FA5 13,296/4/0 (13,300 total), directly measured, **+11 tests net**". **Why it's wrong — three independent proofs:** 1. **FA4 did not close at 13,285.** The FA4 ledger row (line 313) carries TWO figures: an intermediate `13,285 passed / 4 skipped / 0 failed (13,289 total), directly measured` (the FA4 *pre-fix-round* code measurement) and, after FA4's own MUST-FIX/MF-3-REOPEN fix rounds, **`Final full suite 13,286/4/0`**. The FA4-CLOSE baseline is **13,286**, not 13,285 — the implementer grabbed the earlier of the two figures in FA4's own row. 2. **The real net is +10, verified by counting `[Fact]`/`[Theory]` per file before (`f5bd3e56`) vs after (`7ed79eaf`):** - `SocialPanelControllerTests.cs` 22 → 31 = **+9** (10 added, 1 removed — `Allegiance_HasProfile_ShowsBothBlocks` confirmed present at `f5bd3e56`, absent at `7ed79eaf`) - `GameplayConfirmationControllerTests.cs` 4 → 5 = **+1** - `SocialPanelLiveMountProbeTests.cs` 1 → 1 = **+0** (extended in place inside its single env-gated `[Fact]`) - Total **+10**. 3. **The ledger contradicts itself.** Its own itemization reads "nets +9 … +1 … net 0 new tests" = **+10**, directly under the "+11 tests net" headline. `13,286 + 10 = 13,296` — which is exactly the (correct) measured end figure. `13,285 + 11 = 13,296` is arithmetically self- consistent but built on the wrong baseline and a delta one higher than the itemization supports. **Blast:** none functional — 13,296/4/0 is the right end figure and the suite is green (targeted re-runs below all pass). This is a bookkeeping error that will mislead the next slice's baseline pick if left uncorrected. **Fix:** in the FA5 ledger row and (if re-touched) the closeout, change "13,285/4/0 … FA4's own ledger figure" → **13,286/4/0 (FA4's *Final full suite* close figure)** and "+11 tests net" → **+10 tests net**. No code or test change. --- ## NIT 1 — the combined tab-switch-while-shown toggle pairing has no dedicated test Concern #3's headline scenario (window shown, switch Allegiance↔Fellowship, verify BOTH subscriptions flip correctly) is covered only by reasoning + two separate axis tests (`AllegiancePageVisible_...OnWindowShown...` exercises the visibility axis; `ShowFellowship_SwitchesTheActiveTab` exercises the tab axis with the window still closed). The logic is correct (see concern #3 below), but a single test that shows the window, switches tabs, and asserts `allegiance-set-subscription:False` + `fellowship-set-panel-open:True` in one go would pin the no-cross-talk property directly. Optional. ## NIT 2 — the FA5 live-mount probe assertions run in no automated context here `SocialPanelLiveMountProbeTests.ProbeLiveMountShapes` returns early unless `ACDREAM_PROBE_LIVE_MOUNT=1` (line 26-27) AND live DATs are installed (absent in this worktree). Its new FA5 assertions (scoped `0x10000492` dual-resolution, the vassal row template, the checkbox, the five confirmation/label strings, a full production `Bind()` pass) therefore compile but are never exercised by the suite — it passes trivially, adding 0 tests and 0 skips. The implementer disclosed this honestly and tied it to the owed connected gate; recording it here only so the next reviewer does not mistake "37 App tests green" for "the live-mount probe ran". --- ## What verified CLEAN ### Concern #1 — every `Callbacks`/`Bindings` construction site passes the new binding The record field changed from `Func AllegianceSnapshot` to `SocialAllegiancePageController.Bindings Allegiance`. A grep for construction sites finds exactly three, all passing the widened binding — a defaulting site is impossible (positional record; the compiler would reject a missing arg): - **Production:** `RetailUiRuntime.cs:2779` `Allegiance: new …Bindings(...)` — all 17 fields supplied. - **Test builder:** `SocialPanelControllerTests.cs:122` `MakeCallbacks` → `MakeAllegianceBindings` (line 87) — all 17 fields. - **Probe builder:** `SocialPanelLiveMountProbeTests.cs:379` — all 17 fields (test doubles, fine). ### Concern #2 — every `Bindings` accessor fed from a real production seam `RetailUiRuntime.cs:2798-2821` feeds all 17 fields; none null/no-op in production. `Monarch/Patron/Member/Vassals` ← `IRuntimeAllegianceView` (`GameRuntimeGameplayViews.cs:208`, real out-param accessors); `Swear/Break/Kick/SetUpdateSubscription` ← `DeferredGameRuntimeStateCommands` → `commands.Allegiance.*`; `ResolveWorldObjectName` ← `ClientObjectTable.Get()?.GetAppropriateName()`; `ShowConfirmation` ← `RetailUiRuntime.ShowConfirmation` → `DialogFactory.MakeConfirmation`. The `SocialRuntimeBindings` record grew 8 new fields appended at the end (`RetailUiRuntime.cs:279-286`); the composition site (`InteractionRetainedUiComposition.cs:886-922`) uses positional args for the pre-existing 14 and named args for the 8 new ones, in matching order — well-formed. ### Concern #3 — the 0x001F and 0x00A6 toggles share the conjunction with no cross-talk Both are independent edge-triggered latches on separate page controllers (`SocialAllegiancePageController.SetPageVisible` guards `if (_subscribed == visible) return;`; Fellowship's `SetPageVisible` is the same shape). `SocialPanelController._onActivePageChanged`, `OnShown`, and `OnHidden` each drive BOTH `UpdateFellowshipPageVisibility()` and `UpdateAllegiancePageVisibility()`. The two conjunctions (`_visible && IsShowingFellowship`, `_visible && IsShowingAllegiance`) are mutually exclusive on the active-tab axis, so: - Switch → Allegiance (window shown): Allegiance ON, Fellowship OFF. - Switch → Fellowship (window shown): Fellowship ON, Allegiance OFF. - Window hidden: both OFF. No shared mutable state between the two latches; no missed toggle. `RedeclareAfterWorldEntry` deliberately treats them differently — Fellowship stays conjunction-gated (`UpdateFellowshipPageVisibility`), Allegiance is UNCONDITIONAL (`_allegiance?.RedeclareAfterWorldEntry()`), matching retail's differing arm semantics (PlayerDescReceived always sends 1) and side-stepping the FA4 MF-3-REOPEN bug class. The 38 Fellowship App tests pass — no regression from the folded-in second toggle. ### Concern #4 — ResolveWorldObjectName / ShowConfirmation seams `ResolveWorldObjectName` (`guid => _bindings.Inventory.Objects.Get(guid)?. GetAppropriateName()`) is the byte-identical read pattern the Toolbar's `ResolveName` uses (`InteractionRetainedUiComposition.cs:725`, `d.Inventory.Objects.Get(guid)?.GetAppropriateName()`) — a pure `ClientObjectTable` read, no registration, no lifetime coupling. `ShowConfirmation` is a **pre-existing shared method** (`RetailUiRuntime.cs:726`) already used by the Toolbar (line 1545) and the end-session paths (2102/2131); FA5 adds one more caller, each producing a fresh `DialogFactory.MakeConfirmation` dialog. **No collision with Fellowship:** the FA4 Fellowship `Bindings` has no `ShowConfirmation` field at all (production mount lines 2781-2797) — Fellowship commands send directly, so there is nothing to collide with. The server-driven accept-swear (ConfirmationType 1) routes through `GameplayConfirmationController`, which shares the same single `RetailDialogFactory` — but that is a pre-existing multi-consumer design (item/skill-training/end-session confirmations already share it); FA5 introduces no new shared mutable state. ### Concern #5 — the @allegiance info / 0x027C live path is untouched FA5's three commits touch **no** `AcDream.Core.Net` files (diff stat confirms: only App/Composition, App/UI, tests, docs). The parser (`ClientCommandResponses.cs`, `GameEventWiring.cs`) was last touched by FA2 (`4272ad0e` = "0x027C stops seeding", the MF-2 correction). Targeted re-runs (Release, `--no-build`): **52 Core.Net allegiance tests pass** (incl. the 0x027C AllegianceInfoResponse parse path), **16 Runtime allegiance tests pass**. ### Concern #6 — FA5 only CONSUMES RuntimeAllegianceState.SetUpdateSubscription FA5's code commit touches zero `src/AcDream.Runtime/` files (the "Runtime" strings in its stat are `RetailUiRuntime.cs`/`InteractionUiRuntimeSources.cs`, both App). `SetUpdateSubscription` (plus Swear/Break/Kick) exists on `IRuntimeAllegianceCommands` (`GameRuntimeCommands.cs:367`) and **both** host adapters — `DirectGameRuntimeCommandAdapter.cs:1043` and `CurrentGameRuntimeCommandAdapter.cs:1031` — all shipped by FA2. ### Concern #8 — register / gate-script bookkeeping - **Register:** AD-84 (Swear IsPlayer gate, mirrors AD-83), AD-85 (unported StringInfo gap, mirrors AD-81), AD-86 (ACE's seven zeroed fields) are well-formed with file:line anchors and retail citations; the AD-82 addendum is correctly appended to the existing row (no new row for it). Section-2 header `63 → 66 active rows`; **actual table row count is exactly 66** — accurate. - **Gate-script §FA5** (lines 519-739): mirrors §FA4's structure; the CF-1 correction note (0x001F not 0x027B/0x027C), the SF-7 per-relationship steps, the reconnect MF-3-REOPEN analogue, the AD-85 numeric-only honesty, and the ACE-zeroed-field honesty are present; `[SOLO]`/`[TWO-CLIENT]` markings are honest (swear/populated-roster/break/kick/offline-dimming genuinely need a second client; subscription/empty-state are solo). - **Ledger FA5 row:** substantively accurate except the baseline/delta error (SHOULD-FIX 1). ### Concern #9 — the NUL-byte class `7e394cbf` correctly restored the two spaces: `BlankSentinel`'s bytes are now `22 20 62 6C 61 6E 6B 20 22` = `" blank "` (0x20 on both sides of "blank"); the pre-fix `7ed79eaf` version had exactly 2 NUL bytes. **Independent byte-level sweep of ALL 11 FA5-touched files** (5 source, 3 test, 3 doc): **0 NUL, 0 sub-0x20 control bytes (excluding TAB/LF/CR), 0 0x7F** — the implementer's "swept … found none elsewhere" claim is verified. --- ## Targeted test evidence (Release, `--no-build`; artifacts fresh — App.Tests.dll built 08:47 after the 08:43 source edit) | Filter | Result | |---|---| | Core.Net `~Allegiance` | 52 passed / 0 failed / 0 skipped | | Runtime `~Allegiance` | 16 passed / 0 failed / 0 skipped | | App `~SocialPanelController\|~GameplayConfirmationController\|~SocialPanelLiveMountProbe` | 37 passed / 0 failed / 0 skipped | | App `~Fellowship` (regression) | 38 passed / 0 failed / 0 skipped | The full-suite figure was reconciled analytically (13,286 close + 10 net = 13,296), not re-measured — the review charter forbids a full build.