acdream/docs/research/2026-08-05-c5a-architecture-review.md
Erik f8e55ba5e4 fix(physics): route local-player shadow presentation through SyncPose (#318, AP-145)
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>
2026-08-05 14:09:11 +02:00

411 lines
26 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# 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:17971839` (new) vs `18071849` (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-141144, 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:10631085`) 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:264293`
(`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:307308`
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:57105`
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` (`:107133`) 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 (`:90104`). 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:2428`
> "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:474520`
(`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 `:501505` 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 (`:11521194`) 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
D1D3 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-141144,
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 `:8084`; 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 `:159165` (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.