acdream/docs/research/2026-08-04-c4-route-4b-3-architecture-review.md

# C4 route 4b-3 — architecture / adversarial review (2026-08-04)

Reviewer lens: **adversary**. Retail conformance is a separate reviewer's
scope; this review asks only "does this break something, leak something,
deadlock something, or leave an entity in a bad state?"

Subject: the uncommitted working tree on `claude/acdream-physics-divergence-5aa784`
at base `3e002993` (`git diff HEAD` plus the three untracked non-contract
files). Contract:
[`2026-08-04-c4-route-4b-3-contract.md`](2026-08-04-c4-route-4b-3-contract.md).

## Verdict: **FAIL**

Two MAJOR findings. Both are behaviour REGRESSIONS against `3e002993` — not
pre-existing residuals — and both sit on code paths the contract's own
invariant list names as load-bearing (invariant 5, the arm-count partition;
invariant 6 / D3, "the hook's whole point is that no locomotion state
survives a teleport"). Both are invisible to the specified two-client
connected gate, because that gate teleports a *player* character and both
defects are on the NPC/creature arm.

Everything else I checked held. Build is green (0 warnings); focused suites
pass **Runtime 1,125 / 0 skips** and **App 4,078 / 3 skips** at Release with
`--no-build`.

---

## What I verified and found sound (do not churn)

- **Invariant 1 (`StoresAcceptedDestination`).** `ApplyAcceptedRemoteTeleport`
  (`src/AcDream.Runtime/Session/RuntimeRemotePlacementDriveController.cs:820-848`)
  is byte-for-byte the far arm's shape minus the `StopInterpolating` clear,
  including the currency-guarded `StoreAcceptedDestinationPose`. The partition
  is extended, not re-litigated.
- **Invariant 3 (no stranded entity).** The teleport arm reaches
  `SubmitAndResolve` through the *same* `TryExecuteAcceptedRemotePosition`
  the far arm uses; the `DeferredCell` arm still cancels synchronously with
  `restoreCancelledPark: true`. No new park shape, no new withdrawal site.
- **Invariant 10.** `TickLostCellDeadlines` / `TryDequeueExpiredLostCell` /
  `ArmLostFamilyDeadlines` still have zero production callers (4 references,
  all inside `RuntimeSetPositionState.cs`; the rest are tests). The reaper
  stays inert.
- **Invariant 11 (route 1 unchanged).** The shared builder got an *overload*,
  not a third copy, and the route-1 overload forwards
  `committedCellId: canonical.FullCellId` — its pre-slice value verbatim
  (`RuntimeAcceptedPositionRouteRequests.cs:41-56`).
- **D1's plumbing is honest.** `PreMergeCommittedCellId = hadCanonical ? beforeCell : null`
  is measured before `RefreshSnapshot`
  (`RuntimeEntityObjectLifetime.cs:1725-1770`), never re-read, and `null`
  ("no prior canonical record") makes `TryBuild` decline rather than fabricate
  a 0. `RuntimeRemoteTeleportClassificationTests` genuinely discriminates the
  fix from the shipped dead predicate — the companion test fails under the
  post-merge read. This is the strongest test in the diff.
- **The `_activePlacementOwners` deletion is legitimate** — see item 1 below.
- **Shadow suspend/restore across the hook.** `RemoteTeleportHook`'s
  `ReportCollisionEnd` → `ShadowObjects.Suspend` removes the entity from
  `_entityToCells`; the arm tails' `LiveEntityShadowPublisher.TryPublishRemote`
  → `ShadowPositionSynchronizer.Sync` → `RefreshPositionRows` un-suspends it
  (`ShadowObjectRegistry.cs:1336-1393`). I specifically checked for a
  pose-gate that could skip the restore on a zero-distance teleport: the
  OnPosition-tail overload is **ungated** (`RuntimeRemotePhysicsUpdater.cs:1175-1191`);
  only the per-tick loop gates. `TeleportRefused_…`'s
  `Assert.Single(fixture.Shadows.AllEntriesForDebug(), …)` is a real proof of
  this, because `AllEntriesForDebug` enumerates `_entityToCells`.
  **No invisible-and-intangible entity on this path.**
- **Deletion completeness.** All eight wiring sites in the contract's
  inventory are cut, including the two the handoff missed
  (`SessionPlayerComposition`, `LiveSessionResetManifest`). Reset coverage is
  genuinely preserved: `GraphicalSessionEventRoute.cs:152` calls
  `_remotePlacementDrive.DetachRoute`.
- **Register bookkeeping.** AD-42, AP-136, AP-137, AP-138 all updated in the
  diff; AP-138's Risk column does gain the teleport arm as a second producer,
  and AP-137 is rewritten rather than deleted. (One factual defect in the
  AP-137 rewrite — see A7.)

---

## MAJOR findings

### A1 (MAJOR) — the NPC arm silently stops arming `ConstrainTo` for every airborne-body packet; the code documents the opposite

**File:** `src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs:1290-1311`
(`ToConstraintArm`), consumed at `:2643`.

`ToConstraintArm` maps `RemoteContactArm.AirborneSnap` through its `_` arm to
`RuntimeRemoteAcceptedPositionArm.AirborneNoOperation` — the one value
`TryArmConstraintAfterOperation` never arms — and justifies it at `:1296-1297`
with:

> *"There is no Runtime analogue of `AirborneSnap` (a caller reaching it
> always returns before any arming call) … production never calls this with
> that arm."*

**That claim is false for the NPC arm.** After
`npcRouting = ApplyRemoteContactRouting(…)` at `:2600`, the only early return
is the currency guard at `:2619-2626`, which is gated on
`FarSnapPlacement or TeleportPlacement`. Control then falls out of the
`if (!snapSuppressedByStick || isTeleportRoute)` block straight into
`TryArmConstraintAfterOperation(ToConstraintArm(npcRouting.Arm), rmState)` at
`:2641-2644`. `AirborneSnap` is returned by
`ApplyRemoteContactRouting`'s free-flight carve-out at `:1123-1150` for **any**
non-teleport classification whenever `!remote.Body.InContact`.

**Concrete failure scenario.** A creature remote (guid not `0x50xxxxxx`) is
knocked off a ledge or jumps, so its canonical body has no contact plane
(`Body.InContact == false`). ACE broadcasts a wire-grounded `UpdatePosition`
(`IsGrounded: true`) at 5-10 Hz. The classifier returns `Interpolate`
(`playerDistance < 96 m`). Routing takes the airborne carve-out →
`AirborneSnap` → `ToConstraintArm` → `AirborneNoOperation` → **no arm**.

At `3e002993` the same packet armed: the post-operation site was passed the
ROUTE (`earlyRemoteRoute`), and `OwnsAfterOperationConstraint(Interpolate)` was
true with `ConstrainAfterRouting` true. So the arm count for this row goes
**1 → 0**, which is the "never zero times" half of contract invariant 5 and a
`yes` row of D4's partition table turning into a `no`. Retail's near branch
returns 1 @0x005163BE, so `HandleReceivedPosition` arms unconditionally
@0x00454272 — the divergence is in the wrong direction. The same 1→0 applies
to a far (`SetPositionSimple`) classification and to a null/`Rejected*`
classification whenever the NPC's body is out of contact.

Observable consequence: a creature that is knocked airborne on the **first**
accepted Position after spawn never sets `IsConstrained` at all, so the
`ConstraintManager` brake and `IsFullyConstrained()` (which gates
`jump_is_allowed`) stay dead for it; a creature already armed loses the
per-packet re-anchor (`ConstraintPosOffset` reset to 0) for the whole airborne
interval.

**Why no test caught it.** Proof obligation 3 asked for "a test that counts
arming calls per packet across the partition table's rows". What was delivered
(`RuntimeRemoteSteadyStatePositionTests.TryArmConstraintAfterOperation_MatchesTheCompletePartition`)
tests the Runtime *predicate* given an already-chosen arm value. Nothing tests
`ToConstraintArm`, and nothing counts arms per packet through `OnPosition`.
The mapping — the only new code in the arming path — is 100 % uncovered.

**Fix direction.** `AirborneSnap` is not `AirborneNoOperation`: retail's
airborne *no-op* is `arg4 == 0` (the WIRE contact bit, return 0), while
`AirborneSnap` is acdream's carve-out keyed on the BODY's contact for a packet
whose wire bit said grounded — retail returns nonzero for that packet and
arms. Either map `AirborneSnap` to an arming value, or (better) make
`ToConstraintArm` total with an explicit `AirborneSnap` case plus a
`_ => throw`, and add the missing end-to-end arm-count test across the D4 rows
including the body-airborne ones.

---

### A2 (MAJOR) — a teleported NPC gets a ~1,000 m/s synthesized `ServerVelocity` and an animation cycle planned from it

**File:** `src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs:2526-2543`
(synthesis) and `:2683-2709` (consumption).

The NPC arm synthesizes a locomotion velocity for any packet that carries no
wire velocity:

```
serverVelocity = (worldPos - rmState.LastServerPos) / (float)elapsed;   // :2532
… rmState.ServerVelocity = authoritativeVelocity; HasServerVelocity = true;
```

`rmState.LastServerPos` is only advanced at `:2681`, i.e. **after** routing, so
at `:2532` it still holds the pre-teleport position. For an admin teleport the
delta is the whole teleport distance over one packet interval — e.g. 192 m /
0.15 s ≈ 1,280 m/s. The tail then calls
`RemoteServerControlledVelocityCycle.Apply(update.Guid, ae, rmState, rmState.ServerVelocity)`
at `:2704-2709`, which runs `ServerControlledLocomotion.PlanFromVelocity` and
`Sequencer.SetCycle(style, plan.Motion, plan.SpeedMod)`.

At `3e002993` this was unreachable for a teleport: the `remotePlacementRequired`
block sat in the SHARED section, **above** the `IsPlayerGuid` split and above
the NPC synth-velocity code, and it always `return`ed. The diff deleted that
block and routed the teleport through the NPC tail instead, so the synthesis
now runs on every NPC teleport.

**Concrete failure scenario.** `@teleto` a drudge 200 m away while an acdream
client observes it. The teleport arm places it correctly, then the same packet
installs `ServerVelocity ≈ 1.3 km/s` and calls `SetCycle(Run, speedMod≈huge)`.
`RemoteServerControlledVelocityCycle.Apply`'s three guards do not help: the
body is grounded after the commit (`rm.Airborne == false`), the guid is not a
player guid, and `rm.MoveTo` was just set to `Invalid` **by the teleport hook's
own `CancelMoveTo`** — so the hook actively removes the last thing that would
have suppressed this. The creature stands at the destination playing a
run/sprint cycle at an absurd speed multiplier until the stale-velocity
watchdog fires (`ServerControlledVelocityStaleSeconds = 0.60`) or the next
packet's small delta replaces it.

This is directly contrary to `teleport_hook`'s purpose (@0x00514ED0 exists so
that *no* locomotion state survives a teleport) and is exactly the
"stands with correct animation" clause of the contract's live gate — which
cannot see it, because the gate teleports a player character and
`RemoteServerControlledVelocityCycle.Apply` early-returns for `0x50xxxxxx`
guids. The player-remote arm is accidentally immune because its own teleport
block returns at `:2185`, before the player synth-velocity at `:2432-2443`.

**Fix direction.** The teleport arm must suppress the synthesized velocity for
its own packet on the NPC arm too — either by advancing
`rmState.LastServerPos` to the destination before the synthesis, or (cleaner,
and symmetric with the player arm) by giving the NPC teleport its own tail
that skips the synth/cycle step. Whichever is chosen, add a test that asserts
the NPC sequencer's cycle is unchanged across a teleport packet; today nothing
in the tree looks at the animation layer for this arm.

---

## MINOR findings

### A3 (MINOR) — the stress test's teleport step now drives zero production code

**File:** `tests/AcDream.App.Tests/World/LiveEntityLifecycleStressTests.cs:710-730`.

The contract's deletion inventory says `LiveEntityLifecycleStressTests`
"constructs the controller and calls `TryApply` — its scenario must be
re-expressed against the canonical teleport arm, not dropped."
`BeginDeferredTeleport` is now four hand-written field assignments
(`_remote.Body.Position = destination; _remote.CellId = …;
Entity.SetPosition(…); Entity.ParentCellId = …`). It exercises no placement,
no hook, no routing. `RepeatedRetailRecallMotion_HiddenTeleportUnhide_…`
(`:252-296`) would pass identically if the entire teleport arm were deleted.
The Hidden/DeferredShadowRestore half of the scenario still discriminates, so
this is a coverage loss rather than a false pass — but the mid-teleport
Hidden/UnHide interaction the fixture exists for is no longer tested against
the mechanism that now performs the teleport.

### A4 (MINOR) — `TeleportRefused_…` does not discriminate the teleport arm from `UnroutedCatchUp`

**File:** `tests/AcDream.App.Tests/Physics/LiveEntityNetworkRemoteTeleportPresentationTests.cs:157-206`.

Every assertion in this test (`body.Position == destination + offset`,
`Entity.Position == body.Position`, `IsSpatiallyVisible`, one shadow entry at
`body.Position`) is also satisfied if the packet had classified `null` and
taken `UnroutedCatchUp` → `ApplyInterpolate`, because the body-to-target
distance is >192 m so AP-87's `bodyToTarget > 4 m` branch hard-places the body
at exactly the same wire pose and the same NPC tail then syncs entity and
shadow. The test *does* catch removal of the `store_position` fallback, so it
is not worthless, but the sibling commit test is the only one whose expected
value (`+ FootSphereCenterLift`) can only come from a canonical placement.
Adding one `Assert.Equal(0u, …)`-style discriminator (or asserting the hook
ran, as the routing-seam tests do) would close it.

### A5 (MINOR) — the landing block's new comment states something false

**File:** `src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs:2266-2272`.

> *"A landing packet always classifies Interpolate (the teleport/cell-less arm
> dispatches earlier and returns before this block can be reached — D5)."*

The teleport half is true; "always classifies Interpolate" is not. The landing
block is reached for a wire-grounded, body-out-of-contact packet with ANY
non-teleport, non-airborne-no-op classification — including `SetPositionSimple`
(>= 96 m), `null` (login window), `RejectedAuthority`, and `RejectedData`. The
hard-coded `RuntimeRemoteAcceptedPositionArm.NearInterpolate` at `:2273` still
produces the correct arm count for all of those (every one of them is an
"arms" row), so this is a comment defect, not a behaviour defect — but process
rule 6 is explicit, and this is exactly the shape ("a comment asserting
behaviour the code no longer has") that six consecutive slices have shipped.

### A6 (MINOR) — new per-packet allocations on the remote hot path

**File:** `src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs:2148-2153`,
`:2370-2374`, `:2609-2613`, and `:1080-1088`.

`runTeleportHook: () => RunRemoteTeleportHook(update.Guid, entity.Id, () => IsCurrentPositionOwner(entity))`
captures `update`, `entity` and `this`, so a display-class + delegate pair is
allocated for **every** remote accepted Position, not only for teleports (the
pre-slice tree allocated the inner lambda only when `remoteHardTeleport` was
true). Separately, `teleportStatus.ToString()` at `:1087` is evaluated at the
call site, so the probe's own `ProbeRemoteTeleportEnabled` self-guard does not
prevent the string allocation. Given Slice I's "0 B/resolve" discipline this
is worth a `Func<bool>` cached per controller, or hoisting the probe behind
`if (PhysicsDiagnostics.ProbeRemoteTeleportEnabled)` at the call site.

### A7 (MINOR) — D2's "unifies the player and NPC arms" is not delivered, and AP-137 now claims it was

**Files:** `src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs:2186-2210`;
`docs/architecture/retail-divergence-register.md` (AP-137 row, the "**D2 — the
wire-airborne leftover shape**" sentence).

The retail return-0 shape is implemented only inside the `IsPlayerGuid` block.
On the NPC arm a wire-airborne packet with a null/`Rejected*` classification
still falls through the synth-velocity block into
`ApplyRemoteContactRouting`, which either hard-snaps the body (`AirborneSnap`,
if the body is also out of contact) or **enqueues/places** it
(`UnroutedCatchUp` → `ApplyInterpolate`, if the body is in contact) and then
syncs the render entity and publishes the collision shadow. That is not
"AP-135's two bookkeeping writes only, no body/queue/render write, no leash
arm, return". The register row's claim that this "unifies player and NPC
remotes on one behaviour" is therefore factually wrong as shipped, which is a
register-rule-1 problem in its own right. (This is NOT a regression against
`3e002993` — the NPC arm behaved this way before — so the fix can legitimately
be "scope the row to the player arm" rather than "implement the NPC half".)

### A8 (MINOR) — dead local

`src/AcDream.Runtime/Entities/RuntimeEntityObjectLifetime.cs:1727`:
`bool wasCellless = hadCanonical && beforeCell == 0u;` has no remaining reader
(only the comment at `:1759` mentions it). No compiler warning fires because
the initializer is not a constant. Delete it or the comment will outlive the
variable's meaning.

### A9 (MINOR) — proof obligation 1 is not stated anywhere in the tree

The contract requires the `ParkCollisionResidents`-overlap-throw
unreachability argument (with 4b-1's B2 caveat about
`HasOldPrefixPlacementDebt` being a stall rather than a throw) to be stated
"in the contract-conformance section of the implementation commit". The work
is uncommitted, so I cannot verify it; nothing in the diff carries the
statement. The underlying property does hold by reading — the teleport arm
adds packets to the same one-operation-per-key machinery and opens no new
operation shape — but the statement is still owed.

### A10 (MINOR, partly unverified) — the visibility-edge protection `_activePlacementOwners` provided has no replacement

I independently traced the deletion the implementer reported (item 1 of the
task). **Their conclusion is right, their stated reasoning is not.**
`_activePlacementOwners` was NOT write-never at `3e002993`: the writer chain
was `LiveEntityNetworkUpdateController` (`remotePlacementRequired` →
`_remoteTeleportController.BeginPlacement`) → `RemoteTeleportController.BeginPlacement`
→ `RemoteTeleportPlacementPresentation.Begin` →
`LiveEntityPresentationController.BeginAuthoritativePlacement`
(`RemoteShadowPlacementSynchronizer.cs:48` at HEAD). It becomes write-never
*because this slice deletes that chain*, which is exactly what the contract's
deletion-inventory row said. `git grep` over `src` + `tests` at HEAD confirms
no other production writer. So deleting the set and its four `IsPlacementActive`
gates changes nothing for any surviving path, and the four gates degenerating
to "always act" is correct.

What is not replaced is the *protection*: during the old multi-frame pending
placement, an intervening Hidden/UnHide edge was prevented from restoring a
shadow at an unresolved pose. The new teleport is synchronous inside
`OnPosition`, which mostly removes the window — but the canonical placement
publishes its receipt synchronously and the projection sink can raise a
visibility edge from inside it. I could not construct a reachable
Hidden/UnHide-during-placement path in the new design and am flagging this as
**unverified**, not as a defect.

---

## Judgment on the three reported items

**1. `_activePlacementOwners` deletion — sound, reasoning wrong.** See A10.
The set had a real production writer at HEAD; it is the slice's own deletions
that kill it. The four consumers' degeneration is behaviour-neutral for every
surviving path. Not a defect. The commit message should say "its only writer
is deleted by this commit", not "the predicate was permanently false".

**2. The two parallel `OnPosition` copies — a genuine structural hazard, and
it has already produced a defect.** This is not a stylistic complaint: A2 is
precisely the two copies disagreeing. The player copy's teleport block returns
at `:2185` before its synth-velocity at `:2432`; the NPC copy has no teleport
block at all and falls through its synth-velocity at `:2526` on the way to
routing. A1 is the second instance — the player copy cannot produce
`AirborneSnap` (its landing block returns first) while the NPC copy can, so
the shared `ToConstraintArm` was written against the player copy's reality and
is wrong for the NPC one. The implementer's own report that "a sabotage of the
wrong copy left both tests green" is the same signal. The duplication should
be collapsed, or at minimum every arm tail should be one shared helper that
both branches call; shipping a third slice against these two copies without
that is how the next one of these lands.

**3. Declining D7 (consolidating the post-placement currency guard into a
Runtime seam) — acceptable, and not the source of either MAJOR.** The guard is
now duplicated at three App call sites (`:2155-2161`, `:2393-2399`,
`:2619-2626`) instead of two. I checked all three: the predicate is
character-identical (`!IsCurrentPositionOwner(entity) || !ReferenceEquals(positionRecord.RemoteMotionRuntime, rmState)`),
and all three sit BEFORE their arming call, preserving the R5 invariant. The
widened arm predicate (`FarSnapPlacement or TeleportPlacement`) is applied
consistently at the two that need it; the teleport-specific site is
unconditional, which is strictly stronger. So the duplication is not itself a
correctness hazard today. It is, however, the same duplication-of-invariant
pattern as item 2, and a fourth copy (route 5's projectile arm) is next in the
queue — consolidating it is now overdue rather than optional.

---

## Gate status observed

- `dotnet build AcDream.slnx -c Release` → **succeeded, 0 warnings, 0 errors**.
- `dotnet test tests/AcDream.Runtime.Tests -c Release --no-build` →
  **1,125 passed / 0 skipped / 0 failed**.
- `dotnet test tests/AcDream.App.Tests -c Release --no-build` →
  **4,078 passed / 3 skipped / 0 failed**.
- Complete-solution Release suite: **not run by this reviewer** (the contract's
  own gate; the new figure still has to be measured and recorded, not
  inherited from 11,027).
- Two-client connected teleport gate: **not run**, and note the finding above
  that neither MAJOR is observable through it as specified — A1 and A2 both
  need a *creature* teleport (`@teleto` a drudge/mosswart into view), not a
  second player character.