docs: FA2 blast review -- APPROVE-WITH-FIXES (2 MUST-FIX, 7 SHOULD-FIX)
Blast-radius review of FA2 (1c401048,369729f0,cced83b4,12053e61) along the axes the implementer did not traverse. MF-1: GameRuntime.cs:716-725 -- CompletedTeardownStages case 9 claims FellowshipDisposed one stage early (10 flags where the pre-FA2 case had exactly 9, ending at CommunicationDisposed). Only observable on the teardown failure path, which is precisely when the ledger must be honest. No test pins intermediate stages, so nothing caught it. MF-2: seeding RuntimeAllegianceState from 0x027C AllegianceInfoResponse is a retail divergence with no register row. Retail's CM_Allegiance::DispatchUI_AllegianceInfoResponseEvent @0x006a7470 unpacks into a STACK-LOCAL CAllegianceProfile and destroys it on return; Handle_Allegiance__AllegianceInfoResponseEvent @0x0056a1d0 uses it only as a read source for AddTextToScroll. 0x027C is text-only in retail; the panel is fed exclusively by 0x0020. Concrete risk: 0x027C carries no rank, so an @allegiance info before the first 0x0020 leaves the owner at HasProfile=true with a fabricated Rank=0 for FA3's panel to render. SHOULD-FIX: the seam doc still carries the false dispatcher claim FA2 disproved (only the plan ledger and a code comment were corrected); no disposition recorded for the two deliberately-skipped dead events; three count claims wrong (15 delegate holes -> 10; 12 Send wrappers -> 11; 11 S->C events -> 10); no test covers the router->owner plumb including the one non-trivial lambda; ResetSession's disposal guard diverges from the precedent it cites; allegiance reconnect-survival has no register row; GetVassals allocates against the stated view contract. Verified clean and enumerated exhaustively: every WireAll site (one production, shared by both hosts), both bindings sites, every IGameRuntimeCommands/IGameRuntimeView implementer (no bot-reachable stub), zero auto-fire on all 11 new Send wrappers incl. 0x00A6/0x001F, reset- and teardown-stage renumbering at every enumeration point, the central accepting gate, host-adapter self-guid and owner-borrow equivalence, the K-slice bot policies + trace recorder (21/21), the @allegiance info live path (79/79), and the suite accounting -- measured 13,201/4/0 (13,205 total) with the +43 reconciled per test file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
12053e61ea
commit
2e97924801
1 changed files with 453 additions and 0 deletions
453
docs/research/2026-08-12-fa2-review-blast.md
Normal file
453
docs/research/2026-08-12-fa2-review-blast.md
Normal file
|
|
@ -0,0 +1,453 @@
|
||||||
|
# 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:
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
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).
|
||||||
Loading…
Add table
Add a link
Reference in a new issue