diff --git a/docs/research/2026-08-04-c4-route-4b-3-architecture-review-round2.md b/docs/research/2026-08-04-c4-route-4b-3-architecture-review-round2.md new file mode 100644 index 00000000..145065ff --- /dev/null +++ b/docs/research/2026-08-04-c4-route-4b-3-architecture-review-round2.md @@ -0,0 +1,299 @@ +# C4 route 4b-3 — architecture / adversarial DELTA review, round 2 (2026-08-04) + +Reviewer lens: **adversary**. Round 1's findings are +[`2026-08-04-c4-route-4b-3-architecture-review.md`](2026-08-04-c4-route-4b-3-architecture-review.md); +the retail lane's are +[`2026-08-04-c4-route-4b-3-retail-review.md`](2026-08-04-c4-route-4b-3-retail-review.md). + +Subject: the same uncommitted working tree on +`claude/acdream-physics-divergence-5aa784` at base `3e002993`, after the round-1 +fix pass. This review re-attacks the **fixes**, not the whole slice — the +round-1 "verified sound" list is carried forward except where a fix touched it. + +## Verdict: **PASS** + +Both round-1 MAJORs (A1, A2) are fixed, and I verified each fix produces the +retail-correct outcome rather than merely silencing the symptom. The retail +lane's R1/R2/R3 are fixed. **The `RunRemoteArmTail` / +`ApplyWireAirborneLeftoverBookkeeping` extraction — the thing most likely to +introduce a new defect — is behaviour-identical at all five call sites**; I +walked each one against the pre-fix code and found no reordering, no dropped +step, and no widened or narrowed guard. Six MINORs below, none blocking. + +Build `dotnet build AcDream.slnx -c Release`: **succeeded, 0 warnings**. +Focused suites at Release `--no-build`: **Runtime 1,125 / 0 skips**, +**App 4,081 / 3 skips** (up 3 from round 1's 4,078 — the three new App tests). + +--- + +## Part 1 — the extraction, call site by call site + +The blast radius of the fix pass is small and I confirmed it: only +`LiveEntityNetworkUpdateController.cs`, one new method on `LiveEntityRuntime.cs`, +the App teleport test file, and three docs carry round-2 markers. + +### `RunRemoteArmTail` — 3 call sites + +Signature returns `RemoteContactRouting?`; `null` means "the currency guard +tripped, write nothing further". The guard inside it is +`(Arm is FarSnapPlacement or TeleportPlacement) && (!isCurrentPositionOwner() || !ReferenceEquals(positionRecord.RemoteMotionRuntime, remote))`. + +| site | pre-fix sequence | post-fix | verdict | +|---|---|---|---| +| player teleport dispatch (`:2333`) | routing → **unconditional** guard → return | routing → **arm-gated** guard → return | **identical.** `ApplyRemoteContactRouting` returns `TeleportPlacement` iff `OwnsTeleportPlacement(route)`, which the caller has already evaluated on the *same* `earlyRemoteRoute` with the same pure static predicate. The arm at this site is provably always `TeleportPlacement`, so arm-gating the guard cannot weaken it. | +| player grounded routing (`:2566`) | routing → arm-gated guard → return | same, via helper | **identical**, including the R5 guard-before-arm order (`TryArmConstraintAfterOperation` is still the first statement after the null check, `:2596`). | +| NPC dispatch (`:2818`) | routing → arm-gated guard → `return` from `OnPosition` | routing → helper returns `null` → `if (npcRoutingResult is null) return;` inside the same `if (!snapSuppressedByStick \|\| isTeleportRoute)` block | **identical**, same lexical position, same effect. | + +Specific things I checked for and did **not** find: + +- **`willBeDrTicked` moved inside the helper.** The player grounded site used to + hoist it into a local (`HEAD:2317`). I grepped every use in both the HEAD and + current file: its only consumer was ever the routing call itself. Nothing + downstream reads it. No loss. +- **Delegate identity.** `runTeleportHook` now closes over `canonical`/`remote` + and reuses the caller's `isCurrentPositionOwner` for both the hook's per-step + currency checks and the post-routing guard. Previously two separately + allocated but semantically identical lambdas. No change in what is tested. +- **Guard-before-arm (R5) at all three sites.** Held. The helper cannot arm — it + has no access to `ToConstraintArm` and its doc says so — and all three callers + arm immediately after the null check. +- **The NPC sticky-suppressed path still arms.** `npcArm` is initialised to + `UnroutedCatchUp` and `TryArmConstraintAfterOperation` sits *outside* the + `if (!snapSuppressedByStick || isTeleportRoute)` block, exactly as at HEAD. + The extraction did not pull it inside. + +### The LANDING TRANSITION block — confirmed untouched and non-conflicting + +`:2445-2532` still gates on `!rmState.Body.InContact`, still `return`s, and +still arms with its own hard-coded `NearInterpolate`. It therefore cannot +double-run with the shared tail, and — importantly — it is what makes +`playerArm` provably never `AirborneSnap` at the grounded-routing site +(the free-flight case has already returned). The only edit is its comment, +which round 1 flagged as false (A5) and which is now correct **and complete**: +it enumerates `SetPositionSimple` / `null` / `Rejected*` as classifications +that also reach the block, and states why `NearInterpolate` is the right arming +value for every one of them. + +### `ApplyWireAirborneLeftoverBookkeeping` — 2 call sites + +The helper writes exactly three fields: `remote.CellId`, `remote.LastServerPos`, +`remote.LastServerPosTime`. + +- **NPC site (`:2717-2722`)** — new, and the R1 fix. Correctly gated + `!update.IsGrounded && !isTeleportRoute`, placed after the + `IsAirborneNoOperation` return (so `NoPositionOperation` never reaches it) + and before the synth-velocity block, the sticky probe, and routing. I + confirmed the gate is exhaustive: for `RuntimeAcceptedPositionSource.PositionEvent` + a wire-airborne packet can only classify `NoPositionOperation` (handled above), + `null`, or `Rejected*` — the classifier's `effectiveContact` widening applies + only to `SameIncarnationCreate`, which this path never carries. +- **Player site (`:2390-2395`)** — the helper adds a `remote.CellId` write the + hand-rolled block did not have. **I chased this specifically as the classic + extraction hazard and it is benign**: + `RuntimePhysicsState.CommitCanonicalCell:2145` short-circuits on + `fullCellId == record.FullCellId` *before* `SetFullCell` and before raising + `CellCommitted`, and the player arm wrote the identical `p.LandblockId` ~180 + lines earlier with no placement in between. The redundant write is a true + no-op — no spurious rebucket event, no version bump. + +--- + +## Part 2 — the individual fixes + +### A1 — `ToConstraintArm(AirborneSnap) → NearInterpolate`, switch made total + +Correct, and correct for the right reason: `AirborneSnap` is acdream's +*body*-contact carve-out for a packet whose **wire** contact bit said grounded, +so retail's `MoveOrTeleport` returns nonzero and `HandleReceivedPosition` arms +@0x00454272. Mapping it to an arming value restores the 1-arm count and makes +the App-layer partition exactly the contract's D4 table. + +**The throwing default cannot be reached in production.** `RemoteContactArm` +has exactly five members (`AirborneSnap`, `SteadyStateInterpolate`, +`FarSnapPlacement`, `TeleportPlacement`, `UnroutedCatchUp`) and all five have +explicit cases; `default(RemoteContactArm)` is `AirborneSnap` (value 0), which +is handled. The only two producers of the argument are +`ApplyRemoteContactRouting`'s returns and the NPC arm's +`RemoteContactArm.UnroutedCatchUp` initialiser. Reaching `_` requires an +out-of-range cast, which nothing performs. The throw is a genuine +"can't-happen" guard, not a live hazard — this was the right call over a silent +zero-arm fallback. + +**The test genuinely discriminates.** +`NpcAirborneSnap_LandingPacket_StillArmsTheLeash` asserts +`host.PositionManager.Constraint` goes from `null` to non-null across the +packet. I verified `PositionManager.Constraint` is lazily created *only* inside +`ConstrainTo` (`PositionManager.cs:27-28`, `:62`), so its presence is direct +proof of an arm rather than an inference. Under the pre-fix mapping the packet +arms zero times and `Constraint` stays null — the test fails. + +### A2 / R3 — NPC synth-velocity and cycle-apply gated on `!isTeleportRoute` + +Correct. `isTeleportRoute` is hoisted above both the D2 check and the velocity +block, and both the install (`:2741`) and the `RemoteServerControlledVelocityCycle.Apply` +call (`:2886`) carry it. The fix's own claim that leaving `ServerVelocity` +stale is safe holds up: its only consumers are the now-excluded cycle-apply and +`RuntimeRemotePhysicsUpdater`'s stale-velocity watchdog, which only *zeroes*. + +Two incidental changes in the same block are behaviour-neutral and I checked +them rather than assumed: the `!IsPlayerGuid(update.Guid)` guards were dropped +from the synth condition and the `else if` collapsed to `else`. Both were +already dead — the whole NPC section sits after `if (IsPlayerGuid(update.Guid)) { … }` +and every path inside that block `return`s (the last statement before its +closing brace at `:2666` is `return;`), so the section is unreachable for +player guids. + +`NpcTeleport_DoesNotInstallASynthesizedVelocity` seeds `LastServerPos`/ +`LastServerPosTime` so a broken implementation has a real distance and interval +to synthesise from. It fails pre-fix (`HasServerVelocity` would be `true`). + +### R1 — the D2 shape on the NPC arm + +Correct and now genuinely shared. The AP-137 sentence *"unifies player and NPC +remotes on one behaviour"* is true against the code as of this round — I +re-read the row and the two call sites. + +`NullClassifiedNpc_WireAirbornePacket_WritesOnlyBookkeepingNoBodyOrShadow` +discriminates: pre-fix the packet reaches `ApplyRemoteContactRouting`'s +free-flight carve-out and hard-snaps `Body.Position = worldPos`, which the +test's `Assert.Equal(spawnBodyPose, …)` catches. + +### R2 — the teleport hook's collision-end action → `ForceEndCollisionReporting` + +**This was the fix I was asked to attack hardest, and it does not carry any of +this family's failure modes.** `RuntimeCollisionReportingState.LeaveWorld` is +named for the *collision-reporting* state machine's own leave transaction, not +the entity lifecycle's. I traced it end to end: + +- `LeaveWorld:870-887` → `_admissionBlocked.Add(key)` → `ForceEnd:1435-1454` → + `EndExpiredObjectCollisions(force: true)` → `TrimEmptyOwner`. **The only + mutations are the owner's collision table, the reverse-owner index, and the + report queue.** Nothing writes `body.InWorld`, `TransientStateFlags.Active`, + `record.ObjectClock`, `FullCellId`, spatial residency, or visibility. + **Invariant 3 is not at risk.** +- `_admissionBlocked` is released in a `finally`, so the subsequent placement's + `TryPrepareSetPositionBatch:230-235` — which *does* refuse while a key is in + `_leaving` or `_admissionBlocked` — is not blocked by the hook that ran + immediately before it. +- `_mutationRevision` is bumped by `LeaveWorld`, but the placement's batch + captures `expectedMutation` *after* the hook (`:238`) and the + prepare→install window is entirely inside the placement, so the bump cannot + invalidate the teleport's own batch. +- `Publish` swallows observer exceptions (`:1489-1494`), and `_leaving` guards + re-entrancy, so a force-end cannot escape as a throw into the packet path. +- `TrimEmptyOwner` runs on the exit path, so `CaptureOwnership`'s owner count + cannot leak an empty entry — no ledger regression. + +Dropping the old `ShadowObjects.Suspend` is also safe and is arguably the +better half of the fix: the far-snap arm (shipped, user-accepted) has never +suspended the shadow before its own placement, so the teleport arm is now +*consistent* with it rather than uniquely different, and the arm tail's +`TryPublishRemote` → `SyncRemoteShadowToBody` (ungated at this call site) still +re-seeds the registry at the resolved pose. See MINOR B2 for the one thing +about R2 that should be stated rather than left implied. + +--- + +## MINOR findings + +### B1 (MINOR) — contract test-plan item 11 is still unwritten, and it is now the *only* thing guarding the #42 class + +`NullClassifiedNpc_WireAirbornePacket_…` asserts what must NOT happen (no body +write, no shadow republish) but never asserts what MUST happen: that +`ApplyWireAirborneLeftoverBookkeeping` actually wrote `remote.CellId` and +`LastServerPos`/`LastServerPosTime`. A regression that emptied the helper body +would pass every test in the tree. Those writes are exactly AP-135 — the +free-fall sweep gates on `rm.CellId != 0` and without it "an airborne remote +falls through the floor" (#42), and the first grounded packet after an arc +would synthesise its velocity across the whole jump. Two `Assert.Equal`s on the +existing test close it. + +### B2 (MINOR) — R2's fix is correct plumbing, but its *observable* half still goes nowhere; say so before someone records it as closed + +`IRuntimeCollisionReportObserver` has **zero production implementations** — +`git grep OnCollisionReport` returns the interface, the dispatch loop, and four +test doubles. `CollisionReports.Subscribe` has no production caller. So the +bidirectional `DoCollisionEnd` notification retail fires at @0x00514620 still +reaches no gameplay consumer; what the fix actually delivers today is the +**table clear** (real, and it does prevent the later ~1 s stale-end and any +mis-attribution) plus a correctly-wired channel for when a consumer appears. +The hook's new doc doesn't overclaim, but the retail review's phrasing +("neither side receives the immediate `ObjectCollisionEnd`") will read as +closed. State the residual — it is a pre-existing gap in the collision-report +plumbing, wider than this slice, and it deserves one sentence rather than a +silent inheritance. + +### B3 (MINOR, disclosed) — no test for R2 + +Judged **acceptable but worth filing**. A test would assert against a channel +nothing production reads (B2), so its value is pinning the retail *mapping*, +not a behaviour. The cheap version is one assertion that the teleporting +owner's collision table is empty after an `OnPosition` teleport packet — that +would fail if a future edit reverted the action to `ShadowObjects.Suspend`, +which is the actual regression risk. Not blocking. + +### B4 (MINOR, disclosed) — the per-packet `runTeleportHook` closure + +Two delegates + two display classes per remote accepted Position (the outer +`runTeleportHook` lambda and the caller's `isCurrentPositionOwner`), allocated +whether or not the packet is a teleport. The implementer's stated blocker — +that `ApplyRemoteContactRouting`'s `Func` parameter is injected by +existing tests — is real but the parameter is `internal`, so it is a +refactor cost, not a compatibility one. Judged **acceptable to defer**: this is +the per-packet network path (5-10 Hz per remote), not Slice I's per-frame +resolve path where the 0-B target applies, and the objects are gen0-transient. +File it with the probe-family cleanup rather than churning the seam now. + +### B5 (MINOR, disclosed) — the stress test's teleport step + +Round 1's A3 stands as written and is judged **acceptable to leave**. The +scenario it covers (Hidden → DeferredShadowRestore → landblock churn → UnHide) +no longer has any teleport-specific machinery to interact with, because this +slice deletes `_activePlacementOwners` and the placement is now synchronous +inside `OnPosition`. The hand-built step is honestly documented as reproducing +the arm tail's observable state, and the arm tail itself is directly covered by +`LiveEntityNetworkRemoteTeleportPresentationTests`. The residual loss — the +*interaction* between a mid-teleport Hidden edge and the placement — is a +degenerate case now, not an untested one. + +### B6 (MINOR, carried) — proof obligation 1 is still unstated + +Unchanged from round 1's A9. The contract requires the +`ParkCollisionResidents`-overlap-throw unreachability argument (with 4b-1's B2 +caveat that the guarded property is +`TryAcquireCollisionPrefixMutationPermission`'s `HasOldPrefixPlacementDebt` +refusal — a stall, not a throw) in the implementation commit's +contract-conformance section. The work is still uncommitted so I cannot verify +it; the property itself holds by reading (the teleport arm adds packets to the +same one-operation-per-key machinery and opens no new operation shape). + +--- + +## Round-1 MINORs — spot-check + +Verified fixed, not re-derived: **A4** (`TeleportRefused_…` now arms sticky +before the packet and asserts `GetStickyObjectId() == 0` afterward — a real +discriminator, since only the teleport arm's hook calls `UnStick`, so the +`UnroutedCatchUp` look-alike I identified can no longer pass it); **A5** (the +landing-block comment is now correct *and* complete); **A7** (R1, above); +**A8** (`wasCellless` deleted). **A10** was accepted and restated correctly. +The retail lane's **R4** (the prologue comment's `CommittedCellId` claim) and +**R5** (`acdream-architecture.md` / `code-structure.md` still describing the +deleted classes) are both fixed in the diff. + +--- + +## Gate status observed + +- `dotnet build AcDream.slnx -c Release` → **succeeded, 0 warnings, 0 errors**. +- `tests/AcDream.Runtime.Tests -c Release --no-build` → **1,125 / 0 / 0**. +- `tests/AcDream.App.Tests -c Release --no-build` → **4,081 / 3 skipped / 0**. +- Complete Release suite: **still not run and not recorded.** The contract is + explicit that the new figure must be measured, not inherited from 11,027. + Outstanding. +- Two-client connected teleport gate: outstanding. **It must use an NPC + target** (`@teleto` a creature into view), not a second player character — + both round-1 MAJORs lived on the NPC arm, and + `RemoteServerControlledVelocityCycle.Apply` early-returns for `0x50xxxxxx` + guids, so a player-remote teleport exercises neither fix. Confirm at least + one `[remote-teleport]` line per teleport with the expected `cause`. diff --git a/docs/research/2026-08-04-c4-route-4b-3-architecture-review.md b/docs/research/2026-08-04-c4-route-4b-3-architecture-review.md new file mode 100644 index 00000000..07b90cbb --- /dev/null +++ b/docs/research/2026-08-04-c4-route-4b-3-architecture-review.md @@ -0,0 +1,386 @@ +# 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` 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. diff --git a/docs/research/2026-08-04-c4-route-4b-3-contract.md b/docs/research/2026-08-04-c4-route-4b-3-contract.md new file mode 100644 index 00000000..92b0a2ed --- /dev/null +++ b/docs/research/2026-08-04-c4-route-4b-3-contract.md @@ -0,0 +1,676 @@ +# C4 route 4b-3 — remote teleport + cell-less: pinned contract (2026-08-04) + +Split rationale: +[`2026-08-04-c4-route-4b-scoping-and-split.md`](2026-08-04-c4-route-4b-scoping-and-split.md). +4b-1 (infrastructure) landed at `2e8e09ac`; 4b-2 (far snap) landed at +`7f1c1f5a` after four fix rounds; the shared-core park restore it forced is at +`634bc551` and inside 4b-2's rounds 3/4. Read the whole findings chain before +implementing — every defect class it names reappears here at larger scale: +[round 1](2026-08-04-c4-route-4b-2-review-findings.md) → +[round 2 (delta)](2026-08-04-c4-route-4b-2-delta-review-findings.md) → +[round 3](2026-08-04-c4-route-4b-2-round3-correction.md) → +[round 4](2026-08-04-c4-route-4b-2-round4-correction.md). + +**4b-3 flips the LAST remote classification on: `SetPosition` — teleport +(TELEPORT_TS advanced) and cell-less (the body has no committed cell) — through +4b-1's `RuntimeRemotePlacementDriveController`, runs retail's `teleport_hook` +before the placement, and deletes the legacy remote-teleport machinery: +`RemoteTeleportController` (605 lines), `RemoteTeleportPlacement` (85), +`RemoteShadowPlacementSynchronizer` (49, two classes), the +`remotePlacementRequired` predicate, the `TeleportHookRequired` timestamp +plumbing, the legacy pre-operation `ConstrainTo` fallback, and the player arm's +legacy `!update.IsGrounded` fallback — plus 1,709 lines of their tests.** + +This retires AP-137's cell-less enqueue-vs-place delta. AP-135 does NOT retire +(its two writes sit physically inside the method this slice rewrites — see +"Must remain true" item 8). AP-131 does not retire. #276 does not close. + +## Retail ground truth — verified in `acclient_2013_pseudo_c.txt`, verify again yourself + +`CPhysicsObj::MoveOrTeleport` @0x00516330, teleport/cell-less branch: + +``` +00516375 eax_8 = CPhysicsObj::newer_event(this_1, TELEPORT_TS, arg3); +00516386 if ((eax_8 != 0 || this_1->cell == 0)) { +005163ef CPhysicsObj::teleport_hook(this_1, edx_2); +005163f8 SetPositionStruct::SetPositionStruct(&var_64); +00516406 SetPositionStruct::SetPosition(&var_64, arg2); +00516414 SetPositionStruct::SetFlags(&var_64, 0x1012); +00516420 CPhysicsObj::SetPosition(this_1, &var_64); +00516438 return 1; +00516386 } +0051638e if (arg4 != 0) { ... near @0x005163AF / far @0x005163C1-E8 ... } +0051636d return 0; +``` + +Five facts in that listing decide this slice: + +1. **The teleport branch is decided BEFORE the contact test** (`arg4` is only + read @0x0051638E, after the branch). A teleport/cell-less packet places + unconditionally — airborne wire bit, airborne body, any distance. acdream's + routing must therefore decide the teleport arm AHEAD of every + airborne/landing carve-out (see design decision D5). +2. **`this_1->cell == 0` is the BODY's current cell** — "this object has no + resolved cell right now" — not the wire destination's cell. See D1: the + classifier's current input implements a different (and dead) predicate. +3. **`teleport_hook` @0x00514ED0 runs BEFORE the placement** and is, complete + (each guarded on the manager existing): + `MovementManager::CancelMoveTo(0x3C)` @0x00514EDF, + `PositionManager::UnStick` @0x00514EEE, + `PositionManager::StopInterpolating` @0x00514EFD, + `PositionManager::UnConstrain` @0x00514F0C, + `TargetManager::ClearTarget` @0x00514F1B + + `TargetManager::NotifyVoyeurOfEvent(Teleported_TargetStatus)` @0x00514F28, + `CPhysicsObj::report_collision_end(this, 1)` @0x00514F31. +4. **`SetFlags(0x1012)`** (`Teleport|Slide|SendPositionEvent`) @0x00516414 — + acdream's analog is the classifier's `AuthoritativeTeleportFlags`, carried on + `route.SetPositionFlags` into + `RuntimeSetPositionState.TryPrepareAndSubmitAuthoredPlacement`'s `flags` + parameter. (There is NO separate "teleport hook phase support" inside + `RuntimeSetPositionState` — the phase lives on the route, + `RuntimeTeleportHookPhase.BeforePositionOperation`, emitted at the + classifier's remote teleport/cell-less branch. The handoff's phrasing + implied otherwise; this is the correction.) +5. **The branch returns 1 and discards `SetPosition`'s error**, so + `SmartBox::HandleReceivedPosition` @0x00453FD0 arms `ConstrainTo` + @0x00454272 — the remote arm's ONLY arming site, shared by all three + nonzero-returning branches, anchored `&arg2->m_position` read live + (post-move). Retail arms even when the placement failed. So: hook + `UnConstrain` first, place, then ONE post-operation re-arm — never a second + arming site, and arm on non-commit outcomes too (the 4b-2 B8/R5 lesson). + +## What must REMAIN true (process rule 1 — the contract causes the defect) + +For every path this slice adds or rewrites, including every refusal, +contention, rejection, and short-circuit: + +1. **The pose still advances.** A teleport-classified packet whose canonical + placement never reached the engine (`Refused` / `Contention` / + `RejectedPreparation` / `NotApplicable`) still commits the accepted + destination pose to the canonical body — the same + `StoresAcceptedDestination` partition and the same + `StoreAcceptedDestinationPose` the far arm uses, which is retail's + `store_position` @0x00515CE2 on the no-transition branch. `Deferred` and + `RejectedByPlacement` do NOT store, for the reasons already pinned in the + enum's own doc (`ParkDeferred` already snapped; the engine ran and + refused / a settled pose must survive). Do not re-litigate the partition — + extend it to the teleport arm unchanged. +2. **The render entity still advances.** The teleport arm's tail syncs + `WorldEntity` (position, `ParentCellId`, rotation) from the RESOLVED body + and publishes the collision shadow, exactly as both existing grounded arm + tails do (`LiveEntityShadowPublisher.TryPublishRemote`). A teleport must + never leave the rendered pose a packet behind the body. #312's lesson: + presentation state is where this family breaks; tests must assert it + (see Test plan). +3. **The object clock keeps running and the entity stays in the world** on + every non-commit outcome: `body.InWorld`, `TransientStateFlags.Active`, + `record.ObjectClock` active, `FullCellId != 0`, spatial projection intact, + `IsSpatiallyVisible` unchanged. No park survives the controller + (`CancelToken` with `restoreCancelledPark: true`); the pre-flight + (`CanAttemptDestination`) stays an OPTIMISATION, never the reason a remote + stops tracking or becomes invisible. +4. **The interpolation queue is empty after the teleport arm runs** — + cleared by the hook's `StopInterpolating` (@0x00514EFD), NOT by the route + flag: the classifier's teleport branch deliberately carries + `StopInterpolating: false` because retail's clear lives inside the hook. + The far arm's route-flag-driven clear is untouched. +5. **The leash is armed exactly once per accepted packet**, post-operation, + anchored post-move, by the classification partition — never zero times + (the current shipped hole: `remotePlacementRequired` returns ahead of every + arming site while `RemoteTeleportHook` has already `UnConstrain`ed, so a + remote that teleports and stands still is leash-less until the next + packet), and never twice. See D4 for the complete partition. +6. **The per-packet prologue keeps running for every classification**: + `TryApplyGenericRemoteRenderPose` (see D6 for the gate), the + `RebucketLiveEntity(update.Guid, p.LandblockId)` spatial-bucket + transaction, the velocity install + (`TryCommitAuthoritativeVelocity` — retail's PositionPack `set_velocity`, + upstream of `MoveOrTeleport`), and the incarnation re-validation + chain. Retail's remote teleport never writes velocity itself + (`ZeroVelocity` is the LOCAL player's `SmartBox::TeleportPlayer` + @0x004541B4 only); the teleport arm must not add a velocity write. +7. **`AcceptedPositionSource`/authority validation is unchanged.** The + teleport arm executes only an already-classified route from + `ClassifyRemoteAcceptedPosition` — the shared builder + (`RuntimeAcceptedPositionRouteRequests`), never a re-derivation. +8. **AP-135's two writes stay**, on both arms' airborne no-op + neighbourhoods: the server-cell adopt (`rmState.CellId = p.LandblockId`) + and the `LastServerPos`/`LastServerPosTime` sample. They are 4a-owned + acdream-only bookkeeping for the free-fall sweep gate + (`RuntimeRemotePhysicsUpdater`'s `rm.CellId != 0` gate) and the + first-grounded-packet velocity synthesis. This slice rewrites the method + they sit in; they do not go. The register row does not retire. +9. **`ParkCollisionResidents`'s overlap throw stays unreachable** — see + "Proof obligations". +10. **`ArmLostFamilyDeadlines`' reaper gains no production caller.** + `TickLostCellDeadlines` and `TryDequeueExpiredLostCell` + (`RuntimeSetPositionState`) have zero production callers today; this + slice must not add one. `ParkDeferred` arming the family (its + `ArmLostFamilyDeadlines` call) is pre-existing and stays inert. +11. **Route 1's classification inputs are unchanged.** + `RuntimeInitialCreateContinuationExecutor`'s calls into the shared + builder keep their current semantics (D1 adds an overload for the remote + PositionEvent path; the builder's own doc mandates "an overload here, + never a third copy"). +12. **The 4a dispositions are untouched**: `NoPositionOperation` writes + nothing (plus item 8's bookkeeping), `Interpolate` enqueues, the landing + block still hard-snaps a wire-grounded packet for a not-in-contact body + on 4a-owned classifications, AP-87's snap conditions and AP-139's landing + clear are unchanged. +13. **The far arm (4b-2) is untouched** except where a shared gate widens to + include the teleport arm (D6, D7) — widening must not change the far + arm's own behaviour. Everything round 2's/round 4's "do not churn" lists + verified stays: the `StoresAcceptedDestination` partition, the + `IsRestorableQuiescencePark` relocation, the pose-parity composition, the + two-arm guard/arm ordering. + +## Design decisions — pinned, not open for redesign + +### D1 — the classifier's cell-less input becomes the PRE-merge committed cell (resolves trap T1 and undetermined item 3) + +**Finding (undetermined item 3, now determined by reading the code):** the +merge cannot zero a previously-nonzero `FullCellId` — it does the opposite. +`RuntimeEntityObjectLifetime.TryApplyPosition` calls +`Entities.RefreshSnapshot(canonical, snapshot, refreshPosition: acceptedPosition)` +for every accepted Position, and `RuntimeEntityRecord.RefreshDerivedState` +then executes `SetFullCell(position.LandblockId, …)` from the just-merged +snapshot. So at classification time (OnPosition classifies after the +authority gate's merge, before the prologue rebucket), `canonical.FullCellId` +IS the accepted wire cell. A wire cell of 0 fails +`LandDefs.InboundValidCellId` inside `PositionFrameValidation.IsValid`, which +the classifier's `ValidPosition` check turns into `RejectedData` BEFORE the +cell-less test. **Consequence: the classifier's remote `cellless` predicate, +as fed today (`CommittedCellId: canonical.FullCellId` in +`RuntimeAcceptedPositionRouteRequests.Build`), is unreachable for a remote +PositionEvent — the remote `SetPosition` classification currently fires on +`TeleportAdvanced` alone.** It is not a subset relationship with +`remotePlacementRequired`; it is a dead predicate. (The classifier's own +comment above the test — "only a later Runtime SetPosition or simulation +commit may change FullCellId" — is falsified by `RefreshDerivedState` and +must be corrected in this slice; process rule 6.) + +Retail's predicate is the BODY's current cell (`this_1->cell == 0` read at +`MoveOrTeleport` entry, before any placement) — "this object is not resident +anywhere right now", e.g. the first Position after an unwield-to-3D +(`SetFullCell(0,0)` at the pickup/parent leave-world sites) or after any +canonical withdrawal. The acdream value with exactly that meaning is the +committed cell BEFORE this packet's merge — the `wasCellless`/`beforeCell` +pair `TryApplyPosition` already measures. + +**Pinned:** thread the pre-merge committed cell out of the merge and into the +remote classification's `CommittedCellId` input, via an overload of the shared +builder (never a third copy; never a change to the existing overloads' route-1 +semantics). Plumbing shape is the implementer's choice — the natural carrier +is `AcceptedPhysicsTimestamps` (whose `TeleportHookRequired` field this slice +deletes; replacing a policy tri-OR with a data field is strictly better) or +`AcceptedPositionNetworkUpdate`. Constraints: the value is the pre-merge +`FullCellId` measured by the SAME `TryApplyPosition` call that merged the +packet (never re-read after the merge); no fabrication (a known record always +has an honest value — 0 means "was celless", not "unknown"). + +**What is deliberately NOT adopted from `remotePlacementRequired`:** the +graphical `projectionRequiresTeleportHook` arm +(`LiveEntityRuntime.TryApplyPosition`: pre-merge `FullCellId == 0` OR +`!IsSpatiallyProjected` OR `!IsSpatiallyVisible`). Its visibility half is a +presentation predicate with NO retail analogue — it made "not currently +rendered" fire the whole teleport machinery on a routine hot path (trap T1's +warning). After this slice the teleport classification is retail's exact pair +(`TeleportAdvanced || wasCellless`); a not-visible remote's Position +classifies by distance like any other, the canonical placement decides +placeability, and visibility remains presentation-only. This behavioural +change is recorded in the AP-137 rewrite (D8). The whole +`projectionRequiresTeleportHook` computation, the lifetime parameter, the +headless `false` at `RuntimeLiveEntitySessionController`, and the +`TeleportHookRequired` field + its `timestamps with {…}` write are deleted. + +### D2 — classifier-null and `Rejected*` keep 4b-2's stated policy (resolves trap T2); the legacy airborne fallback is replaced by the retail return-0 shape + +The scoping doc's T2 described the pre-4b-2 world; 4b-2 already deleted the +legacy near/far blocks and routed `null` / `RejectedAuthority` / +`RejectedData` to `UnroutedCatchUp` +(`RuntimeRemoteFarSnapPosition.ResolveArm`, AP-137). **4b-3 keeps that policy +unchanged** — during the login window (null `_playerController` → +classification refuses; every remote packet) remotes keep tracking through +AP-87's catch-up, exactly as today. One consequence to state in the register +rewrite: a TELEPORT_TS-advancing packet that arrives while classification is +null consumes its teleport sequence in the timestamp gate but runs no hook — +benign in the only producing window (fresh session: no moveto, stick, leash, +or target exists yet to tear down), stated rather than discovered later. + +What T2 actually leaves 4b-3 is the player arm's legacy +`!update.IsGrounded` fallback (the block whose comment says "4b deletes this +fallback" — a comment this slice at last makes true), which handled +wire-airborne packets for classifications 4a does not own. After D1 the +unowned set shrinks to null and `Rejected*`. **Pinned replacement:** a +wire-airborne packet with a null/`Rejected*` classification takes the retail +return-0 shape applied to the acdream-only states — AP-135's two bookkeeping +writes, no body write, no queue write, no render write, no leash arm, return. +This deletes the legacy block's entity-revert quirk +(`entity.SetPosition(rmState.Body.Position)`) and unifies the player and NPC +arms on one leftover-airborne behaviour. Recorded in the AP-137 rewrite. + +### D3 — one teleport-hook implementation, triggered by the route, run inside the teleport arm before the placement + +The hook trigger moves from `timestamps.TeleportHookRequired` (deleted) to +the classification: `route.TeleportHookPhase == +RuntimeTeleportHookPhase.BeforePositionOperation`, which the classifier +already emits on exactly the remote teleport/cell-less branch. The hook runs +inside the new teleport arm, immediately before +`TryExecuteAcceptedRemotePosition` — retail's order (hook @0x005163EF before +`SetPosition` @0x00516420), and it runs REGARDLESS of what the placement then +yields (retail runs it before knowing the outcome). + +There must be exactly ONE hook implementation. The existing +`RemoteTeleportHook.Execute` sequence — the six actions in retail order with +a currency re-check between every step — is the port and must be preserved +verbatim; whether the file moves into `AcDream.Runtime` (every action is +expressible there: `remote.Movement.CancelMoveTo`, +`rmState.Host.PositionManager.UnStick/UnConstrain`, `remote.Interp.Clear()`, +`host.NotifyTeleported()`, `Physics.Engine.ShadowObjects.Suspend(localId)`) +or stays an App bundle invoked from the arm is the implementer's choice. +Runtime residence is preferred (it is where the sibling arm logic lives and +what a future headless remote-motion consumer needs), but not at the cost of +inventing a second hook path. `RemoteTeleportHookTests` (38 lines) moves or +adapts with it — not silently dropped. + +**Undetermined item 2 is RESOLVED — yes, `EntityPhysicsHost.NotifyTeleported()` +covers retail's TargetManager pair.** Verified on both sides: +`NotifyTeleported` executes `_targetManager.ClearTarget()` then +`_targetManager.NotifyVoyeurOfEvent(TargetStatus.Teleported)` +(`src/AcDream.Runtime/Physics/EntityPhysicsHost.cs`), and retail's +`teleport_hook` executes `TargetManager::ClearTarget` @0x00514F1B then +`NotifyVoyeurOfEvent(Teleported_TargetStatus)` @0x00514F28 under one +`target_manager != 0` guard. One-to-one; no open question remains here. + +### D4 — the single `ConstrainTo` arm, and its complete partition + +The legacy pre-operation arming call (the `OwnsAfterOperationConstraint`-gated +fallback in the player/NPC shared section, whose comment already says "4b-3 +deletes it") is DELETED. The post-operation site — +`RuntimeRemoteSteadyStatePosition.TryArmConstraintAfterOperation`, called once +per arm after routing — becomes the only arm, matching retail's single site +@0x00454272. Its predicate widens from +`OwnsSteadyState || OwnsFarSnap` to the full retail partition: + +| classification (routed arm) | wire contact | retail return | arm? | +|---|---|---|---| +| teleport / cell-less (`SetPosition`, new arm) | any | 1 | **yes** — after the operation, on every placement outcome (retail discards the error), post-move anchor | +| `Interpolate` (near) | grounded | 1 | yes (unchanged, 4a) | +| `SetPositionSimple` (far) | grounded | 1 | yes (unchanged, 4b-2) | +| `NoPositionOperation` (airborne no-op) | airborne | 0 | **no** (unchanged, 4a) | +| null / `Rejected*` → `UnroutedCatchUp` | grounded | (no retail state; analog: nonzero) | yes — via the same post-operation site, which must therefore accept a null route for this case | +| null / `Rejected*`, wire-airborne (D2 shape) | airborne | (analog: 0) | **no** — the current legacy pre-op arm DOES arm these; that was a divergence and it retires with the site | + +The one-packet unarmed residual on a superseded incarnation (the currency +guard returning before arming) is AP-138(3) and extends to the teleport arm +unchanged. This partition closes the shipped hole named in scoping +correction 2: a remote hard teleport currently arms the leash NOWHERE +(`remotePlacementRequired` returns ahead of every arming site after the +hook's `UnConstrain`); after this slice the hook `UnConstrain`s and the +single post-operation site re-arms — retail's exact sequence. + +### D5 — routing order: the teleport arm precedes every contact carve-out; sticky does not suppress it + +Retail decides the teleport branch before reading `arg4`. Therefore: + +- `RuntimeRemoteFarSnapPosition.ResolveArm` (or its successor) returns the + new teleport arm ahead of everything else, and + `ApplyRemoteContactRouting` dispatches it BEFORE the `!remote.Body.InContact` + free-flight carve-out — an airborne-body teleport packet places, it does + not `AirborneSnap`. +- The player arm's landing block (`!rmState.Body.InContact` hard-snap + + return) must not claim a teleport-classified packet: the teleport + classification is routed before it (or the block is gated to classifications + it owns — implementer's choice, pinned outcome: a teleport-classified + packet always reaches the teleport arm regardless of wire or body contact). +- The 4a `IsAirborneNoOperation` early returns are classification-gated + already and cannot claim a `SetPosition` route — unchanged. +- **The NPC arm's TS-44 sticky suppression (`snapSuppressedByStick`) does not + suppress the teleport arm.** Retail's sticky cannot survive a teleport — + `UnStick` is the hook's second action. The suppression remains exactly as + it is for the near/far/leftover arms (its register row describes an + NPC-only steady-state gate, which stays true). + +### D6 — the two per-packet gates widen to the teleport arm, same rule as the far arm + +- `TryApplyGenericRemoteRenderPose`: the gate stays `OwnsSteadyState` — the + teleport arm (like the far arm) takes the early wire-pose write, and its + tail re-syncs the render entity from the resolved body (invariant 2). This + resolves the standing "route 4b-3 revisits the gate" comment: the answer is + "unchanged, now stated"; delete the forward reference. +- `TryAdoptWireCellAfterRouting`: the suppression (currently + `arm is FarSnapPlacement`) widens to the teleport arm, for the same reason — + after a canonical placement the placement is the cell authority; retail + resolves the destination cell through `AdjustPosition`/`set_cell` and + nothing writes the wire cell over it. + +### D7 — the post-placement currency guard covers the teleport arm; consolidation is sanctioned + +Both arms' re-validation ("the far arm is re-entrant — re-validate position +ownership on EVERY placement status before writing anything else, arming +included") widens to `Arm is FarSnapPlacement or `. Round 2's +"do not churn" explicitly named for 4b-3 that this guard "belongs behind a +testable Runtime seam rather than duplicated at two App call sites" — moving +the duplicated guard into the Runtime seam is sanctioned in this slice, but +only if the two arms' observable ordering (guard before arm, both arms +identical — the R5 invariant) is preserved and pinned by test. + +### D8 — register bookkeeping, in the implementation commit + +- **AP-137 is REWRITTEN, not deleted.** Its cell-less enqueue-vs-place delta + (part R2) retires — that is this slice's headline. But the row also records + the two surviving acdream-only states (null classification during the login + window; `RejectedData`/`RejectedAuthority` applied through + `UnroutedCatchUp`), which have no retail mechanism and therefore keep a + row. Rewrite the row to exactly the survivors plus D1's visibility-arm + deletion and D2's wire-airborne leftover shape. (The handoff says "row + deletion"; a deletion that silently dropped the surviving divergences would + violate register rule 1 — this contract overrides that wording, and the + summary reports the contradiction.) +- **AD-42 must be updated**: it cites + `src/AcDream.App/Physics/RemoteTeleportController.cs (ResolvePlacement)` as + a surviving two-call enter-world split path. That citation dies with the + class; the headless portal-arrival resync and + `PhysicsEngine.ResolvePlacement` citations remain. +- **AP-136 / AP-138 (D5 scoping text)**: both name "`RemoteTeleportController`'s + rollback" as the shipped writer that can rebucket `record.FullCellId` to a + third landblock under a retained retry. That writer is deleted; the + surviving non-Position rebucket writers are the projection materializer + (`DatLiveEntityProjectionMaterializer`) and the equipped-child renderer + (`EquippedChildRenderController.TickChild`). Update both rows and the same + claim inside `RuntimeSetPositionState`'s `CurrentCellId` doc and + `RuntimeRemotePlacementDriveController.CanAttemptDestination`'s doc + (process rule 6). +- **AP-138's Risk column gains the teleport arm as a second producer** of the + visible-without-collision residual: a remote that teleports into a + non-published landblock and stands still is exactly the AP-136/AP-138 + shape, now reachable through this arm. No new machinery — the row's + retirement path is already #309. +- **AP-135 is untouched.** + +## Deletion inventory — every file, wiring site, and test + +Files deleted (739 production lines): + +| file | lines | +|---|---| +| `src/AcDream.App/Physics/RemoteTeleportController.cs` | 605 | +| `src/AcDream.App/Physics/RemoteTeleportPlacement.cs` | 85 | +| `src/AcDream.App/Physics/RemoteShadowPlacementSynchronizer.cs` (contains BOTH `RemoteShadowPlacementSynchronizer` and `RemoteTeleportPlacementPresentation`) | 49 | + +`src/AcDream.App/Physics/RemoteTeleportHook.cs` (57) is NOT deleted — it is +the retail `teleport_hook` port and moves/re-wires per D3. + +Tests deleted (1,709 lines): + +| file | lines | +|---|---| +| `tests/AcDream.App.Tests/Physics/RemoteTeleportControllerTests.cs` | 1,515 | +| `tests/AcDream.App.Tests/Physics/RemoteTeleportPlacementTests.cs` | 194 | + +`tests/AcDream.App.Tests/Physics/RemoteTeleportHookTests.cs` (38) moves with +the hook. + +Wiring sites (the handoff's list of eight was a raw grep; the true set is +below — two sites the handoff missed, and three of its entries are +comment-only): + +| site | what happens | +|---|---| +| `src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs` | The heart of the slice. Delete: ctor param + `_remoteTeleportController` field; `remoteHardTeleport` / `remotePlacementRequired`; `RunRemoteTeleportHook` (moves per D3); the `BeginPlacement` call; the whole `remotePlacementRequired` placement block (the `TryApply` call and its `Applied`/`Superseded` tails); the classification gate's `&& !remotePlacementRequired`; the legacy `!update.IsGrounded` fallback (D2); the legacy pre-operation `ConstrainTo` call (D4). Add: the teleport arm dispatch (D5), widened guards (D6/D7). | +| `src/AcDream.App/Composition/LivePresentationComposition.cs` | Delete the `RemoteShadowPlacementSynchronizer` + `RemoteTeleportPlacementPresentation` constructions, the `remoteTeleportLease` acquisition, the `RemoteTeleport` record member, and the lease parameter threading. | +| `src/AcDream.App/Composition/SessionPlayerComposition.cs` | Three `live.RemoteTeleport` pass-throughs (network-update controller ctor, teardown controller ctor, and the third composition site) — replaced by nothing; the drive controller is already threaded. **Missing from the handoff's list.** | +| `src/AcDream.App/Net/LiveSessionRuntimeFactory.cs` | Delete the `RemoteTeleport` record member and the reset-plan binding `RemoteTeleport = _world.RemoteTeleport.Clear`. | +| `src/AcDream.App/Net/LiveSessionResetManifest.cs` | Delete the `required Action RemoteTeleport` member and its `new("remote teleport", …)` stage. **Missing from the handoff's list.** Reset coverage is not lost: the drive controller's teardown is `DetachRoute` (cancels every live operation) and its ledger convergence is already asserted. | +| `src/AcDream.App/Rendering/GameWindow.cs` | Delete the `_remoteTeleportController` field and its assignment from the composition result. | +| `src/AcDream.App/Rendering/GameWindowLifetime.cs` | Delete the `RemoteTeleportController? RemoteTeleport` shutdown-root member and the `Hard("remote teleport", …)` stage. | +| `src/AcDream.App/World/LiveEntityRuntimeTeardownController.cs` | Delete the ctor param, field, and the `_remoteTeleport.Forget(record)` cleanup entry. Per-entity teardown coverage is not lost: the drive controller self-heals on `IsPlacementCurrent` and `Forget`-on-accepted-Position retires operations. | +| `src/AcDream.Runtime/Physics/RuntimeRemotePhysicsUpdater.cs` | **Comment-only** (two doc comments citing `RemoteTeleportPlacement.Apply` — the isCurrent-delegate note and the five-writer `Airborne` list, which becomes a four-writer list; the teleport path's `Airborne` derivation is now the canonical placement commit's, already on the list). Correct both (process rule 6). | +| `src/AcDream.Runtime/Physics/RuntimeSetPositionState.cs` | **Comment-only** (the `CurrentCellId` retained-retry doc naming `RemoteTeleportController`'s rollback — D8). No code change; there is no teleport-hook machinery in this file to touch. | +| `src/AcDream.Core/Physics/EntityCollisionFlags.cs` | **Comment-only** (TS-23 history note naming the pre-P3 inlined call sites). Historical statement — rewrite to past tense or leave verifiably historical; do not let it read as a live citation. | +| `src/AcDream.Runtime/Session/RuntimeRemotePlacementDriveController.cs` | Widen `OwnsPlacement`'s scope comment (it ALREADY matches `SetPosition` + `Teleport` flag — no predicate change needed for the teleport disposition); add the teleport-arm entry point (D3); correct the `Advance()`/`CanAttemptDestination` docs' writer list (D8). | +| `src/AcDream.Runtime/Entities/RuntimeEntityObjectLifetime.cs` + `src/AcDream.App/World/LiveEntityRuntime.cs` + `src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs` + `src/AcDream.Runtime/Entities/InboundPhysicsStateController.cs` | Delete the `TeleportHookRequired` field, its `timestamps with {…}` computation, the `projectionRequiresTeleportHook` parameter/computation/`false` argument, and thread the pre-merge cell instead (D1). One doc comment in `InboundPhysicsStateController` cites `TeleportHookRequired`-adjacent bookkeeping — reword. | +| `src/AcDream.App/World/LiveEntityPresentationController.cs` | `BeginAuthoritativePlacement` and `CompleteAuthoritativePlacement(deferShadowRestore: false)` lose their only production callers (`DeferShadowRestore`, `HasActivePlacement`, `HasDeferredShadowRestore` already have none), leaving `_activePlacementOwners` write-never while `IsPlacementActive` still reads it in the visibility suspend/restore gates. Delete the dead half in this commit — do not leave a zombie set that silently gates nothing — but trace the `IsPlacementActive` consumers first and state in the commit what each gate degenerates to. If tracing shows a live non-teleport dependency, STOP and report rather than deleting blind. | + +Test files updated (not deleted) — each references the deleted machinery +incidentally: `UpdateFrameOrchestratorTests` (a `typeof(RemoteTeleportPlacementPresentation)` +row), `RuntimeEntityOwnershipTests` (two `typeof(RemoteTeleportController)` +exact-key assertions), `GameWindowLiveEntityCompositionTests` +(`[InlineData("RunRemoteTeleportHook")]`), `LiveEntityLifecycleStressTests` +(constructs the controller and calls `TryApply` — its scenario must be +re-expressed against the canonical teleport arm, not dropped), +`LiveSessionResetPlanTests` (the "remote teleport" stage), +`CurrentGameRuntimeAdapterTests` (a noop binding), +`RuntimeInitialCreateResidenceStateTests` +(`RemoteTeleportSuffixIsQueuedBehindInitialAdmission` — verify what it pins; +it is about suffix ordering under initial admission and likely survives with +a rename), plus the eight files matching `TeleportHookRequired`. + +Stale comments this slice must make true (process rule 6 — verify each +against the code beside it, and prefer symbol references): the +`// 4b deletes this fallback` line (D2 deletes the fallback); the +`// 4b-3 deletes it` on the legacy arm site (D4); the `remotePlacementRequired +guarantees the classifier's teleport disposition never reaches here` route +comment; `ClassifyRemoteAcceptedPosition`'s caller-doc sentence "whose +remotePlacementRequired gate is already false"; `SeedRemoteSpawnPlacement`'s +"Mirrors `RemoteTeleportPlacement`'s commit"; the AP-140 comment's citation of +`RemoteTeleportPlacementTests.Apply_PendingGroundToSteepContact_…` as an +`Airborne`-definition dependent (the test dies; the dependency list shrinks); +the classifier's "only a later Runtime SetPosition or simulation commit may +change FullCellId" (D1 shows it false); `TryApplyGenericRemoteRenderPose`'s +"Route 4b-3 revisits the gate" (D6 resolves it). + +## Proof obligations (must prove, not assume) + +1. **`ParkCollisionResidents`'s overlap throw stays unreachable.** The + argument is 4b-1/4b-2's, extended: every operation this route begins goes + through `TryBeginExclusiveAuthoredPlacement` (one live operation per key — + the Begin refuses a second), `DeferredCell` outcomes are cancelled + synchronously (no park survives the controller), and retained entries are + preparation retries bounded by the pre-flight re-check and the `Advance()` + window-drop. The teleport arm adds packets to the same machinery, not a + new operation shape. State this in the contract-conformance section of the + implementation commit and keep the honest caveat 4b-1's B2 established: + the guarded property is `TryAcquireCollisionPrefixMutationPermission`'s + `HasOldPrefixPlacementDebt` refusal (a stall, not a throw), and the + ledger convergence tests are the floor under it. +2. **Ledger convergence with teleports in flight**: teardown, session reset, + and generation change converge `RemotePlacementDrivePendingCount` (both + registrations) to zero with retained teleport retries present — the same + suite shape 4b-1 built, driven through the new arm. +3. **The one-arm partition (D4)**: a test that counts arming calls per packet + across the partition table's rows — exactly one for every "yes" row, + exactly zero for every "no" row. The 4b-2 lesson (R7): assert the + observable (the arm count / the anchor), not the code shape. + +## What this slice does NOT do + +- **AP-131** (shared merge call) — C5. +- **AP-135** — stays, writes preserved (invariant 8). +- **#276** — `SeedRemoteSpawnPlacement` is still not classification-gated; + the AD-61 settle is untouched. +- **#309 / AP-136 / AP-138 residuals** — the lost-cell/hidden-until-cell-load + behaviour is not built here; the teleport arm inherits the far arm's + store-and-stay-visible residual and the register rows say so (D8). +- **Route 5 (projectile)** — `OwnsPlacement` keeps excluding + `ProjectileAuthoritative`; no widening here. +- **No headless remote consumer** — `RuntimeLiveEntitySessionController` + still returns early for non-local GUIDs; the vacuous-satisfaction statement + stays in the interface doc and the AP-137 rewrite. +- **No changes to route 2's local-player ForcePosition/teleport paths**, the + local-player teleport transit (`LocalPlayerTeleportController` keeps its own + `NotifyTeleported` call), or route 1's executor. +- **The recorded-not-consumed route facts stay recorded-not-consumed**: + `UnparentBeforeRouting` / `ApplyPlacementFrameBeforeRouting` have no reader + today (the unparent edge is owned by the merge's `EndChildProjection` and + the hydration recovery); this slice adds no reader and does not delete the + facts. + +## Test plan + +Tests must assert the layer that broke historically (process rule 4 — +presentation/visibility, not only `InWorld`/clock/residency), and every new +test must fail against a broken implementation (no source-text pins, no +tautologies). + +Focused Runtime tests (`tests/AcDream.Runtime.Tests`): + +1. Teleport-classified commit: body at resolved destination, `FullCellId` = + resolved cell, hook ran (moveto cancelled, stick released, interp queue + empty, leash re-armed post-operation at the post-move anchor), clock + active, `InWorld`. +2. Teleport refused (destination outside the service window): pose STILL + advances to the accepted destination (invariant 1), no park opened, no + operation retained, entity `InWorld` + clock active + spatial root intact, + AND the hook still ran (retail runs it before the placement decision). +3. Teleport `RejectedByPlacement` (engine refused): pose does NOT move; + `Cancelled`-after-commit: settled pose survives. (The far arm's tests + exist; these drive the teleport arm through the same partition.) +4. Cell-less classification now fires: a record whose PRE-merge committed + cell is 0 (unwield-to-3D shape) classifies `SetPosition` and places + unconditionally — the AP-137-retiring behaviour. Companion: the same + packet with a nonzero pre-merge cell and no TELEPORT_TS advance does NOT + classify `SetPosition` (proves D1's input is the pre-merge value, not the + post-merge wire cell — this is the test that discriminates the fix from + the shipped dead predicate). +5. D4 partition: arm-count table test (proof obligation 3), including the + wire-airborne null/`Rejected*` no-arm rows and the + teleport-arm-on-every-outcome row. +6. D5 ordering: an airborne-body teleport packet places (does not + `AirborneSnap`, does not take the landing block); a stuck NPC's teleport + packet runs the hook (`UnStick`) and places despite TS-44's suppression. +7. Currency: teleport arm's synchronous receipt deletes/replaces the + incarnation → nothing further written for the packet (guard before arm, + both arms — the R5 shape, now for the teleport arm). +8. Ledger/teardown: proof obligation 2. + +App-layer tests (`tests/AcDream.App.Tests`): + +9. **The presentation assertion (#312's layer):** after a remote teleport + commit, the render `WorldEntity` pose equals the resolved body pose, + `ParentCellId` equals the resolved cell, the entity is spatially visible, + and the collision shadow was published. After a refused teleport, the + render pose tracks the stored destination and the entity REMAINS visible. +10. D2's leftover-airborne shape: wire-airborne null-classified packet writes + exactly AP-135's two fields and nothing else (body, entity, queue, leash + all untouched). +11. AP-135 preservation on the rewritten arms (both airborne no-op paths). +12. The generic render-pose + wire-cell-adoption gates: teleport arm takes + the early write and suppresses the post-routing wire-cell adopt (D6) — + asserted through the existing extracted entry points, not restated + logic. + +Live-execution proof (process rule 5): a `[remote-teleport]` probe line — +`PhysicsDiagnostics`-owned, `ACDREAM_PROBE_REMOTE_TELEPORT=1`, one line per +routed teleport arm with guid, cause (`teleport-ts` vs `cellless`), hook-ran, +and placement status, marked TEMPORARY with the existing probe family. The +connected gate below is recorded as a pass ONLY if the probe line shows the +new arm executed (a clean-looking session with zero probe lines is a +not-run, exactly like #309's park probe). + +## Gates + +- Focused Runtime + App tests above. +- Complete Release suite: `$env:ACDREAM_PAK_PATH` set, + `dotnet test AcDream.slnx -c Release -m:1`. **Baseline 11,027 passed / 4 + skipped / 0 failed** at `2eb39a02`. The net count will move (1,709 test + lines deleted, new tests added) — measure and record the new figure; do not + inherit 11,027 as the expectation. Two known flakes, do not chase and do + NOT conflate: **#302** (`PortalProjectionTests.ClipToRegion_FrameOwnedStore_…`, + GC-allocation assertion, App.Tests) and **#308** + (`NakEmissionTests.LossSoak_…`, wall-clock deadline, Core.Net.Tests, + full-suite load only). If either appears, re-run and say which. +- **Two-client connected teleport gate (user-run).** **CORRECTION (2026-08-04 + fix round, both independent Opus reviews):** the recipe below originally + specified "a second character" as the teleport target. That is WRONG — a + player-character target cannot exercise this arm's NPC/creature code path + at all, and two of the three MAJOR defects the fix round found (A1's + zero-arm leash regression, A2/R3's synthesized-velocity/run-cycle defect) + are BOTH on the NPC-guid branch only; `RemoteServerControlledVelocityCycle.Apply` + itself early-returns for any `0x50xxxxxx` guid, so a player-target run + would report a clean pass while both defects shipped underneath it. **The + target MUST be an NPC/creature** (an ACE admin teleport — `@teleto` / + `@teleloc` — applied to a drudge/mosswart/etc., NOT a second player + character) for the gate to see what it is supposed to see. Recipe: acdream + stands as observer; the creature target is teleported with an ACE admin + teleport (`@teleto` / `@teleloc`) — those route through `Teleport()` / + `SendUpdatePosition(true)` and advance **ObjectTeleport** (TELEPORT_TS), + which is exactly this arm's trigger on the observer (`@pklite` is route + 2's ForcePosition lever, NOT this gate — see + [`2026-08-03-c4-route-2-visual-gate.md`](2026-08-03-c4-route-2-visual-gate.md)). + Run with `ACDREAM_PROBE_REMOTE_TELEPORT=1`. Correct: the observed creature + vanishes from the old spot and appears at the destination in one step (no + glide, no interpolated streak), **stands STILL with correct idle + animation — NOT sprinting/running in place** (A2/R3's specific symptom: + a synthesized teleport-distance velocity planning a RunForward cycle at + the destination), and moves normally afterward (leash re-armed: no + rubber-band, no tether — A1's specific symptom is the ABSENCE of a + rubber-band/re-anchor where one should exist, since a creature that was + knocked airborne on its first accepted Position after this teleport would + otherwise never arm at all). Also teleport the remote OUT of view and + back: it must re-appear correctly. Regressions to watch: a remote gliding + across the map at teleport (queue not cleared), freezing at the old spot + (the 4b-2 round-1 freeze class), invisible-but-audible at the destination + (#312 class), invisible-but-solid (#184 class), a rubber-band after + arrival, or the creature sprinting/running in place at the destination + (A2/R3). Confirm at least one `[remote-teleport]` line per teleport, with + the expected cause. Graceful close (ACE session-clear) per the standing + rule. + +## Budget + +**~400-700 non-comment production lines** (net LOC strongly negative — the +deletions are 739 production + 1,709 test lines). For calibration: 4a was +364, 4b-1 was 230+57, 4b-2 landed within its 350-500. Exceed 700 new lines +and STOP and report rather than pushing through. + +## Open questions routed to the retail-conformance reviewer + +1. **D1's evidence chain** (the merge stamps `FullCellId` with the wire cell + before classification, making the shipped cell-less predicate dead) rests + on the reading `TryApplyPosition` → `RefreshSnapshot(refreshPosition: + acceptedPosition)` → `RefreshDerivedState` → `SetFullCell(wire cell)`, + with classification after the gate and before the prologue rebucket. + Verify independently — it is the load-bearing claim of this contract, and + if it is wrong the D1 plumbing is unnecessary churn. +2. **`report_collision_end(this, 1)` ↔ `ShadowObjects.Suspend(localEntityId)`**: + the existing hook maps retail's collision-end report to a broadphase + shadow suspension. This mapping predates 4b-3 and is carried, not + re-derived. Confirm against @0x00514F31's callee that the "1" argument + (report-to-partners) has no unported half, or file the delta on the + AP-137 successor row. +3. **The `LiveEntityPresentationController._activePlacementOwners` deletion** + (deletion-inventory last row): confirm by reading `IsPlacementActive`'s + two consumers that removing the write-never set cannot change a + Hidden/UnHide or visibility-edge restore for non-teleport entities. +4. **`RemoteTeleportSuffixIsQueuedBehindInitialAdmission`** + (`RuntimeInitialCreateResidenceStateTests`): confirm what it pins and + that it survives (renamed) rather than being deleted as collateral. + +## Contradictions with the handoff/scoping docs — reported, not smoothed over + +- **The handoff's "AP-137 row deletion belongs in the implementation + commit"** conflicts with the row's own content: null and `Rejected*` are + acdream-only divergences that survive this slice and must keep a row. + Pinned as rewrite-in-place (D8). If the reviewer prefers + delete-and-refile-narrow, either satisfies register rule 1; silent whole-row + deletion does not. +- **The handoff's wiring-site list (8 sites) is a raw grep, not a wiring + list**: `SessionPlayerComposition.cs` and `LiveSessionResetManifest.cs` are + real wiring sites it missed; `RuntimeRemotePhysicsUpdater.cs`, + `RuntimeSetPositionState.cs`, and `EntityCollisionFlags.cs` are + comment-only. +- **The handoff's "`RuntimeSetPositionState.cs` — the teleport-hook phase + support and SetFlags analog"**: there is no teleport-hook phase support in + that file. The phase lives on the classifier route + (`RuntimeTeleportHookPhase`), and the SetFlags analog is the `flags` + parameter of `TryPrepareAndSubmitAuthoredPlacement` fed from + `route.SetPositionFlags`. +- **The scoping doc's T1 ("they disagree in both directions")** understates + the finding: as fed today the classifier's cell-less predicate is not + merely different — it is unreachable for remote PositionEvents (D1). The + design decision is therefore not "reconcile two predicates" but "feed the + classifier retail's predicate at all". +- **The scoping doc's T2** describes the pre-4b-2 tree; 4b-2 already + established the null/`Rejected*` policy. What 4b-3 actually decides is the + wire-airborne leftover shape (D2), which neither doc names. diff --git a/docs/research/2026-08-04-c4-route-4b-3-retail-review-round2.md b/docs/research/2026-08-04-c4-route-4b-3-retail-review-round2.md new file mode 100644 index 00000000..1381b512 --- /dev/null +++ b/docs/research/2026-08-04-c4-route-4b-3-retail-review-round2.md @@ -0,0 +1,352 @@ +# C4 route 4b-3 — retail-conformance review, round 2 (delta) — 2026-08-04 + +Delta review of the round-1 fix pass, same working tree, still uncommitted. +Round 1: [`2026-08-04-c4-route-4b-3-retail-review.md`](2026-08-04-c4-route-4b-3-retail-review.md). +Adversarial review: [`2026-08-04-c4-route-4b-3-architecture-review.md`](2026-08-04-c4-route-4b-3-architecture-review.md). + +Scope: the deltas only. Everything round 1 verified sound and did not flag +(the retail address set, D1's evidence chain, D3's hook order, the +`SetFlags(0x1012)` mapping, D5's routing order, D6/D7, invariants 1/2/4/10/11, +the AD-42/AP-136/AP-138 updates) was re-checked for *disturbance*, not +re-derived. + +--- + +## VERDICT: **PASS** + +All three MAJOR findings are fixed, and each fix is retail-correct on the +merits rather than merely symptom-suppressing: + +- **R1/A7** — the D2 return-0 shape is now shared by both branches, so + retail's "no `this == player` distinction" holds on this shape for the first + time. AP-137's claim is now true. +- **R2** — the hook's sixth action now routes to the *actual* port of + @0x00514620, and `LeaveWorld`'s retained-peer / retained-environment + semantics match retail's arg2=1 behaviour exactly (verified below, since + that was the remaining open question). +- **R3/A2** — the synthesized velocity and the cycle plan are both excluded + for a teleport-classified route, and the exclusion is *positive* rather than + relying on the incidental moveto guard the hook itself destroys. +- **A1** — `ToConstraintArm`'s `AirborneSnap` mapping is now an arming value. + **I scrutinized this hardest per the coordinator's request and it is + correct** — see §1. + +Two of my round-1 MINORs (R7, R9) were fixed by rewriting the comments rather +than the code, which is the right call in both cases. R4, R5, R6, R8 are +genuinely fixed. No new retail divergence introduced by any fix. No fix +disturbed anything round 1 had verified. + +Gates observed: `dotnet build AcDream.slnx -c Release` **0 warnings / 0 errors**; +`AcDream.Runtime.Tests` **1,125 / 0 skipped / 0 failed**; `AcDream.App.Tests` +**4,081 / 3 skipped / 0 failed**. Per process rule 3 none of this is treated as +conformance evidence; the discriminating power of the three new tests is +assessed separately in §5. + +--- + +## 1. A1's fix — is `NearInterpolate` the retail-correct arm for `AirborneSnap`? + +**Yes, and for the right reason.** The coordinator asked me to check the +mapping's *whole partition* against "retail returned nonzero", not the arm +label. That is exactly the right framing, because retail's arming is not +per-arm at all — `ConstrainTo` @0x00454272 sits inside +`if (MoveOrTeleport(...) != 0)` @0x00454254, one site, and every branch of +`MoveOrTeleport` that does anything returns 1. + +`TryArmConstraintAfterOperation` consumes the arm value **only** as an +arms/doesn't-arm discriminator (it is a `switch` yielding `bool`), so the label +is cosmetic and the only question that matters is which side of the partition +each `RemoteContactArm` lands on. Re-derived: + +| `RemoteContactArm` | reachable when | retail's `MoveOrTeleport` | must arm? | maps to | arms? | +|---|---|---|---|---|---| +| `TeleportPlacement` | teleport/cell-less classification | branch @0x00516386, `return 1` @0x00516438 | yes | `TeleportPlacement` | ✓ | +| `FarSnapPlacement` | `SetPositionSimple`, ≥96 m | far branch, `return 1` @0x005163E8 | yes | `FarSnapPlacement` | ✓ | +| `SteadyStateInterpolate` | `Interpolate`, <96 m | near branch, `return 1` @0x005163BE | yes | `NearInterpolate` | ✓ | +| `AirborneSnap` | **wire-grounded** packet whose *body* lacks a contact plane | near or far branch (the wire bit is set, so `arg4 != 0`), `return 1` | **yes** | `NearInterpolate` | ✓ **(the fix)** | +| `UnroutedCatchUp` | null/`Rejected*`, wire-grounded | no retail state; analogue is nonzero | yes | `UnroutedCatchUp` | ✓ | +| retail's `arg4 == 0` | — | `return 0` @0x0051636D | no | *not expressible* → `throw` | n/a | + +The load-bearing claim is the fourth row's "wire-grounded". I verified it holds +on **both** branches after the fix, by enumerating every classification that +can reach `ApplyRemoteContactRouting`: + +- A wire-airborne packet can only classify `NoPositionOperation` (classifier's + `!effectiveContact` branch, `RuntimeAuthoritativePositionRouteClassifier.cs:430-448`), + `null`, `RejectedAuthority`/`RejectedData`, or `SetPosition` (the teleport + branch, which precedes the contact test). `Interpolate` and + `SetPositionSimple` are unreachable with a clear wire bit. The + `SameIncarnationCreate` `effectiveContact` override cannot apply here — this + path always passes `RuntimeAcceptedPositionSource.PositionEvent` + (`RuntimeEntityObjectLifetime.cs:643`). +- `NoPositionOperation` returns at each branch's `IsAirborneNoOperation` gate + (player `:2313`, NPC `:2692`). +- `SetPosition` dispatches ahead of the carve-out (player `:2330`, and inside + `ApplyRemoteContactRouting` itself for the NPC path). +- `null`/`Rejected*` now return at each branch's D2 call (player `:2370`, + **NPC `:2717`, the R1 fix** — and this is precisely why the fix had to land + for the mapping to be sound). + +So after the fix, **nothing wire-airborne reaches the carve-out**, and +`AirborneSnap` implies `arg4 != 0` implies retail returned nonzero implies +retail armed. The mapping's partition is exactly "retail returned nonzero", and +the one case it cannot express (retail's true `arg4 == 0` no-op) is the one the +throwing default now enforces as unreachable rather than silently absorbing. + +Two supporting observations: + +- The `NearInterpolate` *label* is semantically loose for a hard snap, but the + justification given ("the same arming value the player arm's own LANDING + TRANSITION block already uses explicitly for the identical scenario — + grounded wire, body not in contact") is accurate: `:2469-2471` hard-codes + `NearInterpolate` for exactly that packet shape. Consistency between the two + sites is worth more here than a new enum member, and the doc block at + `:1454-1480` states the reasoning rather than asserting the conclusion. +- The A1 fix is *dependent on* the R1 fix. Had R1 been declined ("scope the + AP-137 row to the player arm" — the option the architecture review offered + at A7), `AirborneSnap` would still be reachable wire-airborne on the NPC arm + and mapping it to an arming value would have produced the **opposite** + divergence (arming where retail returns 0). The two fixes are only jointly + correct. Worth noting in the commit message so a future revert of one does + not silently invert the other. + +--- + +## 2. R2's fix — `LeaveWorld` vs retail @0x00514620 with `arg2 = 1` + +Per instruction I did not re-litigate the `LeaveWorld`-vs-`ForceEnd` choice +(the coordinator's reasoning about `_admissionBlocked` is right, and `ForceEnd` +is private). The remaining open question was whether `LeaveWorld`'s stated +retained semantics — *"Incoming peer records and the environment latch are +intentionally retained"* — match retail. **They do, on both halves.** + +Re-read of `report_collision_end` @0x00514620 (line 282526ff) and +`report_object_collision_end` @0x00510A90 (line 278590ff): + +| retail behaviour | acdream `LeaveWorld` → `ForceEnd` → `EndExpiredObjectCollisions(force: true)` | match | +|---|---|---| +| Iterates **only** `this->collision_table` (the owner's own table) | `_owners[ownerKey].Order`/`.Records` only (`:1115-1141`) | ✓ | +| `arg2 != 0` bypasses both the >1 s staleness test @0x005146D2 and the still-touching test @0x005146DA (`goto label_514706` in each) | `if (!force && !(age > 1d) && !(collision.Ethereal && age > 0d)) continue;` — `force` short-circuits both (`:1132-1137`) | ✓ | +| Deletes the complete selected set **before** the first callback (`DeleteCurrent` in the walk, callbacks in the tail loop @0x0051474D) | "Retail deletes the complete expired set before issuing any end callback" — precommit loop at `:1149-1155` ahead of the callback loop at `:1164` | ✓ | +| Fires `DoCollisionEnd` on **both** weenies, each gated on its own `state & 8` (REPORT_COLLISIONS) @0x00510AC8 / @0x00510AE0 | `PublishResolvedObjectEnd` publishes owner→target and target→owner, each gated on that side's `PhysicsStateFlags.ReportCollisions` (`:1234-1268`) | ✓ | +| Does **not** touch the partner's own `collision_table` — the partner's record of `this` survives to its own expiry/force pass | only `RemoveReverseOwner(collision.Key, ownerKey)` (`:1154`), which prunes the Runtime-side `_ownersByPeer` **index**, not the peer's `Records` | ✓ ("incoming peer records … retained") | +| Does **not** read or write `colliding_with_environment` — that is `handle_all_collisions`' business @0x00514800 | `CollidingWithEnvironment` is untouched by `ForceEnd`/`EndExpiredObjectCollisions`; only `HandleReports` (`:847-859`) writes it | ✓ ("the environment latch … retained") | + +The doc sentence is therefore not a caveat to be excused — it is a precise +statement of retail's own scoping. **No residual delta; nothing owed to the +register.** Correspondingly, AP-137 carrying no `report_collision_end` row is +now correct (round 1's requirement was "confirm *or* file"; the fix confirms). + +**Dropping the `ShadowObjects.Suspend` side is also right, and safe.** Retail's +`teleport_hook` does not remove shadows; `SetPositionInternal` does that itself +as part of the commit. The strongest argument for safety is one the doc does +not make explicitly and should: **the far arm has never suspended either**, and +it reaches the engine through the identical `TryExecuteAcceptedRemotePosition` +→ `SubmitAndResolve` path. So after this fix the teleport arm's shadow handling +is byte-identical to the far arm's, which has been through two live gates. The +frames-scale intangibility window round 1 flagged is closed by construction +rather than by compensation. + +--- + +## 3. R1's and R3's fixes + +**R1 — `ApplyWireAirborneLeftoverBookkeeping`, both branches.** Retail-correct: +`arg4 == 0` → `return 0` @0x0051636D writes nothing, and the two writes that +remain are AP-135's acdream-only free-fall-sweep bookkeeping, whose row is +untouched. The NPC call site (`:2717-2722`) is correctly placed *after* the +`IsAirborneNoOperation` return and *before* the synth-velocity block and the +sticky gate, so a wire-airborne leftover NPC packet no longer reaches the +free-flight carve-out, the arming site, or the render/shadow publish. **AP-137's +"unifies player and NPC remotes on one behaviour" sentence is now TRUE** — I +re-read the row and traced both call sites. + +One side effect worth stating and not a defect: the NPC arm's synth-velocity +install no longer runs for wire-airborne leftover packets (they return above +it). That moves *toward* retail, which writes no velocity on the `return 0` +branch. + +**R3/A2 — the `!isTeleportRoute` gates.** Both halves are gated (install +`:2741`, cycle apply `:2886`), and the gate is positive rather than relying on +`rm.MoveTo`, which the hook's own `CancelMoveTo` invalidates — the reasoning in +the comment at `:2724-2740` is correct and is the non-obvious part. Retail's +teleport branch writes no velocity (verified round 1: `set_velocity` @0x004541B4 +is inside the *local player's* branch only), so this is the faithful shape. + +`rmState.LastServerPos` is advanced to the destination at `:2884`, after +routing, so the *next* packet's synthesis no longer spans the teleport jump — +the comment claims this and it holds. + +Non-finding, checked because it looked like one: a stale `HasServerVelocity` +from a pre-teleport packet survives the teleport packet, but nothing consumes +it into a cycle in that window (`RuntimeRemotePhysicsUpdater.cs:198-208` only +*zeroes* it, and `RemoteServerControlledVelocityCycle.Apply`'s only other +caller is that zeroing path). And a creature that was mid-run when teleported +keeps its current cycle — which is also what retail does, since `teleport_hook` +cancels the moveto but never calls `SetCycle`. + +--- + +## 4. The two new shared helpers — per-call-site behaviour delta + +Checked each site against the code it replaced. + +**`ApplyWireAirborneLeftoverBookkeeping` (2 sites).** + +- Player `:2390` — previously wrote `LastServerPos`/`LastServerPosTime` only; + now also `remote.CellId = wireCellId`. Verified genuinely redundant: this arm + already wrote `rmState.CellId = p.LandblockId` unconditionally at `:2251`, + and the argument passed is the same `p.LandblockId`. **No behaviour change.** + The comment at `:2386-2389` states the redundancy, which is the honest + framing. +- NPC `:2719` — new, and the intended R1 fix. + +**`RunRemoteArmTail` (3 sites).** + +- Player teleport `:2333` — the old inline guard was *unconditional*; the + helper's is gated on `FarSnapPlacement or TeleportPlacement`. Equivalent + here, because the caller has already tested `OwnsTeleportPlacement(route)` + and `ApplyRemoteContactRouting` tests the same predicate first, so the arm is + always `TeleportPlacement`. **No behaviour change.** +- Player grounded `:2566` — old guard was `FarSnapPlacement` only; the helper + adds `TeleportPlacement`, which is unreachable at this site (dispatched and + returned above). **No behaviour change.** +- NPC `:2818` — old guard was already `FarSnapPlacement or TeleportPlacement`. + **Character-identical.** + +All three previously computed `willBeDrTicked` as +`WillAdvanceRemoteMotion(update.Guid, rmState)`, which the helper reproduces, +and all three retain guard-before-arm (the R5 invariant). The only intended +delta is the hook closure now capturing `canonical`/`remote` instead of +`update.Guid`/`entity.Id` — which is R6's fix, and is a strict improvement: +`RunRemoteTeleportHook` no longer re-resolves the `RemoteMotion` by GUID, so +the six actions operate on the same `rmState` the placement does, and +`hookRan=True` can no longer be printed for a hook that silently no-opped. + +**One incidental simplification I checked separately because it was not flagged +by either review:** the NPC velocity block's `!IsPlayerGuid(update.Guid)` +guards (both the synth condition and the `else if` zeroing branch) were dropped +when the block was wrapped in `if (!isTeleportRoute)`. This is safe — the block +sits below `if (IsPlayerGuid(update.Guid)) { … return; }`, and I confirmed +every path inside that block returns (`:2316`, `:2367`, `:2396`, `:2541`, +`:2668`), so `IsPlayerGuid` is unconditionally false there. **No behaviour +change**, but it is an unremarked edit inside a fix hunk; worth a line in the +commit message. + +--- + +## 5. Test discrimination + +The three new App tests target the three MAJORs and each one fails against the +pre-fix code by construction, not by coincidence: + +- `NpcAirborneSnap_LandingPacket_StillArmsTheLeash` — asserts + `host.PositionManager.Constraint` is null before and non-null after. The only + path that creates it for this packet is + `TryArmConstraintAfterOperation(ToConstraintArm(AirborneSnap))`; the pre-fix + mapping returns `AirborneNoOperation`, which never arms, so the assertion + fails. The packet shape genuinely produces `AirborneSnap` (creature guid, + `teleportSequence` matching the spawn so no TELEPORT_TS advance, wire + grounded, `TransientState = Active` so no contact bit). This is the + end-to-end arm-count coverage proof obligation 3 asked for and round 1 found + missing. +- `NpcTeleport_DoesNotInstallASynthesizedVelocity` — deliberately seeds + `LastServerPos`/`LastServerPosTime` so a broken implementation has a real + distance and interval to synthesize from. Without that seeding it would have + been a degenerate first-packet no-op, i.e. a tautology; the doc says so. +- `NullClassifiedNpc_WireAirbornePacket_WritesOnlyBookkeepingNoBodyOrShadow` — + asserts no body write and exactly one spawn-pose shadow entry. Pre-fix, the + free-flight carve-out hard-snaps the body to `wirePos`, so both fail. + +**Minor gap (not blocking):** that last test's doc claims "body, entity, queue, +leash all untouched" but only asserts body and shadow. Adding +`Assert.Null(host.PositionManager.Constraint)` and an interp-queue-depth +assertion would make the test say what its name and doc say. Cheap, and the +leash half is the one D4's partition table calls out as a "no arm" row. + +--- + +## 6. Spot-check of round-1 MINORs + +| # | status | +|---|---| +| R4 (rebucket comment claimed it feeds `CommittedCellId`) | **fixed** — `:2036-2038` now reads "commits its canonical FullCellId (the ConstraintDistance cell key)"; the false feedback clause is gone and the surviving clause is true. | +| R5 (architecture docs describe deleted classes as live) | **fixed** — `acdream-architecture.md:467-468` and `code-structure.md:265, 418-419` now record the deletion and name the canonical placement owner. | +| R6 (hook re-resolved by GUID; probe could lie) | **fixed** — `RunRemoteTeleportHook(canonical, remote, isCurrent)` takes the caller's own `rmState`; `host` comes from `remote.Host`, the same bound reference. A null host is now genuinely "the manager doesn't exist yet", matching retail's per-manager guards. | +| R7 (`hookRan` consumed only by the probe) | **fixed as documentation** — `:1103-1108` states the retail justification (@0x005163EF runs regardless of what @0x00516420 yields). Correct call: the behaviour was already right. | +| R8 (undocumented second null-producer) | **fixed** — AP-137 now names the dormant initial-residence enqueue path explicitly, *and* carries my "unverified from static reading" flag forward rather than upgrading it to a claim. That is the right handling of a flagged unknown. | +| R9 (duplicate AP-135 writes / misleading comment) | **fixed as documentation** — the shared helper's doc and the player call site both state the write is redundant there and load-bearing on the NPC arm. | +| A5 (landing-block comment said "always classifies Interpolate") | **fixed** — `:2455-2467` now enumerates the four classifications that reach the block and states why `NearInterpolate` is correct for all of them. Consistent with §1's partition. | +| A8 (dead `wasCellless` local) | **fixed** — the local is gone; the surviving comment at `:1757-1766` refers to `beforeCell`, which still exists. | + +--- + +## 7. The corrected gate recipe + +**The correction text is right, and its "why" is right.** I verified both +claims independently: + +- `RemoteServerControlledVelocityCycle.Apply` early-returns for + `0x50xxxxxx` guids (`:27-49`), so A2/R3 is structurally invisible to a + player target — and the player arm is doubly immune because its teleport + block returns at `:2367`, above the player synth-velocity at `:2608`. +- A1 is invisible to a player target because `AirborneSnap` is unreachable on + the player arm: the LANDING TRANSITION block returns whenever + `!Body.InContact`, so routing is only ever entered with a body in contact. + The comment at `:2548-2550` states exactly this and is accurate. + +The wording is precisely scoped ("cannot exercise this arm's **NPC/creature +code path**"), which matters — a player-target teleport is still a useful run: +it exercises the hook, the placement, invariant 2's presentation sync, the +queue clear, the leash re-arm, and the probe. It is just not sufficient. The +recipe now names the two specific symptoms (sprinting in place; absence of a +re-anchor) rather than only the generic ones, which is what makes it a gate +rather than a look-around. + +--- + +## 8. Judgment on the disclosed-not-fixed items + +**Acceptable, all three**, with one filing recommendation. + +1. **No dedicated bidirectional collision-partner-notification test for the R2 + path — ACCEPTABLE.** The bidirectional publication and the + delete-before-callback ordering are already covered by + `RuntimeCollisionReportingStateTests` against `LeaveWorld` itself + (`:1304`, `:1431`, `:1524`), and §2 verifies the semantics against the + decomp directly. What is genuinely untested is the *wiring* — that the + teleport hook reaches `LeaveWorld` at all. That is a one-line assertion on + the existing teleport fixture (observe the report stream, or assert the + owner's table is empty after the packet) and is worth adding, but its + absence does not put a retail divergence in the tree. +2. **A3, the stress test's hand-written teleport step — ACCEPTABLE as + disclosed.** It is a coverage loss, not a false pass: the Hidden/ + DeferredShadowRestore half of that scenario still discriminates, and the + teleport arm itself now has three dedicated App tests plus seven Runtime + ones. Re-expressing it against the canonical arm is a follow-up, not a + blocker. +3. **The per-packet `runTeleportHook` closure — ACCEPTABLE, but file it.** It + is a real regression against `3e002993`, where the inner lambda was + allocated only when `remoteHardTeleport` was true; now a display class plus + delegate is allocated for every remote accepted Position. It is on the + packet path (5-10 Hz per remote), not the physics-resolve path Slice I's + "0 B/resolve" discipline governs, so it does not violate a standing gate. + But it is a one-line fix (cache a `Func` per controller, or pass the + hook as a method group with the state already in scope) and the next slice + adds a fourth call site. The `teleportStatus.ToString()` half is already + fixed by hoisting the probe guard to the call site (`:1123`). + +--- + +## 9. Still owed (unchanged from round 1, not blocking this verdict) + +- The complete Release suite figure the Gates section requires ("measure and + record the new figure; do not inherit 11,027") — the work is still + uncommitted, so there is no commit message carrying it. +- Proof obligation 1's `ParkCollisionResidents` statement (architecture review + A9) — owed to the implementation commit. +- The two-client connected gate, now correctly specified as a **creature** + target. +- The joint dependency between the R1 and A1 fixes (§1) belongs in the commit + message: reverting R1 alone would invert A1's mapping from correct to + wrong-in-the-other-direction. diff --git a/docs/research/2026-08-04-c4-route-4b-3-retail-review.md b/docs/research/2026-08-04-c4-route-4b-3-retail-review.md new file mode 100644 index 00000000..62518b62 --- /dev/null +++ b/docs/research/2026-08-04-c4-route-4b-3-retail-review.md @@ -0,0 +1,449 @@ +# 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.