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>
This commit is contained in:
Erik
parent
392c1e22c1
commit
f8e55ba5e4
11 changed files with 2660 additions and 22 deletions
348
docs/research/2026-08-05-c5a-architecture-review-round2.md
Normal file
348
docs/research/2026-08-05-c5a-architecture-review-round2.md
Normal file
|
|
@ -0,0 +1,348 @@
|
|||
# C5a — architecture / adversarial review, round 2 (delta)
|
||||
|
||||
**Reviewer:** architecture / adversarial (Opus)
|
||||
**Scope:** delta over round 1
|
||||
(`docs/research/2026-08-05-c5a-architecture-review.md`). Working tree at
|
||||
branch `claude/acdream-physics-divergence-5aa784`, HEAD `392c1e22`,
|
||||
uncommitted. Part 2 (the deletion sweep) and the composition-graph
|
||||
verification are carried forward from round 1 and not re-litigated — but I
|
||||
did re-confirm the two survivors and the production blast radius are
|
||||
unchanged by the fix round (see "Regression check" below).
|
||||
**Date:** 2026-08-05
|
||||
|
||||
---
|
||||
|
||||
## VERDICT: **PASS**
|
||||
|
||||
All three MAJORs are genuinely closed. A1 and A2 are now backed by tests I
|
||||
independently traced as discriminating; A2's fix is the *correct* mirror, not a
|
||||
symmetric-looking wrong one, and the constructor-parameter removal is safe on
|
||||
every path. A3 built the real drive rather than arguing around it, and the
|
||||
position half of route 2's B2 is now pinned end-to-end through real production
|
||||
machinery with a value the wire cannot supply.
|
||||
|
||||
Two **MINOR** items to fold in before commit (neither blocks the slice):
|
||||
|
||||
- **M1** — the **cell** half of B2 is still not pinned: `entity.ParentCellId`
|
||||
already equals the asserted value before the drive runs.
|
||||
- **M2** — the `SyncPose`-inherits-the-guard behavioural nuance is **not**
|
||||
documented anywhere, contrary to the handoff's claim.
|
||||
|
||||
Plus one **INFO** (five test-file comments still cite the deleted
|
||||
`PhysicsEngine.Resolve` as live).
|
||||
|
||||
---
|
||||
|
||||
## Gates I re-measured
|
||||
|
||||
| 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,106 passed / 4 skipped / 0 failed** |
|
||||
| Reconciliation | 11,112 − 11 (deleted) + 5 (3 shadow-composition + 1 Withdraw fact + 1 force-position) = **11,106 ✓ exact** |
|
||||
| Skips | 3 (App) + 1 (Core) = **4, unchanged from baseline ✓** |
|
||||
| Per-assembly vs handoff | Core 4,259/1, Runtime 1,176/0, Headless 86/0, App 4,132/3 — **matches the handoff's numbers exactly ✓** |
|
||||
| Sink ctor call sites updated | **6/6** (1 production `LivePresentationComposition.cs:514`, 5 test fixtures) |
|
||||
| Register blast radius | still **3 rows + 2 section headers**; AP-131, AD-60, AD-61/62, AP-135, AP-141–144, AP-146 untouched ✓ |
|
||||
|
||||
**Regression check on Part 2 (carried, re-verified):** `IsSpawnCellReady` +
|
||||
`AdjustPosition` still `diff`-clean against HEAD over the full 45-line span.
|
||||
Production `--numstat` shows executable changes confined to
|
||||
`LivePresentationComposition` / `SessionPlayerComposition` /
|
||||
`RuntimePlacementPresentationSink` (+ the deletions and the one seed rename);
|
||||
`CellTransit`, `ConstraintManager`, `PhysicsBody`, `ResolveResult`,
|
||||
`HeadlessSessionWorldProjection`, `RuntimeSetPositionState`, and
|
||||
`RuntimeAcceptedPositionDriveController` are comment/xmldoc only. The fix round
|
||||
introduced no new executable surface beyond the two sink lines.
|
||||
|
||||
---
|
||||
|
||||
## A1 — the P4 test now genuinely discriminates. **CLOSED.**
|
||||
|
||||
`tests/AcDream.App.Tests/World/RuntimePlacementShadowCompositionTests.cs:296–367`
|
||||
|
||||
I traced the sabotage myself rather than trusting the claim. With the
|
||||
`record.ServerGuid == _localPlayerGuid()` gate at
|
||||
`RuntimePlacementPresentationSink.cs:249` removed:
|
||||
|
||||
1. `SyncPose(childEntity, DestinationPosition, …, DestinationCell, force: true)`
|
||||
runs. `_liveEntities.IsHidden(0x7000A101)` is false; `cellId != 0`;
|
||||
`IsCurrentVisibleProjection(childEntity)` resolves the child's **own** record
|
||||
(`TryGetRecord(entity.ServerGuid)`), `ReferenceEquals` holds, and it is the
|
||||
current spatial root — so the guard does **not** short-circuit.
|
||||
2. `ShadowPositionSynchronizer.Sync` → `UpdatePosition(childId, …)`. The
|
||||
`_entityReg.TryGetValue` at `ShadowObjectRegistry.cs:696` now **succeeds**
|
||||
(the new baseline `Register` at test `:309–320` put the record there), so
|
||||
the early return that made v1 vacuous no longer fires.
|
||||
3. `Register(childId, …, seedCellId: DestinationCell)` → flood from the
|
||||
destination (the same geometry fact 1 proves floods successfully) →
|
||||
`DeregisterCore` → row **moves** to `DestinationCell`.
|
||||
|
||||
Result: `Assert.Contains(GetObjectsInCell(SourceCell), child)` at `:352`
|
||||
**fails**, and `Assert.Null(fixture.LocalShadow.Current)` at `:366` **also
|
||||
fails** (`_state.Set` runs as `SyncPose`'s last step). Two independent
|
||||
discriminators, both keyed to the gate.
|
||||
|
||||
The baseline precondition `Assert.Contains(GetObjectsInCell(SourceCell))` at
|
||||
`:321` is what makes step 2 reachable — it is the thing v1 lacked, and it is
|
||||
now asserted, not assumed. The `Assert.Equal(1, TotalRegistered)` at `:358` is
|
||||
supporting only (a move keeps the count at 1); correctly not relied on. The
|
||||
xmldoc at `:282–293` records the v1 failure honestly rather than quietly
|
||||
replacing it.
|
||||
|
||||
**Verified discriminating. No second vacuous version.**
|
||||
|
||||
---
|
||||
|
||||
## A2 — the Withdraw fix is the *correct* mirror, and the parameter removal is safe. **CLOSED.**
|
||||
|
||||
### Is `Suspend` the right counterpart to `SyncPose`'s publish?
|
||||
|
||||
Yes, and I checked the two ways it could have been subtly wrong.
|
||||
|
||||
- **It is not `Deregister`.** `ShadowObjectRegistry.Suspend`
|
||||
(`src/AcDream.Core/Physics/ShadowObjectRegistry.cs:1480–1498`) removes the
|
||||
entity from every cell bucket and stashes the cell list in
|
||||
`_suspendedEntityCells`, but **retains `_entityReg`** — it early-returns
|
||||
`false` if there is no registration and never removes one. Its own xmldoc
|
||||
calls it "the registry counterpart of retail
|
||||
`CPhysicsObj::remove_shadows_from_cells` during temporary
|
||||
leave-world/pending-cell residence; deliberately not logical teardown."
|
||||
- **The restore path still works.** This is the trap I looked for: if `Suspend`
|
||||
had dropped `_entityReg`, then `TryApplyWithdrawalRestoration` →
|
||||
`TryPublishPlace` → `SyncPose(force: true)` → `UpdatePosition` would hit the
|
||||
`:696` not-registered early return and **silently no-op while still writing
|
||||
the cache** — reintroducing the exact AP-145 class on the restore edge. It
|
||||
does not: `_entityReg` survives `Suspend`, `UpdatePosition` proceeds, and
|
||||
`Register` → `DeregisterCore` (`:1789`) clears `_suspendedEntities` so the
|
||||
entity is no longer treated as suspended by `RefloodOwnerForLandblock`
|
||||
(`:1568`) or the reflood capture (`:1530`). The restore is clean.
|
||||
- **It matches the established App-layer pairing.** `Suspend` is exactly what
|
||||
`LiveEntityProjectionWithdrawalController.LeaveWorld` already does
|
||||
(`:148 _shadows.Suspend(entity.Id)` + `:156 _localPlayerShadow.Clear()`), and
|
||||
`LocalPlayerShadowSynchronizer.Suspend` (`:109–114`) is precisely that pair in
|
||||
one call. This is not a novel choice invented for the fix; the sink was the
|
||||
odd one out.
|
||||
- **No new early-return.** `Suspend` is unconditional — unlike `SyncPose` it has
|
||||
no hidden/celless/current-projection guard — so the Withdraw edge cannot
|
||||
silently skip the way the Place edge theoretically can.
|
||||
|
||||
### Is the constructor-parameter removal safe on every path?
|
||||
|
||||
Yes. `_localPlayerShadow` had exactly two uses in the sink (Place `.Set`,
|
||||
Withdraw `.Clear`); both are now synchronizer calls, so the field is genuinely
|
||||
dead. All **6** `new RuntimePlacementPresentationSink(` sites are updated
|
||||
(1 production + 5 test fixtures) and the Release build is 0-warning. The
|
||||
production site still constructs the synchronizer from `d.LocalPlayerShadow`,
|
||||
so the same single `LocalPlayerShadowState` instance is still the one cache —
|
||||
the removal narrows the sink's surface without changing which object holds
|
||||
state. This is a genuine simplification, not just a shuffle.
|
||||
|
||||
### Does the new 4th fact discriminate?
|
||||
|
||||
`Withdraw_SuspendsRealPhysicsShadow_NotOnlyTheDedupCache` (`:383–…`) establishes
|
||||
a **real** source-cell registration (`:391–406`) plus a non-null cache, then
|
||||
asserts after the Withdraw that `LocalShadow.Current` is null **and**
|
||||
`GetObjectsInCell(SourceCell)` no longer contains the entity. Under the pre-fix
|
||||
`_localPlayerShadow.Clear()` the first passes and the **second fails** — and the
|
||||
test comment at `:419–422` says exactly that, correctly labelling the cache
|
||||
assertion as the non-discriminating half. Right shape.
|
||||
|
||||
---
|
||||
|
||||
## A3 — the real drive was built; the **position** half of B2 is closed. **SUBSTANTIALLY CLOSED**, see M1.
|
||||
|
||||
`tests/AcDream.App.Tests/World/RuntimeForcePositionRenderCommitTests.cs`
|
||||
|
||||
This is a real correction, not a re-labelling. The chain is now:
|
||||
|
||||
`LiveEntityHydrationController.OnCreate` → real `RuntimeFirstEntryDriveController`
|
||||
pump → real `RuntimeEntityObjectLifetime.TryApplyPosition` (asserted to yield
|
||||
`PositionTimestampDisposition.ForcePosition`, `:111`) → real
|
||||
`RuntimeAcceptedPositionDriveController.TryExecuteAcceptedLocalPosition`
|
||||
(`:113–119`, asserted `Committed`) → real
|
||||
`RuntimePlacementProjectionSubscription` on the same placement channel
|
||||
(`:309–312`) → real `RuntimePlacementPresentationSink` → render `WorldEntity`.
|
||||
Nothing between the wire update and the assertion is hand-authored.
|
||||
|
||||
**Why the position assertion is a true discriminator, verified:**
|
||||
|
||||
- The wire carries `Z = 0` (`ForceUpdate` → `ServerPosition(Cell, 15, 15, 0, …)`).
|
||||
The assertion demands `Z = 0.48f` — the grounded foot-sphere clearance the
|
||||
**resolver** produces. A value that cannot be an echo of the input is exactly
|
||||
the right shape for a "came from the committed receipt" claim.
|
||||
- In this fixture the **only** post-materialization writer of
|
||||
`entity.Position` is `LiveEntityRuntime.TryApplyRuntimePlacementProjection`,
|
||||
invoked by the sink. `HostMaterializer` writes it once at create; there is no
|
||||
`LiveEntityNetworkUpdateController` in the composition, so B1's tolerated
|
||||
generic write cannot mask anything. Sever the receipt→render write and the
|
||||
entity stays at the first-entry pose `(10, 10, 0.48)` — `:127`
|
||||
(`Assert.Equal(ForcedPosition, entity.Position)`) is the assertion that fails,
|
||||
and `:126` (`NotEqual(positionBeforeForce, …)`) fails with it.
|
||||
- `positionBeforeForce` is captured live (`:93`) rather than assumed, so the
|
||||
"it moved" claim cannot be satisfied by a coincidence of constants.
|
||||
|
||||
**Fixture seams — both acceptable, neither touches production:**
|
||||
`WorldSession.GameActionCapture` is a pre-existing Phase-I.3 test seam
|
||||
(`src/AcDream.Core.Net/WorldSession.cs:2026`, unmodified by this diff), and
|
||||
`usePositionFromServer: true` is a legitimate autonomy-level-2 configuration,
|
||||
not a suppression flag added for the test. Resolving the three obstacles in the
|
||||
assertions rather than in production code was the correct call.
|
||||
|
||||
**B2's status:** the seam B2 actually named — "canonical body moves, render
|
||||
entity stays put" — is now genuinely covered end to end. I would record B2 as
|
||||
**closed for position**, with the cell half called out (M1) rather than assumed.
|
||||
|
||||
---
|
||||
|
||||
## M1 — MINOR. The **cell** half of B2 is still not pinned
|
||||
|
||||
**File:** `tests/AcDream.App.Tests/World/RuntimeForcePositionRenderCommitTests.cs:128`
|
||||
(`Assert.Equal(Cell, entity.ParentCellId)`), with the staging at `:73–81`.
|
||||
|
||||
`HostMaterializer` sets `ParentCellId = position.LandblockId` = `Cell` at
|
||||
materialization (`:412`), and the test deliberately picks landblock-local
|
||||
`(15,15)` so it lands in the **same** outdoor grid cell as the spawn `(10,10)`
|
||||
(`TerrainSurface.CellSize = 24` → `cx=0, cy=0` → low word `0x0001` for both).
|
||||
So `entity.ParentCellId` already equals `Cell` **before** the drive runs — this
|
||||
is caught vacuous-class #1 (*asserting a field written unconditionally
|
||||
earlier*). The comment at `:73–76` states the choice as a simplification
|
||||
("without coupling this test to the outdoor grid-cell formula"); the
|
||||
consequence is that the cell assertion cannot fail.
|
||||
|
||||
B2's recorded wording is "asserting the render entity's **position/cell** came
|
||||
from the committed placement receipt." The position half is now airtight; the
|
||||
cell half is asserted but unfalsifiable.
|
||||
|
||||
**Why it is MINOR, not MAJOR:** the discriminator carrying the test's claim is
|
||||
the position (including the resolver-only `Z`), it is sabotage-verified, and
|
||||
the round-1 shadow-composition fact 1 already pins a real cross-cell
|
||||
`ParentCellId` change (`SourceCell` → `DestinationCell`) through the same sink
|
||||
code path. Nothing is unprotected; the cell half is simply not proven *by this
|
||||
test*.
|
||||
|
||||
**Fix direction (small):** force to landblock-local `(30,30)` instead of
|
||||
`(15,15)` → `cx=1, cy=1` → outdoor low word `0x000A`, i.e. committed cell
|
||||
`0x0101000A ≠ Cell`. Then assert `entity.ParentCellId` equals the **committed**
|
||||
cell and differs from the spawn cell, and capture `cellBeforeForce` the way
|
||||
`positionBeforeForce` is captured. If that is judged out of scope, record in the
|
||||
commit message that B2 is closed for position and open for cell — do not book
|
||||
it as full closure.
|
||||
|
||||
---
|
||||
|
||||
## M2 — MINOR. The `SyncPose`-inherits-the-guard nuance is not documented
|
||||
|
||||
The handoff states the nuance "is now stated in the sink's comment and the test
|
||||
class doc rather than left implicit." It is not. I grepped both files for
|
||||
`hidden` / `suspend` / `celless` / `not-current` / `visible projection` /
|
||||
`IsCurrentVisibleProjection` / `guard`: the only hit is
|
||||
`RuntimePlacementShadowCompositionTests.cs:347`, inside the P4 test's *sabotage*
|
||||
reasoning (explaining why the child's own projection is current) — not a
|
||||
statement of the Place-edge behaviour change. Neither
|
||||
`RuntimePlacementPresentationSink.cs:245–272` nor the test class doc
|
||||
(`:20–89`) mentions it.
|
||||
|
||||
The nuance is real and worth one sentence: routing Place through `SyncPose`
|
||||
means the Place edge now inherits `SyncPose`'s guard
|
||||
(`LocalPlayerShadowSynchronizer.cs:53–59`) — if `IsHidden(playerGuid)`,
|
||||
`cellId == 0`, or `!IsCurrentVisibleProjection(entity)`, the Place now
|
||||
**suspends** the shadow where the old direct write merely cached. Both
|
||||
`TryApplyInitialCreateCompletion` and `TryApplyWithdrawalRestoration` reach
|
||||
`TryPublishPlace`, so this is reachable on more than the portal edge. The new
|
||||
behaviour is *correct* (it is what the next per-tick `Sync` would do anyway, and
|
||||
it is honest about a shadow that should not be published) — which is exactly
|
||||
why it belongs in a comment and the commit message rather than being discovered
|
||||
later as a surprise.
|
||||
|
||||
Same class as round 1's A5: a statement made in the handoff that the code does
|
||||
not carry.
|
||||
|
||||
---
|
||||
|
||||
## INFO — five test-file comments still cite the deleted `PhysicsEngine.Resolve` as live
|
||||
|
||||
Production is now clean: every remaining mention in `src/` is an explicit
|
||||
"deleted, cite by symbol" correction (`CellTransit.cs:880,:1064`,
|
||||
`HeadlessSessionWorldProjection.cs:797`, `PlayerMovementController.cs:147`). The
|
||||
A4 fixes are accurate and the `PlayerMovementController` class summary no longer
|
||||
claims a per-frame call to a deleted method.
|
||||
|
||||
Still stale, in test comments only (no behavioural weight, no compiler signal):
|
||||
|
||||
- `tests/AcDream.App.Tests/Physics/LiveEntityNetworkRemoteTeleportPresentationTests.cs:89`
|
||||
- `tests/AcDream.Core.Tests/Conformance/Issue107SpawnDiagnosticTests.cs:23,:82`
|
||||
- `tests/AcDream.Core.Tests/Physics/CellMarchLandblockPreservationTests.cs:22`
|
||||
- `tests/AcDream.Core.Tests/Physics/CellTransitFindCellSetTests.cs:301`
|
||||
|
||||
Optional sweep; not a gate.
|
||||
|
||||
---
|
||||
|
||||
## A5 and A6 — **CLOSED, and better than asked**
|
||||
|
||||
- **A5.** `PlayerMovementPlacementTransactionTests.cs:23–41` now states the
|
||||
asymmetry plainly: render-root publish **did** move
|
||||
(`RuntimeSetPositionState.cs:2774`), sticky release **did not move anywhere**
|
||||
(with the `grep` evidence and the `publishSharedState: false` reason), the
|
||||
behaviour was dead code so nothing regresses today, and "no layer pins the
|
||||
invariant … any more. That is disposition 3.6's one real coverage loss." That
|
||||
is the honest version. I re-verified both halves independently.
|
||||
- **A6.** `TransitionScratchDifferentialTests.cs:218–219` and `:236–239` now
|
||||
assert `IsCommitted` on both engines with distinguishing messages. The
|
||||
differential can no longer pass on symmetric failure.
|
||||
|
||||
---
|
||||
|
||||
## Register evidence — re-verified
|
||||
|
||||
- **AP-1 — retire: still justified.** Zero `PhysicsEngine.Resolve` /
|
||||
`.ResolvePlacement` receivers in `src/`; the resolver-shaped entry points no
|
||||
longer exist, so the row's condition is structurally unreopenable.
|
||||
- **AD-1 — retire: still justified.** The recoverable outdoor demote and the
|
||||
outdoor-restore `max(terrainZ, z)` lift were `Resolve`'s body; the body is
|
||||
gone.
|
||||
- **AP-145 — retire: now correctly scoped, and it does not overclaim.** The row
|
||||
(`retail-divergence-register.md:175`) covers **both** halves, names
|
||||
`TryPublishWithdrawal` and the `#184` shape explicitly, states that the sink
|
||||
no longer holds a `LocalPlayerShadowState` reference at all, and — notably —
|
||||
**records that the first version of the P4 fact was vacuous and was corrected
|
||||
at review**. Every claim in it now maps to something I verified: the
|
||||
publish-before-cache ordering, `Register`'s `DeregisterCore`, `Suspend`'s
|
||||
retained registration, the single-instance composition, and four
|
||||
discriminating facts. Nothing in the row claims more than the fix delivers.
|
||||
The one thing it does **not** mention is the M2 guard nuance — worth a clause.
|
||||
|
||||
---
|
||||
|
||||
## Flake attribution — confirmed **#302**, not diff-caused
|
||||
|
||||
The reproduced failure is
|
||||
`PortalProjectionTests.ClipToRegion_FrameOwnedStore_ReusesExactResultArray`
|
||||
(`tests/AcDream.App.Tests/Rendering/PortalProjectionTests.cs:503`) — exactly the
|
||||
test `docs/ISSUES.md:1197` files as **#302**: a
|
||||
`GC.GetAllocatedBytesForCurrentThread()` assertion in `AcDream.App.Tests`,
|
||||
JIT-tiering sensitive, measured 1-in-6 in isolation and once under full-suite
|
||||
load. That is the #302 signature, not the load-sensitive `NakEmissionTests`
|
||||
#308 look-alike that `ISSUES.md:1211–1224` warns has been conflated twice.
|
||||
|
||||
It cannot be diff-caused: the file is untouched (last commit `749e8cee`, zero
|
||||
working-tree diff), no rendering or portal-projection code is in this change
|
||||
set, and the assertion measures thread-local GC bytes in a component this diff
|
||||
does not reach. It passed clean in my own full-suite run
|
||||
(App 4,132 passed / 0 failed). Correctly named and not chased.
|
||||
|
||||
---
|
||||
|
||||
## Before commit
|
||||
|
||||
1. **M1** — extend the B2 test to a different outdoor grid cell (local `(30,30)`
|
||||
→ `0x0101000A`) so the cell half is falsifiable, **or** record B2 as
|
||||
position-closed / cell-open in the commit message. Do not book full closure
|
||||
silently.
|
||||
2. **M2** — add the one-sentence guard nuance to the sink's Place comment and
|
||||
the AP-145 row.
|
||||
3. Carry forward round 1's commit-message requirements: the §3.1 audit outcome
|
||||
(11 deleted / 0 re-pointed), the §3.3 covering-test judgment, the §3.6
|
||||
coverage-loss declaration (now correctly worded in the test's xmldoc), and
|
||||
the count reconciliation **11,112 − 11 + 5 = 11,106 / 4 skips**.
|
||||
411
docs/research/2026-08-05-c5a-architecture-review.md
Normal file
411
docs/research/2026-08-05-c5a-architecture-review.md
Normal file
|
|
@ -0,0 +1,411 @@
|
|||
# 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.
|
||||
470
docs/research/2026-08-05-c5a-contract.md
Normal file
470
docs/research/2026-08-05-c5a-contract.md
Normal file
|
|
@ -0,0 +1,470 @@
|
|||
# C5a contract — legacy deletion sweep + carried parity tests (pinned 2026-08-05)
|
||||
|
||||
Pinned at HEAD **`392c1e22`** (branch `claude/acdream-physics-divergence-5aa784`),
|
||||
i.e. AFTER #319 landed. Every symbol, caller census, and line number below was
|
||||
**re-verified against this HEAD by grep/read**, not inherited from the C5
|
||||
scoping (`2026-08-05-c5-scoping.md`, written at `52175aa1`) — §9 lists every
|
||||
place the scoping's picture moved. Baseline: complete Release suite
|
||||
**11,112 passed / 4 skipped / 0 failed**, measured at `392c1e22` (the #319
|
||||
commit message records the measurement; re-measure at implementation start,
|
||||
never inherit — process rule (c)).
|
||||
|
||||
**Scope, stated negatively first:**
|
||||
|
||||
- **NOT #275.** The steady-state inbound-Position merge
|
||||
(`InboundPhysicsStateController.TryApplyPosition`, the simple overload) and
|
||||
`RuntimeEntityObjectLifetime`'s wire-derived `FullCellId` refresh (the
|
||||
`refreshPosition: acceptedPosition` call, **now at `:1926`** post-#319) are
|
||||
the C5b behaviour change with its own contract. C5a must not modify either
|
||||
file's executable code (one test-file doc-comment correction is the only
|
||||
permitted touch near this surface, §1 D7).
|
||||
- **NOT the probe strip.** All six `ACDREAM_PROBE_*` temporary flags stay
|
||||
(C5c); they are env-gated and inert to everything here.
|
||||
- **NOT AP-131, NOT AD-60's legacy half, NOT AP-145's seam** (except the
|
||||
pre-authorized red branch in §5.1). Those rows stay in the register
|
||||
untouched.
|
||||
|
||||
**Scope, positively:** the six deletion groups in §1 (~490 production lines),
|
||||
the seven test-caller dispositions in §3, retirement of register rows **AP-1**
|
||||
and **AD-1** in the same commit as the deletions, and the two carried parity
|
||||
tests in §5 (#318 composition; route-2 B2).
|
||||
|
||||
---
|
||||
|
||||
## 1. Deletion inventory — re-verified at `392c1e22` by symbol
|
||||
|
||||
Caller censuses below are exhaustive over `src/` (all `*.cs`). Method: for
|
||||
`Resolve`, every `.Resolve(` receiver in `src/` was enumerated and typed — 38
|
||||
distinct receiver/site classes, **none** a `PhysicsEngine` (see the grep-hygiene
|
||||
note in §3.8: two of them are #319's NEW `ParentAttachmentState.Resolve`, a
|
||||
name collision that did not exist when the scoping ran its census). For the
|
||||
others, direct symbol grep over `src/` and `tests/`.
|
||||
|
||||
| # | Symbol | Location at HEAD | Production callers | Test callers | ~Lines |
|
||||
|---|---|---|---|---|---|
|
||||
| D1 | `PhysicsEngine.Resolve(Vector3, uint, Vector3, float)` | `src/AcDream.Core/Physics/PhysicsEngine.cs:1863`–`~2200` (body ends before `ResolveWithTransition`'s xmldoc; the live method at `:2223` is a **different member** and stays) | **ZERO** | `PhysicsEngineTests.cs` ×12 (`:41,:48,:66,:88,:111,:150,:186,:211,:391,:434,:446,:460`); `Issue133DungeonTeleportPrefixTests.cs:58` | ~360 |
|
||||
| D2 | `PhysicsEngine.HasCellSurface` | `PhysicsEngine.cs:1767`–`~1789` | only `Resolve` itself (`:1887`) — deletes with D1 | none | ~23 |
|
||||
| D3 | `PhysicsEngine.ResolvePlacement` | `PhysicsEngine.cs:2748`–`~2815` | **ZERO** (sole non-test mention is the already-recorded retirement comment at `HeadlessSessionWorldProjection.cs:794`) | `InitialPlacementOverlapTests.cs:42`; `TransitionScratchDifferentialTests.cs:185,:194,:208,:217` | ~70 |
|
||||
| D4 | `PlayerMovementController.SetPosition` (both overloads) | `src/AcDream.Runtime/Gameplay/PlayerMovementController.cs:1746,:1760` | **ZERO** — every `.SetPosition(` in `src/` outside `PhysicsEngine.cs` is `WorldEntity.SetPosition` (receivers `entity.`/`child.`) or Core `PhysicsEngine.SetPosition` via `_physics.Engine.` from `RuntimeSetPositionState` (`:2028,:3125,:4789`), the canonical path | 19 test files, ~80 sites (44 in `PlayerMovementControllerTests.cs` alone) — **fixture setup**, not subject (§3.5) | ~30 gross; ~15 net after the retained seed (§3.5) |
|
||||
| D5 | `PlayerMovementController.CommitPreparedPosition` | `PlayerMovementController.cs:1789` | **ZERO** — production replacement is `ArmConstraintLeashAtCommittedPlacement` (`:1815`), called from `RuntimeLocalPlayerPhysicsPublicationState.cs:774`; `PreparePositionForCommit` (`:1776`) remains production via `RuntimeLocalPlayerPhysicsPublicationState.cs:219` | `PlayerMovementPlacementTransactionTests.cs:42`; `PlayerMovementControllerTests.cs:1158`; `RuntimeLocalPlayerPhysicsPublicationStateTests.cs:3007` | ~25 |
|
||||
| D6 | `RuntimeSetPositionState.BeginAcceptedPlacement` / `BeginAuthoredPlacement` | `src/AcDream.Runtime/Physics/RuntimeSetPositionState.cs:1321,:1333` | **ZERO** — pure pass-throughs to `BeginAcceptedPlacementCore`; production reaches the core via `Apply` (`:1304`) and the authored sequence | **39 sites across 9 Runtime test files** (scoping said ~40/10; re-censused) | **0 — KEEP as documented test seam** (§3.7) |
|
||||
| D7 | Doc hygiene | stale `BlipPosition`/`PlayerMovementController.SetPosition` doc refs at `src/AcDream.Core/Physics/Motion/ConstraintManager.cs:25` (note: the scoping's path lacked `Motion/`) and `src/AcDream.Core/Physics/PhysicsBody.cs:442`; the stale `TryApplyPickup (:1116)` citation in `RuntimeAcceptedPositionDriveControllerTests.cs` (~`:221` region; the method lives in `RuntimeEntityObjectLifetime`, currently ~`:1258+`) — **cite by symbol, not line**, in the correction | — | — | ~20 comment lines |
|
||||
|
||||
Net production deletion: **~490 lines** (the scoping's ~540 minus D6's kept
|
||||
~25 and D4's retained seed). All deletions are compile-loud.
|
||||
|
||||
**Confirmed unchanged from the scoping's §1c (NOT deletable, re-spot-checked):**
|
||||
`ILocalPlayerTeleportPlacement`/`LocalPlayerTeleportPlacement.Place` (live
|
||||
post-commit presentation suffix, `LocalPlayerTeleportController.cs:243` still
|
||||
calls `entity.SetPosition(controller.Position)` inside it);
|
||||
`PlayerMovementController.SetPositionCore` (`:1845`, production via
|
||||
`PreparePositionForCommit`); the route-4 leftovers; AP-135's bookkeeping;
|
||||
`RuntimeLiveEntitySessionController.cs:141`'s pre-flip path (its file WAS
|
||||
touched by #319, but the C4/C5-revisit comment's unblock condition is still
|
||||
unmet).
|
||||
|
||||
---
|
||||
|
||||
## 2. The survivor hazard — TWO production members inside the deletion region, not one
|
||||
|
||||
The scoping named one. Re-verification at HEAD finds **two**:
|
||||
|
||||
1. **`IsSpawnCellReady` (`PhysicsEngine.cs:1807`)** — production callers
|
||||
`RuntimeSetPositionState.cs:2169,:4378` and
|
||||
`SessionPlayerComposition.cs:374`. Sits between `HasCellSurface` (delete)
|
||||
and `Resolve` (delete).
|
||||
2. **`AdjustPosition` (`PhysicsEngine.cs:1813`)** — **the scoping never
|
||||
dispositioned it.** It is production: `PhysicsCameraCollisionProbe.cs:38,:100`
|
||||
(the camera collision probe), plus six-plus test files
|
||||
(`PhysicsEngineAdjustPositionTests`, the camera replay suites,
|
||||
`Issue177StairDescentCameraFloodTests`, ...). It sits between
|
||||
`IsSpawnCellReady` and `Resolve` — dead centre of the physical block.
|
||||
|
||||
**Pinned survival rule:** the deletion is **member-wise, never region-wise**.
|
||||
Delete exactly the bodies of `HasCellSurface`, `Resolve`, and
|
||||
`ResolvePlacement`; `IsSpawnCellReady` (`:1807`–`:1811`) and `AdjustPosition`
|
||||
(`:1813`–`:1861`) remain byte-identical in executable code.
|
||||
|
||||
**Xmldoc fallout (same commit):** `IsSpawnCellReady`'s summary contains
|
||||
`<see cref="Resolve"/>` (in the `:1791`–`:1806` block) and `HasCellSurface`'s
|
||||
summary names "the Resolve safety net"; the second dies with its method, the
|
||||
first must be rewritten (the "loud outdoor-demote safety net" sentence
|
||||
describes machinery this commit deletes — rewrite the paragraph to describe
|
||||
the canonical `SetPosition` reality, do not leave a cref to a deleted symbol).
|
||||
Sweep `PhysicsEngine.cs` for any other `cref="Resolve"`/`cref="ResolvePlacement"`
|
||||
after the deletion; the build must be warning-clean on missing crefs.
|
||||
|
||||
---
|
||||
|
||||
## 3. The seven test-only-caller dispositions — re-verified, each carried forward
|
||||
|
||||
Framing (from the scoping, still correct): deleting a method whose callers are
|
||||
tests is compile-loud. The silent hazard is the **disposition of the tests
|
||||
afterward** — deleting a test that pinned MOVED behaviour loses the pin with a
|
||||
green build. Each case below is a binding disposition; any deviation must be
|
||||
argued in the commit message.
|
||||
|
||||
### 3.1 `PhysicsEngine.Resolve` unit tests — behaviour GONE → DELETE, after a one-pass audit
|
||||
|
||||
`PhysicsEngineTests.cs` (565 lines, 12 `engine.Resolve(` sites). The
|
||||
outdoor-demote / legacy floor-snap / terrain-lift semantics die with the
|
||||
method; canonical replacements have their own suites (`PhysicsSetPositionTests`,
|
||||
`RuntimeSetPositionStateTests`). **Audit before deleting:** any individual
|
||||
assertion that actually pins canonical-owned behaviour — specifically
|
||||
`AdjustPosition` semantics, which SURVIVE — is re-pointed at `AdjustPosition`
|
||||
directly (note `PhysicsEngineAdjustPositionTests.cs` already exists; a
|
||||
re-point may land there). The audit's outcome (N assertions re-pointed, M
|
||||
deleted) goes in the commit message.
|
||||
|
||||
### 3.2 `Issue133DungeonTeleportPrefixTests` — behaviour MOVED → RE-POINT (named-bug regression pin)
|
||||
|
||||
Verified at HEAD: the defect mechanism this test pins — the `lbPrefix`
|
||||
resident-landblock scan that re-stamped a validated dungeon claim with a
|
||||
neighbour's prefix — lives **entirely inside `Resolve`'s body**
|
||||
(`PhysicsEngine.cs:1928,:1937,:1984-1995,:2193`). The canonical
|
||||
`PhysicsSetPosition` path has **no lbPrefix scan** — the defect class cannot
|
||||
recur there by construction. That is precisely why the pin must be
|
||||
**re-pointed, not deleted**: #133 is a closed named bug, and the invariant
|
||||
(a validated dungeon claim's landblock prefix is authoritative; a committed
|
||||
cell never gets re-stamped from a neighbouring resident block) must stay
|
||||
assertable against whatever path owns placement now.
|
||||
|
||||
**Re-point spec:** drive a teleport-classified canonical placement
|
||||
(`RuntimeSetPositionState`, or Core `PhysicsEngine.SetPosition` if the fixture
|
||||
cost is lower) with the test's exact geometry — dungeon claim `0x00070143`,
|
||||
dungeon block world-offset so its local Y is negative, resident neighbour
|
||||
block at origin containing the same XY — and assert the committed cell keeps
|
||||
the `0x0007` prefix. Alternative accepted by this contract: PROVE an existing
|
||||
canonical test already pins prefix authority for an off-bounds dungeon claim
|
||||
and record the proof (test name + assertion) in the deleting commit. Silent
|
||||
deletion is a contract violation.
|
||||
|
||||
### 3.3 `InitialPlacementOverlapTests` — behaviour MOVED → VERIFY-THEN-DELETE
|
||||
|
||||
The ring-search half of enter-world placement is ported inside canonical
|
||||
`SetPosition` (`TransitionTypes.cs:1596` `FindPlacementPosition`, retail
|
||||
0x0050C170; `PhysicsSetPositionTests.cs` header cites it and has
|
||||
placement-probe scenarios, e.g. the `placementPasses >= 2` retry arm at
|
||||
`:1152-1194`). **The audit criterion:** confirm `PhysicsSetPositionTests`
|
||||
covers the **other-entity-occupancy** ring search this test pins (a relogging
|
||||
player overlapping a registered creature sphere searches outward to the
|
||||
nearest clear ring) — occupancy-driven, not merely BSP-failure-driven. If
|
||||
covered: delete, citing the covering test by name. If not: re-point this
|
||||
test's scenario through canonical `SetPosition` with a placement class that
|
||||
reaches `FindPlacementPosition` (~50–100 lines), then delete the
|
||||
`ResolvePlacement` call.
|
||||
|
||||
### 3.4 `TransitionScratchDifferentialTests` — differential arm → RE-POINT OR DROP EXPLICITLY
|
||||
|
||||
Verified at HEAD: the spec-based sequence arms (`ResolveSpec.Resolve` at
|
||||
`:618`) call `ResolveWithTransition` — **untouched by this slice**. Only
|
||||
`ReusedScratch_MatchesFreshPlacementSearch` (`:180`–`:227`, four
|
||||
`ResolvePlacement` sites) is affected. It is a Slice-I zero-alloc scratch-reuse
|
||||
differential over the placement search (including the hostile-identity
|
||||
leak check). **Disposition:** re-point the arm at the canonical entry that
|
||||
reaches `FindPlacementPos` (Core `SetPosition` with the appropriate placement
|
||||
class), preserving both the bitwise fresh-vs-reused comparison and the
|
||||
second-identity leak check. If re-pointing is disproportionate, the arm may be
|
||||
dropped ONLY with an explicit commit-message decision naming what coverage the
|
||||
I-slice differential loses — never silently.
|
||||
|
||||
### 3.5 `PlayerMovementController.SetPosition` fixture usage — NEITHER gone nor moved → RETAINED SEED + mechanical re-point
|
||||
|
||||
Census at HEAD: **19 test files** reference `PlayerMovementController` and
|
||||
call `.SetPosition(`; ~80 sites total; 44 in `PlayerMovementControllerTests.cs`,
|
||||
8 in `LocalPlayerTeleportControllerTests.cs`, 5 in `HeadlessSessionHostTests.cs`,
|
||||
the rest 1–4 each. (Per-site care: a file can reference the controller and
|
||||
still call `WorldEntity.SetPosition` — type each site during the re-point,
|
||||
don't regex-replace blind.)
|
||||
|
||||
**Pinned design (the scoping's "cheaper and honest" option, adopted):** keep
|
||||
**ONE** internal, explicitly-named test seed on the controller — rename the
|
||||
3-arg overload to `SeedPlacementForTest(Vector3 pos, uint cellId, Vector3 cellLocal)`
|
||||
(internal; xmldoc states it exists ONLY to seed fixtures and that production
|
||||
placement flows through `PreparePositionForCommit` →
|
||||
`ArmConstraintLeashAtCommittedPlacement`), delete the 2-arg overload, and
|
||||
mechanically re-point all ~80 sites. Semantics are reproduced by construction:
|
||||
the seed calls the SAME `SetPositionCore` (which stays production), so the
|
||||
grounded, zero-velocity start (the AD-61 force-seed) that dozens of movement
|
||||
tests assume is unchanged. This is the one production-file signature change
|
||||
in the slice; its body is untouched.
|
||||
|
||||
### 3.6 `CommitPreparedPosition` tests — behaviour MOVED → RE-POINT at the arm/commit replacement
|
||||
|
||||
Three sites, each audited individually:
|
||||
|
||||
- `PlayerMovementPlacementTransactionTests.cs:42` (100-line file): the
|
||||
prepared-position transaction assertions run against the production pair
|
||||
(`PreparePositionForCommit` + `ArmConstraintLeashAtCommittedPlacement`) —
|
||||
rewrite the test against that pair, or delete it if
|
||||
`RuntimeLocalPlayerPhysicsPublicationStateTests` provably covers the same
|
||||
transaction shape (cite which test).
|
||||
- `PlayerMovementControllerTests.cs:1158` and
|
||||
`RuntimeLocalPlayerPhysicsPublicationStateTests.cs:3007` both assert
|
||||
`Throws<InvalidOperationException>` on the uncommitted/displaced state.
|
||||
Audit whether the replacement arm carries an equivalent guard; if yes,
|
||||
re-point the throw assertion at it; if the guard died with the method,
|
||||
delete the assertion WITH a commit-message note (guard semantics gone, not
|
||||
overlooked).
|
||||
|
||||
### 3.7 `BeginAcceptedPlacement`/`BeginAuthoredPlacement` — NEITHER → KEEP AS DOCUMENTED SEAM
|
||||
|
||||
39 sites across 9 Runtime test files at HEAD. The wrappers are pure
|
||||
pass-throughs to the production core (`BeginAcceptedPlacementCore`); deleting
|
||||
them buys zero behaviour and costs broad mechanical churn across the Runtime
|
||||
suite. **Disposition: keep, with an xmldoc sentence on each wrapper naming it
|
||||
a test seam** (so a future sweep does not re-litigate this). This is a
|
||||
recorded deliberate exception to "delete every superseded legacy path": the
|
||||
wrappers are not a legacy PATH — the core they call IS the canonical path.
|
||||
|
||||
### 3.8 Landmines and grep hygiene
|
||||
|
||||
- **The #316-preserving pair** (`LiveEntityNetworkOnPositionCollapseMatrixTests.cs:131,:180`)
|
||||
pins a defect **preserved verbatim**. C5a must not touch it; it inverts only
|
||||
with #316's measured fix (C5-gate session / later).
|
||||
- **#319 introduced `ParentAttachmentState.Resolve`** (`ParentAttachmentState.cs:432`),
|
||||
called at `EquippedChildRenderController.cs:920` and
|
||||
`RuntimeLiveEntitySessionController.cs:390`. A mechanical grep for
|
||||
`.Resolve(` now hits relation resolution — neither site is `PhysicsEngine`.
|
||||
Any "prove zero callers" re-run during implementation must type receivers,
|
||||
not count matches.
|
||||
- The affected-file overlap between #319 and this slice is **empty**: #319
|
||||
touched `EquippedChildRenderController`, `LiveEntityHydrationController`,
|
||||
`LiveEntityPresentationController`, `ParentAttachmentState`,
|
||||
`RuntimeEntityObjectLifetime`, `RuntimeLiveEntitySessionController` — none
|
||||
contains a C5a deletion target. Verified.
|
||||
|
||||
---
|
||||
|
||||
## 4. Register retirements — AP-1 and AD-1, with the code evidence; four rows explicitly untouchable
|
||||
|
||||
A row retires because the code proves its condition met. Both retirements ride
|
||||
**in the same commit as the D1–D5 deletions** (register rule 1).
|
||||
|
||||
### AP-1 — RETIRE. Evidence at `392c1e22`:
|
||||
|
||||
Row text: "Production zero-delta routes deliberately remain on the legacy
|
||||
resolver until 4B2..." — **false at HEAD**:
|
||||
|
||||
1. The "legacy resolver" is `PhysicsEngine.Resolve`/`ResolvePlacement`. The
|
||||
exhaustive receiver census (§1) shows **zero** `PhysicsEngine.Resolve` or
|
||||
`.ResolvePlacement` call sites in `src/`.
|
||||
2. Every production placement writer reaches Core `PhysicsEngine.SetPosition`
|
||||
**only** through `RuntimeSetPositionState` (`:2028,:3125,:4789` — the only
|
||||
three `_physics.Engine.SetPosition` sites in `src/`).
|
||||
3. The row's named prerequisites (authored mover, rebucketing,
|
||||
prefix-quiescence, body publication, atomic route cutover) landed across
|
||||
C0–C4; the local controller's body adoption landed at C3c.
|
||||
4. Deleting D1–D5 makes the retirement **structural**: the resolver-shaped
|
||||
entry points cease to exist, so no future caller can re-open the row's
|
||||
condition.
|
||||
|
||||
The narrower survivors (#276 settle-cell discard, AD-61 force-seed, AD-62
|
||||
non-commit outcomes) are separately filed rows/issues and do not block AP-1's
|
||||
own condition — deleting AP-1 does not orphan them.
|
||||
|
||||
### AD-1 — RETIRE. Evidence at `392c1e22`:
|
||||
|
||||
Row text: "Production authoritative placement still routes through the legacy
|
||||
recoverable outdoor demote and outdoor-restore `max(terrainZ, z)` lift" —
|
||||
**false at HEAD**: that code is `Resolve`'s body (demote at
|
||||
`PhysicsEngine.cs:~1890-1910`, the outdoor `max(terrain, z)` lift inside the
|
||||
snap block ~`:2160-2175`) and `Resolve` has zero production callers. The
|
||||
lost-cell stand-in the row describes is unreachable from production. Deleting
|
||||
D1/D2 removes the divergent mechanism outright.
|
||||
|
||||
### Must NOT be touched (each blocked on work outside this slice):
|
||||
|
||||
- **AP-131** — retires only with **#275** (C5b): the legacy
|
||||
`TryApplyPosition` unconditional `installPlacementFrame: true, clearParent: true`
|
||||
is still the ONLY steady-state production Position merge at HEAD.
|
||||
- **AD-60's legacy half** — same gate (#275): the
|
||||
`RefreshSnapshot(..., refreshPosition: acceptedPosition)` site — **`:1926`
|
||||
at HEAD** (the scoping's `:1918` and the register's `:1338` are both stale;
|
||||
cite the symbol) — still derives `FullCellId` from bare wire acceptance.
|
||||
- **AP-145** — retires with **#318's fix**, never with its test. The §5.1
|
||||
composition test makes the asymmetry falsifiable; only the pre-authorized
|
||||
red branch may touch the seam, and then AP-145 retires in THAT commit.
|
||||
- **AD-61 / AD-62 / AP-135 / AP-141–146** — all carry their own retirement
|
||||
conditions; none is met by anything in this slice. (AP-146 and the AP-132
|
||||
amendment are #319's, three days old — do not disturb.)
|
||||
|
||||
---
|
||||
|
||||
## 5. The two carried parity tests
|
||||
|
||||
Both are test-only against HEAD's production code, land BEFORE the deletion
|
||||
commit (they are independent of it and de-risk the slice's review), and both
|
||||
follow process rule (e): sabotage-verified, with the WHICH-assertion-fails
|
||||
check, both directions for dual-layer assertions.
|
||||
|
||||
### 5.1 #318 composition test (~150–300 lines, App.Tests)
|
||||
|
||||
Drive a real portal arrival through the canonical drive controller + the
|
||||
**REAL** `RuntimePlacementPresentationSink` + the **REAL** `PhysicsEngine`
|
||||
(fixture patterns exist: `RuntimePlacementPresentationSinkTests.cs`,
|
||||
`RuntimeFirstEntryHostIntegrationTests.cs`). The discriminating assertion:
|
||||
|
||||
> **`PhysicsEngine.ShadowObjects` (`PhysicsEngine.cs:147`) holds a row at the
|
||||
> destination cell/position** — NEVER merely `LocalPlayerShadowState`'s dedup
|
||||
> cache. AP-145's bypass (`RuntimePlacementPresentationSink.cs:243`
|
||||
> `_localPlayerShadow.Set(...)` skipping `LocalPlayerShadowSynchronizer.SyncPose`'s
|
||||
> publish) both skips the publish AND pre-seeds `SyncPose`'s dedup — a
|
||||
> cache-only assertion is satisfied by the bug.
|
||||
|
||||
Plus the T8 write-ordering assertion from route 3 §8. Sabotage: perturb
|
||||
`TryPublishPlace` to the cache-only shape and confirm the `ShadowObjects`
|
||||
assertion (not an incidental one) fails; separately confirm a cache-only
|
||||
assertion would pass under the same sabotage — proving the discriminator
|
||||
discriminates.
|
||||
|
||||
**Pre-authorized red branch:** this test may legitimately FAIL at HEAD — the
|
||||
composition drives placement with no subsequent movement tick, which is
|
||||
exactly the window AP-145 says is unpublished. If red: **C5a's deletion work
|
||||
does not absorb the fix.** The seam fix (routing the placement's shadow update
|
||||
through the real publish) is a production behaviour change on a
|
||||
narrow, low-frequency path; it lands as its **own reviewed commit** together
|
||||
with the now-green test, retires **AP-145**, and closes **#318** — and the
|
||||
composition test itself is its designed gate (the C4 handoff explicitly ruled
|
||||
the connected route out as #318 coverage). If green: land as-is; #318 closes;
|
||||
AP-145's row is then re-argued (its "why not observed" column may become its
|
||||
retirement argument) — but only with the green evidence cited.
|
||||
|
||||
### 5.2 Route-2 B2 parity test (~100–200 lines, App.Tests)
|
||||
|
||||
The campaign plan's recorded acceptance gap (plan §C4 route 2, recorded unmet
|
||||
since 2026-08-03): 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** — closing the
|
||||
"canonical body moves, render entity stays put" silent seam. Expected green
|
||||
at HEAD (route 2 landed; the seam is merely uncovered). If red, the same
|
||||
stop-and-report protocol as 5.1: a red parity test is a found defect, not a
|
||||
test problem; it gets its own investigation before any deletion lands.
|
||||
|
||||
Sabotage: sever the receipt→render write and confirm the position/cell
|
||||
assertion is the one that fails.
|
||||
|
||||
---
|
||||
|
||||
## 6. What must REMAIN true — the slice's invariants
|
||||
|
||||
1. **Zero production behaviour change.** The production diff consists of:
|
||||
member deletions with zero callers (D1–D5), comment/xmldoc edits (D7, §2),
|
||||
and exactly one signature change with an untouched body (§3.5's seed
|
||||
rename). No executable production statement is added or modified —
|
||||
**except** in the pre-authorized 5.1 red-branch commit, which is its own
|
||||
reviewed landing with its own register action.
|
||||
2. **The two survivors survive.** `IsSpawnCellReady` and `AdjustPosition`
|
||||
keep their exact executable bodies and all production callers
|
||||
(`RuntimeSetPositionState.cs:2169,:4378`; `SessionPlayerComposition.cs:374`;
|
||||
`PhysicsCameraCollisionProbe.cs:38,:100`).
|
||||
3. **Every deleted symbol's absence is proven** by the compiler (all deletions
|
||||
are compile-loud) AND every test caller has an explicit §3 disposition
|
||||
executed in the same commit — no test deleted whose pinned behaviour moved
|
||||
without its re-point landing alongside.
|
||||
4. **AP-1 and AD-1 retire in the SAME commit as the D1–D5 deletions** — never
|
||||
before (the code proof is the deletion), never after (register rule 1).
|
||||
5. **The DO-NOT-TOUCH set holds:** AP-131, AD-60, AP-145 (modulo 5.1 red
|
||||
branch), AD-61/62, AP-135, AP-141–146; the #275 surface files' executable
|
||||
code; the six probe flags; the #316-preserving test pair.
|
||||
6. **No skips.** The suite ends at 0 failed with the same 4 skips as
|
||||
baseline — a new skip is a contract violation (process rule (d)).
|
||||
7. **Counts are measured and reconciled.** The final suite total will move
|
||||
(deleted legacy tests down, re-points and two parity tests up); the commit
|
||||
message reconciles the net against baseline 11,112 explicitly (N deleted,
|
||||
M added, expected total), never hand-waves it.
|
||||
|
||||
---
|
||||
|
||||
## 7. Gates
|
||||
|
||||
- **Complete Release suite** (`dotnet test AcDream.slnx -c Release -m:1` with
|
||||
`ACDREAM_PAK_PATH` set), baseline **11,112 passed / 4 skipped / 0 failed**
|
||||
at `392c1e22` — re-measured at slice start AND at each commit. Known flakes,
|
||||
never conflated (they have been conflated twice): **#302**
|
||||
(`PortalProjectionTests.ClipToRegion_FrameOwnedStore_…`, GC-allocation,
|
||||
App.Tests — the `WarmedSteadyContactRefreshDoesNotAllocate` look-alike is
|
||||
this class) and **#308** (`NakEmissionTests.LossSoak_…`, wall-clock,
|
||||
Core.Net.Tests, full-suite load only).
|
||||
- **NO connected gate for C5a — argued, not assumed.** Process rule (g): a
|
||||
gate must be able to see the defect it gates. C5a's reachable defect
|
||||
classes are (i) a compile break — seen by the build; (ii) silent coverage
|
||||
loss — seen only by §3's dispositions and the review, invisible to any live
|
||||
session; (iii) a behavioural regression — **structurally excluded** by
|
||||
invariant 1: the production binary's reachable code is byte-equivalent, so
|
||||
a connected session would exercise identical behaviour and measure nothing.
|
||||
Precedents: route 5 recorded "no live gate can exist" rather than inventing
|
||||
one; route 6 was a zero-production-line closure. The 5.1 red-branch commit,
|
||||
if taken, ALSO needs no connected gate: #318's evidence channel is the
|
||||
composition test **by design** — the C4 handoff explicitly refused to score
|
||||
the connected route against it.
|
||||
- **The review IS the coverage gate.** One dual review over the combined
|
||||
slice diff (deletions + dispositions + parity tests), reviewers on Opus per
|
||||
the standing audit rule, with §3's table as the review checklist: for each
|
||||
of the seven cases, the reviewer confirms the disposition was executed as
|
||||
pinned or the deviation argued.
|
||||
|
||||
---
|
||||
|
||||
## 8. Size, commit plan, and the split call
|
||||
|
||||
Calibration: campaign landings ran ~127 (route 7) to ~418 (route 3) to
|
||||
~500 (4b-2) production lines each under full discipline.
|
||||
|
||||
| Piece | Production lines | Test lines | Risk |
|
||||
|---|---|---|---|
|
||||
| D1–D5+D7 deletions + §2 xmldoc | ~490 deleted, ~0 added | ~1,500–2,000 deleted/re-pointed across ~25 files | Low — compile-loud; the §3 dispositions are the judgment work |
|
||||
| §5.1 #318 composition test | 0 (green) / ~10–40 (red branch, own commit) | ~150–300 | Low; red branch is a decision point, pre-planned |
|
||||
| §5.2 route-2 B2 test | 0 | ~100–200 | Low |
|
||||
|
||||
**Call: C5a HOLDS as one slice, in two (possibly three) ordered commits under
|
||||
this single contract:**
|
||||
|
||||
1. **Commit 1 — the two parity tests** (test-only). Lands first: independent
|
||||
of the deletions, de-risks review, and settles 5.1's green/red question
|
||||
before the sweep. If 5.1 is red, its fix is **commit 1b** (own review
|
||||
round, retires AP-145, closes #318) before proceeding.
|
||||
2. **Commit 2 — the deletion sweep**: D1–D5, D7, §2 xmldoc, all §3
|
||||
dispositions, AP-1 + AD-1 row deletions. One diff, reviewable as one unit.
|
||||
|
||||
The ~490-line figure is at the top of the campaign's calibration band, but a
|
||||
deletion of caller-free code is a different risk class from route 3's ~418
|
||||
changed lines — the compiler proves most of it. What justifies keeping it
|
||||
whole rather than splitting D1/D2/D3 (Core) from D4/D5 (Runtime): AP-1's
|
||||
retirement evidence spans BOTH groups ("the last resolver-shaped entry points"
|
||||
includes D4/D5), so splitting would either retire AP-1 on a half-proof or
|
||||
leave the register straddling two commits — both worse than one larger
|
||||
reviewable deletion. Do NOT fold in: #276's remainder, #317's audit, any
|
||||
probe change (including the gate-4 `cause=` label improvement — C5c), or any
|
||||
#275-adjacent edit.
|
||||
|
||||
---
|
||||
|
||||
## 9. What moved between the scoping (`09911821`, at `52175aa1`) and this contract (`392c1e22`)
|
||||
|
||||
1. **`AdjustPosition` is a second production survivor inside the deletion
|
||||
region** (`PhysicsCameraCollisionProbe.cs:38,:100`) — the scoping's hazard
|
||||
note named only `IsSpawnCellReady`. A region-wise delete would have taken
|
||||
the camera collision probe's cell resolver with it. §2 pins member-wise
|
||||
deletion.
|
||||
2. **#319 created a `.Resolve(` name collision**: `ParentAttachmentState.Resolve`
|
||||
(`:432`), called from two files. The scoping's census predates it. Callers
|
||||
must be typed, not counted (§3.8).
|
||||
3. **AD-60's legacy-half site moved to `RuntimeEntityObjectLifetime.cs:1926`**
|
||||
(scoping said `:1918`; the register row still says `:1338`). C5a doesn't
|
||||
touch it, but C5b's contract must cite by symbol.
|
||||
4. **Begin* wrapper census: 39 sites / 9 files** (scoping: ~40 / 10).
|
||||
Immaterial to the disposition.
|
||||
5. **The scoping's D7 path `ConstraintManager.cs` is actually
|
||||
`Motion/ConstraintManager.cs`** (`src/AcDream.Core/Physics/Motion/`).
|
||||
6. **Everything else in the scoping's §1a/§1c/§2/§3 holds exactly at
|
||||
`392c1e22`**: zero production callers re-proven for all six groups; #319
|
||||
added no caller to any deletable symbol; the #319-touched file set is
|
||||
disjoint from every deletion target; the seven dispositions carry forward
|
||||
unchanged in substance.
|
||||
7. **New since the scoping, absorbed here:** gate 4 closed 2026-08-05 as a
|
||||
probe-label artifact (`af828a8a`) — the cell-less falsification is no
|
||||
longer C5-gate-session work; and route 7's gate criterion was corrected to
|
||||
the positive child-cell-equals-parent assertion (`2687d893`), whose
|
||||
still-owed connected run belongs to the C4/#319 ledger, not to C5a.
|
||||
Loading…
Add table
Add a link
Reference in a new issue