From b6c4a4fa3a3e1d9f4e215c788685550f578fc96e Mon Sep 17 00:00:00 2001 From: Erik Date: Wed, 12 Aug 2026 08:58:30 +0200 Subject: [PATCH] docs: FA5 blast-radius review -- APPROVE-WITH-FIXES (1 SHOULD-FIX, 2 NIT) Suite-accounting SHOULD-FIX: the FA5 ledger/commit cite FA4's INTERMEDIATE 13,285 figure as the baseline and claim +11 net, but FA4 CLOSED at 13,286 (its 'Final full suite' figure) and the real net is +10 (verified per-file [Fact] counts: SocialPanelControllerTests 22->31, Confirmation 4->5, probe 1->1) -- the ledger's own itemization already sums to +10, contradicting its +11 headline. End figure 13,296/4/0 is itself correct; documentation fix only. Verified clean: all three Callbacks/Bindings construction sites pass the widened Allegiance binding; every production accessor fed from a real seam; the 0x001F and 0x00A6 toggles are independent edge-triggered latches with no cross-talk (38 Fellowship tests green); ResolveWorldObjectName reuses the Toolbar's ClientObjectTable read and ShowConfirmation is a pre-existing shared method with no Fellowship collision; @allegiance info/0x027C path untouched (52 Core.Net + 16 Runtime allegiance tests green); FA5 makes zero Runtime changes; register 63->66 rows accurate (AD-84/85/86 + AD-82 addendum); NUL-fix correct and no residual control bytes in any of the 11 touched files. Co-Authored-By: Claude Fable 5 --- docs/research/2026-08-12-fa5-review-blast.md | 215 +++++++++++++++++++ 1 file changed, 215 insertions(+) create mode 100644 docs/research/2026-08-12-fa5-review-blast.md diff --git a/docs/research/2026-08-12-fa5-review-blast.md b/docs/research/2026-08-12-fa5-review-blast.md new file mode 100644 index 00000000..f67adf59 --- /dev/null +++ b/docs/research/2026-08-12-fa5-review-blast.md @@ -0,0 +1,215 @@ +# 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.