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

C5a — independent architecture / adversarial review

Reviewer: architecture / adversarial (Opus) Scope: the uncommitted working-tree diff at branch claude/acdream-physics-divergence-5aa784, HEAD 392c1e22 (git diff HEAD + the two untracked test files). Contract: docs/research/2026-08-05-c5a-contract.md (input, not under review). Date: 2026-08-05

---

VERDICT: FAIL

The deletion sweep (Part 2) is clean and I would pass it on its own. Every structural claim I could falsify held: the two survivors are byte-identical, the build is 0-warning/0-error, the suite reconciles to the line, the register edits are exactly three rows, and six of the seven test dispositions are executed as pinned (one better than pinned).

The failure is concentrated in Part 1 — the parity tests and the AP-145 retirement's evidence chain:

• A1 — the route-7-P4 test cited by name in the AP-145 retirement row as proof does not discriminate. Removing the behaviour it claims to pin leaves all three of its assertions green.
• A2 — the AP-145 fix closes the Place half of the cache-vs-publish asymmetry and leaves the exact mirror image on the Withdraw half of the same method pair, unfixed and unfiled, with a real collision consequence during a park.
• A3 — §5.2's carried route-2 B2 acceptance gap is not closed. The new test never drives an accepted ForcePosition; it hand-authors the receipt, and the surface it exercises is already covered by an existing test.

Under this campaign's own standard — "the review IS the coverage gate", and four vacuous-test classes already caught this session — A1 alone is disqualifying: a divergence-register retirement must not rest on a test that passes under its own sabotage.

---

Gate evidence I measured myself

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,105 passed / 4 skipped / 0 failed
Reconciliation vs baseline 11,112	11,112 − 11 (PhysicsEngineTests methods deleted) + 4 (3 shadow-composition + 1 force-position) = 11,105 ✓ exact
Skips	3 (App) + 1 (Core) = 4 — same as baseline ✓
Survivors byte-identical	IsSpawnCellReady + AdjustPosition: PhysicsEngine.cs:1797–1839 (new) vs 1807–1849 (HEAD) — diff clean over the whole 45-line span ✓
Survivor production callers intact	RuntimeSetPositionState.cs:2188,:4397; SessionPlayerComposition.cs:374; PhysicsCameraCollisionProbe.cs:38,:100 — all present ✓
Register blast radius	git diff -U0 = 6 changed lines: AD/AP section headers + AD-1, AP-1, AP-145 rows. AP-131, AD-60, AD-61/62, AP-135, AP-141–144, AP-146 untouched ✓
#316-preserving pair	LiveEntityNetworkOnPositionCollapseMatrixTests.cs not in the modified-file set — zero diff ✓
SetPosition → SeedPlacementForTest re-point	83 removals / 83 additions, receivers all controller-typed; zero entity./child./Entity.SetPosition lines touched ✓

---

The composition-graph change — my judgment: CORRECT, and correctly argued

I attacked this first as instructed. It holds.

• Exactly one instance, on every host path. new LocalPlayerShadowSynchronizer( now has one production site in the tree (LivePresentationComposition.cs:508); SessionPlayerComposition.cs:804 consumes live.LocalPlayerShadowSynchronizer. RuntimePlacementPresentationSink has exactly one production construction site, also in LivePresentationComposition.cs:514. No other host constructs either — grep over src/ for LivePresentationCompositionPhase / SessionPlayerCompositionPhase / new RuntimePlacementPresentationSink returns only GameWindow.cs:1342/1395 and that one file. Headless and the no-window Runtime host never touch this sink at all.
• Same arguments before and after. GameWindow.cs:1359 feeds _localPlayerShadow into LivePresentationDependencies.LocalPlayerShadow and GameWindow.cs:1429 feeds the same field into SessionPlayerDependencies.PlayerShadow; _physicsEngine, _liveWorldOrigin and _localPlayerIdentity are likewise the same instances in both records. liveEntities is the same LiveEntityRuntime the old live.LiveEntities read. The relocated construction therefore receives an argument-identical closure.
• Ordering is safe. Construction at :508 precedes the sink at :514; LivePresentationResult has a single construction site (:1209) reached only after :508; the field is non-nullable and the sink's ctor throws on null (RuntimePlacementPresentationSink.cs:60). There is no path to a null or a second instance.
• Lifetime unchanged. Both phases publish into the same GameWindow shell through PublishSessionPlayer, whose "already owns session/player state" guard (GameWindow.cs:1063–1085) proves the two phases are composed as one transaction. Moving construction one phase earlier does not straddle a reset boundary.

One behavioural nuance worth recording (not a defect): routing through SyncPose means the Place edge now inherits SyncPose's guard — if IsHidden, cellId == 0, or !IsCurrentVisibleProjection, the Place suspends the shadow where the old direct write merely cached. That is the correct, symmetric behaviour (it is what the very next per-tick Sync would do anyway) and it is inside the §5.1 pre-authorised production change, so it needs no separate row. It should be stated in the commit message, since it is the one place the fix does more than "also publish".

---

Findings

A1 — MAJOR. The route-7-P4 test does not discriminate; the AP-145 retirement row cites it as proof

File: tests/AcDream.App.Tests/World/RuntimePlacementShadowCompositionTests.cs:264–293 (Place_ForNonLocalPlayerEntity_NeverTouchesShadowObjects)

The test's stated job is to prove that the player-only gate at src/AcDream.App/World/RuntimePlacementPresentationSink.cs:249 (record.ServerGuid == _localPlayerGuid()) is what keeps a committed CHILD from gaining a broadphase row — "This drives that directly rather than arguing it from inspection."

It does not. The fixture never registers the child in ShadowObjects, and ShadowObjectRegistry.UpdatePosition returns immediately when the entity has no registration record:

src/AcDream.Core/Physics/ShadowObjectRegistry.cs:696
    if (!_entityReg.TryGetValue(entityId, out var reg))
        return; // not registered — no-op (callers don't have to gate)

Concrete failure scenario (the sabotage that should fail and doesn't): delete the record.ServerGuid == _localPlayerGuid() gate so every Place calls SyncPose. Trace it: IsHidden(0x7000A101) is false (that guid was never materialised); IsCurrentVisibleProjection(childEntity) resolves the child's own record and returns true; ShadowPositionSynchronizer.Sync → UpdatePosition(childId, …) → the early return above → nothing registered. TotalRegistered is still 0, GetObjectsInCell(DestinationCell) is still empty, entity.Position still equals DestinationPosition. All three assertions pass with the gate removed. (If instead the Suspend branch were taken, ShadowObjects.Suspend on an unregistered id is likewise a no-op — the test passes either way. It is vacuous on both branches.)

This is caught vacuous-class #4: a precondition that made the sabotage irrelevant. It is also caught class #2 in part — the two load-bearing assertions are pure negatives against a registry the fixture guaranteed empty.

Why it matters beyond the test file: the retired AP-145 row (docs/architecture/retail-divergence-register.md:175) lists, among the four things "#318's composition test … proves", "a Place for a non-local-player entity never touches ShadowObjects at all (route 7 P4 …)". A register retirement is now standing on a claim the cited test does not establish.

Fix direction: give the child a real registration first — mirror fact 1's baseline ShadowObjects.Register(entity.Id, …, seedCellId: SourceCell) and Synchronizer.Sync(…, force: true) — then assert after the Place that the child's row is still at SourceCell and absent from DestinationCell. Add Assert.Null(fixture.LocalShadow.Current) so the dedup cache is proven un-polluted too (removing the gate writes the child's pose into the player's cache — a second thing the current test cannot see).

---

A2 — MAJOR. The fix closes Place and leaves the identical asymmetry on Withdraw, unfixed and unfiled

Files: src/AcDream.App/World/RuntimePlacementPresentationSink.cs:307–308 vs src/AcDream.App/World/LiveEntityProjectionWithdrawalController.cs:148,:156

AP-145 was, verbatim, "a plain cache write with no side effect beyond recording Current" on the local-player shadow. The fix routes TryPublishPlace through the real publisher. Six lines further down in the same class, TryPublishWithdrawal still does:

src/AcDream.App/World/RuntimePlacementPresentationSink.cs:307
        if (record.ServerGuid == _localPlayerGuid())
            _localPlayerShadow.Clear();

— a bare cache clear with no ShadowObjects.Suspend. The correct pairing exists elsewhere in the same subsystem and shows what the sink is missing:

src/AcDream.App/World/LiveEntityProjectionWithdrawalController.cs:148,156
        if (!retainedProjectileShadow)
            _shadows.Suspend(entity.Id);        // registry
        ...
            _localPlayerShadow.Clear();          // cache

Concrete failure scenario: a local-player park (Withdraw) — the path TryApplyWithdrawalRestoration's own xmldoc (:202) names as touching "the local-player shadow". The cache says "no shadow"; the registry still carries a live row for the player at the park's source cell. For the whole park window every other entity's collision sweep in that cell collides with a phantom player, and nothing self-heals, because a withdrawn player receives no per-tick Sync. Restoration papers over it (TryPublishPlace → SyncPose force-republishes), so the symptom is a transient phantom obstruction during a park — exactly the "why not observed live" shape AP-145 itself carried.

This is pre-existing, not introduced by C5a. But (a) register rule 1 makes an unrecorded deviation "a bug twice over", (b) this diff is the commit that retires AP-145 and its retirement text asserts the seam is now symmetric with ordinary per-tick movement, and (c) it is six lines from the line being fixed — this is precisely the review's job to catch.

Fix direction: either route the withdrawal through _localPlayerShadowSync.Suspend(entity) (a production behaviour change → its own commit with its own gate, per the no-workarounds rule), or file a new AP row / issue in this same commit recording the Withdraw-half asymmetry and its "risk if the assumption breaks" column, and narrow AP-145's retirement text to the Place edge it actually covers.

---

A3 — MAJOR. §5.2's route-2 B2 acceptance gap is not closed; the test largely duplicates existing coverage

File: tests/AcDream.App.Tests/World/RuntimeForcePositionRenderCommitTests.cs:57–105

The contract's §5.2 deliverable: "an App-layer test driving an accepted ForcePosition end to end through RuntimePlacementPresentationSink / TryApplyRuntimePlacementPlace and asserting the render entity's position/cell came from the committed placement receipt." B2's original finding is about a ForcePosition producing a receipt that the render entity then follows.

What landed does not drive a ForcePosition at all. It hand-authors a RuntimePlacementProjectionSnapshot (:107–133) and calls Sink.TryApply. The test's own xmldoc concedes it: "rather than driving the full RuntimeAcceptedPositionDriveController pipeline." The receipt's contents are therefore the test's assumption, not the ForcePosition path's output — the half of B2 that could actually be wrong ("canonical body moves, render entity stays put") is asserted by narrative.

Worse, the surface it does exercise is already pinned at HEAD:

tests/AcDream.App.Tests/World/RuntimePlacementPresentationSinkTests.cs:29
    Place_ReframesAndRebucketsExactSidecarWithoutMutatingRuntimePhysics
      :57  Assert.Equal(place.WorldPosition, entity.Position);
      :58  Assert.Equal(place.Orientation,   entity.Rotation);
      :59  Assert.Equal(DestinationCell,     entity.ParentCellId);
      :60  Assert.True(record.IsSpatiallyProjected);
      :61  Assert.True(record.IsSpatiallyVisible);

Those are the same five facts the new test asserts (:90–104). The only deltas are a stale-wire-pose pre-state and Portal: default. That is a real but small increment; it is not the recorded gap.

Fix direction: drive RuntimeAcceptedPositionDriveController .TryExecuteAcceptedLocalPosition (or the accepted-ForcePosition entry the route-2 landing added) so the receipt is produced by the path under test, then assert the render entity against the emitted receipt. If that fixture cost is judged disproportionate, then B2 must be recorded as still unmet in the plan and the commit message, not marked closed — a partial closure silently booked as full is how an acceptance gap disappears.

---

A4 — MINOR (one line is borderline MAJOR). Stale citations of the deleted PhysicsEngine.Resolve survive the D7 sweep

All four are plain <c>/comment text, so the 0-warning build cannot catch them:

File:line	Text	Why it's wrong now
src/AcDream.Runtime/Gameplay/PlayerMovementController.cs:145	"PhysicsEngine.Resolve is still used each frame to snap the player to terrain/cell floor Z and detect ground contact."	Class-summary architecture note asserting a per-frame call to a method that no longer exists. First thing a reader of the local-movement controller sees.
src/AcDream.Core/Physics/CellTransit.cs:878	"…mirrors the NO-LANDBLOCK contract in PhysicsEngine.Resolve."	Cites a deleted contract as the authority for a live early-return.
src/AcDream.Core/Physics/CellTransit.cs:1059	"handled at the SNAP by PhysicsEngine.Resolve's AdjustPosition validation since #107/#111"	The snap path is gone; AdjustPosition survives but is now reached only from PhysicsCameraCollisionProbe.
src/AcDream.Headless/Hosting/HeadlessSessionWorldProjection.cs:794	"retiring the duplicate Resolve/ResolvePlacement/SetPosition authority"	The contract acknowledged this mention; the retirement it forecasts has now happened, so the comment should close, not linger as a to-do.

The contract's D7 enumerated only three doc targets and scoped the cref sweep to PhysicsEngine.cs, so this is strictly beyond-contract — but the campaign's "cite by symbol, these move" discipline exists for exactly this, and PlayerMovementController.cs:145 is materially misleading.

Fix direction: rewrite :145 to name ResolveWithTransition (the real per-frame resolver) and correct the two CellTransit notes to cite PhysicsEngine.SetPosition / AdjustPosition by symbol.

---

A5 — MINOR. A factual error in the rewritten CommitPreparedPosition test's xmldoc

File: tests/AcDream.App.Tests/Input/PlayerMovementPlacementTransactionTests.cs:24–28

"The render-root-publish-on-commit and sticky-target-release-on-commit invariants this test originally pinned now live INSIDE that Runtime final-commit transaction."

Half true. Render-root publish does live there — RuntimeSetPositionState.cs:2774 _physics.Engine.UpdatePlayerCurrCell(result.CellId) inside the dormant-activation final commit. Verified.

The sticky release does not. grep -rn "UnStick" src/ returns zero call sites in src/AcDream.Runtime/ on that path; the only local-player UnStick is PlayerMovementController.cs:1892, inside SetPositionCore and gated on if (publishSharedState) — and PreparePositionForCommit passes publishSharedState: false. So the unstick-at-first-entry-commit behaviour was not relocated; it went away with the (already caller-free, therefore already dead) CommitPreparedPosition. That is harmless — it was never running in production — but the comment states a relocation that did not happen, and a future reader chasing "where does first-entry unstick happen now?" will be sent to a method that does not do it.

Fix direction: say plainly that the sticky release had no production caller and is not performed at first-entry commit today; cite RuntimeSetPositionState's UpdatePlayerCurrCell by symbol for the half that did move.

The rewritten test itself is good: Assert.Null(Constraint) before / Assert.NotNull(Constraint) + IsConstrained after makes it discriminating, and the two retained negatives are correctly framed as "this layer does not do this" rather than as the pin.

---

A6 — MINOR. The re-pointed scratch differential never asserts a placement actually happened

File: tests/AcDream.Core.Tests/Physics/TransitionScratchDifferentialTests.cs:474–520 (AssertSetPositionBitwise), used by ReusedScratch_MatchesFreshPlacementSearch

The differential compares expected against actual field-by-field, but there is no positive assertion that either result committed (Assert.True(expected.IsCommitted), or Assert.NotEqual(input, expected.Position) as InitialPlacementOverlapTests does). A future regression that makes canonical SetPosition fail identically on the fresh and reused engines leaves the differential green while the scratch-reuse surface it guards goes unexercised.

The pre-deletion ResolvePlacement arm had the same weakness (it compared Ok rather than asserting it), so this is not a regression introduced here — but the re-point was the moment to close it, and the new AssertSetPositionBitwise is otherwise excellent (the ImmutableArray.Equals reference-comparison note at :501–505 is the kind of thing that would have produced a false-fail).

---

Dispositions — did any of the seven lose coverage?

#	Contract disposition	Executed	Coverage verdict
3.1	PhysicsEngineTests — audit, then delete	11 methods deleted, 0 re-pointed, 6 ResolveWithTransition methods retained	No loss. I re-audited all 11 against HEAD: none touches AdjustPosition or IsSpawnCellReady; Resolve_ZeroDeltaSnapTrace_IsExplicitlyOptIn pinned the [snap] diagnostic emitted inside the deleted body; the rest pinned legacy floor-snap / step-height / portal-transition semantics that die with the method. PhysicsEngineAdjustPositionTests (3 tests: sibling-resolve, no-cell, outdoor-snap) covers the survivor. The §3.1 audit outcome must still appear in the commit message.
3.2	Issue133… — re-point (named-bug pin)	Re-pointed to canonical PhysicsEngine.SetPosition with the exact #133 geometry (dungeon claim 0x00070143, dungeon block at world-Y 130 → local Y −60, resident Holtburg neighbour at origin)	No loss. Asserts result.CellId == 0x00070143 and CellId & 0xFFFF0000 == 0x00070000 on a committed result. If anyone reintroduced an lbPrefix resident-block scan into the canonical path, this fails. Best available pin for a defect whose mechanism no longer exists.
3.3	InitialPlacementOverlapTests — verify-then-delete-or-re-point	Re-pointed	Judgment correct, verified independently. grep -c "ShadowObjects.Register" tests/…/PhysicsSetPositionTests.cs = 0 — that suite has zero other-entity occupancy; its placementPasses >= 2 arm (:1152–1194) is BSP-hook-injection driven, exactly as the implementer said. The re-point is discriminating (Assert.NotEqual(savedFeet, result.Position) + the centre-distance ≥ 2r check + the 4 m bound). The two-sphere capsule reconstruction matches the legacy scalar InitPath(0.48, 1.835) shape.
3.4	TransitionScratchDifferentialTests — re-point or drop explicitly	Re-pointed with AssertSetPositionBitwise	Preserved. Both the bitwise fresh-vs-reused comparison and the second-identity (0x80000102, hostile) leak check survive; the new asserter covers every PhysicsSetPositionResult member including the three id arrays. See A6 for the one gap.
3.5	SetPosition → SeedPlacementForTest	83 sites / 19 files	No loss, no meaning change. 83 removals ↔ 83 additions; the 2-arg → 3-arg conversions all pass pos as cellLocal, byte-for-byte what the deleted 2-arg overload did; the seed calls the same production SetPositionCore, so the AD-61 grounded/zero-velocity start is unchanged. Zero entity./child./Entity.SetPosition (WorldEntity) lines touched — verified by regex over the whole test diff. Both Assert.Throws<InvalidOperationException> guard sites re-pointed, preserving EnsurePublishedForRuntimeOperation coverage.
3.6	CommitPreparedPosition — re-point at the replacement	1 rewrite + 2 throw-site re-points	Partial, honestly declared. The two throw sites re-point cleanly onto ArmConstraintLeashAtCommittedPlacement, which carries the same guard. The rewrite is discriminating. Coverage genuinely lost: render-root-publish-on-commit and sticky-release-on-commit are no longer pinned at any layer — the former does exist in Runtime (RuntimeSetPositionState.cs:2774) but is not asserted by the re-point; the latter does not exist at all (see A5). The contract permitted this only with a commit-message note; the note is in the xmldoc and is half wrong.
3.7	Begin* wrappers — keep as documented seam	Kept, xmldoc added to both (RuntimeSetPositionState.cs:1321,:1343)	Correct, as pinned.

Summary: one disposition (3.6) lost real coverage, declared but mis-described. The other six are clean. The silent-coverage-loss risk the slice was designed around did not materialise in the deletion sweep — it materialised in the new tests (A1, A3).

---

Register retirements — verified

• AP-1 — retire: justified. I re-ran the census: zero PhysicsEngine.Resolve / .ResolvePlacement receivers in src/; the only _physics.Engine.SetPosition sites are in RuntimeSetPositionState. After D1–D3 the resolver-shaped entry points do not exist, so the row's condition is structurally unreopenable. Correct.
• AD-1 — retire: justified. The recoverable outdoor demote and the outdoor-restore max(terrainZ, z) lift were Resolve's body; the body is gone. Correct.
• AP-145 — retire: justified in mechanism, overstated in evidence. The mechanism claim ("SyncPose publishes before it records the cache, so the cache can no longer be pre-seeded ahead of the real publish") is true and I verified the Register → DeregisterCore ordering (ShadowObjectRegistry.cs:403) that backs the no-stale-source-row claim. The row's fourth proof bullet (route-7 P4) rests on the vacuous test — see A1; and the row's framing implies a symmetry the Withdraw half does not have — see A2.
• Untouchable set held. AP-131, AD-60, AD-61, AD-62, AP-135, AP-141–144, AP-146 — all unmodified. Section counts updated correctly (AD 48→47, AP 103→101 for two retirements). Strikethrough-plus-RETIRED matches the file's established convention (31 existing | ~~…~~ rows).
• #275 surface untouched. InboundPhysicsStateController and RuntimeEntityObjectLifetime are not in the modified-file set. ✓

---

Vacuous-test hunt — all four classes, result

Class	Hunted in	Result
Asserting a field written unconditionally earlier	all new/changed assertions	Clean. The shadow-composition tests assert ShadowObjects rows, which only SyncPose writes; the B2 test's entity.Position is written only by TryApplyRuntimePlacementProjection.
Asserting only negatives	Place_ForNonLocalPlayerEntity_…, the rewritten transaction test	HIT on Place_ForNonLocalPlayerEntity_… (two of three assertions are negatives against a guaranteed-empty registry). The transaction test is clean — it pairs its negatives with a positive (Constraint non-null) and a precondition (Constraint null before).
Fixture staging makes the wrong expression compute the right answer	the destination-cell flood geometry (DestinationPosition = (202,10,5) with worldOffsetX: 192), the two-sphere capsule reconstructions	Clean. The flood-lands-under-DestinationCell construction is load-bearing and documented at :80–84; if it were wrong the discriminating assertion would fail, not falsely pass.
A precondition that made the sabotage irrelevant	all three shadow-composition facts	HIT on Place_ForNonLocalPlayerEntity_… — the child is never registered, so UpdatePosition's not-registered early return makes the gate's presence unobservable. Facts 1 and 2 are clean: both establish a real source-cell registration first, which is exactly what makes the "row moved to destination / source row gone" assertions bite.

Facts 1 and 2 of RuntimePlacementShadowCompositionTests are genuinely good discriminators, and the negative-control comment at :159–165 (naming the cache assertion as the shape that would have passed under the bug) is exactly the right way to document a sabotage argument. The problem is confined to the third fact.

---

What must happen before a re-review

1. A1 — make Place_ForNonLocalPlayerEntity_NeverTouchesShadowObjects discriminate (register the child first; assert its row stays at SourceCell; assert the player's cache is not polluted). Re-verify by sabotage: remove the _localPlayerGuid() gate and confirm the new assertion is the one that fails.
2. A2 — either fix the Withdraw half as its own reviewed commit, or file the deviation as a register row in this commit and narrow AP-145's retirement text to the Place edge.
3. A3 — drive an actual accepted ForcePosition, or record B2 as still open.
4. A4/A5 — correct the four stale PhysicsEngine.Resolve citations and the sticky-release claim in the transaction test's xmldoc.
5. A6 — add the "a placement actually committed" positive to the differential.
6. Commit message must carry: the §3.1 audit outcome (11 deleted / 0 re-pointed, with the reason), the §3.3 covering-test judgment, the §3.6 coverage-loss declaration, the count reconciliation (11,112 − 11 + 4 = 11,105 / 4 skips), and the SyncPose suspend-guard nuance noted in the composition section above.