36255af0f6
Ports retail's missile Position handling into the canonical Runtime
placement owner instead of the deleted ApplyAuthoritativePosition
short-circuit. The Create/residence-window halves of the projectile
pipeline (RuntimeProjectile binding, TryBind's adopted-body branch,
the collision/shadow registration) were already canonical from prior
slices; this closes the remaining gap — how an ACCEPTED Position for
an in-flight missile is classified, placed, and presented.
Byte-decode (Step 1 hard gate, before any code was written):
CPhysicsObj::MoveOrTeleport @0x00516330-0x00516438 disassembled from
the PDB-paired binary (Capstone, x86 32-bit thiscall). `ret 0x10`
establishes four stack args; [esp+0x7c] (arg5, the velocity pointer)
is never referenced in any of the three branches (teleport/near/far).
The retail reviewer independently reproduced this by searching the
whole function body for the `24 7c` mod/rm+disp8 encoding a
`[esp+0x7c]` read would require and found zero occurrences. This
retired a fabricated `?? Vector3.Zero` fallback in the deleted method
— retail's PositionPack::UnPack initializes an absent velocity to
zero and MoveOrTeleport never installs it; the projectile's Vector
channel (RuntimeProjectilePhysicsUpdater.ApplyAuthoritativeVector)
remains the sole velocity authority for a missile. D-P5 in the
contract; the Runtime seam commits no velocity from the Position
packet at all.
The unbound-missile fix: RuntimeEntityObjectLifetime's
ClassifyRemoteAcceptedPosition now derives ProjectileAuthoritative
from a CONJUNCTIVE predicate — the Missile bit AND a bound
RuntimeProjectile whose Body is the canonical PhysicsBody — never the
bit alone. Retail places every non-player CPhysicsObj unconditionally
(there is no missile-specific placement gate in MoveOrTeleport or its
callers), so an unbindable or not-yet-bound missile taking the
ordinary remote tail is retail-faithful, not a fallback: the earlier
bit-only discriminator would have silently frozen it instead.
AP-141 records this as a deliberate, recorded divergence, not
fidelity. Retail mechanically WOULD arm a missile's ConstrainTo leash
on any nonzero MoveOrTeleport return: HandleReceivedPosition
@0x00453FD0's only kind test is player-vs-not, ConstrainTo
@0x00454272 has no kind test of its own, and CPhysicsObj::ConstrainTo
@0x00510520 creates a PositionManager on demand via
MakePositionManager @0x00510523 if one doesn't exist. acdream
deliberately does not construct that EntityPhysicsHost/
PositionManager/InterpolationManager chain for a ballistic body — the
route-5b split the C4 route 5 contract rejected — so a live missile
never shows an armed leash and never catches up via the near/
UnroutedCatchUp policy. This divergence is safe specifically because
ACE never sends UpdatePosition for a missile
(references/ACE/Source/ACE.Server/WorldObjects/WorldObject_Tick.cs:
333-334, SendUpdatePosition() commented out inside the
PhysicsState.Missile branch at :265) — every half of this row is
deterministic-test-gated only, never exercised against a real server.
AP-141 also records the surviving ConstrainTo re-anchor divergence
under clause (b): for the adopted-body case (TryBind's shared-body
branch — an ordinary remote whose Missile bit is set by a later
State packet, so it still carries a live RemoteMotion), acdream now
ports retail's teleport-branch and far-branch StopInterpolating
action (Interp.Clear()), but never re-arms or re-anchors the
inherited ConstrainTo leash the way retail's HandleReceivedPosition
@0x00454254/@0x00454272 does on every nonzero return. The risk
column's earlier wording — that a stale leash "would drag the body
toward a stale anchor" — was wrong and is retracted in this same
commit: ConstraintManager.ConstraintPos is write-only in both retail
and the port (never read by AdjustOffset), and
ConstraintManager::adjust_offset @0x00556180 only tapers or zeroes an
already-composed per-tick offset while InContact — a leash brakes
motion the interp/sticky chain already produced, it cannot pull
anything toward the anchor. The real residual is one tick of un-reset
brake accumulator, contact-gated, and it cannot move an airborne
far-snapped missile at all (the clamp branch does not run while
airborne).
NO CONNECTED GATE EXISTS for this route, by design: ACE never sends a
missile UpdatePosition (see above), so retail's own server never
exercises this code path in play. Every proof obligation here is
test-gated only — Runtime and App-level fixtures constructing the
packet directly — never a live client/server capture.
Three review rounds closed 8 MAJOR findings before this landed:
round 1 (A1 App discarded the seam's status; A2/R1 silent swallow on
an unbound missile; A3/R2 the adopted-body teleport_hook never
wired; A4/A5 zero Runtime/App test coverage); round 2 (a
ParentCellId regression introduced by round 1's own R6 finding,
which the retail reviewer retracted the following round as factually
wrong — the fix here is the REVERT to record.FullCellId, not the
relocation round 1 shipped; B2 the far-branch StopInterpolating skip
never extended to the adopted-body case; residual App/Runtime store-
path coverage; a per-packet closure contradicting the file's own
#315 cached-delegate pattern). Round 3 closed on coverage alone (no
defect): the Advance() retry arm's projectile branch — added at
round 2, semantically reordered at round 2's B5 fix (skip prediction
invalidation on a re-parked Contention, since it writes nothing) —
had never been executed by any test; two new tests drive it directly
and are sabotage-verified against both the reordering and the
retry-arm's own SyncProjectilePresentation call site. The one
recorded defect this campaign produced (the ParentCellId regression)
was caused by complying with a review finding that its own author
later retracted — the standing lesson recorded for future rounds is
that review findings are evidence to re-verify against the code, not
commands to obey unconditionally.
Complete Release suite: 11,063 passed / 4 skipped / 0 failed
(baseline 11,036 at 30d3d114, +27 new tests across this campaign).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
338 lines
22 KiB
Markdown
338 lines
22 KiB
Markdown
# C4 route 5 — architecture / adversarial DELTA review, round 2 (2026-08-04)
|
|
|
|
**Verdict: FAIL.** Three findings: two MAJOR, one MAJOR-coverage. Both MAJORs
|
|
are narrow and cheap to close (one expression, one call, one assertion, one
|
|
test), and one of them (**B2**) is a **new defect introduced by the fix
|
|
round**, in the same class as the finding it was fixing.
|
|
|
|
Round 1 raised 5 MAJOR + 6 MINOR. Eight are closed well, one is closed for the
|
|
teleport half only (**B1**), one is closed but with a regression attached
|
|
(**B2**), one is declined acceptably (A11). Detailed disposition in §3.
|
|
|
|
Scope: the uncommitted working tree, 1,626 insertions / 398 deletions across 10
|
|
files, against HEAD `30d3d114`. Independent gates run for this review: Release
|
|
build green (0 warnings / 0 errors); focused Runtime suites 59/59; focused App
|
|
suites (`ProjectileControllerTests` + `LiveEntityNetworkOnPositionCollapseMatrixTests`)
|
|
64/64. A green suite is not evidence — every claim below is traced to source.
|
|
|
|
---
|
|
|
|
## 1. New MAJOR findings
|
|
|
|
### B1 — A3's fix covers only the teleport branch; the adopted-body FAR snap still leaves the interpolation queue armed, and AP-141 now asserts the opposite as retail-faithful
|
|
|
|
**Severity: MAJOR (half-closed fix + a register row asserting behaviour the code
|
|
does not have — the defect class the contract lists by name).**
|
|
|
|
`LiveEntityNetworkUpdateController.cs:2151-2159` runs the six-action hook only
|
|
on `SetPosition`:
|
|
|
|
```csharp
|
|
if (route.Disposition
|
|
is RuntimeAuthoritativePositionDisposition.SetPosition
|
|
&& acceptedPositionCanonical.RemoteMotion is RemoteMotion adoptedRemote)
|
|
{
|
|
RunRemoteTeleportHook(...);
|
|
}
|
|
```
|
|
|
|
Retail's far branch is not covered by `teleport_hook` — it has its own
|
|
`StopInterpolating`, guarded on `position_manager != 0`
|
|
(@0x005163C9-@0x005163CB), and the remote far arm ports it exactly
|
|
(`ApplyAcceptedRemoteFarSnap`: `if (route.StopInterpolating) remote.Interp.Clear();`).
|
|
The projectile far arm ports nothing.
|
|
|
|
For the **adopted-body** shape — the one A3/R2 established as real and which
|
|
this round now has a passing test for
|
|
(`MissileAdoptedBody_TeleportCommit_UnConstrainsAndClearsInterpQueue`) — the
|
|
missile **does** have a `PositionManager` and a populated `Interp` queue, so
|
|
retail's guard passes and retail would clear it.
|
|
|
|
Concrete scenario: an ordinary remote with a live `Interp` queue gets the
|
|
Missile bit from a State packet; `TryBind`'s shared-body branch adopts the same
|
|
body; the next accepted Position classifies **far** (>=96 m), not teleport. The
|
|
projectile arm places the body at the destination and returns. The surviving
|
|
`RemoteMotion` is still in `_spatialRemotes` with a stale waypoint, and the
|
|
remote stepper drags the freshly-placed missile back toward it — the exact
|
|
symptom A3 described, on the branch the fix did not reach. The
|
|
`MissileAdoptedBody_…` test uses `teleportSequence: 5` (teleport); there is no
|
|
adopted-body far test.
|
|
|
|
Compounding this, **AP-141 (`docs/architecture/retail-divergence-register.md`)
|
|
now asserts the false justification unconditionally**:
|
|
|
|
> "The far branch's `StopInterpolating` skip is NOT part of this divergence —
|
|
> retail's own `position_manager != 0` guard @0x005163C9 already skips it for a
|
|
> never-interpolated missile, so acdream's identical skip is retail-faithful by
|
|
> consequence."
|
|
|
|
That justification is conditioned on "a never-interpolated missile", which the
|
|
adopted-body case is not. The row therefore claims retail-faithfulness for a
|
|
shape where the code is not retail-faithful. This is the recurring class the
|
|
route-5 contract names in its predecessor list ("a register row asserting
|
|
behaviour the code does not have") and which AP-136 was previously amended for.
|
|
|
|
**Fix direction:** on the `SetPositionSimple` arm, when
|
|
`record.RemoteMotion is RemoteMotion adopted && route.StopInterpolating`, clear
|
|
`adopted.Interp` before the placement — mirroring `ApplyAcceptedRemoteFarSnap`'s
|
|
one line and retail's own ordering (@0x005163CB before @0x005163D9). Then narrow
|
|
AP-141's far-branch sentence to the bare-missile case it is actually true of.
|
|
Add the adopted-body far test alongside the teleport one.
|
|
|
|
---
|
|
|
|
### B2 — R6 traded A1's wrong-cell symptom for a new one: on every STORED outcome the render entity is now positioned at the destination but parented to the cell it left
|
|
|
|
**Severity: MAJOR (regression introduced by this fix round).**
|
|
|
|
`ProjectileController.SyncPresentationFromResolvedBody:530` changed from
|
|
`record.FullCellId` to:
|
|
|
|
```csharp
|
|
entity.ParentCellId = runtime.Body.CellPosition.ObjCellId;
|
|
```
|
|
|
|
Trace the three outcome classes:
|
|
|
|
| outcome | `body.Position` after | `body.CellPosition.ObjCellId` | `record.FullCellId` |
|
|
|---|---|---|---|
|
|
| Committed | resolved destination | result cell (`SnapToCell`) | result cell (`CommitCanonical`'s `SetFullCell`) |
|
|
| Stored (`Refused`/`Contention`/`RejectedPreparation`/`NotApplicable`) | **accepted destination, resolved in the WIRE cell's frame** | **source cell — `StoreAcceptedDestinationPose` never writes the cell** | **wire cell — the merge's `RefreshDerivedState` → `SetFullCell`** |
|
|
| `Deferred`/`RejectedByPlacement` | ack does not run (A1 gate) | — | — |
|
|
|
|
On a committed outcome the two sources are identical, so R6 is a no-op there —
|
|
and `MissileTeleportCommit_…` asserts both, confirming the equality rather than
|
|
discriminating between the sources.
|
|
|
|
On a **stored** outcome they diverge, and R6 picked the wrong one.
|
|
`StoreAcceptedDestinationPose:1123-1132` computes
|
|
`accepted.PositionX + worldOffset(accepted.LandblockId)` — the world point of
|
|
the **wire** cell-local coordinates — and `record.FullCellId` is that same wire
|
|
cell. So `record.FullCellId` is the cell the stored position belongs to;
|
|
`body.CellPosition.ObjCellId` is the cell the body **left**.
|
|
|
|
The inconsistency is visible *inside this fix round*: on the same stored packet,
|
|
Runtime's own `SyncProjectilePresentation:1043-1051` publishes the shadow row
|
|
with `record.FullCellId` (wire cell) paired with the new position, while App
|
|
publishes the render entity with the **source** cell paired with the same new
|
|
position. Two presentation surfaces, two different cells, one body.
|
|
|
|
Concrete failure: a missile crosses a landblock boundary at the streaming edge;
|
|
the destination landblock is not yet in the collision service window, so
|
|
`CanAttemptDestination` refuses. The body stores to the destination. The render
|
|
entity is placed there but `entity.ParentCellId` is the source cell —
|
|
and `ParentCellId` is precisely what
|
|
`RetailPViewRenderer.cs:925/945` uses for the indoor/outdoor stage split and
|
|
`viewcone.SphereVisibleInCell(e.ParentCellId!.Value, …)`. The arrow is
|
|
visibility-tested against a cell it is no longer in: culled, or drawn in the
|
|
wrong stage.
|
|
|
|
The R6 doc comment's own retail citation argues against its choice — it states
|
|
that retail's `store_position` @0x00515CE2 "writes the object's whole
|
|
`Position` **including `objcell_id`**". If retail's store wrote the cell, the
|
|
body's cell after a store *would be* the wire cell; `record.FullCellId` is the
|
|
faithful stand-in for that, and the body's stale cell is the acdream residual
|
|
(AP-138), not the truth.
|
|
|
|
**Fix direction:** revert `ParentCellId` to `record.FullCellId` (round 1's
|
|
value) and keep the A1 status gate, which is what actually fixed the no-op
|
|
case. That pairing is correct on all three outcome classes and agrees with
|
|
Runtime's own shadow publication. If the body's stale cell is considered the
|
|
truth, then `StoreAcceptedDestinationPose` must write the cell too — but that is
|
|
AP-138 scope and would change the remote arms as well.
|
|
|
|
---
|
|
|
|
### B3 — the one branch where `SyncProjectilePresentation` is the sole writer — spatial+visible on the STORE path — is still unasserted at both layers, and it is exactly where B2 and the surviving A1 sabotage hide
|
|
|
|
**Severity: MAJOR (coverage; one assertion + one test wide).**
|
|
|
|
I independently reproduced the implementer's "2 of 5" trace and confirm it:
|
|
|
|
| new test | discriminates a gutted `SyncProjectilePresentation`? | why |
|
|
|---|---|---|
|
|
| `TeleportCommit_…ForceEndsCollision…` (shadow assertion) | **No** | the shared pipeline's `ShadowObjects.CommitSetPosition` publishes the same row |
|
|
| `FarCommit_…` (shadow assertion) | **No** | same |
|
|
| `TeleportCommit_ReenteringWorldReactivatesBody` | **No** | `RuntimeSetPositionState.cs:2983-2985` sets `EnteringWorldFromCelllessResidence \|= !body.InWorld \|\| FullCellId == 0` at **prepare** time (before `SnapToCell`), and `:4979-4984` then sets `Active` + `LastUpdateTime` itself |
|
|
| `TeleportCommit_HiddenSuspendsShadowStaysInWorld` | **Yes** | nothing else suspends on Hidden |
|
|
| `Refused_NonSpatialDeactivatesAndSuspends` | **Yes** | the Refused path never reaches the engine |
|
|
|
|
The implementer's trace is accurate and was reported honestly. But the table
|
|
also shows *why* only 2 discriminate: on the committed path the shared pipeline
|
|
independently produces the same observable, so `SyncProjectilePresentation`'s
|
|
spatial+visible branch is only load-bearing on the **store** path — and no test
|
|
asserts it there. `Refused_StillAdvancesPoseNoParkPredictionInvalidated` is
|
|
spatial+visible+Refused and asserts `body.Position`, prediction,
|
|
`body.InWorld` (vacuous — `AttachBody`'s `SnapToCell` already set it) and
|
|
`ObjectClock.IsActive`, but **not the shadow row**.
|
|
|
|
The same hole exists at the App layer: the collapse matrix now has teleport
|
|
commit, far commit, near no-op, airborne no-op, null classification, unbound
|
|
fall-through, and adopted-body teleport — but **no store/refused scenario**.
|
|
That is why the reported A1 sabotage survived: with R6 reading the body's own
|
|
cell, an unconditional ack on a *no-op* outcome writes nothing observable
|
|
(position and cell both unchanged), so the gate is invisible on the no-op path.
|
|
It is visible on the **store** path — which is untested.
|
|
|
|
Both gaps are cheap:
|
|
|
|
- **Runtime**: add `Assert.Equal(body.Position, shadowEntry.Position)` (using
|
|
the file's existing `AllEntriesForDebug` pattern) to
|
|
`Refused_StillAdvancesPoseNoParkPredictionInvalidated`. Gutting the sync then
|
|
leaves the shadow at the spawn pose and the test fails.
|
|
- **App**: add a `MissileFarRefused_…` scenario to the collapse matrix. The
|
|
fixture already supports it with **no new machinery** — `PublishDestinationCollision()`
|
|
and `ServiceWindow.Allow(DestinationLandblock)` are separate opt-in calls, so
|
|
simply omitting `Allow` yields `Refused` from `CanAttemptDestination`. Assert
|
|
`entity.Position == body.Position` **and** `entity.ParentCellId` against the
|
|
wire cell. That single test catches B2 and gives the A1 gate a discriminating
|
|
home.
|
|
|
|
---
|
|
|
|
## 2. New MINOR findings
|
|
|
|
### B4 — the App hook call allocates a per-packet closure, contradicting its own #315-pattern claim
|
|
|
|
`LiveEntityNetworkUpdateController.cs:2155-2158` passes
|
|
`() => _liveEntities.IsCurrentPositionAuthority(acceptedPositionRecord, acceptedPositionAuthorityVersion)` —
|
|
a fresh display class + delegate on every teleport-classified adopted-body
|
|
missile packet. The comment immediately above claims it uses "the SAME ordered
|
|
hook seam and per-packet currency check the remote teleport arm already uses
|
|
(`RunRemoteTeleportHook`, **#315 pattern**)". The remote arm's #315 pattern is
|
|
precisely the opposite: `_remoteArmCallbacks` cached delegates over scratch
|
|
fields (`:1474-1492`), introduced by the collapse's second commit to remove
|
|
per-packet closures from this exact method. D-P4 also states the wiring must be
|
|
"without per-packet closures". Narrow reach (SetPosition + RemoteMotion
|
|
present), so MINOR — but the comment asserts compliance the code does not have.
|
|
|
|
### B5 — the retained-retry arm invalidates prediction on an outcome that writes nothing
|
|
|
|
`RuntimeRemotePlacementDriveController.Advance:1265-1268`: the else branch calls
|
|
`pendingProjectile?.InvalidatePrediction()` and then `SubmitAndResolve`. When
|
|
`SubmitAndResolve` returns `Contention` (retryable → re-parked into `_pending`),
|
|
`Advance` performs **no** `StoreAcceptedDestinationPose` — so nothing was
|
|
written, yet the prediction version advanced. Contract invariant 4 pins
|
|
"prediction invalidation accompanies every body write on this route… The no-op
|
|
dispositions invalidate nothing." Effect is one aborted in-flight quantum per
|
|
re-park; small, but it is the invariant's stated shape violated on one arm only.
|
|
The entry point does not have this problem (Contention there always stores).
|
|
|
|
---
|
|
|
|
## 3. Round-1 findings — disposition
|
|
|
|
| # | status | verification |
|
|
|---|---|---|
|
|
| **A1** (unconditional ack) | **CLOSED, gate correct — but see B2** | `:2170-2179` gates on `placementStatus is not null and not Deferred and not RejectedByPlacement`, an exact set-equality mirror of the Runtime gate (`:975-983` plus the `null` returns). Verified by enumerating every `RuntimeRemotePlacementExecutionStatus` producer. |
|
|
| **A2** (Missile bit without a bound projectile) | **CLOSED, well** | The discriminator became conjunctive at the classifier (`RuntimeEntityObjectLifetime.cs:673-677`) with the App's null-route fallback mirroring it exactly (`:2122-2131`). Retail justification is sound and better than my suggested fix: retail places every non-player object unconditionally, so an unbindable missile taking the ordinary remote path *is* the faithful behaviour, not a fallback. Blast radius enumerated below. `MissileUnbound_FallsThroughToRemoteTail_TracksInsteadOfFreezing` is discriminating (revert the conjunct and the body never moves). |
|
|
| **A3** (teleport hook) | **PARTIALLY closed — see B1** | Teleport branch wired through the existing `RemoteTeleportHook` bundle with a real per-packet currency guard; the adopted-body test is genuinely discriminating (removing the call leaves `Constraint.IsConstrained` true and `Interp.IsActive` true). Far branch not covered. |
|
|
| **A4** (sync untested) | **PARTIALLY closed — see B3** | 2 of 5 new tests discriminate; the store-path spatial+visible branch remains unasserted. |
|
|
| **A5** (no App missile coverage) | **CLOSED, well** | 7 tests on the shared collapse fixture. Fixture-regression check below. |
|
|
| **A6** (`wasInWorld` read after placement) | **CLOSED (behaviourally inert, correctly so)** | `wasInWorld` is now captured before dispatch at both call sites (entry point `:942`, retry `:1226`) and threaded as a parameter. Note it has **no observable effect**: on commit, `EnteringWorldFromCelllessResidence` already re-activates from the same pre-`SnapToCell` condition; on store, `body.InWorld` is unchanged so before == after. Correct and trap-removing either way. |
|
|
| **A7** (hidden `LastUpdateTime`) | **CLOSED** | Restored at `:1055`. Clock basis verified equivalent: `UpdateFrameOrchestrator.CurrentScriptTime => _runtime.SimulationTimeSeconds`, i.e. `_physicsScriptGameTime` and `_clock.SimulationTimeSeconds` are the same clock, so no mixed-basis write into `body.LastUpdateTime`. |
|
|
| **A8** (silent shadow skip) | **CLOSED** | `else { physics.ThrowIfWorldFrameUnreachable(record.FullCellId); }` at `:1053`. Verified non-throwing during the legitimate pre-Create window (`RuntimePhysicsState.cs:596-606` returns early unless the local-player Create was observed with a zero frame). |
|
|
| **A9** (local player not fenced) | **CLOSED** | `update.Guid != _playerServerGuid &&` added to the fallback (`:2125`). |
|
|
| **A10** (`OwnsFarSnap` doc) | **CLOSED** | Doc-only change; the predicate body is byte-identical (verified — the diff adds only comment lines). No effect on the far arm the collapse and 4b-2 gated. |
|
|
| **A11** (stale cutover inventory) | **DECLINED — acceptable** | `docs/research/2026-08-02-cutover-route-inventory.md` is a dated research/planning record, and the project's documentation rules treat those as historical. D-P7's grep was scoped to `src/` and `docs/architecture/`, both of which are clean. Reasonable call; the risk is a future reader treating a dated inventory as current, which the date already signals. |
|
|
| **`_lastFiniteGameTime`** | **CLOSED** | `SyncPresentationFromResolvedBody:521-522` restores the rebase under `double.IsFinite`, matching the deleted method's semantics (it also skipped the assignment on a non-finite clock). |
|
|
|
|
### A2's fix — blast radius, enumerated
|
|
|
|
Everything that now depends on the conjunctive kind derivation, and its status:
|
|
|
|
1. `RuntimeAcceptedPositionRouteRequests.TryBuild` → `RuntimeAuthoritativePositionRouteClassifier` — re-verified that the classifier's only kind branch past `ValidEntityKind` is `LocalPlayer`, so `Projectile` and `Remote` remain disposition-, flag-, `StopInterpolating`- and `ConstrainPhase`-identical. Kind changes the `OperationKind` only.
|
|
2. `RuntimeRemotePlacementDriveController.OwnsPlacement` — admits both; single production reader (`TryExecuteAcceptedRemotePosition:640`).
|
|
3. `RuntimeRemoteFarSnapPosition.OwnsFarSnap` / `RuntimeRemoteTeleportPosition.OwnsTeleportPlacement` / `ApplyAcceptedRemoteFarSnap` / `ApplyAcceptedRemoteTeleport` / `TryArmConstraintAfterOperation` — still `RemoteAuthoritative`-gated. An **unbound** missile now legitimately reaches them (it is classified `Remote`), which is the intended pre-route-5 behaviour; a **bound** one never can. The throwing guards stay unreachable for projectiles.
|
|
4. App `isMissilePacket` — derived from `route.OperationKind` whenever a route exists, so it cannot disagree with the classifier; the null-route arm restates the identical conjunct.
|
|
5. **Kind is now derived from mutable binding state**, so it can flip between packets without the Missile bit changing. The one place that stores a kind across packets is `_pending[key].Route`; `Advance` re-validates `pending.Record.Projectile` and body identity before syncing (`:1214-1224`), and `TryPrepareAndSubmitAuthoredPlacement` compares `route.OperationKind` against the `operation.Kind` stamped at the same Begin. Consistent — this was handled deliberately (R3), not by luck.
|
|
6. First-entry admission, the residence Create half, and the continuation executor all pass explicit kinds and are untouched.
|
|
|
|
I found no dependent that broke.
|
|
|
|
### A5's fix — did the shared fixture regress for its original dual-guid purpose?
|
|
|
|
**No.** Verified line by line:
|
|
|
|
- The remote construction is moved verbatim into an `else` branch; the
|
|
`RemoteMotion` + `Shadows.Register(… Cylinder, cylHeight: 1.835f …)` block is
|
|
byte-identical to the original.
|
|
- `baseState` evaluates to exactly `PhysicsStateFlags.ReportCollisions` when
|
|
`isMissile: false`, so both `RawState` and `PhysicsState` are unchanged for
|
|
every pre-existing test (all of which use the default `isMissile: false`).
|
|
- `Remote` stays `null!` for missile fixtures; no missile test dereferences it.
|
|
The two tests that do use `fixture.Remote` (`MissileUnbound_…`,
|
|
`MissileAdoptedBody_…`) both construct with `isMissile: false` — deliberately,
|
|
because both scenarios *are* remote-shaped records that acquire the Missile
|
|
bit later. That is the right modelling.
|
|
- The new `Projectile` property is additive.
|
|
- All pre-existing dual-guid tests in the file pass in the 64/64 run.
|
|
|
|
`MissileAdoptedBody_…` is worth calling out as a genuinely good test: it builds
|
|
the adopted-body shape through the production seam (`BindProjectile` over the
|
|
record's canonical body, which `SetRemoteMotion` had already adopted from the
|
|
component), and its two discriminating assertions (`Constraint.IsConstrained`,
|
|
`Interp.IsActive`) fail if the hook call is removed.
|
|
|
|
---
|
|
|
|
## 4. The two reported residuals — judgment
|
|
|
|
### Residual 1 — A1's fix has no App-level regression test
|
|
|
|
**Judgment: the stated gap is acceptable; the surrounding gap is not.**
|
|
|
|
The narrow claim is correct and I verified it: `Deferred` requires a
|
|
`DeferredCell` park (collision-generation quiescence) and `RejectedByPlacement`
|
|
requires the engine's own sweep to refuse, and no sibling remote test in that
|
|
file constructs either — the fixture has no machinery for it. The gate is a
|
|
literal set-equality mirror of a Runtime gate whose behaviour *is* exercised by
|
|
`Refused`/no-op/rejected tests, so code review is a defensible verification for
|
|
those two statuses specifically.
|
|
|
|
But the reason the sabotage survived is not that `Deferred` is unreachable — it
|
|
is that **R6 made the ack idempotent on the no-op path**, so the gate has no
|
|
observable effect on any scenario the matrix currently contains. The status the
|
|
gate matters for that *is* trivially constructible is the store path, and the
|
|
fixture already supports it (omit `ServiceWindow.Allow`). That test is required
|
|
(B3), and it also catches B2. So: accept the `Deferred`/`RejectedByPlacement`
|
|
carve-out, reject the absence of any store-path App scenario.
|
|
|
|
### Residual 2 — only 2 of 5 A4 tests discriminate
|
|
|
|
**Judgment: 2 is not sufficient for invariant 2, but the shortfall is one
|
|
assertion, not a suite.**
|
|
|
|
The trace is accurate (I reproduced all five verdicts independently, including
|
|
the non-obvious `EnteringWorldFromCelllessResidence` mechanism behind the
|
|
reactivation test). Keeping the redundant assertions as correct facts is the
|
|
right call — they are true, cheap, and they pin the shared pipeline's
|
|
contribution.
|
|
|
|
The shortfall is specific: the spatial+visible branch is redundant **only on the
|
|
committed path**. On the store path `SyncProjectilePresentation` is the sole
|
|
writer of the shadow row, and no test asserts it. Invariant 2 is "#312's layer"
|
|
— an entity left rendered a packet behind its body — and the store path is
|
|
exactly the outcome class where that can happen without the shared pipeline
|
|
noticing. One `Assert.Equal(body.Position, shadowEntry.Position)` in the
|
|
existing `Refused_…` test converts the third branch from redundant to
|
|
discriminating and closes invariant 2 at the Runtime layer. Combined with the
|
|
App store scenario from B3, invariant 2 is then covered at both layers by
|
|
discriminating assertions.
|
|
|
|
---
|
|
|
|
## 5. What would make this PASS
|
|
|
|
1. **B2**: `entity.ParentCellId = record.FullCellId` (keep the A1 gate). One
|
|
expression.
|
|
2. **B1**: clear the adopted `Interp` queue on the `SetPositionSimple` arm when
|
|
`route.StopInterpolating`; narrow AP-141's far-branch sentence to the
|
|
bare-missile case. One call + one clause + one test.
|
|
3. **B3**: one shadow assertion in `Refused_StillAdvancesPoseNoParkPredictionInvalidated`;
|
|
one `MissileFarRefused_…` App scenario asserting position **and**
|
|
`ParentCellId` (which is the B2 regression test).
|
|
4. **B4/B5**: cached delegate for the hook currency check; move the retry arm's
|
|
`InvalidatePrediction` to the paths that actually write.
|
|
|
|
Nothing here needs new machinery, a new fixture, or a design change.
|