RuntimePlacementPresentationSink.TryPublishPlace previously published the local player's collision-shadow pose with a direct LocalPlayerShadowState.Set call — a plain cache write that never touched PhysicsEngine.ShadowObjects. Because LocalPlayerShadowSynchronizer.SyncPose's own dedup check compares against that same cache, the direct write could pre-seed the cache with the destination pose and cause the next real SyncPose call to see "nothing changed" and skip its own ShadowObjects publish — leaving the real collision shadow at the pre-teleport position until an unrelated movement tick forced a real publish. Fix: TryPublishPlace now calls _localPlayerShadowSync.SyncPose(..., force: true), the same publisher ordinary per-tick movement uses, so Place always drives a real ShadowObjects write before the cache updates. TryPublishWithdrawal carried the exact mirror asymmetry (a bare LocalPlayerShadowState.Clear with no ShadowObjects.Suspend, leaving a live phantom shadow row at the park's source cell for the whole park window — the #184 shape) and is fixed in the same commit, same one-call shape: _localPlayerShadowSync.Suspend(entity). The sink no longer holds a direct LocalPlayerShadowState reference; both halves route exclusively through the one synchronizer, which owns the cache internally. The single LocalPlayerShadowSynchronizer instance is now constructed in LivePresentationComposition (before the sink) and threaded through LivePresentationResult to SessionPlayerComposition, which no longer builds its own — this guarantees the sink's Place/Withdraw edge and ordinary per-tick movement publish through the exact same publisher and cache rather than two independent instances that could drift out of sync with each other. TryPublishPlace's xmldoc now states the behavioural nuance directly: routing through SyncPose means Place inherits SyncPose's own admission guard (IsHidden, cellId == 0, not-current-visible-projection), which the old direct .Set() call never consulted. Under those conditions SyncPose now calls Suspend instead of publishing — correct and symmetric, but new behaviour worth flagging at the call site, not just in a test comment. RuntimePlacementShadowCompositionTests.cs (#318) proves four facts against the real ShadowObjects registry, not the cache: a bare Place publishes a real row at the destination cell with the source cell's row gone; a subsequent ordinary per-tick Sync is then a correct no-op; a Place for a registered non-local-player entity leaves its row at the source cell untouched and never touches the player's cache (route 7 P4 — the fix lives entirely inside the pre-existing player-only gate); and Withdraw suspends the real registry row, not just the cache, with the retained (suspendable) registration surviving for a later restore. All four were sabotage-verified in both directions. RuntimeForcePositionRenderCommitTests.cs (B2) drives a real end-to-end accepted ForcePosition through RuntimeEntityObjectLifetime.TryApplyPosition and RuntimeAcceptedPositionDriveController.TryExecuteAcceptedLocalPosition against a live HostFixture, asserting both the committed render position AND a cell change that deliberately crosses out of the spawn's outdoor grid cell, so the cell assertion is independently falsifiable rather than riding along with the position assertion. Retires AP-145 (this fix) in docs/architecture/retail-divergence-register.md. AP-1 and AD-1 are untouched by this commit — they retire separately in the deletion-sweep commit that follows. Evidence chain: docs/research/2026-08-05-c5a-contract.md (the governing C5a slice contract), docs/research/2026-08-05-c5a-architecture-review.md (round 1, FAIL — three MAJORs: vacuous route-7 P4 test, unfixed Withdraw-side mirror asymmetry, non-driving B2 test), docs/research/2026-08-05-c5a-architecture-review-round2.md (round 2, PASS with two MINORs — an unfalsifiable B2 cell assertion and the undocumented SyncPose guard nuance, both fixed here). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
411 lines
26 KiB
Markdown
411 lines
26 KiB
Markdown
# C5a — independent architecture / adversarial review
|
||
|
||
**Reviewer:** architecture / adversarial (Opus)
|
||
**Scope:** the uncommitted working-tree diff at branch
|
||
`claude/acdream-physics-divergence-5aa784`, HEAD `392c1e22`
|
||
(`git diff HEAD` + the two untracked test files). Contract:
|
||
`docs/research/2026-08-05-c5a-contract.md` (input, not under review).
|
||
**Date:** 2026-08-05
|
||
|
||
---
|
||
|
||
## VERDICT: **FAIL**
|
||
|
||
The **deletion sweep (Part 2) is clean and I would pass it on its own.** Every
|
||
structural claim I could falsify held: the two survivors are byte-identical,
|
||
the build is 0-warning/0-error, the suite reconciles to the line, the register
|
||
edits are exactly three rows, and six of the seven test dispositions are
|
||
executed as pinned (one better than pinned).
|
||
|
||
The failure is concentrated in **Part 1 — the parity tests and the AP-145
|
||
retirement's evidence chain**:
|
||
|
||
- **A1** — the route-7-P4 test cited *by name in the AP-145 retirement row* as
|
||
proof does not discriminate. Removing the behaviour it claims to pin leaves
|
||
all three of its assertions green.
|
||
- **A2** — the AP-145 fix closes the `Place` half of the cache-vs-publish
|
||
asymmetry and leaves the **exact mirror image on the `Withdraw` half of the
|
||
same method pair**, unfixed and unfiled, with a real collision consequence
|
||
during a park.
|
||
- **A3** — §5.2's carried route-2 B2 acceptance gap is **not closed**. The new
|
||
test never drives an accepted ForcePosition; it hand-authors the receipt, and
|
||
the surface it exercises is already covered by an existing test.
|
||
|
||
Under this campaign's own standard — "the review IS the coverage gate", and
|
||
four vacuous-test classes already caught this session — A1 alone is
|
||
disqualifying: a divergence-register retirement must not rest on a test that
|
||
passes under its own sabotage.
|
||
|
||
---
|
||
|
||
## Gate evidence I measured myself
|
||
|
||
| Gate | Result |
|
||
|---|---|
|
||
| `dotnet build AcDream.slnx -c Release -m:1` | **Build succeeded. 0 Warning(s), 0 Error(s)** |
|
||
| Complete Release suite (`--no-build -m:1`) | **11,105 passed / 4 skipped / 0 failed** |
|
||
| Reconciliation vs baseline 11,112 | 11,112 − 11 (`PhysicsEngineTests` methods deleted) + 4 (3 shadow-composition + 1 force-position) = **11,105 ✓ exact** |
|
||
| Skips | 3 (App) + 1 (Core) = **4 — same as baseline ✓** |
|
||
| Survivors byte-identical | `IsSpawnCellReady` + `AdjustPosition`: `PhysicsEngine.cs:1797–1839` (new) vs `1807–1849` (HEAD) — **`diff` clean over the whole 45-line span ✓** |
|
||
| Survivor production callers intact | `RuntimeSetPositionState.cs:2188,:4397`; `SessionPlayerComposition.cs:374`; `PhysicsCameraCollisionProbe.cs:38,:100` — **all present ✓** |
|
||
| Register blast radius | `git diff -U0` = **6 changed lines**: AD/AP section headers + AD-1, AP-1, AP-145 rows. AP-131, AD-60, AD-61/62, AP-135, AP-141–144, AP-146 **untouched ✓** |
|
||
| #316-preserving pair | `LiveEntityNetworkOnPositionCollapseMatrixTests.cs` **not in the modified-file set — zero diff ✓** |
|
||
| `SetPosition` → `SeedPlacementForTest` re-point | **83 removals / 83 additions**, receivers all controller-typed; **zero `entity.`/`child.`/`Entity.SetPosition` lines touched ✓** |
|
||
|
||
---
|
||
|
||
## The composition-graph change — my judgment: **CORRECT, and correctly argued**
|
||
|
||
I attacked this first as instructed. It holds.
|
||
|
||
- **Exactly one instance, on every host path.** `new LocalPlayerShadowSynchronizer(`
|
||
now has **one** production site in the tree
|
||
(`LivePresentationComposition.cs:508`);
|
||
`SessionPlayerComposition.cs:804` consumes `live.LocalPlayerShadowSynchronizer`.
|
||
`RuntimePlacementPresentationSink` has exactly one production construction
|
||
site, also in `LivePresentationComposition.cs:514`. **No other host
|
||
constructs either** — `grep` over `src/` for `LivePresentationCompositionPhase`
|
||
/ `SessionPlayerCompositionPhase` / `new RuntimePlacementPresentationSink`
|
||
returns only `GameWindow.cs:1342/1395` and that one file. Headless and the
|
||
no-window Runtime host never touch this sink at all.
|
||
- **Same arguments before and after.** `GameWindow.cs:1359` feeds
|
||
`_localPlayerShadow` into `LivePresentationDependencies.LocalPlayerShadow`
|
||
and `GameWindow.cs:1429` feeds *the same field* into
|
||
`SessionPlayerDependencies.PlayerShadow`; `_physicsEngine`, `_liveWorldOrigin`
|
||
and `_localPlayerIdentity` are likewise the same instances in both records.
|
||
`liveEntities` is the same `LiveEntityRuntime` the old
|
||
`live.LiveEntities` read. The relocated construction therefore receives an
|
||
argument-identical closure.
|
||
- **Ordering is safe.** Construction at `:508` precedes the sink at `:514`;
|
||
`LivePresentationResult` has a single construction site (`:1209`) reached only
|
||
after `:508`; the field is non-nullable and the sink's ctor
|
||
`throw`s on null (`RuntimePlacementPresentationSink.cs:60`). There is no path
|
||
to a null or a second instance.
|
||
- **Lifetime unchanged.** Both phases publish into the same `GameWindow` shell
|
||
through `PublishSessionPlayer`, whose "already owns session/player state"
|
||
guard (`GameWindow.cs:1063–1085`) proves the two phases are composed as one
|
||
transaction. Moving construction one phase earlier does not straddle a reset
|
||
boundary.
|
||
|
||
One behavioural nuance worth recording (not a defect): routing through
|
||
`SyncPose` means the Place edge now inherits `SyncPose`'s guard — if
|
||
`IsHidden`, `cellId == 0`, or `!IsCurrentVisibleProjection`, the Place
|
||
**suspends** the shadow where the old direct write merely cached. That is the
|
||
correct, symmetric behaviour (it is what the very next per-tick `Sync` would do
|
||
anyway) and it is inside the §5.1 pre-authorised production change, so it needs
|
||
no separate row. It should be stated in the commit message, since it is the one
|
||
place the fix does more than "also publish".
|
||
|
||
---
|
||
|
||
## Findings
|
||
|
||
### A1 — MAJOR. The route-7-P4 test does not discriminate; the AP-145 retirement row cites it as proof
|
||
|
||
**File:** `tests/AcDream.App.Tests/World/RuntimePlacementShadowCompositionTests.cs:264–293`
|
||
(`Place_ForNonLocalPlayerEntity_NeverTouchesShadowObjects`)
|
||
|
||
The test's stated job is to prove that the player-only gate at
|
||
`src/AcDream.App/World/RuntimePlacementPresentationSink.cs:249`
|
||
(`record.ServerGuid == _localPlayerGuid()`) is what keeps a committed CHILD
|
||
from gaining a broadphase row — "This drives that directly rather than arguing
|
||
it from inspection."
|
||
|
||
It does not. The fixture never registers the child in `ShadowObjects`, and
|
||
`ShadowObjectRegistry.UpdatePosition` returns immediately when the entity has
|
||
no registration record:
|
||
|
||
```
|
||
src/AcDream.Core/Physics/ShadowObjectRegistry.cs:696
|
||
if (!_entityReg.TryGetValue(entityId, out var reg))
|
||
return; // not registered — no-op (callers don't have to gate)
|
||
```
|
||
|
||
**Concrete failure scenario (the sabotage that should fail and doesn't):**
|
||
delete the `record.ServerGuid == _localPlayerGuid()` gate so every Place calls
|
||
`SyncPose`. Trace it: `IsHidden(0x7000A101)` is false (that guid was never
|
||
materialised); `IsCurrentVisibleProjection(childEntity)` resolves the child's
|
||
own record and returns true; `ShadowPositionSynchronizer.Sync` →
|
||
`UpdatePosition(childId, …)` → the early return above → nothing registered.
|
||
`TotalRegistered` is still `0`, `GetObjectsInCell(DestinationCell)` is still
|
||
empty, `entity.Position` still equals `DestinationPosition`. **All three
|
||
assertions pass with the gate removed.** (If instead the `Suspend` branch were
|
||
taken, `ShadowObjects.Suspend` on an unregistered id is likewise a no-op — the
|
||
test passes either way. It is vacuous on both branches.)
|
||
|
||
This is caught vacuous-class #4: *a precondition that made the sabotage
|
||
irrelevant*. It is also caught class #2 in part — the two load-bearing
|
||
assertions are pure negatives against a registry the fixture guaranteed empty.
|
||
|
||
**Why it matters beyond the test file:** the retired AP-145 row
|
||
(`docs/architecture/retail-divergence-register.md:175`) lists, among the four
|
||
things "#318's composition test … proves", "*a Place for a non-local-player
|
||
entity never touches `ShadowObjects` at all (route 7 P4 …)*". A register
|
||
retirement is now standing on a claim the cited test does not establish.
|
||
|
||
**Fix direction:** give the child a real registration first — mirror fact 1's
|
||
baseline `ShadowObjects.Register(entity.Id, …, seedCellId: SourceCell)` and
|
||
`Synchronizer.Sync(…, force: true)` — then assert after the Place that the
|
||
child's row is **still at `SourceCell` and absent from `DestinationCell`**.
|
||
Add `Assert.Null(fixture.LocalShadow.Current)` so the dedup cache is proven
|
||
un-polluted too (removing the gate writes the child's pose into the *player's*
|
||
cache — a second thing the current test cannot see).
|
||
|
||
---
|
||
|
||
### A2 — MAJOR. The fix closes `Place` and leaves the identical asymmetry on `Withdraw`, unfixed and unfiled
|
||
|
||
**Files:** `src/AcDream.App/World/RuntimePlacementPresentationSink.cs:307–308`
|
||
vs `src/AcDream.App/World/LiveEntityProjectionWithdrawalController.cs:148,:156`
|
||
|
||
AP-145 was, verbatim, "a plain cache write with no side effect beyond
|
||
recording `Current`" on the local-player shadow. The fix routes `TryPublishPlace`
|
||
through the real publisher. Six lines further down in the same class,
|
||
`TryPublishWithdrawal` still does:
|
||
|
||
```
|
||
src/AcDream.App/World/RuntimePlacementPresentationSink.cs:307
|
||
if (record.ServerGuid == _localPlayerGuid())
|
||
_localPlayerShadow.Clear();
|
||
```
|
||
|
||
— a bare cache clear with **no** `ShadowObjects.Suspend`. The correct pairing
|
||
exists elsewhere in the same subsystem and shows what the sink is missing:
|
||
|
||
```
|
||
src/AcDream.App/World/LiveEntityProjectionWithdrawalController.cs:148,156
|
||
if (!retainedProjectileShadow)
|
||
_shadows.Suspend(entity.Id); // registry
|
||
...
|
||
_localPlayerShadow.Clear(); // cache
|
||
```
|
||
|
||
**Concrete failure scenario:** a local-player park (`Withdraw`) — the path
|
||
`TryApplyWithdrawalRestoration`'s own xmldoc (`:202`) names as touching "the
|
||
local-player shadow". The cache says "no shadow"; the registry still carries a
|
||
live row for the player at the park's **source** cell. For the whole park
|
||
window every other entity's collision sweep in that cell collides with a
|
||
phantom player, and nothing self-heals, because a withdrawn player receives no
|
||
per-tick `Sync`. Restoration papers over it (`TryPublishPlace` → `SyncPose`
|
||
force-republishes), so the symptom is a transient phantom obstruction during a
|
||
park — exactly the "why not observed live" shape AP-145 itself carried.
|
||
|
||
This is **pre-existing**, not introduced by C5a. But (a) register rule 1 makes
|
||
an unrecorded deviation "a bug twice over", (b) this diff is the commit that
|
||
retires AP-145 and its retirement text asserts the seam is now symmetric with
|
||
ordinary per-tick movement, and (c) it is six lines from the line being fixed —
|
||
this is precisely the review's job to catch.
|
||
|
||
**Fix direction:** either route the withdrawal through
|
||
`_localPlayerShadowSync.Suspend(entity)` (a production behaviour change → its
|
||
own commit with its own gate, per the no-workarounds rule), **or** file a new
|
||
AP row / issue in this same commit recording the Withdraw-half asymmetry and
|
||
its "risk if the assumption breaks" column, and narrow AP-145's retirement text
|
||
to the `Place` edge it actually covers.
|
||
|
||
---
|
||
|
||
### A3 — MAJOR. §5.2's route-2 B2 acceptance gap is not closed; the test largely duplicates existing coverage
|
||
|
||
**File:** `tests/AcDream.App.Tests/World/RuntimeForcePositionRenderCommitTests.cs:57–105`
|
||
|
||
The contract's §5.2 deliverable: *"an App-layer test driving an **accepted
|
||
ForcePosition end to end** through `RuntimePlacementPresentationSink` /
|
||
`TryApplyRuntimePlacementPlace` and asserting the render entity's position/cell
|
||
came from the committed placement receipt."* B2's original finding is about a
|
||
**ForcePosition** producing a receipt that the render entity then follows.
|
||
|
||
What landed does not drive a ForcePosition at all. It hand-authors a
|
||
`RuntimePlacementProjectionSnapshot` (`:107–133`) and calls `Sink.TryApply`.
|
||
The test's own xmldoc concedes it: *"rather than driving the full
|
||
`RuntimeAcceptedPositionDriveController` pipeline."* The receipt's contents are
|
||
therefore the **test's assumption**, not the ForcePosition path's output — the
|
||
half of B2 that could actually be wrong ("canonical body moves, render entity
|
||
stays put") is asserted by narrative.
|
||
|
||
Worse, the surface it does exercise is already pinned at HEAD:
|
||
|
||
```
|
||
tests/AcDream.App.Tests/World/RuntimePlacementPresentationSinkTests.cs:29
|
||
Place_ReframesAndRebucketsExactSidecarWithoutMutatingRuntimePhysics
|
||
:57 Assert.Equal(place.WorldPosition, entity.Position);
|
||
:58 Assert.Equal(place.Orientation, entity.Rotation);
|
||
:59 Assert.Equal(DestinationCell, entity.ParentCellId);
|
||
:60 Assert.True(record.IsSpatiallyProjected);
|
||
:61 Assert.True(record.IsSpatiallyVisible);
|
||
```
|
||
|
||
Those are the same five facts the new test asserts (`:90–104`). The only deltas
|
||
are a stale-wire-pose pre-state and `Portal: default`. That is a real but small
|
||
increment; it is not the recorded gap.
|
||
|
||
**Fix direction:** drive `RuntimeAcceptedPositionDriveController
|
||
.TryExecuteAcceptedLocalPosition` (or the accepted-ForcePosition entry the
|
||
route-2 landing added) so the receipt is **produced** by the path under test,
|
||
then assert the render entity against the emitted receipt. If that fixture cost
|
||
is judged disproportionate, then B2 must be recorded as **still unmet** in the
|
||
plan and the commit message, not marked closed — a partial closure silently
|
||
booked as full is how an acceptance gap disappears.
|
||
|
||
---
|
||
|
||
### A4 — MINOR (one line is borderline MAJOR). Stale citations of the deleted `PhysicsEngine.Resolve` survive the D7 sweep
|
||
|
||
All four are plain `<c>`/comment text, so the 0-warning build cannot catch
|
||
them:
|
||
|
||
| File:line | Text | Why it's wrong now |
|
||
|---|---|---|
|
||
| `src/AcDream.Runtime/Gameplay/PlayerMovementController.cs:145` | "PhysicsEngine.Resolve is still used each frame to snap the player to terrain/cell floor Z and detect ground contact." | Class-summary architecture note asserting a **per-frame** call to a method that no longer exists. First thing a reader of the local-movement controller sees. |
|
||
| `src/AcDream.Core/Physics/CellTransit.cs:878` | "…mirrors the NO-LANDBLOCK contract in `PhysicsEngine.Resolve`." | Cites a deleted contract as the authority for a live early-return. |
|
||
| `src/AcDream.Core/Physics/CellTransit.cs:1059` | "handled at the SNAP by `PhysicsEngine.Resolve`'s `AdjustPosition` validation since #107/#111" | The snap path is gone; `AdjustPosition` survives but is now reached only from `PhysicsCameraCollisionProbe`. |
|
||
| `src/AcDream.Headless/Hosting/HeadlessSessionWorldProjection.cs:794` | "retiring the duplicate `Resolve`/`ResolvePlacement`/`SetPosition` authority" | The contract acknowledged this mention; the retirement it forecasts has now happened, so the comment should close, not linger as a to-do. |
|
||
|
||
The contract's D7 enumerated only three doc targets and scoped the `cref` sweep
|
||
to `PhysicsEngine.cs`, so this is strictly beyond-contract — but the campaign's
|
||
"cite by symbol, these move" discipline exists for exactly this, and
|
||
`PlayerMovementController.cs:145` is materially misleading.
|
||
|
||
**Fix direction:** rewrite `:145` to name `ResolveWithTransition` (the real
|
||
per-frame resolver) and correct the two `CellTransit` notes to cite
|
||
`PhysicsEngine.SetPosition` / `AdjustPosition` by symbol.
|
||
|
||
---
|
||
|
||
### A5 — MINOR. A factual error in the rewritten `CommitPreparedPosition` test's xmldoc
|
||
|
||
**File:** `tests/AcDream.App.Tests/Input/PlayerMovementPlacementTransactionTests.cs:24–28`
|
||
|
||
> "The render-root-publish-on-commit and **sticky-target-release-on-commit**
|
||
> invariants this test originally pinned now live INSIDE that Runtime
|
||
> final-commit transaction."
|
||
|
||
Half true. Render-root publish **does** live there —
|
||
`RuntimeSetPositionState.cs:2774` `_physics.Engine.UpdatePlayerCurrCell(result.CellId)`
|
||
inside the dormant-activation final commit. Verified.
|
||
|
||
The sticky release does **not**. `grep -rn "UnStick" src/` returns **zero**
|
||
call sites in `src/AcDream.Runtime/` on that path; the only local-player
|
||
`UnStick` is `PlayerMovementController.cs:1892`, inside `SetPositionCore` and
|
||
gated on `if (publishSharedState)` — and `PreparePositionForCommit` passes
|
||
`publishSharedState: false`. So the unstick-at-first-entry-commit behaviour
|
||
was not relocated; it went away with the (already caller-free, therefore
|
||
already dead) `CommitPreparedPosition`. That is harmless — it was never running
|
||
in production — but the comment states a relocation that did not happen, and a
|
||
future reader chasing "where does first-entry unstick happen now?" will be sent
|
||
to a method that does not do it.
|
||
|
||
**Fix direction:** say plainly that the sticky release had no production caller
|
||
and is not performed at first-entry commit today; cite
|
||
`RuntimeSetPositionState`'s `UpdatePlayerCurrCell` by symbol for the half that
|
||
did move.
|
||
|
||
The rewritten test itself is **good**: `Assert.Null(Constraint)` before /
|
||
`Assert.NotNull(Constraint)` + `IsConstrained` after makes it discriminating,
|
||
and the two retained negatives are correctly framed as "this layer does not do
|
||
this" rather than as the pin.
|
||
|
||
---
|
||
|
||
### A6 — MINOR. The re-pointed scratch differential never asserts a placement actually happened
|
||
|
||
**File:** `tests/AcDream.Core.Tests/Physics/TransitionScratchDifferentialTests.cs:474–520`
|
||
(`AssertSetPositionBitwise`), used by `ReusedScratch_MatchesFreshPlacementSearch`
|
||
|
||
The differential compares `expected` against `actual` field-by-field, but there
|
||
is no positive assertion that either result committed (`Assert.True(expected.IsCommitted)`,
|
||
or `Assert.NotEqual(input, expected.Position)` as `InitialPlacementOverlapTests`
|
||
does). A future regression that makes canonical `SetPosition` fail *identically*
|
||
on the fresh and reused engines leaves the differential green while the
|
||
scratch-reuse surface it guards goes unexercised.
|
||
|
||
The pre-deletion `ResolvePlacement` arm had the same weakness (it compared `Ok`
|
||
rather than asserting it), so this is not a regression introduced here — but
|
||
the re-point was the moment to close it, and the new `AssertSetPositionBitwise`
|
||
is otherwise excellent (the `ImmutableArray.Equals` reference-comparison note
|
||
at `:501–505` is the kind of thing that would have produced a false-fail).
|
||
|
||
---
|
||
|
||
## Dispositions — did any of the seven lose coverage?
|
||
|
||
| # | Contract disposition | Executed | Coverage verdict |
|
||
|---|---|---|---|
|
||
| 3.1 | `PhysicsEngineTests` — audit, then delete | 11 methods deleted, 0 re-pointed, 6 `ResolveWithTransition` methods retained | **No loss.** I re-audited all 11 against HEAD: none touches `AdjustPosition` or `IsSpawnCellReady`; `Resolve_ZeroDeltaSnapTrace_IsExplicitlyOptIn` pinned the `[snap]` diagnostic emitted *inside* the deleted body; the rest pinned legacy floor-snap / step-height / portal-transition semantics that die with the method. `PhysicsEngineAdjustPositionTests` (3 tests: sibling-resolve, no-cell, outdoor-snap) covers the survivor. **The §3.1 audit outcome must still appear in the commit message.** |
|
||
| 3.2 | `Issue133…` — re-point (named-bug pin) | Re-pointed to canonical `PhysicsEngine.SetPosition` with the exact #133 geometry (dungeon claim `0x00070143`, dungeon block at world-Y 130 → local Y −60, resident Holtburg neighbour at origin) | **No loss.** Asserts `result.CellId == 0x00070143` and `CellId & 0xFFFF0000 == 0x00070000` on a committed result. If anyone reintroduced an lbPrefix resident-block scan into the canonical path, this fails. Best available pin for a defect whose mechanism no longer exists. |
|
||
| 3.3 | `InitialPlacementOverlapTests` — verify-then-delete-or-re-point | Re-pointed | **Judgment correct, verified independently.** `grep -c "ShadowObjects.Register" tests/…/PhysicsSetPositionTests.cs` = **0** — that suite has zero other-entity occupancy; its `placementPasses >= 2` arm (`:1152–1194`) is BSP-hook-injection driven, exactly as the implementer said. The re-point is discriminating (`Assert.NotEqual(savedFeet, result.Position)` + the centre-distance ≥ 2r check + the 4 m bound). The two-sphere capsule reconstruction matches the legacy scalar `InitPath(0.48, 1.835)` shape. |
|
||
| 3.4 | `TransitionScratchDifferentialTests` — re-point or drop explicitly | Re-pointed with `AssertSetPositionBitwise` | **Preserved.** Both the bitwise fresh-vs-reused comparison and the second-identity (`0x80000102`, hostile) leak check survive; the new asserter covers every `PhysicsSetPositionResult` member including the three id arrays. See **A6** for the one gap. |
|
||
| 3.5 | `SetPosition` → `SeedPlacementForTest` | 83 sites / 19 files | **No loss, no meaning change.** 83 removals ↔ 83 additions; the 2-arg → 3-arg conversions all pass `pos` as `cellLocal`, byte-for-byte what the deleted 2-arg overload did; the seed calls the same production `SetPositionCore`, so the AD-61 grounded/zero-velocity start is unchanged. **Zero `entity.`/`child.`/`Entity.SetPosition` (`WorldEntity`) lines touched** — verified by regex over the whole test diff. Both `Assert.Throws<InvalidOperationException>` guard sites re-pointed, preserving `EnsurePublishedForRuntimeOperation` coverage. |
|
||
| 3.6 | `CommitPreparedPosition` — re-point at the replacement | 1 rewrite + 2 throw-site re-points | **Partial, honestly declared.** The two throw sites re-point cleanly onto `ArmConstraintLeashAtCommittedPlacement`, which carries the same guard. The rewrite is discriminating. **Coverage genuinely lost:** render-root-publish-on-commit and sticky-release-on-commit are no longer pinned at any layer — the former does exist in Runtime (`RuntimeSetPositionState.cs:2774`) but is not asserted by the re-point; the latter does not exist at all (see **A5**). The contract permitted this only with a commit-message note; the note is in the xmldoc and is **half wrong**. |
|
||
| 3.7 | `Begin*` wrappers — keep as documented seam | Kept, xmldoc added to both (`RuntimeSetPositionState.cs:1321,:1343`) | **Correct, as pinned.** |
|
||
|
||
**Summary: one disposition (3.6) lost real coverage, declared but mis-described.
|
||
The other six are clean.** The silent-coverage-loss risk the slice was designed
|
||
around did **not** materialise in the deletion sweep — it materialised in the
|
||
*new* tests (A1, A3).
|
||
|
||
---
|
||
|
||
## Register retirements — verified
|
||
|
||
- **AP-1 — retire: justified.** I re-ran the census: zero `PhysicsEngine.Resolve`
|
||
/ `.ResolvePlacement` receivers in `src/`; the only
|
||
`_physics.Engine.SetPosition` sites are in `RuntimeSetPositionState`. After
|
||
D1–D3 the resolver-shaped entry points do not exist, so the row's condition
|
||
is structurally unreopenable. Correct.
|
||
- **AD-1 — retire: justified.** The recoverable outdoor demote and the
|
||
outdoor-restore `max(terrainZ, z)` lift were `Resolve`'s body; the body is
|
||
gone. Correct.
|
||
- **AP-145 — retire: justified in mechanism, overstated in evidence.** The
|
||
mechanism claim ("`SyncPose` publishes before it records the cache, so the
|
||
cache can no longer be pre-seeded ahead of the real publish") is true and I
|
||
verified the `Register` → `DeregisterCore` ordering
|
||
(`ShadowObjectRegistry.cs:403`) that backs the no-stale-source-row claim. The
|
||
row's fourth proof bullet (route-7 P4) rests on the vacuous test — see **A1**;
|
||
and the row's framing implies a symmetry the `Withdraw` half does not have —
|
||
see **A2**.
|
||
- **Untouchable set held.** AP-131, AD-60, AD-61, AD-62, AP-135, AP-141–144,
|
||
AP-146 — all unmodified. Section counts updated correctly (AD 48→47, AP
|
||
103→101 for two retirements). Strikethrough-plus-RETIRED matches the file's
|
||
established convention (31 existing `| ~~…~~` rows).
|
||
- **#275 surface untouched.** `InboundPhysicsStateController` and
|
||
`RuntimeEntityObjectLifetime` are not in the modified-file set. ✓
|
||
|
||
---
|
||
|
||
## Vacuous-test hunt — all four classes, result
|
||
|
||
| Class | Hunted in | Result |
|
||
|---|---|---|
|
||
| Asserting a field written unconditionally earlier | all new/changed assertions | Clean. The shadow-composition tests assert `ShadowObjects` rows, which only `SyncPose` writes; the B2 test's `entity.Position` is written only by `TryApplyRuntimePlacementProjection`. |
|
||
| Asserting only negatives | `Place_ForNonLocalPlayerEntity_…`, the rewritten transaction test | **HIT on `Place_ForNonLocalPlayerEntity_…`** (two of three assertions are negatives against a guaranteed-empty registry). The transaction test is clean — it pairs its negatives with a positive (`Constraint` non-null) and a precondition (`Constraint` null before). |
|
||
| Fixture staging makes the wrong expression compute the right answer | the destination-cell flood geometry (`DestinationPosition = (202,10,5)` with `worldOffsetX: 192`), the two-sphere capsule reconstructions | Clean. The flood-lands-under-`DestinationCell` construction is load-bearing and documented at `:80–84`; if it were wrong the discriminating assertion would fail, not falsely pass. |
|
||
| A precondition that made the sabotage irrelevant | all three shadow-composition facts | **HIT on `Place_ForNonLocalPlayerEntity_…`** — the child is never registered, so `UpdatePosition`'s not-registered early return makes the gate's presence unobservable. Facts 1 and 2 are clean: both establish a **real** source-cell registration first, which is exactly what makes the "row moved to destination / source row gone" assertions bite. |
|
||
|
||
Facts 1 and 2 of `RuntimePlacementShadowCompositionTests` are genuinely good
|
||
discriminators, and the negative-control comment at `:159–165` (naming the
|
||
cache assertion as the shape that would have passed under the bug) is exactly
|
||
the right way to document a sabotage argument. The problem is confined to the
|
||
third fact.
|
||
|
||
---
|
||
|
||
## What must happen before a re-review
|
||
|
||
1. **A1** — make `Place_ForNonLocalPlayerEntity_NeverTouchesShadowObjects`
|
||
discriminate (register the child first; assert its row stays at `SourceCell`;
|
||
assert the player's cache is not polluted). Re-verify by sabotage: remove the
|
||
`_localPlayerGuid()` gate and confirm the **new** assertion is the one that
|
||
fails.
|
||
2. **A2** — either fix the `Withdraw` half as its own reviewed commit, or file
|
||
the deviation as a register row **in this commit** and narrow AP-145's
|
||
retirement text to the `Place` edge.
|
||
3. **A3** — drive an actual accepted ForcePosition, or record B2 as still open.
|
||
4. **A4/A5** — correct the four stale `PhysicsEngine.Resolve` citations and the
|
||
sticky-release claim in the transaction test's xmldoc.
|
||
5. **A6** — add the "a placement actually committed" positive to the differential.
|
||
6. **Commit message** must carry: the §3.1 audit outcome (11 deleted / 0
|
||
re-pointed, with the reason), the §3.3 covering-test judgment, the §3.6
|
||
coverage-loss declaration, the count reconciliation (11,112 − 11 + 4 =
|
||
11,105 / 4 skips), and the `SyncPose` suspend-guard nuance noted in the
|
||
composition section above.
|