# C5a — architecture / adversarial review, round 2 (delta) **Reviewer:** architecture / adversarial (Opus) **Scope:** delta over round 1 (`docs/research/2026-08-05-c5a-architecture-review.md`). Working tree at branch `claude/acdream-physics-divergence-5aa784`, HEAD `392c1e22`, uncommitted. Part 2 (the deletion sweep) and the composition-graph verification are carried forward from round 1 and not re-litigated — but I did re-confirm the two survivors and the production blast radius are unchanged by the fix round (see "Regression check" below). **Date:** 2026-08-05 --- ## VERDICT: **PASS** All three MAJORs are genuinely closed. A1 and A2 are now backed by tests I independently traced as discriminating; A2's fix is the *correct* mirror, not a symmetric-looking wrong one, and the constructor-parameter removal is safe on every path. A3 built the real drive rather than arguing around it, and the position half of route 2's B2 is now pinned end-to-end through real production machinery with a value the wire cannot supply. Two **MINOR** items to fold in before commit (neither blocks the slice): - **M1** — the **cell** half of B2 is still not pinned: `entity.ParentCellId` already equals the asserted value before the drive runs. - **M2** — the `SyncPose`-inherits-the-guard behavioural nuance is **not** documented anywhere, contrary to the handoff's claim. Plus one **INFO** (five test-file comments still cite the deleted `PhysicsEngine.Resolve` as live). --- ## Gates I re-measured | Gate | Result | |---|---| | `dotnet build AcDream.slnx -c Release -m:1` | **Build succeeded. 0 Warning(s), 0 Error(s)** | | Complete Release suite (`--no-build -m:1`) | **11,106 passed / 4 skipped / 0 failed** | | Reconciliation | 11,112 − 11 (deleted) + 5 (3 shadow-composition + 1 Withdraw fact + 1 force-position) = **11,106 ✓ exact** | | Skips | 3 (App) + 1 (Core) = **4, unchanged from baseline ✓** | | Per-assembly vs handoff | Core 4,259/1, Runtime 1,176/0, Headless 86/0, App 4,132/3 — **matches the handoff's numbers exactly ✓** | | Sink ctor call sites updated | **6/6** (1 production `LivePresentationComposition.cs:514`, 5 test fixtures) | | Register blast radius | still **3 rows + 2 section headers**; AP-131, AD-60, AD-61/62, AP-135, AP-141–144, AP-146 untouched ✓ | **Regression check on Part 2 (carried, re-verified):** `IsSpawnCellReady` + `AdjustPosition` still `diff`-clean against HEAD over the full 45-line span. Production `--numstat` shows executable changes confined to `LivePresentationComposition` / `SessionPlayerComposition` / `RuntimePlacementPresentationSink` (+ the deletions and the one seed rename); `CellTransit`, `ConstraintManager`, `PhysicsBody`, `ResolveResult`, `HeadlessSessionWorldProjection`, `RuntimeSetPositionState`, and `RuntimeAcceptedPositionDriveController` are comment/xmldoc only. The fix round introduced no new executable surface beyond the two sink lines. --- ## A1 — the P4 test now genuinely discriminates. **CLOSED.** `tests/AcDream.App.Tests/World/RuntimePlacementShadowCompositionTests.cs:296–367` I traced the sabotage myself rather than trusting the claim. With the `record.ServerGuid == _localPlayerGuid()` gate at `RuntimePlacementPresentationSink.cs:249` removed: 1. `SyncPose(childEntity, DestinationPosition, …, DestinationCell, force: true)` runs. `_liveEntities.IsHidden(0x7000A101)` is false; `cellId != 0`; `IsCurrentVisibleProjection(childEntity)` resolves the child's **own** record (`TryGetRecord(entity.ServerGuid)`), `ReferenceEquals` holds, and it is the current spatial root — so the guard does **not** short-circuit. 2. `ShadowPositionSynchronizer.Sync` → `UpdatePosition(childId, …)`. The `_entityReg.TryGetValue` at `ShadowObjectRegistry.cs:696` now **succeeds** (the new baseline `Register` at test `:309–320` put the record there), so the early return that made v1 vacuous no longer fires. 3. `Register(childId, …, seedCellId: DestinationCell)` → flood from the destination (the same geometry fact 1 proves floods successfully) → `DeregisterCore` → row **moves** to `DestinationCell`. Result: `Assert.Contains(GetObjectsInCell(SourceCell), child)` at `:352` **fails**, and `Assert.Null(fixture.LocalShadow.Current)` at `:366` **also fails** (`_state.Set` runs as `SyncPose`'s last step). Two independent discriminators, both keyed to the gate. The baseline precondition `Assert.Contains(GetObjectsInCell(SourceCell))` at `:321` is what makes step 2 reachable — it is the thing v1 lacked, and it is now asserted, not assumed. The `Assert.Equal(1, TotalRegistered)` at `:358` is supporting only (a move keeps the count at 1); correctly not relied on. The xmldoc at `:282–293` records the v1 failure honestly rather than quietly replacing it. **Verified discriminating. No second vacuous version.** --- ## A2 — the Withdraw fix is the *correct* mirror, and the parameter removal is safe. **CLOSED.** ### Is `Suspend` the right counterpart to `SyncPose`'s publish? Yes, and I checked the two ways it could have been subtly wrong. - **It is not `Deregister`.** `ShadowObjectRegistry.Suspend` (`src/AcDream.Core/Physics/ShadowObjectRegistry.cs:1480–1498`) removes the entity from every cell bucket and stashes the cell list in `_suspendedEntityCells`, but **retains `_entityReg`** — it early-returns `false` if there is no registration and never removes one. Its own xmldoc calls it "the registry counterpart of retail `CPhysicsObj::remove_shadows_from_cells` during temporary leave-world/pending-cell residence; deliberately not logical teardown." - **The restore path still works.** This is the trap I looked for: if `Suspend` had dropped `_entityReg`, then `TryApplyWithdrawalRestoration` → `TryPublishPlace` → `SyncPose(force: true)` → `UpdatePosition` would hit the `:696` not-registered early return and **silently no-op while still writing the cache** — reintroducing the exact AP-145 class on the restore edge. It does not: `_entityReg` survives `Suspend`, `UpdatePosition` proceeds, and `Register` → `DeregisterCore` (`:1789`) clears `_suspendedEntities` so the entity is no longer treated as suspended by `RefloodOwnerForLandblock` (`:1568`) or the reflood capture (`:1530`). The restore is clean. - **It matches the established App-layer pairing.** `Suspend` is exactly what `LiveEntityProjectionWithdrawalController.LeaveWorld` already does (`:148 _shadows.Suspend(entity.Id)` + `:156 _localPlayerShadow.Clear()`), and `LocalPlayerShadowSynchronizer.Suspend` (`:109–114`) is precisely that pair in one call. This is not a novel choice invented for the fix; the sink was the odd one out. - **No new early-return.** `Suspend` is unconditional — unlike `SyncPose` it has no hidden/celless/current-projection guard — so the Withdraw edge cannot silently skip the way the Place edge theoretically can. ### Is the constructor-parameter removal safe on every path? Yes. `_localPlayerShadow` had exactly two uses in the sink (Place `.Set`, Withdraw `.Clear`); both are now synchronizer calls, so the field is genuinely dead. All **6** `new RuntimePlacementPresentationSink(` sites are updated (1 production + 5 test fixtures) and the Release build is 0-warning. The production site still constructs the synchronizer from `d.LocalPlayerShadow`, so the same single `LocalPlayerShadowState` instance is still the one cache — the removal narrows the sink's surface without changing which object holds state. This is a genuine simplification, not just a shuffle. ### Does the new 4th fact discriminate? `Withdraw_SuspendsRealPhysicsShadow_NotOnlyTheDedupCache` (`:383–…`) establishes a **real** source-cell registration (`:391–406`) plus a non-null cache, then asserts after the Withdraw that `LocalShadow.Current` is null **and** `GetObjectsInCell(SourceCell)` no longer contains the entity. Under the pre-fix `_localPlayerShadow.Clear()` the first passes and the **second fails** — and the test comment at `:419–422` says exactly that, correctly labelling the cache assertion as the non-discriminating half. Right shape. --- ## A3 — the real drive was built; the **position** half of B2 is closed. **SUBSTANTIALLY CLOSED**, see M1. `tests/AcDream.App.Tests/World/RuntimeForcePositionRenderCommitTests.cs` This is a real correction, not a re-labelling. The chain is now: `LiveEntityHydrationController.OnCreate` → real `RuntimeFirstEntryDriveController` pump → real `RuntimeEntityObjectLifetime.TryApplyPosition` (asserted to yield `PositionTimestampDisposition.ForcePosition`, `:111`) → real `RuntimeAcceptedPositionDriveController.TryExecuteAcceptedLocalPosition` (`:113–119`, asserted `Committed`) → real `RuntimePlacementProjectionSubscription` on the same placement channel (`:309–312`) → real `RuntimePlacementPresentationSink` → render `WorldEntity`. Nothing between the wire update and the assertion is hand-authored. **Why the position assertion is a true discriminator, verified:** - The wire carries `Z = 0` (`ForceUpdate` → `ServerPosition(Cell, 15, 15, 0, …)`). The assertion demands `Z = 0.48f` — the grounded foot-sphere clearance the **resolver** produces. A value that cannot be an echo of the input is exactly the right shape for a "came from the committed receipt" claim. - In this fixture the **only** post-materialization writer of `entity.Position` is `LiveEntityRuntime.TryApplyRuntimePlacementProjection`, invoked by the sink. `HostMaterializer` writes it once at create; there is no `LiveEntityNetworkUpdateController` in the composition, so B1's tolerated generic write cannot mask anything. Sever the receipt→render write and the entity stays at the first-entry pose `(10, 10, 0.48)` — `:127` (`Assert.Equal(ForcedPosition, entity.Position)`) is the assertion that fails, and `:126` (`NotEqual(positionBeforeForce, …)`) fails with it. - `positionBeforeForce` is captured live (`:93`) rather than assumed, so the "it moved" claim cannot be satisfied by a coincidence of constants. **Fixture seams — both acceptable, neither touches production:** `WorldSession.GameActionCapture` is a pre-existing Phase-I.3 test seam (`src/AcDream.Core.Net/WorldSession.cs:2026`, unmodified by this diff), and `usePositionFromServer: true` is a legitimate autonomy-level-2 configuration, not a suppression flag added for the test. Resolving the three obstacles in the assertions rather than in production code was the correct call. **B2's status:** the seam B2 actually named — "canonical body moves, render entity stays put" — is now genuinely covered end to end. I would record B2 as **closed for position**, with the cell half called out (M1) rather than assumed. --- ## M1 — MINOR. The **cell** half of B2 is still not pinned **File:** `tests/AcDream.App.Tests/World/RuntimeForcePositionRenderCommitTests.cs:128` (`Assert.Equal(Cell, entity.ParentCellId)`), with the staging at `:73–81`. `HostMaterializer` sets `ParentCellId = position.LandblockId` = `Cell` at materialization (`:412`), and the test deliberately picks landblock-local `(15,15)` so it lands in the **same** outdoor grid cell as the spawn `(10,10)` (`TerrainSurface.CellSize = 24` → `cx=0, cy=0` → low word `0x0001` for both). So `entity.ParentCellId` already equals `Cell` **before** the drive runs — this is caught vacuous-class #1 (*asserting a field written unconditionally earlier*). The comment at `:73–76` states the choice as a simplification ("without coupling this test to the outdoor grid-cell formula"); the consequence is that the cell assertion cannot fail. B2's recorded wording is "asserting the render entity's **position/cell** came from the committed placement receipt." The position half is now airtight; the cell half is asserted but unfalsifiable. **Why it is MINOR, not MAJOR:** the discriminator carrying the test's claim is the position (including the resolver-only `Z`), it is sabotage-verified, and the round-1 shadow-composition fact 1 already pins a real cross-cell `ParentCellId` change (`SourceCell` → `DestinationCell`) through the same sink code path. Nothing is unprotected; the cell half is simply not proven *by this test*. **Fix direction (small):** force to landblock-local `(30,30)` instead of `(15,15)` → `cx=1, cy=1` → outdoor low word `0x000A`, i.e. committed cell `0x0101000A ≠ Cell`. Then assert `entity.ParentCellId` equals the **committed** cell and differs from the spawn cell, and capture `cellBeforeForce` the way `positionBeforeForce` is captured. If that is judged out of scope, record in the commit message that B2 is closed for position and open for cell — do not book it as full closure. --- ## M2 — MINOR. The `SyncPose`-inherits-the-guard nuance is not documented The handoff states the nuance "is now stated in the sink's comment and the test class doc rather than left implicit." It is not. I grepped both files for `hidden` / `suspend` / `celless` / `not-current` / `visible projection` / `IsCurrentVisibleProjection` / `guard`: the only hit is `RuntimePlacementShadowCompositionTests.cs:347`, inside the P4 test's *sabotage* reasoning (explaining why the child's own projection is current) — not a statement of the Place-edge behaviour change. Neither `RuntimePlacementPresentationSink.cs:245–272` nor the test class doc (`:20–89`) mentions it. The nuance is real and worth one sentence: routing Place through `SyncPose` means the Place edge now inherits `SyncPose`'s guard (`LocalPlayerShadowSynchronizer.cs:53–59`) — if `IsHidden(playerGuid)`, `cellId == 0`, or `!IsCurrentVisibleProjection(entity)`, the Place now **suspends** the shadow where the old direct write merely cached. Both `TryApplyInitialCreateCompletion` and `TryApplyWithdrawalRestoration` reach `TryPublishPlace`, so this is reachable on more than the portal edge. The new behaviour is *correct* (it is what the next per-tick `Sync` would do anyway, and it is honest about a shadow that should not be published) — which is exactly why it belongs in a comment and the commit message rather than being discovered later as a surprise. Same class as round 1's A5: a statement made in the handoff that the code does not carry. --- ## INFO — five test-file comments still cite the deleted `PhysicsEngine.Resolve` as live Production is now clean: every remaining mention in `src/` is an explicit "deleted, cite by symbol" correction (`CellTransit.cs:880,:1064`, `HeadlessSessionWorldProjection.cs:797`, `PlayerMovementController.cs:147`). The A4 fixes are accurate and the `PlayerMovementController` class summary no longer claims a per-frame call to a deleted method. Still stale, in test comments only (no behavioural weight, no compiler signal): - `tests/AcDream.App.Tests/Physics/LiveEntityNetworkRemoteTeleportPresentationTests.cs:89` - `tests/AcDream.Core.Tests/Conformance/Issue107SpawnDiagnosticTests.cs:23,:82` - `tests/AcDream.Core.Tests/Physics/CellMarchLandblockPreservationTests.cs:22` - `tests/AcDream.Core.Tests/Physics/CellTransitFindCellSetTests.cs:301` Optional sweep; not a gate. --- ## A5 and A6 — **CLOSED, and better than asked** - **A5.** `PlayerMovementPlacementTransactionTests.cs:23–41` now states the asymmetry plainly: render-root publish **did** move (`RuntimeSetPositionState.cs:2774`), sticky release **did not move anywhere** (with the `grep` evidence and the `publishSharedState: false` reason), the behaviour was dead code so nothing regresses today, and "no layer pins the invariant … any more. That is disposition 3.6's one real coverage loss." That is the honest version. I re-verified both halves independently. - **A6.** `TransitionScratchDifferentialTests.cs:218–219` and `:236–239` now assert `IsCommitted` on both engines with distinguishing messages. The differential can no longer pass on symmetric failure. --- ## Register evidence — re-verified - **AP-1 — retire: still justified.** Zero `PhysicsEngine.Resolve` / `.ResolvePlacement` receivers in `src/`; the resolver-shaped entry points no longer exist, so the row's condition is structurally unreopenable. - **AD-1 — retire: still justified.** The recoverable outdoor demote and the outdoor-restore `max(terrainZ, z)` lift were `Resolve`'s body; the body is gone. - **AP-145 — retire: now correctly scoped, and it does not overclaim.** The row (`retail-divergence-register.md:175`) covers **both** halves, names `TryPublishWithdrawal` and the `#184` shape explicitly, states that the sink no longer holds a `LocalPlayerShadowState` reference at all, and — notably — **records that the first version of the P4 fact was vacuous and was corrected at review**. Every claim in it now maps to something I verified: the publish-before-cache ordering, `Register`'s `DeregisterCore`, `Suspend`'s retained registration, the single-instance composition, and four discriminating facts. Nothing in the row claims more than the fix delivers. The one thing it does **not** mention is the M2 guard nuance — worth a clause. --- ## Flake attribution — confirmed **#302**, not diff-caused The reproduced failure is `PortalProjectionTests.ClipToRegion_FrameOwnedStore_ReusesExactResultArray` (`tests/AcDream.App.Tests/Rendering/PortalProjectionTests.cs:503`) — exactly the test `docs/ISSUES.md:1197` files as **#302**: a `GC.GetAllocatedBytesForCurrentThread()` assertion in `AcDream.App.Tests`, JIT-tiering sensitive, measured 1-in-6 in isolation and once under full-suite load. That is the #302 signature, not the load-sensitive `NakEmissionTests` #308 look-alike that `ISSUES.md:1211–1224` warns has been conflated twice. It cannot be diff-caused: the file is untouched (last commit `749e8cee`, zero working-tree diff), no rendering or portal-projection code is in this change set, and the assertion measures thread-local GC bytes in a component this diff does not reach. It passed clean in my own full-suite run (App 4,132 passed / 0 failed). Correctly named and not chased. --- ## Before commit 1. **M1** — extend the B2 test to a different outdoor grid cell (local `(30,30)` → `0x0101000A`) so the cell half is falsifiable, **or** record B2 as position-closed / cell-open in the commit message. Do not book full closure silently. 2. **M2** — add the one-sentence guard nuance to the sink's Place comment and the AP-145 row. 3. Carry forward round 1's commit-message requirements: the §3.1 audit outcome (11 deleted / 0 re-pointed), the §3.3 covering-test judgment, the §3.6 coverage-loss declaration (now correctly worded in the test's xmldoc), and the count reconciliation **11,112 − 11 + 5 = 11,106 / 4 skips**.