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.