acdream/docs/research/2026-08-03-c4-route-2-review-findings.md

C4 route 2 — dual review findings and required fixes (2026-08-03)

Both mandated reviews returned FAIL on the first implementation pass. Nothing is committed. This is the consolidated fix list; it supersedes the implementer's own closing report where they disagree.

Reviews: retail-conformance (Opus) and architecture/adversarial (Opus), run independently against the same uncommitted diff.

Verified correct — do not churn these

Both reviews independently confirmed, with addresses:

• AuthoritativeTeleportFlags = Teleport|Slide|SendPositionEvent = 0x1012, byte-exact against CPhysicsObj::SetPositionSimple @0x005162B0.
• Leash re-arm removal is retail-correct: the FORCE_POSITION branch returns at 0x0045409D, strictly before all three ConstrainTo sites (0x00454272, 0x0045418A, 0x004541EC). The teleport / CommitPreparedPosition / ArmConstraintLeashAtCommittedPlacement callers correctly still constrain.
• Heading preservation happens exactly once, upstream in InboundPhysicsStateController.ApplyAcceptedPosition:788-798. The seam does not re-apply or drop it.
• Ack ordering is correct and fires exactly once on both the synchronous and the deferred path; the CanSendPositionEvent gate matches retail's CommandInterpreter::SendPositionEvent @0x006B4770, and correctly does NOT apply ShouldSendPositionEvent's rate limit (retail's force branch calls SendPositionEvent directly).
• Route-request field derivation matches the continuation executor field for field.
• Contract items 2, 4 (the Runtime command itself), 6, and 7 pass. AP-131 correctly not retired.
• The re-modelled PlayerMovementControllerTests are relocations, not weakenings.

Required fixes, in priority order

R1 — HIGH — the DeferredCell park cannot survive in production

RuntimeEntityObjectLifetime.cs:1636 calls Physics.SetPosition.Forget(canonical) on EVERY accepted Position, which unconditionally cancels the entity's in-flight operation. ACE broadcasts at 5-10 Hz, so any park lasting longer than ~100-200 ms is guaranteed to be cancelled before its collision generation commits — exactly the far-destination case the deferred path exists to serve.

Chain: park -> cancelled -> RetryDeferred never runs -> no Place receipt -> ReconcileAndAcknowledge never runs -> the body is never moved and no ack is ever sent. The old LocalForcePositionTransaction / BlipLocalPlayer pair applied the correction synchronously and unconditionally. Retail's BlipPlayer @0x00453940 has no "give up quietly" state at all.

Park-and-hope is not a valid mechanism here. Required direction:

1. Do not open a park that can never wake. Before submitting, establish that the destination's collision is publishable (R2), and mirror the existing C3c-R1-F7 guard shape (IHeadlessCollisionNeighborhood.IsWithinServiceWindow, HeadlessSessionWorldProjection.cs:20-27) rather than inventing a new one.
2. Where a park still legitimately occurs, the seam must detect that its operation was cancelled — RuntimeSetPositionState.IsPlacementCompletionTracked (:1245) is the existing read-only query — and re-issue the placement from the current canonical snapshot on the next accepted Position or Advance(). The snapshot already carries the latest accepted pose, so re-issuing is correct, not a replay of stale state.
3. A force correction must never be silently dropped. That is the retail invariant this route exists to preserve.

Do NOT resolve this with a timeout, a settle window, a retry counter, or by exempting ForcePosition from Forget. If you conclude the correct answer is a deliberate, cited divergence, STOP and report rather than shipping one.

R2 — HIGH — headless lost its destination-collision publication

HeadlessSessionWorldProjection.BlipLocalPlayer (deleted) called _collision.CenterOn(position.LandblockId). The headless collision neighborhood is a hard 3x3 window (BuildPublicationPlan, :462-488) moved ONLY by CenterOn. The surviving callers are spawn (:562), the controller-null login branch (:603), teleport prep (:640) and portal arrival (:683) — a ForcePosition on a live local player now reaches none of them. PumpFirstEntry (:624) polls the stale _requestedLocalPlayerCell, which nothing updates on this path either.

Restore a real mechanism (re-center plus the _requestedLocalPlayerCell update), or gate on IsWithinServiceWindow and handle out-of-window explicitly. The deleted CenterCount assertion in HeadlessSessionHostTests.cs:430 is the invariant; restore it rather than the changed number.

The in-test justification ("retail's BlipPlayer has no streaming-window concept") is true of retail and irrelevant: the window is OUR adaptation, and retail has no equivalent because retail has every landblock resident.

R3 — HIGH — login-window ForcePosition now does nothing at all

RuntimeLiveEntitySessionController.cs:229-254. Previously every accepted local Position ran _worldProjection.ProjectPosition, whose controller-null branch (HeadlessSessionWorldProjection.cs:593-607) set _requestedLocalPlayerCell, called CenterOn, and pumped _firstEntry.DriveAll(). Now a ForcePosition takes the new branch, the drive returns NotApplicable (residence active), and nothing happens.

Restore the pump for the controller-absent case, and delete the comment at :239-240 claiming "there is no legacy fallback to run instead" — there was one; it is the else branch this change routed around.

R4 — MEDIUM-HIGH — the force-ack steals a receipt the sink declined

RuntimeAcceptedPositionDriveController.cs:366-375 unconditionally calls AcknowledgeProjection(outcome.Projection). RuntimePlacementProjectionSubscription deliberately leaves a declined Place at the FIFO head for a later retry (:118-121); this consumes and destroys it.

The sink declines for real production reasons — !_spatial.IsLoaded(landblock) (LiveEntityRuntime.cs:1210-1219) and stale transit authority (RuntimePlacementPresentationSink.cs:90-96). In those cases entity.SetPosition / Rotation / ParentCellId / RebucketLiveEntity / IsSpatiallyProjected are never written, and because the generic tail is now skipped there is no second writer to cover it — the render entity silently stays put while the canonical body moved.

The RuntimeFirstEntryDriveController mirror is safe ONLY because a residence makes the sink decline by design and a follow-up ExecutorCompleted receipt re-binds presentation. Route 2 has no such follow-up. Drop the force-ack and let the subscription's retry contract stand, or provide a real follow-up binding.

R5 — MEDIUM-HIGH — Contention and Rejected silently drop the correction

RuntimeAcceptedPositionDriveController.cs:265-271 returns Contention when TryBeginExclusiveAuthoredPlacement fails; neither status creates a pending entry, and both hosts then return without blipping or acking (LiveEntityNetworkUpdateController.cs:1140). The Contention doc comment promises "a later accepted Position, or this controller's own Advance pump, retries" — the pump provably cannot retry something never recorded, and a later Position carries a different pose. Retail always applies.

The most likely trigger is R1's parked operation, which makes every subsequent ForcePosition Contention. Fixing R1 largely fixes this; the status handling must still not silently drop.

Also fix the Rejected enum doc: it claims "No SetPosition ran; no ack was sent", which is false at the two SubmitAndResolve sites (:361, :415) where a SetPosition ran and was cancelled.

R6 — MEDIUM — _pending leaks and can be silently overwritten

RuntimeAcceptedPositionDriveController.cs:289-324. Advance() exits only on record-key release or acknowledged completion. A mid-session cancellation (supersession, lost-cell deadline, ParkCollisionResidents, generation cancel — all route through ForgetPlacementCompletionCore) leaves _pending set forever, so AcceptedPositionDrivePendingCount keeps IsConverged false for the rest of the session. GameWindowLifetime.DisposeGameRuntime:490-498 throws on non-convergence.

Plan §4c required dropping the pending ack on cancellation, supersession, teardown, generation change and reset. Only teardown and reset shipped. Use IsPlacementCompletionTracked. Also: the _pending = null cleanups are all guarded by if (!firstAttempt), and SubmitAndResolve(firstAttempt: true) assigns _pending without inspecting an existing one — make it refuse to overwrite a live pending.

R7 — MEDIUM — the 0.48 fixture, the changed assertion, and the false doc

This is a test-fixture artifact, NOT a production regression. The headless fixture's LoadedSetupCollisionSource returns one sphere (Vector3.Zero, 0.48f) — centre AT the origin, bottom 0.48 m below the feet. The real human Setup 0x02000001 is (0,0,0.475) r=0.48 plus (0,0,1.350) r=0.48 (Ts46SphereListConformanceTests.cs:35-39), so the foot sphere's bottom is origin - 0.005 and a settled origin lands on the floor within 5 mm. The control is in this same changeset: the new Runtime fixture uses the dummy sphere (offset == radius) and asserts the origin lands exactly on the floor (RuntimeAcceptedPositionDriveControllerTests.cs:178).

Required:

1. Give the headless fixture the retail offset (0f, 0f, 0.475f) r=0.48 and restore the Z == 50f assertion. Changing an assertion to match new output is the plan's own forbidden move.
2. Delete the false comment at HeadlessSessionHostTests.cs:419-427 — it describes the sphere CENTRE and then asserts it about controller.Position, which is the ORIGIN (PlayerMovementController.cs:304 -> PhysicsBody.cs:153, retail CPhysicsObj::m_position.frame.origin).
3. Correct the docs/ISSUES.md #285 "retail fidelity gain" paragraph. Retail's BlipPlayer has never lifted the origin by a sphere radius. Left as-is this becomes the citation a future session trusts.

R8 — MEDIUM — acceptance gaps

• No App-layer test exists proving the generic tail no longer double-writes the local player and that the committed projection is what moves the render entity. The plan's acceptance item 2 is unmet; three App tests were deleted and replaced with a comment. Given R4, this is precisely the seam that is broken.
• RuntimeAcceptedPositionDriveControllerTests.cs:310-311 asserts acksAfterFirstResolve <= 1, so the test passes with zero acks — the DeferredCell park -> wake -> commit -> single-ack sequence is unverified, while docs/ISSUES.md claims it is covered. Fix the fixture so the contact gate is satisfied and assert exactly one, or state plainly that it is unverified. Do not leave the overclaim in the record.

R9 — LOW — hygiene

• RuntimeAcceptedPositionDriveController is public sealed with an internal ctor and all-internal members; its template RuntimeFirstEntryDriveController is internal sealed. Make it internal unless the public surface is genuinely required (if it is, say why).
• PlayerMovementController.cs:632 has a stale <see cref="BlipPosition"/> to a deleted member. Harmless only while GenerateDocumentationFile is off; TreatWarningsAsErrors is on, so it breaks the build the day docs are enabled.
• HeadlessSessionHost._currentSession is never cleared on teardown.
• Headless without a content lease leaves the drive controller null, so the ForcePosition and its ack are dropped entirely (HeadlessSessionHost.cs:568-581); previously the ack fired unconditionally.
• LiveEntityNetworkUpdateController.cs:1140-1155 fires MarkLiveOwnerPoseDirty and ObserveAcceptedLocalPosition for ANY non- NotApplicable status including Rejected and Contention — moving the streaming observer to a landblock we explicitly refused to place into.

Gate

Unchanged: complete Release solution suite, not a focused subset. Pre-change baseline is 10,844 / 4 skipped / 0 failed; the first pass reached 10,848 with the defects above, so a green suite is necessary and demonstrably not sufficient. Both reviews must be re-run on the fixed diff before commit.

---

ROUND 2 — residuals after the R1-R9 fix round (2026-08-03)

Both delta reviews returned FAIL again. Suite is green at 10,853 / 4 / 0, which again proves nothing. R2, R3, R4, R5, R6, R7 and R9 are confirmed genuinely fixed and must not be churned. Three blocking residuals remain, and two of them are defects in the R1 reissue mechanism itself.

Root of the problem: _pending has no single owner and no single lifecycle rule. Round 1 bolted reissue onto ad-hoc per-branch bookkeeping. B1 wants MORE reissuing, N1 wants LESS, and N2 wants reissue to be a DIFFERENT route — they look contradictory only because there is no unifying rule. There is one.

The unified mechanism (implement exactly this — it replaces the ad-hoc rules)

RuntimeEntityPlacementToken already carries PositionAuthorityVersion (RuntimeSetPositionState.cs:50). Make that the single decision input.

One rule: the drive owns at most one in-flight placement for the local player. After any terminal outcome, and on every Advance(), compare the committed/parked token's PositionAuthorityVersion against the live record's current PositionAuthorityVersion:

• Equal — the canonical accepted authority has not moved since this operation began. Nothing is outstanding. Clear _pending. Do not reissue.
• Advanced — a newer accepted Position arrived while we were in flight, and it may have been the thing that killed our operation. Consult the newest accepted event's disposition (do NOT reuse stale.Route):
  • still ForcePosition — reissue, re-classifying from the current record.
  • now an ordinary Apply — clear _pending and do NOT reissue. The correction was superseded by newer server truth; the ordinary route owns that pose. This is not a silent drop: retail applies each event as it arrives, and a force correction overtaken by a newer position is moot.

Route every terminal branch through one _pending funnel. No branch may assign or clear it directly.

B1 — BLOCKING — a force correction is still silently dropped (conformance)

When a parked operation wakes and its Place is ACCEPTED by the sink, the operation leaves _operations but the completion is retained. CancelCoreDeferred then returns early at RuntimeSetPositionState.cs:5141 without reaching ForgetPlacementCompletionCore, so the retained completion survives. The next ForcePosition hits HasRetainedCompletion (:1319) -> invalid token -> Contention (RuntimeAcceptedPositionDriveController.cs:289-295); both hosts return without placing or acking, and the next Advance() consumes the OLD completion and acks the OLD pose. That packet's correction is lost.

One-frame window on the graphical host only (_session.Tick() inbound dispatch precedes RetryPending() in RetailLiveFrameCoordinator); headless is immune because its pump is adjacent to the readiness check. The DECLINED-Place variant is unaffected and needs no change.

The unified rule fixes this: the new packet advanced PositionAuthorityVersion past the committed token, and the newest event is a ForcePosition, so it reissues.

N1 — BLOCKING — stale _pending causes a second placement AND a second ack

SubmitAndResolve(firstAttempt: true) (:297) never inspects _pending, and only the !firstAttempt branches clear it (:508-511, :482-484, :549-551). So: park -> Forget wipes the watch -> the same packet's ForcePosition Begins cleanly and Commits -> ack fires -> _pending still holds the dead P1 -> next Advance() finds the watch dead -> ReissueFromCanonical -> a second canonical placement and a second outbound AutonomousPosition for one server correction.

That is the double-apply/double-ack class this entire slice exists to delete (670f307c), reintroduced. AssignPending does not catch it because it is only reached on the DeferredCell/retryable branches. The unified funnel fixes it: equal versions -> clear, no reissue.

N2 — BLOCKING — reissue applies force semantics to an ordinary pose

ReissueFromCanonical (:418-447) reuses stale.Route verbatim — SetPositionSimple + Teleport|Slide|SendPositionEvent + SendPositionImmediately: true. But the commonest way a park dies is an ordinary Apply Position, whose retail route (RuntimeAuthoritativePositionRouteClassifier.cs:368-388) is Interpolate/NoPositionOperation, PhysicsSetPositionFlags.None, ConstrainPhase.BeforePositionOperation, SendPositionImmediately: false.

So the reissue converts an ordinary server echo into a hard Teleport|Slide canonical placement, sends an ack retail would never send on that branch, and skips the ConstrainTo the ordinary branch runs. My round-1 direction sanctioned re-issuing THE CORRECTION; it did not sanction re-classifying a different disposition's pose as a force. The unified rule fixes this by consulting the newest accepted disposition.

If any residual divergence remains after this, it needs a docs/architecture/retail-divergence-register.md row in the same commit.

B2 — BLOCKING (record accuracy) — the plan claims coverage it does not have

docs/plans/2026-08-02-placement-cutover.md:287 reads "R8 added the App-layer double-write source pins the plan's own acceptance item required." That is a claim of coverage. The truth, per the adversarial review:

• acceptance item 2's first half is source-pinned, not proven — the Assert.Single regex would still pass if a second write were spelled differently, and no test exercises the branch;
• acceptance item 2's second half — "the committed projection is what moves the render entity" — is uncovered at any layer. No test drives a route-2 ForcePosition through RuntimePlacementPresentationSink / TryApplyRuntimePlacementPlace and asserts the WorldEntity moved.

Correct the text to record the gap explicitly. A documented gap is acceptable; a false claim of coverage is not. Same rule that produced R7.

Non-blocking — record, do not fix in this round

• N3 — headless never calls RetryPending after construction (grep finds no caller outside src/AcDream.App/). R4's fix depends on the subscription's retry, so a declined headless Place would wedge the ordered stream. Latent, not proven reachable. File it.
• N4 — Advance() lacks the _driving reentrancy latch its template RuntimeFirstEntryDriveController.DriveAll:128-148 has. No live re-entrant path today. Hygiene.
• N5 — CenterOnAcceptedForcePosition does not restore controller.LocalEntityId = record.LocalEntityId ?? 0u (inert today, but an unreplaced deletion); _movementTruthDiagnostics.OnServerEcho no longer fires for a local ForcePosition (diagnostic only).
• R9 residue — ConstraintManager.cs:25 and PhysicsBody.cs:442 still cite the deleted BlipPosition in <c> tags (build-safe, but false docs).
• Route-1 ack — the plan required confirming whether the ack fires while an initial-Create residence owns the record. It does not; SendPositionImmediately is consumed only as a trace fact in the continuation executor (:717, :2576). Not a regression, but the plan said file it. File it.
• AD register row — the headless 3x3 collision window is now a named member of the Runtime-facing IRuntimeDirectWorldProjection contract with an ordering requirement retail has no analogue for, and no existing row covers it (AD-6 is retired; AD-2 is the graphical reveal barrier). Recommend a row.
• Stale comment — the HeadlessSessionHostTests comment references "the previous 50.48f assertion", which does not exist at HEAD.

---

ROUND 3 — final round (2026-08-03)

The round-3 adversarial review returned PASS. The round-3 conformance review returned FAIL on one item. This round closes that item and files the adversarial review's non-blocking findings. The round-2 unified _pending funnel was confirmed sound by both reviewers and was NOT restructured.

The blocker — a terminal-without-commit sent no ack (CLOSED)

SettlePending's Equal branch was reached both by a successful commit and by a terminal outcome that never committed (non-retryable prepare failure, the default: Rejected/Cancelled branch, and both Advance death branches). In the non-commit case the body never moved AND no outbound AutonomousPosition left.

The conformance reviewer proposed a committed flag plus a one-re-issue-per-Advance guard. That is more than retail requires, and the retail evidence was re-verified from docs/research/named-retail/acclient_2013_pseudo_c.txt before coding:

• SmartBox::BlipPlayer @0x00453940 (line 92528) calls CPhysicsObj::SetPositionSimple(this->player, edi_1, 1) @0x00453968 and discards its return value. BlipPlayer itself returns void.
• CPhysicsObj::SetPositionSimple @0x005162B0 (line 284276) is declared enum SetPositionError __thiscall. Other retail call sites DO test it — if (CPhysicsObj::SetPositionSimple(...) == OK_SPE) at @0x0055605D and @0x00556021 — which proves the discard in BlipPlayer is deliberate, not a decompiler artifact.
• SmartBox::HandleReceivedPosition @0x00453FD0's FORCE_POSITION branch calls SmartBox::BlipPlayer @0x00454074, stamps update_times[0] @0x00454079, then runs cmdinterp->SendPositionEvent() @0x00454091 unconditionally and returns @0x0045409D.

Retail's semantics are therefore: attempt the placement once; if it fails the body simply does not move; acknowledge regardless; never retry. No commit flag and no recursion guard are needed to express that.

Implemented. The ack is now sent exactly once per BEGUN placement, at its terminal outcome — from ReconcileAndAcknowledge on the commit path, or from SettlePending on a non-commit terminal. SettlePending gained a positionEventOwed parameter; Pending gained a PositionEventOwed field so the re-issue retry marker (which stands for a packet whose placement was never begun) cannot double-ack. The commit paths pass false because their ack has already left. The non-commit ack runs no reconciliation — the body did not move, so there is no committed frame to reconcile — and therefore carries the body's unchanged pose, which is exactly what retail's ack carries after a failed SetPositionSimple and is informative to the server: its force did not take. The CanSendPositionEvent gate was left untouched; it is retail's own (CommandInterpreter::SendPositionEvent @0x006B4770 tests the transient-state contact bits), so a legitimately airborne body still suppresses the send on both paths.

ReconcileAndAcknowledge was split so both paths share one outbound site, SendPositionEvent.

Other items closed this round

• AD-62 rewritten. The DeferredCell-park precondition is dropped — it was never required, and the never-parked failure paths reach the same outcome. The row now leads with the general rule and keeps the named shapes as examples, adds the externally-blocked Contention shape (nothing recorded, nothing pumps it) and the other PositionAuthorityVersion advances (TryApplyPickup RuntimeEntityObjectLifetime.cs:1116, CommitPositionChannelUpdate :2041, AdvanceCreateAuthority :2466), and separates the shapes that now lose only the re-apply from the narrower shapes that still lose the ack too.
• Overstated doc corrected. AcceptedForceObservation's comment claimed the record's current version equals the recorded force's version if and only if the newest accepted event was that force. False — AdvancePositionAuthority has four call sites. It is now stated as the one-way test it actually is.
• Vacuous assertion deleted. The gameActions.Count <= 2 assertion in Advanced_ReissuesWhenTheNewestAcceptedEventIsStillAForcePosition could not fail: that fixture's CommitLandblockCollision adds both landblocks at worldOffsetX/Y: 0f while the world frame places the deferred landblock at +192/+192, so the body lands over no terrain, InContact is false, and every ack is suppressed — the count is 0. It was also too loose to encode "at most one per packet". Deleted rather than shipped; the test's real discriminators (body position, PendingCount) stay.

Tests

Two added, both with a verified discrimination check (the implementation was temporarily broken in each direction and the intended test observed to fail, then reverted and re-verified):

• TerminalWithoutCommit_SendsExactlyOnePositionEventAndLeavesTheBodyUnmoved — a park retired without committing sends exactly one AutonomousPosition, performs no placement of its own, and never repeats on further pumps.
• Committed_SendsExactlyOnePositionEventAcrossTheCommitAndTheSettle — the new settle-side ack does not become a second ack on the committed path.

Two existing tests changed their ack expectation from Assert.Empty to Assert.Single, because they exercise terminal-without-commit paths whose Empty encoded the defect this round removes: Equal_ClearsPendingWithoutReissuingWhenNoNewerAcceptedAuthorityArrived and Advanced_DoesNotReissueWhenTheNewestAcceptedEventIsAnOrdinaryApply. In the latter the single ack belongs to the FORCE packet, not to the ordinary echo — retail's ordinary branch has no unconditional SendPositionEvent.

Measurement note, recorded because it corrects an assumption in this file's round-2 text: the DeferredCell park these fixtures use is the POST-engine quiescence park, so _physics.Engine.SetPosition has already moved the canonical body to the destination while the placement itself is withdrawn and parked. The new test therefore captures its unmoved/no-further-placement baselines at the park, and asserts them across the terminal settle, which is the thing under test.

Filed, not fixed

docs/ISSUES.md #293 (the DeferredCell branch still consumes Withdraw receipts the sink may have declined — the same shape R4 removed for Place), #294 (ReconcileAndAcknowledge runs before the funnel's currency guard on the deferred wake), #295 (the retry marker inflates AcceptedPositionDrivePendingCount, which is an in-flight-placement counter), #296 (a retryable prepare is reported to hosts as Contention, conflating a retained-and-pumping case with a dropped one).

Gate

Complete Release solution suite, unchanged discipline: green is necessary and demonstrably not sufficient — all four prior states were green and three were defective.