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.