acdream/docs/research/2026-08-06-ad10-review-architecture.md

# AD-10 retirement-by-deletion — architecture review

**Reviewer scope:** completeness, blast radius, test quality. Retail fidelity is
a separate reviewer's report (`2026-08-06-ad10-review-retail.md`).
**Range:** `ef976c6d..2223ed17` on `claude/acdream-physics-divergence-5aa784`
(`fe6ee877`, `886333a2`, `fb454b74`, `2223ed17`).
**Date:** 2026-08-06.

## Verdict: **PASS**, with one process defect and one under-scoped issue

The Stage 0 measurement is **sound and I believe it** — I re-derived both
reported numbers independently and added four cases the implementer did not
report. The deletion is complete, the blast radius claim is correct, all three
reported sabotages reproduce with matching numerals, and the two deleted tests
were genuinely obsolete.

Nothing here is a workaround, a suppression flag, a grace period, a symptom
guard, a new skip, or a weakened test. No known flake was conflated.

The defects are: one **mandatory contract test silently dropped** (D1), a
**stale roadmap entry** (D2), and a **malformed doc comment** (D3). None
justifies blocking the change; D1 should be settled before the visual gate is
called done.

---

## Part 1 — Is the Stage 0 measurement sound? (priority 1)

### Method

I did not take the implementer's word for it. I deleted all 44 `bin`/`obj`
directories, built Release clean (0 errors), byte-scanned
`AcDream.Core.dll` to confirm the string `SampleTerrainNormal` is absent from
the compiled metadata (0 occurrences; `SampleTerrainWalkable` still present at
2, proving the scan works), then drove `RemoteRampHarness` through the
production `RuntimeRemotePhysicsUpdater.Tick` for 30 ticks and dumped every
position as **raw IEEE-754 bits**.

I then restored the pre-deletion production files verbatim
(`git checkout ef976c6d -- ` the three `src/` files — those files changed only
in `886333a2`, so this is an exact revert) and re-ran the identical dump.

### Result — reproduced, and extended

| Case | gradient | root motion / tick | pre-deletion vs HEAD |
|---|---|---|---|
| downhill 31° | 0.6 | (0, +0.10, 0) | **bit-identical, all 30 ticks** |
| downhill 8.4° | 0.1477 | (0, +0.10, 0) | X, Y bit-identical; Z differs by **2.9e-5 m** at tick 30 |
| **cross-slope 31°** | 0.6 | (+0.10, 0, 0) | **bit-identical** |
| **diagonal 31°** | 0.6 | (+0.0707, +0.0707, 0) | **bit-identical** |
| **flat** | 0.0 | (0, +0.10, 0) | **bit-identical** |
| uphill 31° | 0.6 | (0, −0.10, 0) | bit-identical — **but vacuous, see L1** |

Both reported numbers land exactly. The last three rows are mine; they close
the obvious "only one direction was measured" objection.

### Is "bit-identical" vacuous — did neither path do anything?

**No, for the downhill case.** Over 30 ticks the body advances Y by 2.206 m and
descends 1.324 m. Those are not noise: they are exactly the analytically
predicted projected step. For `N = (0, 0.5145, 0.8575)` and a requested
`(0, 0.1, 0)`, the projection `v − N·(v·N)` gives `(0, 0.07353, −0.04412)`;
the measured per-tick advance is `0.07353 / −0.04412`. The sweep alone
reproduces the deleted projection to the last digit.

The idempotency argument the commit message gives also holds in the code, not
just on paper: `Transition.AdjustOffset`
(`src/AcDream.Core/Physics/TransitionTypes.cs:5246-5258`) subtracts the full
normal component in **both** its `collisionAngle <= 0` and `> 0` arms, so a
vector already on the plane is returned unchanged. That is why the composition
collapses.

**Yes, for the uphill case** — and the implementer said so and refused to ship
the test. That refusal was correct (see L1).

### Does the fixture make it vacuous?

The concern is real and worth naming: the ramp is one constant-gradient plane
over the whole landblock, so `SampleTerrainNormal(x, y)` **equals**
`Body.ContactPlane.Normal` everywhere. That is precisely the case where
idempotency guarantees no change — the friendliest possible geometry for the
"redundant" claim.

But it is also the **right** geometry for the question actually being asked.
Deletion can only regress where the two planes agree (there the old layer was
doing correct work that must now come from somewhere else); where they
disagree the old layer was applying a *wrong* plane, so removing it cannot be a
regression. The measurement covers the regression-capable case exactly, and my
cross-slope/diagonal/flat additions cover it in three more directions.

**Verdict on Stage 0: I believe it.** The redundancy claim is established for
terrain. What is *not* established is the claimed improvement — see D1.

---

## Part 2 — Findings

### Defects

#### D1 — a mandatory contract test was dropped with no record (process)

`docs/research/2026-08-06-ad10-contract.md:546-577` specifies **T2**, "the
discriminating test", on a fixture where the committed contact plane differs
from the terrain sample at the same XY. Construction (a), a two-gradient
terrain crest, is marked **"Mandatory."** Construction (b), an off-terrain
walkable surface, is **"Required, with a documented fallback: if the fixture
cannot be built in reasonable time, say so explicitly in the closeout, ship on
(a), and record (b) as an untested axis rather than silently dropping it."**
Line 690 of the same contract puts T2–T5 in the deletion commit.

Neither exists. `grep -rn -i "crest|ridge|two-gradient" tests/AcDream.Runtime.Tests/`
returns nothing, and no commit message, register row, or doc in the range
records the disposition of T2.

T3, T4 and T5 are defensibly moot under deletion (T3 and T4 test guards that no
longer exist; T5's intent is superseded by the stronger compile-error
guarantee). **T2 is not moot.** Under deletion it becomes the test for the one
behavioural *benefit* the change claims — and that claim is stated as fact in
two places without evidence:

- `docs/architecture/retail-divergence-register.md:122` (the retired AD-10 row);
- `886333a2` commit message: *"What deletion does improve is the case #32 never
  covered: a remote on a WALKABLE non-terrain surface … That surface now gets
  the body's own committed contact plane."*

**Concrete failure scenario.** A remote runs along a sloped wooden bridge or a
dock over sloped terrain. After deletion the *only* projection is the sweep's,
which runs `only when ci.ContactPlaneValid` (`TransitionTypes.cs:5202-5224`
takes the no-contact-plane branch otherwise). If `check_contact` ever fails to
seed a contact plane on a tick where the body is `OnWalkable`, the offset is
not projected at all, where before the terrain sample supplied one. The body
holds Z between server updates — the exact ~5 Hz staircase the projection
existed to remove — and **nothing in CI goes red**, because every automated
assertion about this behaviour lives on a pure-terrain fixture.

I judge the residual risk *low* (on flat terrain under a bridge the deleted
projection was a near-no-op anyway), but "low risk" is a different statement
from "measured", and the contract explicitly forbade making it silently.

**Ask:** either build T2(a) — the harness already supports it, it is a
two-gradient heightmap in `RemoteRampHarness.Ramp` — or add one paragraph to
the register row recording T2 as a deliberately untested axis, per the
contract's own fallback clause.

#### D2 — stale roadmap entry (docs)

`docs/plans/2026-04-11-roadmap.md:108` still lists

> `AP-22 authored object shapes, and AD-10 remote contact-plane projection.`

as open campaign work. AD-10 is retired. CLAUDE.md's roadmap discipline rule 3
requires the roadmap update in the same commit as, or immediately after, the
work. The register row, both downstream `docs/ISSUES.md` cross-references
(`:10513-10515`, `:10672-10676`) and the historical record were all swept
correctly — the roadmap was the one miss.

#### D3 — malformed XML doc comment (style)

`src/AcDream.Core/Physics/RemoteMotionCombiner.cs:38-53`. The AD-10 retirement
rationale is written as a `<para>` block placed *after* the closing
`</summary>` on line 38, and terminated by a second `</summary>` on line 52:

```
38    /// </summary>
39    ///
40    /// <para><b>AD-10, retired 2026-08-06.</b> …
52    /// …unchanged.</para>
53    /// </summary>      <- unmatched close tag
```

Two closes, one open, one orphaned `<para>`. The build is green only because
this project does not generate documentation files; the rationale will not
render in IntelliSense, and the file would emit CS1570 the moment
`GenerateDocumentationFile` is turned on. Same paragraph reads fine — just move
it inside the first `</summary>`.

### Latent risks

#### L1 — #331 is real, is under-scoped, and one cheap probe moves it a long way

The issue as filed (`docs/ISSUES.md:75-150`) is honest and unusually thorough:
it rules out gradient, step size, cell boundaries, Z seating, and axis, and it
names the vacuous test that found it. I reproduced every one of those
exclusions. But the "fixture-versus-production" question it leaves open is
answerable more cheaply than it says, and the answer points away from the lead
the issue names.

**What I measured**, calling `PhysicsEngine.ResolveWithTransition` *directly*
on the harness's engine (no remote tick involved):

| call | result |
|---|---|
| uphill, `body: rm.Body`, `isOnGround: true` | `ok=False`, moved `(0,0,0)` |
| uphill, `body: rm.Body`, `isOnGround: false` | `ok=False`, moved `(0,0,0)` |
| **uphill, `body: null`** | **`ok=True`, moved `(0, −0.0999, +0.060)`** |
| uphill, `body: null`, lifted 0.5 m | `ok=True`, moved `(0, −0.0999, 0)` |
| uphill, with `IsPlayer\|EdgeSlide` + the human two-sphere Setup list | `ok=False` |
| uphill diagonal `(0.1, −0.1, 0)`, `body:` supplied | `ok=True`, moved **`(0.1, 0, 0)`** |
| downhill / cross-slope / straight-up / straight-down, `body:` supplied | `ok=True`, all correct |

Three things follow that the issue does not yet carry:

1. **The discriminator is the `body:` parameter**, i.e. the seeded
   contact-plane / retained-walkable-polygon path (retail `check_contact`).
   Without a body the same uphill sweep climbs; with one it refuses. Production
   *always* passes a body — the local player at
   `src/AcDream.Runtime/Gameplay/PlayerMovementController.cs:2646`, the remote
   at `src/AcDream.Runtime/Physics/RuntimeRemotePhysicsUpdater.cs:424`.
2. **It is not remote-specific and not sphere-list-specific.** It reproduces
   under a call profile identical to the local player's, including
   `IsPlayer | EdgeSlide` and the human Setup's two-sphere list. The issue's
   stated "first thing to check" — the terrain publication path
   (`AddLandblock` vs `LandblockPhysicsContentBuilder.PublishStaticCollision`)
   — is now the *less* likely lead of the two.
3. **The up-slope component is clipped, not the whole step.** The diagonal case
   returns `ok=True` and keeps its cross-slope X while zeroing its up-slope Y.
   A walkable 31° ramp is behaving like a wall for up-slope motion, and a
   1.1° ramp (gradient 0.02) does the same.

**Nothing in the repository proves walkable uphill progress works anywhere.**
`RetailEdgeResponseOrderingTests.cs:277-312` runs a downhill / uphill /
tangential theory and asserts horizontal progress for *downhill and tangential
only* — the `uphill` arm is routed to a no-bounce assertion instead. That is
correct for its unwalkable steep-roof fixture, but it means the suite has no
uphill-progress assertion to lean on.

**This is not a regression from AD-10** — I confirmed the uphill trajectory is
bit-identical before and after the deletion, so nothing here blocks this change.
But #331's severity should not stay UNKNOWN: the trigger is now known to be a
production-shaped call, and the remaining unknown is narrow enough to settle in
one probe.

#### L2 — the surviving tests cannot see the projection, only the Z seating

Verified, and quantified. With `Transition.AdjustOffset` short-circuited to
`return offset;`:

- `RuntimeRemoteSlopeProjectionTests` — **all green**, confirming the
  implementer's rejection of the contract's proposed T1 sabotage;
- the downhill Y advance goes from **0.0735 m/tick to 0.1000 m/tick**, i.e. the
  remote runs **36% faster downhill**, and the tests do not notice;
- Z still tracks the surface exactly, because `ValidateWalkable`'s push-out
  (`TransitionTypes.cs:3445-3452`, reached from `FindEnvCollisions`) re-seats
  the sphere at its natural resting distance every sub-step. The implementer's
  stated third mechanism is real, both structurally and empirically.

This is disclosed, at length and unprompted, in the test's own doc comment
(`RuntimeRemoteSlopeProjectionTests.cs:97-113`), including the instruction that
it "must not be cited as" a unit test of `adjust_offset`. That is the correct
handling of a non-discriminating test.

It is also not a coverage hole overall: running the full suite under that same
sabotage produces **15 failures** — 14 in `AcDream.Core.Tests` plus
`RuntimeRemoteSteepContactSlideTests.SteepContactKeepsTheBodySlidingDownhill`
("expected a slide, body moved 0.0000 m"), which is the Runtime remote path's
own guard. No action required; recorded so nobody later mistakes the new file
for `adjust_offset` coverage.

#### L3 — "bit-identical" is a property of this fixture, not of the change

The 8.4° ramp differs by 2.9e-5 m in Z because `dot(v', N)` is not exactly
zero in float after the first projection. The 31° case lands bit-identical; a
third gradient might not. The commit message already reports both numbers and
calls the difference float ordering noise, which is the honest framing. Nobody
should generalise "bit-identical" into a guarantee.

---

## Part 3 — What I checked and found clean (so the PASS is auditable)

**Deletion completeness (priority 2).** Three independent sweeps:
`SampleTerrainNormal` (zero code references — only comments and docs),
`terrainNormal` case-insensitive (same), and the formula shape
`* Vector3.Dot(` / `-= N *` across all of `src/` (only an unrelated
`Vfx/ParticleSystem.cs:1245` axis projection). No fourth copy, no dead
diagnostic, no test reference. Both fork branches carried the block verbatim
pre-deletion (`:272-282` and `:317-325`) and both are gone — the AP-22 shape was
handled. `SampleTerrainWalkable`, the internal the deleted wrapper called, is
**not** orphaned: it retains live callers at `TransitionTypes.cs:2909` and
`:3432`. `ComputeOffset` confirmed production-dead — the only callers are in
`tests/AcDream.Core.Tests/`. Removing the parameter rather than passing null
does make a one-site reintroduction a compile error, as claimed.

**Sabotage reproduction (priority 3).** All three reproduce, from clean builds:

| sabotage | reported | reproduced |
|---|---|---|
| `rm.Body.Position = postIntegratePos` (`:457`) | RED at tick 1, 0.05999 m off surface | **RED at tick 1, "body root sits 0.05999 m above the terrain under it"** |
| flatten ramp to gradient 0 | RED on the anti-vacuity guard, `dz = 0.0000` | **RED, "fixture is not exercising slope descent: dz = 0.0000 m"** |
| short-circuit `Transition.AdjustOffset` | GREEN | **GREEN** (see L2) |

The anti-vacuity guard is real and load-bearing: note that the *other* fixture
test, `TheFixtureRampIsWalkableAndItsPlaneIsTheGeometricOne`, passes happily on
a flattened ramp (`RampNormal(0)` is `(0,0,1)` and so is the flat contact
plane), so the guard is the only thing standing between this file and a
silently-flattened fixture.

**Deleted tests (priority 4).** Both were genuinely obsolete.
`..._SlopedTerrainNormal_ProjectsZOntoSlope` asserted only the deleted formula,
against expected values re-derived from that same formula in a comment — it
could catch a wrong multiply but never a wrong plane, exactly as the commit
message says. `..._FlatTerrainNormal_NoZChange` is a *literal duplicate* of the
surviving `ComputeOffset_AnimationOnly_Forward_BodyAdvances`
(`RemoteMotionCombinerTests.cs:51-68`): same `dt: 0.1`, same
`rootMotionLocalDelta: (0, 0.4, 0)`, same identity orientation, same asserted
`(0, 0.4, 0)`. Nothing was asserted by either test that is not still asserted.
The tombstone comment left in their place names both and points at the
replacement.

**Harness extraction (`fe6ee877`).** Filtering the diff of
`RuntimeRemoteSteepContactSlideTests.cs` to `Assert` / `[Fact]` / `[Theory]`
lines yields **zero** added or removed assertions — the extraction moved the
private nested `Harness` out verbatim and nothing else. Its start position
constants are unchanged (96, 96). All ten Bug B tests still pass.

**Blast radius (priority 6).** Confirmed exactly as stated.
`new RuntimeRemotePhysicsUpdater(` has one production site,
`src/AcDream.App/Physics/RemotePhysicsUpdater.cs:46`; every other match is a
test fixture. `src/AcDream.Headless/` contains no reference to
`RuntimeRemotePhysicsUpdater`, `RemotePhysicsUpdater`, `RemoteMotion` or
`RemoteMotionCombiner`. The class is `internal` to `AcDream.Runtime` and reaches
production only via `InternalsVisibleTo` into `AcDream.App`
(`AcDream.Runtime.csproj:12`). `RemoteMotionCombiner` lives in `AcDream.Core`
and is instantiated in `RemoteMotion.cs:242`, but the only things that *drive*
it are the three `ComposeOffset` sites in `RuntimeRemotePhysicsUpdater`. The
inverse of C5b's lesson does apply here and the implementer got it right:
assembly placement is not reachability, and a green headless gate would have
been vacuous evidence. #332 records that reasoning correctly.

**`TickHidden` (priority 7).** Correct to leave untouched. I byte-compared the
call at `RuntimeRemotePhysicsUpdater.cs:940-948` against `ef976c6d`: it is
character-for-character identical, and it used the named argument
`inContact:` while skipping the optional `terrainNormal`, so the hidden-remote
path always received `null`. Deleting the parameter is a no-op there by
construction.

**Gates.** From a state with all 44 `bin`/`obj` directories deleted:
Release build **0 errors**; full solution suite **11,196 passed / 4 skipped /
0 failed**, matching the claim exactly and reconciling with the stated
`11,195 / 4 / 0` baseline as +3 new Runtime `[Fact]`s and −2 deleted Core tests.
Run twice — once before my sabotage experiments and once after restoring the
tree — with identical totals. Skip count unchanged at 4 (3 App, 1 Core); no
skip was added.

**Tree state.** `git status --porcelain` and `git diff HEAD` are clean. The one
untracked file, `docs/research/2026-08-06-ad10-review-retail.md`, belongs to the
concurrent retail-fidelity reviewer and was not touched. Every temporary
sabotage and probe file I introduced was reverted or deleted and verified gone.

**Rule compliance.** No workaround, no suppression flag, no grace period, no
`if (badState) return`, no swallowed exception, no widened tolerance, no new
skip. The 5 mm tolerance in the new tracking test is justified in its own doc
comment against both the failure it must catch (~1.8 m) and the noise it must
tolerate (<1e-4 m), and I confirmed the ratio by measurement. Known flakes
#302 / #308 / #321 were not touched or conflated. AD-65 and AD-66 were filed as
separate register rows rather than folded into this change, which is the right
call — both change local-player feel and need their own gate.