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

Campaign FA slice FA2 — BLAST-RADIUS review

Date: 2026-08-12 Reviewer lens: blast radius — what else could these commits have touched, along the axes the implementer did not traverse. Target commits (branch claude/latest-commits-cb0c8f):

Commit	Subject
1c401048	feat(net) — fellowship/allegiance outbound wrappers + inbound wiring
369729f0	feat(runtime) — RuntimeFellowshipState + RuntimeAllegianceState sibling J-owners
cced83b4	feat(app) — fellowship/allegiance command routing for graphical + headless hosts
12053e61	docs — divergence register TS-81/TS-80 + campaign ledger update

Method: read-only. Full enumeration of every construction/call site of the four widened contracts across src/, tests/, tools/; retail decomp cross-check of the one inbound semantic decision; targeted dotnet test --no-build -c Release runs on the post-FA2 binaries (built 2026-08-12 01:32:45–01:32:50, i.e. at the FA2 tip). No builds, no client launches, no subagents.

---

VERDICT: APPROVE-WITH-FIXES — 2 MUST-FIX, 7 SHOULD-FIX

The slice's structural claim holds: one inbound registration site serves both hosts, both host command adapters borrow the exact canonical owners, no new outbound wrapper auto-fires, and the K-slice bot policies + trace recorder are genuinely unbroken. The suite accounting reconciles to the test, not approximately. Two defects: an off-by-one in the teardown-stage ledger that no test can see, and one inbound seeding decision that retail's own handler refutes and that carries no register row.

---

MUST-FIX

MF-1 — CompletedTeardownStages case 9 is off by one: it claims FellowshipDisposed one stage early

src/AcDream.Runtime/GameRuntime.cs:716-725

_disposeStage == N means stages 0..N-1 have completed (Dispose() increments after DrainCurrentStage() returns true, GameRuntime.cs:641-656). Stage 8 disposes Communication; stage 9 disposes Fellowship (DrainCurrentStage, :775-780). So at _disposeStage == 9 the completed set must end at CommunicationDisposed — 9 flags.

The pre-FA2 code got this right by construction: 9 => Complete & ~IdentityDisposed & ~EntityObjectsDisposed = 11 − 2 = 9 flags. The FA2 rewrite spelled the case out longhand and added one flag too many:

9 => GameRuntimeTeardownStage.HostLeasesReleased
    | ... | GameRuntimeTeardownStage.InventoryDisposed
    | GameRuntimeTeardownStage.CommunicationDisposed
    | GameRuntimeTeardownStage.FellowshipDisposed,   // ← one stage early

That is 10 flags. Every other case is correct (10 = Complete & ~Allegiance & ~Identity & ~EntityObjects = 13 − 3 = 10 ✓; 11 = 11 ✓; 12 = 12 ✓).

Why it matters, not just cosmetics. The one moment CompletedTeardownStages is observable at an intermediate value is the failure path: Dispose() throws "GameRuntime teardown stage 9 did not complete." (:651-655) or an inner exception propagates, and the caller then reads Ownership.CompletedTeardownStages to learn how far teardown got. In exactly that case the ledger asserts FellowshipDisposed for the stage that just failed to dispose the fellowship. This is the class of ledger lie the Slice-J ownership accounting exists to prevent.

Why no test caught it. Only two assertions touch this property, and both sample the endpoints: GameRuntimeTests.cs:97 (None at stage 0) and :204-205 (Complete at the end). No test pins an intermediate stage. Grep of CompletedTeardownStages across src/ + tests/ returns exactly those two plus the definition.

Fix: delete the | GameRuntimeTeardownStage.FellowshipDisposed line from case 9, or restore the original subtraction spelling (9 => Complete & ~Fellowship & ~Allegiance & ~Identity & ~EntityObjects), which is self-checking. Add one test that walks the intermediate ledger so the next owner insertion cannot repeat this.

---

MF-2 — Seeding RuntimeAllegianceState from 0x027C is a retail divergence with no register row, and it fabricates Rank

src/AcDream.Core.Net/GameEventWiring.cs:231-241 (the fold), src/AcDream.Runtime/Gameplay/RuntimeAllegianceState.cs:95-116 (ApplyInfoResponseSelf), class doc :29-41.

The class doc states RuntimeAllegianceState is "Seeded by 0x0020 AllegianceUpdate … and, self-gated, by 0x027C AllegianceInfoResponse". The retail client does not do the second half.

Primary source. CM_Allegiance::DispatchUI_AllegianceInfoResponseEvent @0x006a7470 (named-retail/acclient_2013_pseudo_c.txt:686294-686320) constructs a stack-local CAllegianceProfile var_118, unpacks the wire into it, hands it to the handler, and destroys it on return — nothing is retained. ClientAllegianceSystem::Handle_Allegiance__AllegianceInfoResponseEvent @0x0056a1d0 (:375144-375228) uses that profile purely as a read source for AddTextToScroll lines (the asterisk note, the Allegiance information for %hs%s header, Patron:, Vassals: and the per-vassal lines) and destroys its own local CAllegianceData var_38 on exit. 0x027C is text-only in retail. Retail's allegiance panel is fed exclusively by 0x0020.

Concrete downstream risk, not theoretical. ApplyInfoResponseSelf replaces _monarch, _records, _allegianceName, _totalMembers, _totalVassals, sets _hasProfile = true and latches _hasServerSeed = true — but the 0x027C wire carries no rank, so _rank keeps whatever it had, which is 0 before any 0x0020 lands. A player who types @allegiance info (empty name ⇒ self) before the first 0x0020 push leaves the canonical owner reporting HasProfile = true, HasServerSeed = true, Rank = 0. FA3's panel — whose whole point is reading this snapshot — would then render rank 0 for a real allegiance member, sourced from a message retail treats as chat text. The code comment at :98-99 ("the last known rank (if any) is retained") shows the gap was seen and classified as a retain rather than as a divergence.

The self-gate (TargetGuid == playerGuid()) is a good instinct and correctly prevents the worse failure — a by-name query overwriting your own tree — but it does not make the seeding retail-faithful.

Fix (either is acceptable, the register row is not optional): (a) drop the onAllegianceInfoResponseSelf seeding entirely and let 0x0020 be the sole profile source, matching retail; or (b) keep it as a deliberate adaptation and add the register row in the same commit per CLAUDE.md's binding rule, naming the retail oracle above and the Rank-fabrication risk in the "Risk if assumption breaks" column. If (b), gate the seed on _hasServerSeed (or carry a RankIsAuthoritative flag) so a 0x027C-only profile cannot present a fabricated rank as truth.

---

SHOULD-FIX

SF-1 — The seam doc still carries the false dispatcher claim the slice disproved

docs/research/2026-08-11-fa-acdream-seams.md:262-264 still reads that a second registrar.Register for the same type is fine because "the dispatcher supports multiple owned handlers per type … and accept that both fire"; :853 repeats it ("The dispatcher permits multiple owned handlers per type"). Both are wrong — GameEventDispatcher.Dispatch (src/AcDream.Core.Net/Messages/GameEventDispatcher.cs:95-117) does _handlers.TryGetValue(...) and invokes the single stored RegistrationNode; RegisterOwned replaces.

The correction was recorded in the plan ledger row and in a GameEventWiring.cs code comment — but not in the seam map, which is the document FA3/FA4/FA5 are contracted to read. The FA1 round established the pattern of fixing this doc in place with a dated addendum (511ba6e5, 1bb707e2); FA2 did not follow it. Add the dated addendum at :262-264 and :853.

SF-2 — Seam doc §2.3's event table has no FA2 disposition for 0x01C9/0x01CA

The table at :240-254 is titled "The 11 events FA must register" and lists 12 rows. FA2 registered 10 (9 new + the 0x027C fold). The two it skipped — 0x01C9 FellowshipFellowUpdateDone, 0x01CA FellowshipFellowStatsDone — are correctly skipped: FA1 established both COMDAT-fold onto the identical no-op body in the Sept-2013 client and are parse-and-ignore only (src/AcDream.Core.Net/Messages/GameEvents.cs:779-803). But nothing in the FA2 commits records that disposition, so FA3 will re-derive it or, worse, "fix" the gap. One line in the seam-doc table closes it.

SF-3 — Three count claims in the ledger/commit messages are wrong

• Plan ledger (docs/plans/2026-08-11-fellowship-allegiance-campaign.md, FA2 row) and 1c401048's message both say "15 new GameEventWiring.WireAll delegate holes". The actual count is 10 (GameEventWiring.cs:107-120: onFellowshipFullUpdate, onFellowshipUpdateFellow, onFellowshipQuit, onFellowshipDismiss, onFellowshipDisband, onAllegianceUpdate, onAllegianceInfoResponseSelf, onAllegianceUpdateDone, onAllegianceUpdateAborted, onAllegianceLoginNotification).
• Plan ledger says "12 new WorldSession.Send* wrappers". The actual count is 11 (WorldSession.cs:2318-2404: 7 fellowship + 4 allegiance). SendAllegianceInfoRequest (:2312) pre-dates FA2. The 12 figure is correct for cced83b4's *RuntimeCmd records / LiveSessionCommandBindings send delegates (12, because SendAllegianceInfoRequest gets an App-bus record over the pre-existing wrapper) — the ledger conflated the two.
• 1c401048's message says the holes cover "the 11 S→C fellowship/allegiance events"; 10 event types are handled.

These are the numbers a future reviewer reconciles against. Correct them in the ledger.

SF-4 — Nothing tests the router → owner plumb, the one non-trivial lambda included

The single production registration site (src/AcDream.Runtime/Session/LiveSessionEventRouter.cs:237-264) is untested. Both Runtime router-test factories still construct LiveSocialSessionBindings without the new trailing arguments (tests/AcDream.Runtime.Tests/Session/LiveSessionEventRouterTests.cs:110-115 and :770-775), so Fellowship/Allegiance default to null and every FA2 lambda no-ops through ?. in every router test.

GameEventWiringTests proves the delegates fire; RuntimeFellowshipStateTests proves Apply* behaves. Neither covers the seam between them, and that seam is not uniformly trivial: onFellowshipQuit/onFellowshipDismiss supply inventory.PlayerGuid() as the self-guid, which is what selects "remove one member" vs "clear the whole snapshot". A transposed argument or a wrong guid source there is invisible to every test in the slice. Add one router test that wires real owners and dispatches a self-quit and an other-quit.

(inventory.PlayerGuid itself is safe: non-nullable Func<uint> on LiveInventorySessionBindings:35, null-checked in LiveSessionEventRouter.Validate:626.)

SF-5 — RuntimeFellowshipState.ResetSession() throws when disposed, unlike the precedent it cites

src/AcDream.Runtime/Gameplay/RuntimeFellowshipState.cs:205-209 opens with ObjectDisposedException.ThrowIf(IsDisposed, this). The commit message and the class doc both claim this owner matches "the ExternalContainer precedent" — but RuntimeInventoryState.ResetExternalContainer (:104) and RuntimeCommunicationState.ResetNegotiatedChannels (:167) are bare delegations with no disposal guard.

Not reachable today (teardown drains pending resets at stage 3, fellowship disposes at stage 9), but the reset transaction is retryable — a RuntimeGenerationResetStage.Fellowship that throws can never converge on retry, since disposal is terminal. Either drop the guard to match the precedent, or make ResetSession a no-op when disposed.

SF-6 — RuntimeAllegianceState surviving generation reset has no register row

The design (allegiance persists across reconnect; HasServerSeed clears only at Dispose) is deliberate, documented at RuntimeAllegianceState.cs:16-27, and pinned by RuntimeGenerationResetTests.FellowshipClearsAtResetButAllegianceSurvivesReconnect. It is also un-retail-able: retail's client cannot survive a reconnect in-process, so there is no retail behavior this matches — it is an acdream adaptation, and adaptations get rows. The stale window is narrow today (reconnect targets the same character; in-process character switching does not exist — AD-76), but the row is what stops a future in-process character-select from silently inheriting the previous character's monarch/records.

SF-7 — IRuntimeAllegianceView.GetVassals allocates against the stated view convention

RuntimeAllegianceState.cs:254-271 builds and returns a fresh List<> per call. The seam doc's view contract line (:815) states the convention as "Snapshot record struct + TryGet*; no allocation". The reason is understandable (cannot yield inside the lock), but a per-frame panel poll in FA3 will allocate. Either document the exception at the call site or return a caller-supplied buffer.

---

VERIFIED CLEAN

Each item below was enumerated exhaustively, not spot-checked.

1. WireAll call sites — every one, src + tests + tools. Exactly one production site: LiveSessionEventRouter.cs:185. Because LiveSessionEventRouter is shared by both hosts (the K-slice unification), the "single registration site serves both hosts" claim is structurally true, not merely asserted. Every other caller is a test: GameEventWiringTests.cs (24 sites), Messages/ClientCommandResponsesTests.cs (4), WorldSessionWiringOwnershipTests.cs (3), AcDream.Runtime.Tests/Gameplay/RuntimeVendorLifecycleTests.cs (2). All use named/positional prefixes and take the new parameters as null — which is the correct pre-FA2 behavior, since the FA2 registrations are individually is not null-gated (GameEventWiring.cs:246-320). No site silently drops events for one host. No tools/ site exists.

2. Both LiveSocialSessionBindings production sites pass the same owners. src/AcDream.App/Net/LiveSessionRuntimeFactory.cs:262-263 (_domain.Runtime.FellowshipOwner / AllegianceOwner) and src/AcDream.Headless/Hosting/HeadlessSessionHost.cs:786-787 (Runtime.FellowshipOwner / AllegianceOwner) — same canonical instances off the same GameRuntime. No semantic divergence between hosts at the bindings layer. The two test sites (LiveMovementStatsApplierTests.cs:68-72, LiveSessionEventRouterTests.cs:110/770) default to null — see SF-4.

3. No competing registration for any newly-registered GameEventType. Grep of GameEventType.Fellowship* / GameEventType.Allegiance* across src/ returns only GameEventWiring.cs. The 0x027C collision the implementer found was the only one, and the fold at :231-241 preserves the pre-existing @allegiance info chat output unconditionally before the (gated) Runtime callback. The dispatcher-replacement bug they describe is real — confirmed at GameEventDispatcher.cs:95-117.

4. The accepting gate covers all 9 new registrations. It is applied centrally inside OwnedGameEventRegistrar.Register (GameEventWiring.cs:1050-1061), not per-lambda, so the new handlers inherit retiring-generation suppression with no per-site opt-in. Same for RegistrationBuildScope's all-or-nothing ownership.

5. IGameRuntimeCommands implementers — all four found, none is a reachable throwing stub.

• DirectGameRuntimeCommandAdapter (src/AcDream.Runtime/Session/, :31-32, :85-86) — real implementations of both new interfaces. This is the headless bot command surface.
• CurrentGameRuntimeAdapter (src/AcDream.App/Runtime/, :118-121) delegating to CurrentGameRuntimeCommandAdapter (:30-31) — real implementations.
• InteractionUiRuntimeSourcesTests.FakeRuntime (tests/AcDream.App.Tests/ Composition/InteractionUiRuntimeSourcesTests.cs:255-256) — null! stubs. App-layer double, never handed to a bot policy.
• HeadlessProcessSchedulerTests' recording policies (:726-780) accept IGameRuntimeCommands but never dereference Fellowship/Allegiance.

No HeadlessBotPolicy touches either group — grep of commands. across src/AcDream.Headless/Policies/HeadlessBotPolicy.cs returns only Chat/Portal/Session/Movement. No bot policy can reach a stub.

6. IGameRuntimeView implementers (the same widening, one axis over). GameRuntime:499-500 and CurrentGameRuntimeAdapter:89-90 are real; the two test doubles (InteractionUiRuntimeSourcesTests.cs:237-238 → null!, GameplayInputCommandControllerTests.cs:230-231 → throw NotSupportedException) are App-only and unreachable from headless.

7. No new Send* wrapper auto-fires; 0x00A6 and 0x001F have zero live callers. Every one of the 11 new WorldSession.Send* methods is reached only from DirectGameRuntimeCommandAdapter and, via LiveSessionCommandBindings delegates + *RuntimeCmd records, from CurrentGameRuntimeCommandAdapter. SendFellowshipUpdateRequest (0x00A6) is reached only by IRuntimeFellowshipCommands.SetPanelOpen on both adapters (DirectGameRuntimeCommandAdapter.cs:949, CurrentGameRuntimeCommandAdapter.cs SetPanelOpen), and SetPanelOpen has no callers anywhere — grep of FellowshipCommands/AllegianceCommands and .Fellowship./.Allegiance. across src/ + tools/ returns only the interface declarations, the adapter properties, and the trace-recorder field reads. Same for SendAllegianceUpdateRequest (0x001F) via SetUpdateSubscription. Nothing fires until FA3/FA4 wires a caller.

8. Reset-stage renumbering is safe at every enumeration point. RuntimeGenerationResetStage is enumerated in exactly one place — the while switch in RuntimeGenerationReset.Drain (:250-360) — and the insertion of Fellowship = 12 shifted the tail 12→21 consistently across the enum, the switch, and the state.Stage = ... jumps. No test pins a stage count or a numeric ordinal; the three tests that name a stage (RuntimeGenerationResetTests.cs:121/160, LiveSessionResetPlanTests.cs:279) name stage identities (DrainHostProjection, CompleteHostProjection, RetireEntities) whose meaning is unchanged. No assertion was updated to pass without understanding — the only reset-test edit in the slice is the new FellowshipClearsAtResetButAllegianceSurvivesReconnect fact, which pins real behavior on both sides. Headless teardown has no separate stage list.

9. Teardown-stage renumbering is consistent everywhere except MF-1. TeardownStageCount 11→13 (GameRuntime.cs:129), DrainCurrentStage (:778-790) and IsCurrentStageComplete (:808-812) both renumbered in lockstep, flag bits shifted 1<<9..1<<12 with Complete extended (:34-53), construction fault points added (GameRuntimeConstructionPoint.FellowshipCreated/AllegianceCreated) and covered by two new [InlineData] cases. Only CompletedTeardownStages case 9 is wrong.

10. Both host adapters read the same self-guid and borrow the same owner. DirectGameRuntimeCommandAdapter.Quit uses _runtime.PlayerIdentity.ServerGuid; CurrentGameRuntimeCommandAdapter.Quit uses _view.Lifecycle.PlayerGuid — and GameRuntime.Lifecycle (:484-488) populates that field from PlayerIdentity.ServerGuid, with CurrentGameRuntimeAdapter.Lifecycle (:68-79) only overriding State and HasTransport. Same value. CurrentGameRuntimeAdapter:52 passes runtime.FellowshipOwner — the exact canonical instance, not a second copy. The leader-hand-off rule therefore behaves identically on both hosts.

11. The EmitUnsupported vs EmitResult spelling difference is not a semantic divergence. DirectGameRuntimeCommandAdapter uses EmitUnsupported for its FA2 argument rejections, CurrentGameRuntimeCommandAdapter uses EmitResult. EmitUnsupported(domain, op, status, guid) (DirectGameRuntimeCommandAdapter.cs:1355-1367) differs from EmitResult(domain, op, status, guid, text: null) (:1369-1383) only in its default status — and every FA2 site passes RuntimeCommandStatus.Rejected explicitly. Identical emitted event and identical returned result.

12. Both hosts gate the FA2 commands with their own standard in-world gate. Direct: Validate(gen, out session) requires _route is not null && session is not null && _runtime.Session.IsInWorld (:1385-1406). App: Validate(gen, requireWorld: true) (:1045-1074) — the same value every other gameplay command family in that adapter passes. No FA2 command is looser than its neighbours.

13. K-slice bot policies + trace recorder: confirmed unbroken (the D2 promise). No IRuntimeEventObserver member was added — the trace recorder gained two appended interpolated fields only (GameRuntimeEvents.cs:184-189), reading RuntimeStateCheckpoint's new = default parameters, so any checkpoint built without them prints fellowship=0:False:0;allegiance=0:False:0 rather than faulting. RuntimeCommandDomain gained two trailing values (11, 12) — additive, no renumber. Targeted run: AcDream.Headless.Tests --filter "Policy|Scheduler|Bot" → 21/21 passed.

14. The @allegiance info live path is still green on post-FA2 binaries. Targeted run AcDream.Core.Net.Tests --filter "Allegiance|Fellowship" → 79/79 passed, including the FA1 live-surface pins (WireAll_AllegianceInfoResponse_ReachesChatTranscript, WireAll_AllegianceInfoResponse_MalformedTree_PrintsNothing) and the new WireAll_AllegianceInfoResponse_SelfGated_FiresOnlyForOwnGuid. The fold preserves the chat output unconditionally; the Runtime callback is additionally gated on playerGuid is not null, so the two WireAll(...) sites in ClientCommandResponsesTests that pass no playerGuid behave exactly as before. (The semantics of that seeding are MF-2; the chat path is intact.)

15. Suite accounting reconciles exactly — measured, not quoted. Full per-project runs on the post-FA2 Release binaries:

Project	Passed	Skipped	Total
AcDream.Runtime.Tests	1593	0	1593
AcDream.Core.Net.Tests	895	0	895
AcDream.App.Tests	4853	3	4856
AcDream.Headless.Tests	119	0	119
five untouched projects (from FA1's measured breakdown)	—	—	5742
total	13,201	4	13,205

Matches the ledger's 13,201/4/0 (13,205 total) claim exactly. The +43 delta reconciles against the actual test-file diffs, per file:

File	Added
RuntimeFellowshipStateTests.cs	10 [Fact] + 2 [Theory]×2 [InlineData] = 14
RuntimeAllegianceStateTests.cs	7
RuntimeGenerationResetTests.cs	1
GameRuntimeTests.cs	2 (new [InlineData] on an existing [Theory])
Session/DirectGameRuntimeCommandAdapterTests.cs	10
GameEventWiringTests.cs	9
total	43 ✓

cced83b4 added zero tests — its three test-file edits (InteractionUiRuntimeSourcesTests, GameplayInputCommandControllerTests, LiveSessionCommandRouterTests) are compile-only interface/record fills, which matches App.Tests holding at 4856. RuntimeGameplayOwnershipTests.cs and GameRuntimeContractTests.cs are likewise compile-only — and importantly, no assertion in either was loosened: the ownership tests construct the two new owners, dispose them alongside the existing ones, and keep every Assert.True(retired.IsConverged) intact (which now additionally requires the two new IsConverged clauses added at RuntimeGameplayOwnership.cs:20-26).

16. Divergence-register bookkeeping is exact. Section-4 header 47 → 48 active rows, and the measured | TS- row count in the section is 48. TS-81 is well-formed (7 pipes / 6 columns, matching the table header) with a real "Where" cite (RuntimeAllegianceState.cs ApplyLoginNotification), a rationale grounded in CLAUDE.md's no-invented-English rule, and the retail oracle chain (ClientAllegianceSystem::Handle_Allegiance__AllegianceLoginNotificationEvent @0x00569ff0 → CM_Allegiance::SendNotice_AllegianceLogin @0x006a7330 → gmAllegianceUI::RecvNotice_AllegianceLogin @0x00492220). TS-80's narrowing is dated and correctly scopes the remaining gap to FA4. The BN-mislabel caution is the right call — this is exactly the artifact class the project has been burned by before.

The one register gap is MF-2 (and SF-6): FA2 introduced deviations that got no rows.

---

Appendix — what I did not review

Mechanism-level correctness of the FA1 parsers, the retail-faithfulness of the fellowship apply rules (REPLACE / UPSERT / self-vs-other), and the leader-hand-off ordering against lane B §2.5/§3.6 are the mechanism review's scope, not this one. I checked them only where a blast-radius question forced it (MF-2's retail decomp read; the Quit self-guid equivalence across hosts).