acdream/docs/research/2026-08-05-c5a-architecture-review-round2.md

# 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**.