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

C4 route 4b-3 — retail-conformance review (2026-08-04)

Reviewer lens: does this diff do what the retail client does? Architecture, style, and layering are a separate reviewer's lane.

Subject: the uncommitted working tree on claude/acdream-physics-divergence-5aa784 at HEAD 3e002993 (git diff HEAD + four untracked files; the contract docs/research/2026-08-04-c4-route-4b-3-contract.md is itself untracked and is not part of the change under review).

Build: dotnet build AcDream.slnx -c Release — green, 0 warnings. Focused check: AcDream.Runtime.Tests --filter Teleport — 29/29 pass. Per process rule 3, neither is treated as evidence of conformance.

---

VERDICT: FAIL

Three MAJOR findings. R1 and R2 are contract requirements that were pinned and not met, and R1 additionally ships a register row asserting behaviour the code does not have — the exact defect class process rule 6 exists to stop. R3 is a newly-reachable retail divergence whose symptom is precisely what the connected gate recipe lists as its acceptance criterion ("stands with correct animation").

None of the three is hard to fix. The core of the slice — the D1 pre-merge cell, the arm ordering, the single ConstrainTo site, the hook order, the flags — is correct and verified against the decomp. The failures are at the edges the findings chain keeps warning about: the NPC copy, and an unported half nobody re-derived.

---

Part 1 — independent verification of the contract's retail claims

Every address in the contract's "Retail ground truth" section was re-read in docs/research/named-retail/acclient_2013_pseudo_c.txt. All five load-bearing facts confirmed.

claim	verified
MoveOrTeleport @0x00516330; branch @0x00516386 if (eax_8 != 0 || this_1->cell == 0)	✓ line 284304ff. this_1 = this is assigned @0x00516334 from the incoming CPhysicsObj*, so this_1->cell is the body's own current cell, read at entry, before any placement. The whole D1 design rests on this and it is right.
arg4 is read only @0x0051638E, after the branch	✓ — the teleport branch's return 1 @0x00516438 executes without arg4 ever being touched. A teleport/cell-less packet places unconditionally: airborne wire bit, airborne body, any distance.
teleport_hook @0x005163EF runs BEFORE SetPosition @0x00516420	✓
teleport_hook @0x00514ED0 action list and order	✓ line 283115ff, exactly: CancelMoveTo(0x3C) @0x00514EDF → UnStick @0x00514EEE → StopInterpolating @0x00514EFD → UnConstrain @0x00514F0C → ClearTarget @0x00514F1B + NotifyVoyeurOfEvent(Teleported_TargetStatus) @0x00514F28 (one target_manager != 0 guard over the pair) → report_collision_end(this, 1) @0x00514F31. Each manager guarded on non-null.
SetFlags(0x1012) @0x00516414	✓ = Teleport(0x002) | Slide(0x010) | SendPositionEvent(0x1000). acdream's AuthoritativeTeleportFlags (RuntimeAuthoritativePositionRouteClassifier.cs:197-200) is bit-for-bit the same against PhysicsSetPosition.cs:64-76.
ONE ConstrainTo @0x00454272 via HandleReceivedPosition @0x00453FD0	✓ line 92896ff. The remote branch (arg2 != this->player @0x0045414D) is if (MoveOrTeleport(...) != 0) @0x00454254 → ConstrainTo(arg2, &arg2->m_position, …) @0x00454272 → return. Single site, shared by all three nonzero-returning branches, anchor read live off arg2->m_position (post-move). MoveOrTeleport discards SetPosition's error and returns 1 regardless, so retail arms even when the placement failed — confirmed.
ZeroVelocity is local-player-only	✓ set_velocity(player_2, {0,0,0}, 1) @0x004541B4 sits inside the arg2 == this->player + newer_event(TELEPORT_TS) branch. The remote branch writes no velocity at all.

Contract errata (does not change any decision)

C1 (MINOR). The contract's pseudo-C excerpt presents return 0 @0x0051636D as the fallthrough of the arg4 test. It is actually the else-label of an outer sequence gate at @0x00516364 (if (-((eax_4 - eax_4)) == 0) — a Binary-Ninja- mangled POSITION_TS/update_times[4] comparison that wraps the entire body). The arg4 == 0 path does fall into the same label, so the behavioural reading ("writes nothing, returns 0") is correct; the listing just implies a flatter control flow than the binary has. Worth correcting if the contract is reused.

C2 (MINOR, out of scope but noted). HandleReceivedPosition runs unset_parent(arg2) @0x00454129 and SetPlacementFrame @0x00454142 (when !HasAnims) before MoveOrTeleport. The contract's "recorded-not-consumed" list (UnparentBeforeRouting / ApplyPlacementFrameBeforeRouting) is therefore accurate — retail really does perform both ahead of the teleport branch, and acdream still records-without-reading them. Correctly deferred, correctly stated.

---

Part 2 — answers to the four open questions routed to this review

Q1 — D1's evidence chain: CONFIRMED. The plumbing is necessary, not churn.

Verified end to end in source, not inferred:

• RuntimeEntityObjectLifetime.TryApplyPosition measures the pre-merge value at src/AcDream.Runtime/Entities/RuntimeEntityObjectLifetime.cs:1723-1727 (hadCanonical / beforeCell / wasCellless) before calling Entities.TryApplyPosition at :1728.
• The merge onto the canonical record happens later, at :1786-1789: Entities.RefreshSnapshot(canonical, snapshot, refreshPosition: acceptedPosition).
• RuntimeEntityDirectory.RefreshSnapshot:238 → record.RefreshDerivedState(refreshPosition).
• RuntimeEntityRecord.RefreshDerivedState:230-237 → SetFullCell(position.LandblockId, …), and SetFullCell:244-251 writes FullCellId outright.

So after the merge canonical.FullCellId is the accepted wire cell, and the classifier — which the App calls after the merge and before the prologue rebucket (LiveEntityNetworkUpdateController.cs:1832-1840, ahead of RebucketLiveEntity at :1865) — was reading the wire cell through the route-1 Build overload. A wire cell of 0 is refused as RejectedData at RuntimeAuthoritativePositionRouteClassifier.cs:321-322, before the cell-less test at :402-404. The shipped remote cell-less predicate was unreachable. D1's characterisation is exactly right and the fix is correct: the pre-merge value is threaded on AcceptedPhysicsTimestamps.PreMergeCommittedCellId and consumed through a genuine overload (never a third copy), with null meaning "no opinion" rather than a fabricated 0.

One consequence D1 did not name — see R8 below.

Q2 — report_collision_end(this, 1) ↔ ShadowObjects.Suspend: there IS an unported half. See R2.

Q3 — the _activePlacementOwners deletion: safe. Confirmed behaviour-preserving.

IsPlacementActive had four consumers (Hidden suspend, UnHide restore, RestoreOrdinaryShadowInsideProjection, SuspendOrdinaryShadowOutsideProjection). The set's only writers were BeginAuthoritativePlacement / CompleteAuthoritativePlacement, whose only production caller was the deleted RemoteTeleportPlacementPresentation. With the writers gone the set is permanently empty, so every consumer degenerates to the false branch — which is precisely what the diff hard-codes. No Hidden/UnHide or visibility-edge restore changes for non-teleport entities. HasDeferredShadowRestore and _suspendedShadowOwners are untouched.

Q4 — RemoteTeleportSuffixIsQueuedBehindInitialAdmission: survives, correctly re-pointed.

It pins that a dormant initial-residence FIFO preserves teleport-sequence ordering (0,1,2) across continuations. The two TeleportHookRequired assertions were replaced with Assert.Null(...PreMergeCommittedCellId) on the same two continuations — a real assertion about the new field, not a tautology. But see R8: what that assertion proves is a behaviour change nobody recorded.

---

Part 3 — findings

R1 (MAJOR) — the D2 wire-airborne return-0 shape exists only on the player arm; the NPC arm still writes. AP-137 asserts otherwise.

Where: src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs:2188-2211 (the player-remote D2 block) has no counterpart on the NPC arm. The NPC arm's only wire-airborne early return is :2516-2523, gated on IsAirborneNoOperation(earlyRemoteRoute), which (RuntimeRemoteSteadyStatePosition.cs:81-86) matches only the NoPositionOperation disposition — never null and never Rejected*.

What happens instead. An NPC remote with a null or Rejected* classification and a clear wire contact bit falls through :2525-2543, :2560, and reaches ApplyRemoteContactRouting at :2600, where:

• body not in contact → :1145-1148 writes remote.Body.Position = worldPos; remote.Body.Orientation = rotation; and returns AirborneSnap;
• body in contact → :1196-1203 ApplyInterpolate enqueues a waypoint, then :2642 arms the leash.

Retail: MoveOrTeleport @0x0051638E reads arg4, finds it 0, and falls to return 0 — no body write, no queue write, and (because ConstrainTo sits inside if (MoveOrTeleport(...) != 0) @0x00454254) no leash arm. Retail makes no player/NPC distinction anywhere in MoveOrTeleport. The null classification is reachable for every remote through the whole login window (ClassifyRemoteAcceptedPosition returns null until _playerController exists), so this is not a corner.

Why MAJOR rather than "pre-existing". The behaviour predates the slice, but two things make it a failure of this slice:

1. D2 pinned the outcome — "unifies the player and NPC arms on one leftover- airborne behaviour" — and only half of it was implemented.
2. The rewritten AP-137 row now states as fact: "This deletes the legacy player-arm fallback's entity-revert quirk … and unifies player and NPC remotes on one behaviour." That sentence is false against docs/architecture/retail-divergence-register.md line 287's own code. A register row that describes behaviour the code does not have is worse than no row: it is the thing that stops the next reader from finding the divergence.

Correct behaviour: give the NPC arm the same early return the player arm now has — AP-135's two bookkeeping writes, then return — gated on the same predicate (classification is null/Rejected* and !update.IsGrounded). Or, if the intent is to keep the NPC snap deliberately, rewrite the AP-137 sentence to say so and give the asymmetry its own row.

Test gap that let it through: contract test-plan item 10 ("D2's leftover- airborne shape: wire-airborne null-classified packet writes exactly AP-135's two fields and nothing else") was not written for either arm, and item 11 (AP-135 preservation on both airborne no-op paths) was not written either. The D4 partition test that was written (RuntimeRemoteSteadyStatePositionTests.TryArmConstraintAfterOperation_MatchesTheCompletePartition) asserts the Runtime helper's arm→bool mapping, which is correct, but the partition table's two "no arm" rows for the wire-airborne leftover case are a property of the caller returning early — and that property is only true on the player arm. The test cannot see the gap.

---

R2 (MAJOR) — report_collision_end(this, 1) is mismapped; the faithful port exists in-tree and is not called.

Where: src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs:254 — ReportCollisionEnd: () => _physicsEngine.ShadowObjects.Suspend(localEntityId).

Retail @0x00514F31 → @0x00514620. report_collision_end(this, arg2) walks this->collision_table. With arg2 != 0 the staleness/still-touching tests are bypassed (@0x005146F3, @0x005146E7 both goto label_514706), so every record is deleted and each is passed to report_object_collision_end @0x00510A90, which fires weenie_obj->DoCollisionEnd(partnerId) on this object (gated on this->state & 8 = REPORT_COLLISIONS) and DoCollisionEnd(this->id) on the partner (gated on the partner's own 0x8, and skipped wholesale when the partner carries state & 0x200000). It is a force-end-all with bidirectional notification.

What acdream substitutes. ShadowObjectRegistry.Suspend (src/AcDream.Core/Physics/ShadowObjectRegistry.cs:1480-1499) removes the entity from every cell collision list while retaining its registration. That method's own doc comment says what it is: "the registry counterpart of retail CPhysicsObj::remove_shadows_from_cells" — a different retail function, which teleport_hook does not call. (Retail's shadow removal for a teleport happens later and internally, inside SetPositionInternal.)

The faithful port already exists. RuntimeCollisionReportingState (src/AcDream.Runtime/Physics/RuntimeCollisionReportingState.cs) is an explicit port of CPhysicsObj::collision_table and publishes bidirectional ObjectCollisionEnd with the same ReportCollisions gates on both sides (PublishResolvedObjectEnd:1225-1270). It has a private ForceEnd(record, key) (:1435-1450) that is exactly report_collision_end(this, 1) — reached today only from the destruction / leave-world / batch-retirement edges (:547, :881, :913, :943). The teleport hook does not reach it.

The canonical placement does not cover the gap: the SetPosition batch dispatch runs EndExpiredObjectCollisions(..., force: false, ...) (:493-500), which is retail's other call — handle_all_collisions → report_collision_end(this, 0) @0x005147F0. Retail runs both: force-end-all in the hook, then the ordinary non-forcing pass inside the placement.

Observable delta. After a remote teleports away, any object still holding a CollisionRecord against it keeps it until the ordinary ~1 s staleness pass, and neither side receives the immediate ObjectCollisionEnd retail fires. Retail ends it on the spot, on both sides.

Added-divergence half. Conversely, suspending the shadow at hook time is something retail does not do there: between the hook and the placement — and, on any non-commit outcome, until the next SyncRemoteShadowToBody — the remote is non-collidable. The window is bounded (the placement's Register/RefreshPositionRows both clear _suspendedEntities, and the DR tick's ShouldSynchronizeShadow re-syncs on the next pose delta), so this is a frames-scale residual rather than the #184 permanent class — but it is real, and it is not what retail does at this call site.

Contract compliance: the contract's open question 2 required the reviewer to "confirm … or file the delta on the AP-137 successor row." The delta is real and the row does not mention report_collision_end, ShadowObjects.Suspend, or the notification half. No row anywhere in the register covers it.

Correct behaviour: route the hook's sixth action at a RuntimeCollisionReportingState force-end entry point (the analogue of ForceEnd), and drop or separately justify the shadow suspension. If the force-end is deliberately deferred, it needs its own register row naming @0x00514F31 and @0x00514620.

---

R3 (MAJOR) — a teleported NPC now plans a RunForward cycle from a teleport-distance-derived velocity.

Where: src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs:2525-2543 then :2684-2709.

Newly reachable. In the pre-slice tree the shared if (remotePlacementRequired) block (HEAD:1958) sat above the NPC path and returned on every branch, so a teleport packet never reached the NPC synth-velocity code. This slice deletes that block, and a teleport-classified NPC packet now flows straight through it.

What it does. serverVelocity is update.Velocity; when ACE omits velocity (the ordinary UpdatePosition case) :2530-2532 synthesises (worldPos − rmState.LastServerPos) / elapsed — across the whole teleport distance, over one packet interval. :2536-2537 installs it. At :2704 RemoteServerControlledVelocityCycle.Apply then calls ServerControlledLocomotion.PlanFromVelocity (src/AcDream.Core/Physics/ServerControlledLocomotion.cs:40-62), which returns MotionCommand.RunForward for anything above RunThreshold, and ae.Sequencer.SetCycle(style, RunForward, …) fires. None of the guards block it: rm.Airborne is false at a walkable destination; rm.MoveTo was just invalidated by the hook's own CancelMoveTo; a standing NPC's Ready passes CanApplyVelocityCycle. SpeedMod is clamped, the motion is not.

The cycle persists until the next packet or the 0.6 s ServerControlledVelocityStaleSeconds pass in RuntimeRemotePhysicsUpdater.cs:198-208.

Retail. The teleport branch writes no velocity at all (contract invariant 6, verified above), and teleport_hook's first action cancels the moveto. A retail observer sees the creature appear at the destination and stand. acdream will show it sprinting in place.

Impact is animation-only — ServerVelocity drives cycle selection, not translation (RuntimeRemotePhysicsUpdater.cs:188-193) — so this is not a body-motion defect. But "stands with correct animation" is a literal line in the contract's own two-client gate recipe, so the gate is expected to catch it if it is run against an NPC. It will not be caught against a player remote: RemoteServerControlledVelocityCycle.Apply:27-49 returns early for player GUIDs.

Correct behaviour: the teleport arm must not leave a teleport-derived ServerVelocity installed. The narrow fix is to skip the synth (and the cycle apply) for a teleport-classified packet, which is also what invariant 6's "the teleport arm must not add a velocity write" was reaching for; the register row for the AP-80 velocity-cycle adaptation should name the exclusion.

---

R4 (MINOR) — stale comment: the prologue rebucket no longer feeds the classifier's CommittedCellId.

LiveEntityNetworkUpdateController.cs:1848-1853 still reads: "it is the only site that moves an ordinary moving remote's draw bucket, commits its canonical FullCellId (which feeds back as the classifier's own CommittedCellId and as the ConstraintDistance cell key)…". After D1 the remote classifier's CommittedCellId is the pre-merge value threaded on AcceptedPhysicsTimestamps, never the rebucket's commit. The comment sits four lines below the classification call the slice rewrote and directly contradicts the fix; a reader who believes it would "simplify" D1 away. Correct it in the same commit (process rule 6). The ConstraintDistance half is still true.

R5 (MINOR) — the architecture docs still describe the deleted classes as live.

docs/architecture/acdream-architecture.md:314, 316, 469-471 and docs/architecture/code-structure.md:265, 267, 420-423 document RemoteTeleportController / RemoteTeleportPlacement as the live remote placement owners. CLAUDE.md: "When the architecture doc and reality diverge, update one or the other — never leave them out of sync." The deletion inventory covered every .cs wiring site but not these two docs.

R6 (MINOR) — the [remote-teleport] probe can report hookRan=True for a hook that did nothing.

RunRemoteTeleportHook (LiveEntityNetworkUpdateController.cs:234-256) re-resolves the RemoteMotion and EntityPhysicsHost by GUID, rather than using the rmState the arm already holds and hands to ApplyAcceptedRemoteTeleport. Every action is remote?. / host?., so on a null lookup all six silently no-op and RemoteTeleportHook.Execute still returns true — and PhysicsDiagnostics.LogRemoteTeleport prints hookRan=True. Since the probe exists specifically to satisfy process rule 5 ("a clean-looking live session is not a passed gate"), a probe that cannot distinguish "ran" from "no-opped" undercuts the gate it was added for. Passing rmState through (it is in scope at all three call sites) removes both the mismatch risk and the false-positive.

R7 (MINOR) — hookRan is consumed only by the probe. Correct, but say so.

ApplyRemoteContactRouting:183 captures the hook's currency result and the routing proceeds regardless. That is retail-faithful — retail has no currency concept and runs the hook before knowing the placement outcome — but the code reads as if a false were being dropped on the floor. One line of comment naming @0x005163EF's unconditional ordering would close it.

R8 (MINOR) — D1's null-widening has a second, undocumented producer.

The AP-137 rewrite names exactly one new null-producing reason: "the merge observed no PRIOR canonical record for this entity." There is a second: the dormant initial-residence enqueue path. TryApplyPosition returns at RuntimeEntityObjectLifetime.cs:1705-1721 (EnqueueDormant) before the :1767-1770 PreMergeCommittedCellId write, so those timestamps carry null — which the diff's own changed assertion in RuntimeInitialCreateResidenceStateTests (Assert.Null(...PreMergeCommittedCellId)) now pins. A TELEPORT_TS-advancing packet on that path previously classified SetPosition (via TeleportAdvanced, which is unaffected by the cell input) and now classifies null → UnroutedCatchUp. I could not determine from static reading whether the App's OnPosition reaches ClassifyRemoteAcceptedPosition for an enqueued packet — flagging as unverified rather than guessing. Either way the AP-137 row should name the producer the test now pins.

R9 (MINOR) — the D2 block's "AP-135's two writes" are the packet's second copy.

LiveEntityNetworkUpdateController.cs:2206-2209 writes LastServerPos / LastServerPosTime, but :2106-2107 already wrote both unconditionally for this packet (and rmState.CellId at :2076), with a second DateTime.UtcNow read producing a slightly later timestamp. Harmless, but the block's comment reads as though these are the AP-135 writes being preserved, when the preservation actually happened 100 lines earlier. Contract invariant 8 is satisfied on both arms (player: :2076 + :2106-2107; NPC: :2519-2521) — this is a clarity issue, not a correctness one.

---

Part 4 — what the diff gets right (verified, not assumed)

Recorded so a fix round does not disturb it:

• D1 plumbing. Pre-merge cell measured by the same TryApplyPosition call that merged the packet, never re-read; null propagated honestly through both TryBuild and Build; route-1 semantics untouched (the old overload still passes canonical.FullCellId, RuntimeAcceptedPositionRouteRequests.cs:38-54). The discriminating test pair (CellLessRecord_… / CompanionTest_NonzeroPreMergeCellWithNoTeleportAdvance_…) genuinely distinguishes the fix from the shipped dead predicate.
• D3 hook. RemoteTeleportHook.Execute preserves retail's six actions in retail's order with a currency re-check between each; WeenieError.ITeleported is 0x3C; EntityPhysicsHost.NotifyTeleported is the correct one-to-one for the ClearTarget + NotifyVoyeurOfEvent pair under retail's single guard.
• D4 single arming site. The legacy pre-operation call is gone; the post-operation site is the only one, matching @0x00454272. Dropping the ConstrainAfterRouting route-flag check is behaviour-preserving — Interpolate and SetPositionSimple both carry ConstrainPhase.AfterPositionOperation (RuntimeAuthoritativePositionRouteClassifier.cs:473), and NoPositionOperation's None is already excluded by the arm mapping. Keying on the routing outcome rather than the raw classification is the right call: it is the only input that separates a grounded UnroutedCatchUp (arms, retail's nonzero analogue) from AirborneSnap (does not).
• D5 ordering. The teleport dispatch is the first thing ApplyRemoteContactRouting does, ahead of the !remote.Body.InContact carve-out; on the player arm it precedes both the wire-airborne block and the landing block; on the NPC arm the sticky gate is widened (!snapSuppressedByStick || isTeleportRoute) rather than duplicated. All three match "decided before arg4". UnStick being the hook's second action is the right justification for the sticky widening.
• D6/D7. TryApplyGenericRemoteRenderPose gate unchanged and now stated; TryAdoptWireCellAfterRouting suppression widened to the teleport arm; the re-entrancy guard covers both placement arms and sits before the arming on both, preserving the R5 invariant.
• Invariant 1. ApplyAcceptedRemoteTeleport reuses the far arm's StoresAcceptedDestination partition and StoreAcceptedDestinationPose unchanged — the partition was not re-litigated.
• Invariant 2. Both teleport tails sync the render entity from the resolved body and publish the shadow (:2171-2184 player, :2719-2732 NPC). The placement writes the resolved cell back through RuntimeSetPositionState.cs:5013 (remote.CellId = result.CellId), so entity.ParentCellId really is the resolved cell.
• Invariant 4. The classifier's teleport branch carries StopInterpolating: false deliberately; the queue clear comes from the hook's Interp.Clear(), matching @0x00514EFD.
• Invariant 10. No new production caller for TickLostCellDeadlines / TryDequeueExpiredLostCell; ArmLostFamilyDeadlines keeps its single pre-existing ParkDeferred call.
• Register bookkeeping. AP-137 rewritten in place rather than deleted (correct — the contract overrides the handoff here); AD-42's dead citation removed; AP-136 and AP-138 updated to the two surviving non-Position rebucket writers; AP-138's Risk column gains the teleport arm; AP-135 untouched. All correct except the false sentence named in R1.

---

Part 5 — gate evidence not present

Stated, not held against the verdict:

• The complete Release suite figure the contract's Gates section requires ("measure and record the new figure; do not inherit 11,027") is not recorded — the change is uncommitted and there is no commit message. I ran the build and a focused filter only.
• The two-client connected teleport gate is user-run and cannot be evidenced by a reviewer. Note that per R3 an NPC target is the discriminating case: a player-remote teleport will not exercise the velocity-cycle path at all.
• Contract test-plan items 10 (D2 shape) and 11 (AP-135 preservation on both airborne no-op paths) have no corresponding test in the diff. Item 10's absence is what left R1 invisible.

---

Recommended fix order

1. R1 — give the NPC arm the same D2 return-0 shape, and make the AP-137 sentence true (or split the asymmetry into its own row). Add contract test 10 for both arms.
2. R2 — route the hook's sixth action at the collision-reporting force-end, or file the delta on AP-137 with the @0x00514F31/@0x00514620 citations.
3. R3 — exclude a teleport-classified packet from the NPC synth-velocity install and the cycle apply; note the exclusion on the AP-80 row.
4. R4/R5 — the two stale-documentation fixes, same commit.
5. R6-R9 — clarity and probe-honesty, at the implementer's discretion.