acdream/docs/research/2026-08-04-c4-route-4b-3-retail-review-round2.md
Erik 8c269ad1a6 docs: C4 route 4b-3 contract and both dual-review rounds
The pinned contract plus the four review documents behind 6dc7ba51:
round 1 (retail FAIL 3 MAJOR / architecture FAIL 2 MAJOR) and round 2
(both PASS on the fixed diff).

Process findings worth carrying into the remaining routes:

1. The contract's 13 explicit "what must REMAIN true" invariants were the
   fix for 4b-2's round-1 defect (a contract that said what must change but
   not what must stay). They did NOT prevent a round-1 FAIL here. What the
   round-1 MAJORs actually shared was a STRUCTURAL cause the invariant list
   could not express: two parallel inline copies of the same routing tail.
   An invariant list constrains behaviour; it cannot see duplication.

2. Both reviews independently found the same NPC synth-velocity defect
   (retail R3 = architecture A2). Independent convergence on one finding is
   the strongest signal this process produces — weight it accordingly.

3. The specified two-client gate could not have observed three of the four
   MAJORs: it teleported a player character, and all three live on the NPC
   arm. Caught by both reviewers before the gate ran, not after. Check that
   a gate can structurally see the defect class it is gating.

4. A reviewer named a symbol that does not exist (RuntimeCollisionReportingState
   .ForceEnd is a private helper, not the public entry point). The implementer
   silently substituted the correct one (LeaveWorld) while reporting "no
   disagreements with either review". Verify implementer claims against source
   even when they report full agreement.

5. A new test asserted only what must NOT happen, so an emptied
   ApplyWireAirborneLeftoverBookkeeping passed every test in the tree. Caught
   in round 2 and closed with positive assertions, sabotage-verified. Negative
   assertions alone cannot detect a deleted write.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-04 16:00:26 +02:00

22 KiB

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. Adversarial review: 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.
  • A1ToConstraintArm'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 expressiblethrow 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 LeaveWorldForceEndEndExpiredObjectCollisions(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 TryExecuteAcceptedRemotePositionSubmitAndResolve 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 == 0return 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) fixedacdream-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) fixedRunRemoteTeleportHook(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<bool> 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.