docs: FA1 mechanism re-review -- CLOSED, all 7 findings verified fixed
Narrow re-review of the fix round (ed308087code+tests,511ba6e5docs) against the mechanism findings doc. Verdict CLOSED, no REOPEN. All 2 MUST-FIX and 5 SHOULD-FIX verified in the actual diffs, each re-derived rather than taken on the commit message's word: - MF-1: zero-id rejection on BOTH the monarch and child paths, which is also what makes treeParent == 0 provably fatal (knownIds can never contain 0); three boundary tests. - MF-2: the 0x001F builder re-verified against primary source -- CM_Allegiance::Event_UpdateRequest @0x006A7260 allocates 0x10, stores 0x1f at 006a72ba, writes the arg as a full u32 at 006a72cb. Both golden vectors correct; all five new anchors resolve; the ACE claim (GameActionAllegianceUpdateRequest.cs:12 reads and ignores the value) is accurate. - SF-1: monarch clear placed at retail's own position/guard; the fixture relocation onto a vassal is not just correct but necessary, since the clear would otherwise mask the legacy-compat fallback. - SF-2/SF-3/SF-5 all closed; SF-5 resolved better than asked, renumbering to the real AllegianceVersion enum values (verified against acclient.h:2979-2994) and naming gate 5 as real-but-gating-nothing. Spot-verified all six blast dispositions: AP-90 re-pointed without being wrongly retired; four seam-map corrections applied as dated strikes (its open-question-8 answer independently re-verified against PackString16L and ACE's ReadString16L pad skip); D9 + slice row struck and annotated; ledger arithmetic now closes (13,153 total sums correctly, -4 skips = 13,149). Suite claim corroborated: the fix diff adds exactly +9 [Fact]/[Theory] and removes 0, and the post-fix Release binaries (stamped aftered308087, so --no-build is legitimate here) measure AcDream.Core.Net.Tests at 886/0/0 -- exactly the blast doc's 877 pre-fix anchor plus 9, with all 9 new tests in that project. One carry-forward, NOT a reopen: blast MF-2's enumeration stopped at four rows; lane D still names the deleted AllegianceTree at :128 and :214, both forward-looking FA2 design guidance of the same danger class as the :791 row that was corrected. Two more dated addenda close it; FA2 should not start before that. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
511ba6e5d4
commit
96df892d21
1 changed files with 94 additions and 0 deletions
|
|
@ -498,3 +498,97 @@ test classes matched only the 16 pre-existing `SocialActionsTests`/
|
||||||
treat that run as evidence of nothing and reviewed every new test by
|
treat that run as evidence of nothing and reviewed every new test by
|
||||||
reading it instead. All test *logic* above is verified by inspection
|
reading it instead. All test *logic* above is verified by inspection
|
||||||
against the primary sources cited.
|
against the primary sources cited.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# 7. Re-review of the fix round — 2026-08-12
|
||||||
|
|
||||||
|
**Fix commits:** `ed308087` (code + tests), `511ba6e5` (docs).
|
||||||
|
|
||||||
|
## VERDICT: **CLOSED** — all 7 findings (2 MUST-FIX, 5 SHOULD-FIX) verified fixed. No REOPEN.
|
||||||
|
|
||||||
|
One **carry-forward** for FA2's start (CF-1, §7.3) — not a reopen of any
|
||||||
|
finding, and not a defect in the fix round: it is residual staleness the
|
||||||
|
blast doc's own MF-2 enumeration did not reach.
|
||||||
|
|
||||||
|
## 7.1 Disposition of my findings
|
||||||
|
|
||||||
|
| # | Finding | Disposition | Evidence |
|
||||||
|
|---|---|---|---|
|
||||||
|
| MF-1 | `Add`'s `_id == 0` rule unmodeled | **CLOSED** | `ClientCommandResponses.cs:509-510` rejects a zero-id monarch before it is assigned; `:533` adds `record.CharacterId == 0u` to the child-record guard. Both `return null` = whole-message discard, matching `005b6e9e`→`005b6ecd`. `treeParent == 0` is now provably fatal for every non-monarch record: `knownIds` seeds from a monarch id that cannot be 0, so `Contains(0)` is always false. Three boundary tests added (`AllegianceProfileVersionGateTests.cs:355-405`: zero-id monarch, zero-id child, zero treeParent), each asserting `null`. |
|
||||||
|
| MF-2 | `0x001F AllegianceUpdateRequest` builder missing | **CLOSED** | `AllegianceRequests.cs:40-53` (`AllegianceUpdateRequestOpcode = 0x001Fu`) + `:93-102` (`BuildAllegianceUpdateRequest`). Re-derived from primary source: `CM_Allegiance::Event_UpdateRequest @0x006A7260` allocates `0x10`, stores `0x1f` at `006a72ba` (the cited anchor is the opcode store itself), and writes `arg1` as a full u32 at `006a72cb` — the builder's `[0xF7B1][seq][0x1F][on]` is byte-exact. Two golden vectors (on/off) at `AllegianceRequestsTests.cs:92-127`, both hand-derived and correct. All five anchors in the new doc comment resolve: `0x006A7260` exact; `0x004911C6` / `0x00490D59` / `0x004912DD` / `0x00491311` inside `gmAllegianceUI::PostInit`, `::RecvNotice_PlayerDescReceived`, and `::OnVisibilityChanged` respectively. The ACE claim is accurate — `GameActionAllegianceUpdateRequest.cs:12` reads the u32 and replies unconditionally. |
|
||||||
|
| SF-1 | Monarch `MayPassupExperience` not force-cleared | **CLOSED** | `ClientCommandResponses.cs:544-553`: `monarch = monarch.Value with { MayPassupExperience = false }`, placed after the record loop and inside `if (recordCount > 0)` — the same position and guard as retail's `005b77a7`/`005b77b3` (`if (m_pMonarch != 0) SetMayPassupExperience(…, 0)`), and unreachable from the `return null` paths exactly as in retail. The fixture relocation is correct **and necessary**: the old legacy-compat test sat on the monarch, where the new clear would make "fallback fired" indistinguishable from "clear fired". It now carries a two-record wire (monarch bitfield `0xC`, vassal bitfield `0x4`) and asserts both halves separately; the decisive new test feeds the monarch bitfield `0x1C` (the wire bit explicitly SET) and still asserts `false`. |
|
||||||
|
| SF-2 | `ParseFellowshipDisband` validated a body length retail never inspects | **CLOSED** | `GameEvents.cs:777` is now `=> true`, with the rationale in its doc comment. Test flipped: `ParseFellowshipDisband_NonEmptyBody_StillReturnsTrue` (`FellowshipEventsTests.cs:224-232`). |
|
||||||
|
| SF-3 | D5 `<<1` shape unpinned at `0x02C0` | **CLOSED** | `ParseFellowshipUpdateFellow_ShareLootIsRawNotBool_D5` (`FellowshipEventsTests.cs:169-195`) writes `shareLoot = 2` on the `0x02C0` leg and asserts `== 2u` / `!= 1u`, mirroring the `0x02BE` pin. |
|
||||||
|
| SF-4 | AP-90 cites the deleted `AllegianceTree` | **CLOSED** (shared with blast MF-1 — see §7.2) | |
|
||||||
|
| SF-5 | Gate comment numbering stopped at ten; bogus "lane B §12" citation | **CLOSED, and better than asked** | `ClientCommandResponses.cs:411-424` renumbers to the **actual `AllegianceVersion` enum values** rather than wire-appearance order, and states explicitly that the wire order is a different sequence. I re-checked all ten labels against `acclient.h:2979-2994`: SpokespersonAdded 1, PoolsAdded 2, MotdAdded 3, ChatRoomIDAdded 4, MultipleAllegianceOfficersAdded 6, Bindstones 7, AllegianceName 8, OfficersTitlesAdded 9, LockedState 10, ApprovedVassal 11 — every label now matches its enum member. Gate 5 (BannedCharactersAdded) is called out by name as real-but-gating-nothing, which is what made the old numbering appear to lose a gate. `SocialActions.cs:184-185` now cites "master-table row 12". |
|
||||||
|
|
||||||
|
No path I flagged was fixed by suppression, early return, or a narrowing
|
||||||
|
guard; each is the retail mechanism ported at the site retail performs it.
|
||||||
|
|
||||||
|
## 7.2 Spot-verification of the blast doc's six dispositions
|
||||||
|
|
||||||
|
| Item | Verdict |
|
||||||
|
|---|---|
|
||||||
|
| **MF-1 / AP-90 re-point** | ✔ `retail-divergence-register.md:298` re-points the evidence column at `ClientCommandResponses.cs` (`AllegianceProfileLookups`) and `GameEvents.cs` (fellowship parsers), keeps the deviation **active** rather than retiring it (correct — no live owner exists until FA2), and dates the edit. `ClientCommandResponses.cs:250-295` is a valid range: the class opens at `:250` and `FindVassals` is at `:288`. |
|
||||||
|
| **MF-2 / four seam-map corrections** | ✔ all four applied as dated strike-through + addendum rather than silent rewrite (the right call for a committed research record): `:791` allegiance-owner "wrapping `AllegianceTree`" and its `AllegianceTree.cs:56-162` precedent both struck and re-pointed; `:666` `SetOpen → BuildFellowshipUpdate` struck and re-pointed at `BuildFellowshipChangeOpenness` (`0x0291`) with the semantic-bug warning; the 4-arg `BuildFellowshipCreate` corrected in **both** places (`:429` dimming discussion and `:668` bot-command table); open question 8 struck and answered. I verified question 8's answer independently — `PackString16L` pads the record to a 4-byte multiple *including* its own u16 length, so `12 + name.Length` is already aligned before the trailing u32, and ACE's `BinaryReaderExtensions.ReadString16L:47` skips the identical `CalculatePadMultiple(2 + length, 4)`. The answer is correct and the question is properly closed. Cited line numbers `SocialActions.cs:144-155`, `:222-230`, `:389-403` all resolve. |
|
||||||
|
| **MF-3 / D9 + slice-row annotation** | ✔ D9 (`:103`) and the FA1 slice-map row (`:130`) both struck and annotated with the shipped finding, each pointing at §2(a) of this doc and at the conformance `[Theory]`. The annotation correctly states that no register row is owed (acdream matches retail, so there is no deviation to record). The slice row also picks up `0x001F` in its opcode list. |
|
||||||
|
| **MF-4 / ledger arithmetic** | ✔ reconciled and now internally closed: 13,153 total = 4+916+1559+15+130+119+4856+877+4677 (I summed it: 13,153), minus 4 skips = **13,149 passed**, matching the prior figure; the "+50" framing is replaced with the diff-verified **+58 / −9 = +49**, and the one-test residual is attributed to the §5 baseline being captured at a different point in history rather than papered over. That is an honest reconciliation, not a re-transcription. |
|
||||||
|
| **SF-1 / two live-command pins** | ✔ `ClientCommandResponsesTests.cs:317-390`: a three-vassal test through `FormatAllegianceInfoLines` pinning the exact reversed print order (`VassalC / VassalB / VassalA`) — the pre-existing full-tree test had one vassal and could not distinguish direction — and a `GameEventWiring`-layer test proving a malformed tree now prints nothing (`Assert.Empty(chat.Snapshot())`). Both pin the *observable* change at the surface a user sees. |
|
||||||
|
| **SF-4 / doc-comment fix + SF-5 discriminator note** | ✔ `GameEvents.cs:596` now cites the real `ConfirmationTripleTests`. The SF-5 split is cross-referenced at all three sites (`:506-512` request, `:536-539` done, `:561-573` the enum) and states the decision FA4 inherits rather than leaving an unexplained inconsistency. |
|
||||||
|
|
||||||
|
## 7.3 Carry-forward for FA2 (not a reopen)
|
||||||
|
|
||||||
|
**CF-1 — lane D still names the deleted `AllegianceTree` in four more
|
||||||
|
places, two of them forward-looking.** The fix round applied blast MF-2's
|
||||||
|
enumeration exactly; the enumeration itself stopped at four rows. A
|
||||||
|
repo-wide sweep finds four further hits in
|
||||||
|
`docs/research/2026-08-11-fa-acdream-seams.md`:
|
||||||
|
|
||||||
|
* `:128` — the FA2 owner-graph diagram: "`AllegianceOwner :
|
||||||
|
RuntimeAllegianceState (new) └── owns the ported
|
||||||
|
AcDream.Core.Allegiance.AllegianceTree`".
|
||||||
|
* `:214` — "`AllegianceTree` already lives in `AcDream.Core.Allegiance` —
|
||||||
|
**so the allegiance tree can be passed directly as a state parameter,
|
||||||
|
exactly like `friends`**".
|
||||||
|
|
||||||
|
Both are **design guidance FA2 will follow**, i.e. the same danger class
|
||||||
|
as the `:791` row that *was* corrected — an FA2 implementer reading either
|
||||||
|
is told to own or pass a class that no longer exists. (`:23` and `:890`
|
||||||
|
are the other two hits; both are historical "what exists today" inventory
|
||||||
|
prose and are fine left alone, as is every surviving
|
||||||
|
`BuildFellowshipUpdate` mention in lane B and in the plan's D3 — those
|
||||||
|
correctly describe the pre-FA1 defect and must NOT be rewritten.)
|
||||||
|
|
||||||
|
This is the single-lens-enumeration failure mode the project's own
|
||||||
|
`feedback_blast_radius_single_lens` note describes: the sweep leaked along
|
||||||
|
the axis it did not traverse. One more dated addendum at each of `:128`
|
||||||
|
and `:214` closes it; FA2 should not start before that.
|
||||||
|
|
||||||
|
## 7.4 Suite claim — consistent, and corroborated by direct measurement
|
||||||
|
|
||||||
|
The `13,158 passed / 4 skipped / 0 failed` claim is **arithmetically
|
||||||
|
consistent and independently corroborated**, though I did not run the full
|
||||||
|
solution suite myself.
|
||||||
|
|
||||||
|
* **Mechanical test-count delta.** `ed308087`'s test diff adds exactly
|
||||||
|
**9** `[Fact]`/`[Theory]` attributes, removes **0**, and adds **0** new
|
||||||
|
`InlineData` rows. 13,149 + 9 = **13,158**; 13,153 + 9 = **13,162**
|
||||||
|
total. Both stated figures close.
|
||||||
|
* **The ledger's own +9 attribution is correct**: 2 (`0x001F` golden
|
||||||
|
vectors) + 1 (D5 `<<1` at `0x02C0`) + 4 (MF-1 ×3 and SF-1's monarch
|
||||||
|
test) + 2 (blast SF-1 live-surface pins) = 9, and that matches the files
|
||||||
|
line by line.
|
||||||
|
* **Direct measurement.** The Release binaries in this worktree are
|
||||||
|
stamped 00:47:19–00:47:20, **after** `ed308087` (00:46:42), so
|
||||||
|
`--no-build` is legitimate evidence here (unlike the stale Debug
|
||||||
|
binaries noted in §6). `AcDream.Core.Net.Tests` now runs
|
||||||
|
**886 passed / 0 failed / 0 skipped** — exactly the blast doc's measured
|
||||||
|
pre-fix anchor of 877 plus 9, and all 9 new tests live in that project.
|
||||||
|
A narrower FA1-family filter (the six FA1 test classes plus
|
||||||
|
`ClientCommandResponsesTests`) runs **92 passed / 0 failed**.
|
||||||
|
|
||||||
|
That is as far as this lens can take it: every number in the ledger row
|
||||||
|
now reconciles with every other, and the one project that changed measures
|
||||||
|
exactly the claimed delta. Whether the *other eight* projects still total
|
||||||
|
what the row says is the regression lens's call.
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue