docs: FA2 mechanism re-review -- CLOSED, no reopen (1 carry-forward)
All 2 MUST-FIX and all 6 SHOULD-FIX dispositions re-derived in the actual fix-round diffs (4272ad0e,ded23067,ed8b3ec9); every blast disposition spot-verified. Nothing skipped, no fix introduced a new mechanism defect. MF-1: RuntimeGenerationResetStage.Allegiance added and drained, the owner clears the profile AND drops HasServerSeed, the inverted test flipped. Stage enumeration verified consistent everywhere -- every reference outside the enum's own file is by NAME, nothing serializes the ordinal, so the +1 shift is inert. MF-2: ApplyInfoResponseSelf, the delegate hole and the self-gate are all gone (0 whole-tree hits); the test was rewritten to pin text-only output for self and other guids alike. SF-3's RecalculateEvenXPSplitting port checked line-for-line against lane B 2.10, including the deliberate leaderless-table departure -- lane B 7.4 says verbatim "treat a leaderless table as leave _even_xp_split at 1", so the citation is accurate. SF-4's 900s gate confirmed to have real data (FA1 does parse 0x02BE field 8) and to gate only the new-guid branch. SF-1/2/5/6 all land as specified. Blast: the teardown table re-derived for every N in 0..13 (case 9 was genuinely one flag over); the new reflection walk pins every intermediate stage; seam-doc and plan addenda are dated and accurate; the corrected 11/10 counts are right. Audited blast SF-6's no-register-row conclusion and AGREE -- clear-at-reset plus 0x0020-only seeding means acdream now matches retail, so no deviation remains for a row to name. Suite claim 13,201/4/0 -> 13,215/4/0 (+14) reproduced exactly by counting discovered cases per file. Targeted post-fix Release runs: 95/95 Runtime, 67/67 Core.Net. CF-1 (FA5, not a reopen): nothing re-subscribes 0x001F now that the reset clears the owner, and the plan's FA5 row cites 0x027B for the panel-show path -- which after MF-2 is text-only and feeds nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
ed8b3ec96d
commit
cc1a319c1f
1 changed files with 219 additions and 0 deletions
|
|
@ -476,3 +476,222 @@ build permitted for this review).
|
|||
| SF-4 | SHOULD-FIX | Locked/departed admission gate + `0x02BE` field 8 not modelled |
|
||||
| SF-5 | SHOULD-FIX | `AllegianceProfileLookups` duplicated in Runtime instead of reused |
|
||||
| SF-6 | SHOULD-FIX | Defaulted checkpoint snapshots carry null `Name`/`AllegianceName` |
|
||||
|
||||
---
|
||||
|
||||
# 5. Narrow re-review — 2026-08-12
|
||||
|
||||
**Scope:** verify each finding's disposition in the actual fix-round diffs
|
||||
(`4272ad0e`, `ded23067`, `ed8b3ec9`), spot-verify the blast dispositions, and
|
||||
check the suite claim. Read-only.
|
||||
|
||||
## VERDICT: CLOSED — no reopen. One carry-forward (CF-1) for FA5.
|
||||
|
||||
All 2 MUST-FIX and all 6 SHOULD-FIX from this doc are correctly applied, and
|
||||
every blast disposition I spot-checked holds. Nothing was skipped, and no fix
|
||||
introduced a new mechanism defect. The two MUST-FIX corrections are real ports,
|
||||
not suppressions.
|
||||
|
||||
## 5.1 MUST-FIX dispositions
|
||||
|
||||
**MF-1 — FIXED.** `RuntimeAllegianceState.ResetSession()` (`:192-209`)
|
||||
delegates to a new `ClearLocked()` that clears the profile **and** drops
|
||||
`_hasServerSeed`, exactly the `RuntimeCharacterOptionsState.ResetSession`
|
||||
clear-and-relatch shape. `RuntimeGenerationResetStage.Allegiance = 13` is new
|
||||
(`RuntimeGenerationReset.cs:39-58`), drained at
|
||||
`case …Allegiance: Advance(state, _allegiance.ResetSession)` (`:320-322`), and
|
||||
the owner is constructor-injected with a null check (`:113,148-149`). The enum
|
||||
stays contiguous (`Allegiance = 13` … `Complete = 22`), which `Advance`'s
|
||||
`state.Stage++` requires. **Stage enumerations are consistent everywhere:** a
|
||||
whole-tree grep for `RuntimeGenerationResetStage.` outside its own file returns
|
||||
only by-**name** references (`LiveSessionResetPlanTests.cs:279`,
|
||||
`RuntimeGenerationResetTests.cs:121,160`, two doc-comment `<see cref>`s) — no
|
||||
consumer depends on an ordinal and nothing serializes it, so the +1 shift is
|
||||
inert. The test is inverted, not deleted:
|
||||
`FellowshipClearsAtResetButAllegianceSurvivesReconnect` →
|
||||
`FellowshipAndAllegianceBothClearAtGenerationReset`, now asserting
|
||||
`!HasProfile`, `!HasServerSeed`, `AllegianceName == string.Empty`,
|
||||
`MonarchGuid == 0`, `RecordCount == 0`.
|
||||
|
||||
One shape worth naming as correct rather than sloppy: `ResetSession` carries
|
||||
**no** disposed guard, and `ClearLocked` on already-cleared fields computes
|
||||
`changed == false` so it does not even bump the revision — a true no-op after
|
||||
`Dispose`. That is right for a *retryable* reset transaction (a throwing guard
|
||||
could never converge on retry), it matches the
|
||||
`RuntimeInventoryState.ResetExternalContainer` /
|
||||
`RuntimeCommunicationState.ResetNegotiatedChannels` precedent, and
|
||||
`RuntimeFellowshipState.ResetSession` was brought to the same shape in
|
||||
`ded23067` (blast SF-5). Note this is deliberately the opposite of SF-2's
|
||||
lock-first *mutator* guard — mutators must reject post-dispose writes, reset
|
||||
must tolerate them. Both are right.
|
||||
|
||||
**MF-2 — FIXED.** `ApplyInfoResponseSelf` is deleted from
|
||||
`RuntimeAllegianceState`; the `onAllegianceInfoResponseSelf` delegate hole is
|
||||
deleted from `GameEventWiring.WireAll`; the `TargetGuid == playerGuid()`
|
||||
self-gate is deleted from the `AllegianceInfoResponse` registration, which is
|
||||
now the pre-FA2 three-line parse → null-check → `FormatAllegianceInfoLines`
|
||||
chat emission and nothing else; the router's forwarding lambda is gone. A
|
||||
whole-tree grep for `ApplyInfoResponseSelf|onAllegianceInfoResponseSelf`
|
||||
returns **0 hits** in `src/` and `tests/`. The test was rewritten rather than
|
||||
dropped: `WireAll_AllegianceInfoResponse_SelfGated_FiresOnlyForOwnGuid` →
|
||||
`…_IsTextOnly_ForSelfAndOtherGuidsAlike`, pinning the chat output firing
|
||||
unconditionally for both a self query and a by-name query. The stale-`Rank`
|
||||
second-order defect dies with the seeding path.
|
||||
|
||||
## 5.2 SHOULD-FIX dispositions
|
||||
|
||||
**SF-1 — FIXED.** All nine delegate holes are now passed conditionally
|
||||
(`social.Fellowship is { } x ? x.ApplyFullUpdate : null`, etc.,
|
||||
`LiveSessionEventRouter.cs`), so `GameEventWiring` registers a handler only
|
||||
when an owner exists and `GameEventDispatcher.GetUnhandledCount` reports
|
||||
correctly for owner-less callers. Nine, not ten — `onAllegianceInfoResponseSelf`
|
||||
was removed by MF-2, which is consistent.
|
||||
|
||||
**SF-2 — FIXED.** Every mutator in both owners now takes `_gate` first and
|
||||
checks the plain `_disposed` field inside it
|
||||
(`RuntimeFellowshipState.ApplyFullUpdate`/`ApplyUpdateFellow`/`ApplyQuit`/
|
||||
`ApplyDismiss`/`ApplyDisband`; `RuntimeAllegianceState.ApplyUpdate`/
|
||||
`ApplyLoginNotification`/`ApplyUpdateDone`/`ApplyUpdateAborted`), matching
|
||||
`RuntimeCommunicationState.cs:377-379`. The decode-thread-vs-host-thread window
|
||||
that could repopulate a disposed owner and permanently falsify
|
||||
`CaptureOwnership().IsConverged` is closed.
|
||||
|
||||
**SF-3 — FIXED, and the port is faithful.** `RecalculateEvenXpSplit()` is a
|
||||
line-for-line port of lane B §2.10's transcription of
|
||||
`Fellowship::RecalculateEvenXPSplitting @0x005B92E0`: early-return on
|
||||
`!_shareXp` leaving the flag untouched; min/max scan; `_evenXpSplit = true`;
|
||||
then `if (minLevel < 50) { if (maxLevel > leaderLevel + 5) false;
|
||||
if (minLevel + 5 < leaderLevel) false; }`. Called from `ApplyUpdateFellow`
|
||||
after the upsert (retail: `Fellow::operator=` then
|
||||
`RecalculateEvenXPSplitting`, `@0x005B977E`/`@0x005B9785`) and from
|
||||
`ApplyQuit`/`ApplyDismiss` on the removal branch only (retail `RemoveFellow
|
||||
@0x005B96EE`) — **not** from `ApplyFullUpdate`, which is correct and is the
|
||||
part that could easily have been got wrong: retail's `Fellowship::operator=`
|
||||
copies the server's authoritative flag and does not recompute. `ClearLocked`
|
||||
does not recompute either, which is right — a full clear is retail's
|
||||
`DeleteFellowship`, not `RemoveFellow`.
|
||||
|
||||
Two deliberate departures, both flagged in the code and both defensible:
|
||||
`minLevel` seeds from `uint.MaxValue` rather than retail's `100000` (identical
|
||||
for any reachable level, and the empty-table case converges to the same
|
||||
result); and the leaderless-table case returns `_evenXpSplit = true` instead of
|
||||
replaying retail's unsigned wraparound against the `0xFFFFFFFF` sentinel. I
|
||||
verified that second one against the cited source rather than trusting the
|
||||
comment: lane B §7.4 (`2026-08-11-fa-fellowship-wire.md:962-966`) says verbatim
|
||||
*"treat a leaderless table as 'leave `_even_xp_split` at 1'"*. The citation is
|
||||
accurate and the lane is the pinned spec.
|
||||
|
||||
**SF-4 — FIXED.** `ApplyFullUpdate` now stores `update.Departed` into a
|
||||
`_fellowsDeparted` dictionary (cleared alongside `_members` in both
|
||||
`ApplyFullUpdate` and `ClearLocked`), and `ApplyUpdateFellow` gates the
|
||||
new-guid branch through `IsAdmissibleWhileLocked` — `_fellowsDeparted`
|
||||
membership plus a 900 s (`0x384`) window, matching `Fellowship::AddFellow
|
||||
@0x005B94A5`. The gate correctly applies **only** to the "is this guid NEW"
|
||||
branch; an existing member's refresh is never gated, matching retail's
|
||||
`UpdateFellow`-hits-the-table path, and a refused guid skips the recompute
|
||||
exactly as retail's early-returning `AddFellow` does. I confirmed the gate has
|
||||
real data to work with: FA1's parser does read `0x02BE` field 8
|
||||
(`GameEvents.cs:693-701` — `[u16 count][u16 buckets]` then
|
||||
`count × [u32 guid][i32 timestamp]`), so this is not an always-refuse gate.
|
||||
|
||||
**SF-5 — FIXED.** `AllegianceProfileLookups` promoted `private` → `internal`
|
||||
with `<InternalsVisibleTo Include="AcDream.Runtime" />` added to
|
||||
`AcDream.Core.Net.csproj`; `TryGetMember`/`TryGetPatron`/`GetVassals` now call
|
||||
`FindData`/`FindPatron`/`FindVassals` and the duplicated `TryGetMemberLocked`
|
||||
helper is deleted. One implementation of the retail walk again.
|
||||
|
||||
**SF-6 — FIXED.** `RuntimeStateCheckpoint`'s `Fellowship`/`Allegiance`
|
||||
parameters are now required rather than `= default`, and both snapshot types
|
||||
gained an explicit parameterless constructor seeding `string.Empty`. The stated
|
||||
reasoning is correct C#: `default(T)` bypasses a struct's parameterless
|
||||
constructor, and CS1736 forbids a non-constant `new(...)` as an optional
|
||||
parameter default — so required parameters really were the only way to
|
||||
guarantee a non-null `Name`/`AllegianceName`.
|
||||
|
||||
## 5.3 Blast dispositions (spot-verified)
|
||||
|
||||
- **Teardown ledger (blast MF-1) — FIXED, better than the report asked for.**
|
||||
Case 9 was genuinely one flag too many (it included `FellowshipDisposed`, the
|
||||
stage it had not yet run); it is now
|
||||
`Complete & ~Fellowship & ~Allegiance & ~Identity & ~EntityObjects` = 9
|
||||
flags. I re-derived the whole table: at `_disposeStage == N` exactly N flags
|
||||
are set, for every N in 0..13. The new
|
||||
`CompletedTeardownStagesAccumulatesExactlyOneFlagPerStage` reflection walk
|
||||
pins **every** intermediate stage (the previous tests only sampled stage 0
|
||||
and `Complete`, which is precisely why the off-by-one survived), and its
|
||||
closing `Assert.Equal(Complete, expected)` ties the hand-written ordered flag
|
||||
list back to the enum so a future owner insertion cannot silently drop or
|
||||
duplicate a flag.
|
||||
- **Router self-guid test (blast SF-4) — FIXED.**
|
||||
`FellowshipQuit_RoutesSelfGuidToClearAndOtherGuidToRemove` builds real
|
||||
`RuntimeFellowshipState`/`RuntimeAllegianceState` owners, wires them through
|
||||
the one production `LiveSessionEventRouter` registration site with
|
||||
`PlayerGuid: () => self`, and dispatches real `0x00A3` envelopes for both an
|
||||
other-quit (one member removed, still in fellowship) and a self-quit (whole
|
||||
snapshot cleared). This was the slice's only non-trivial lambda and it now
|
||||
has end-to-end coverage.
|
||||
- **`ResetSession` guard alignment (blast SF-5) — FIXED**, see MF-1 above.
|
||||
- **`GetVassals` allocation (blast SF-7) — documented** as an intentional
|
||||
exception with the reason (`yield return` is illegal inside a `lock`) and a
|
||||
caching hint for FA3. Acceptable disposition for a convention note.
|
||||
- **Seam-doc addenda (blast SF-1/SF-2) — LANDED and dated.** §1.3's inverted
|
||||
`RuntimeCharacterOptionsState` citation, §2.3/§9's "dispatcher permits
|
||||
multiple owned handlers", the §8 table's Allegiance row, and the
|
||||
executive-summary ownership bullet all carry
|
||||
`[FA2 fix-round addendum, 2026-08-12 …]` strikes citing the review docs;
|
||||
§2.3 additionally gains the `0x01C9`/`0x01CA` "correctly left unregistered"
|
||||
disposition it was missing. The plan's D2, its architecture blurb, and FA2's
|
||||
slice-map contract row all carry matching dated corrections, so the
|
||||
"allegiance survives reconnect" claim does not survive uncorrected anywhere.
|
||||
- **Ledger count corrections (blast SF-3) — LANDED:** 11 `Send*` wrappers (not
|
||||
12 — `SendAllegianceInfoRequest` pre-dates FA2) and 10 delegate holes (not
|
||||
15). I re-counted both against the diffs; both corrected figures are right.
|
||||
- **Register-row re-evaluation (blast SF-6) — I audited the conclusion and
|
||||
AGREE.** With MF-1's clear-at-reset landed, acdream's allegiance lifetime now
|
||||
*matches* `ClientAllegianceSystem::OnEndCharacterSession @0x00569FA0` →
|
||||
`AllegianceProfile::Clear`, and with MF-2 landed `0x0020 AllegianceUpdate` is
|
||||
the sole seeder, matching retail's sole `AllegianceProfile::operator=` writer
|
||||
at `@0x0056A129`. The register tracks acdream-vs-retail *deviation*; there is
|
||||
no deviation left for a row to name — the same reasoning FA1's D9 addendum
|
||||
used for the absent WeenieError strings. **No register row is owed, and no
|
||||
other FA2 deviation surfaced during this re-review.** TS-81 (login-notice
|
||||
text) and TS-80 (narrowed) remain correct as filed.
|
||||
|
||||
## 5.4 Suite claim
|
||||
|
||||
**Consistent with the diffs, exactly.** Counting discovered xunit cases
|
||||
(`[Fact]` + `[InlineData]` rows) at `12053e61` vs the fix-round tip:
|
||||
`RuntimeAllegianceStateTests` 7 → 9 (**+2**: one `ApplyInfoResponseSelf_…`
|
||||
deleted, three `ResetSession_*` added); `RuntimeFellowshipStateTests`
|
||||
10 Facts + 4 InlineData = 14 → 17 Facts + 7 InlineData = 24 (**+10**);
|
||||
`GameRuntimeTests` **+1**; `LiveSessionEventRouterTests` **+1**;
|
||||
`RuntimeGenerationResetTests` and `GameEventWiringTests` **+0** (one in-place
|
||||
rename each). Total **+14**, matching the ledger's 13,201/4/0 → 13,215/4/0
|
||||
claim. The absolute totals were not independently re-measured (no build
|
||||
permitted); the delta is exact.
|
||||
|
||||
Targeted runs on the post-fix Release binaries (built 02:19, after the last
|
||||
source edit at 02:06 and the final code commit at 02:17):
|
||||
`AcDream.Runtime.Tests` filtered to Fellowship/Allegiance/GenerationReset/
|
||||
GameRuntimeTests/LiveSessionEventRouter — **95 passed, 0 failed, 0 skipped**;
|
||||
`AcDream.Core.Net.Tests` filtered to GameEventWiring — **67 passed, 0 failed,
|
||||
0 skipped**.
|
||||
|
||||
## 5.5 Carry-forward (FA5, not a reopen)
|
||||
|
||||
**CF-1 — nothing re-subscribes allegiance now that the reset clears it, and the
|
||||
plan's FA5 row names the wrong opcode for the panel-show path.** Retail sends
|
||||
`Event_UpdateRequest(1)` (`0x001F`) at `gmAllegianceUI::PostInit @0x004911C6`,
|
||||
at `RecvNotice_PlayerDescReceived @0x00490D59` (i.e. every character-session
|
||||
start), and on the panel's visible branch `@0x004912DD` (lane C §1.2). acdream
|
||||
wires `0x001F` to no lifecycle event, so post-reset the owner stays empty until
|
||||
ACE happens to push `0x0020` on a tree change. Before MF-1 this was masked by
|
||||
persistence; the fix correctly exposes it. No register row is owed while there
|
||||
is no allegiance panel at all (the whole surface is FA3/FA5 scope) and no
|
||||
consumer reads the owner, but FA5 must wire it — and the plan's FA5 row
|
||||
currently says *"profile parse on show (`0x027B`)"*, which is the by-name
|
||||
**info request**, not the subscribe toggle. That distinction is now
|
||||
load-bearing: after MF-2, `0x027B`'s response (`0x027C`) is text-only and no
|
||||
longer feeds the owner at all, so an FA5 that follows the plan literally would
|
||||
open a panel with chat text and no data. Correct the plan row to `0x001F`
|
||||
before FA5 starts.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue