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

# AP-22 architecture review — `bc4679cd`

**Scope:** completeness, blast radius, test quality. Retail fidelity is a
separate reviewer's. **Verdict: PASS**, with one required correction to the
commit record (Finding A) and four minor accuracy items.

**Worktree:** `.claude/worktrees/peaceful-visvesvaraya-e0a196`, branch
`claude/acdream-physics-divergence-5aa784`, HEAD `bc4679cd`, tree clean at
review start and at review end (the only untracked file is the parallel
retail reviewer's `2026-08-06-ap22-review-retail.md`, untouched).

---

## 1. The crux — do I believe the reachability claim?

**Yes. Verified by a third route that shares no code with either prior
parser, and the strict-guard variant the contract and the test both left
unverified is also zero.**

### 1.1 Method

I wrote a raw `client_portal.dat` parser in Python from scratch — B-tree
directory walk (root at header `0x140 + 0x20` = `0x0232D400`, 1024-byte block
chain), Setup record decode against retail's own `CSetup` field order
(`docs/research/named-retail/acclient.h:31119` + `CSetup::UnPack` @
`0x00520c50`). It shares no code with `DatReaderWriter`, with
`FlatCollisionAssetBuilder.FlattenSetup`, or with the implementer's C#
scratch parser. Script: scratchpad `ap22_sweep.py` (not committed).

Self-validation, stronger than the contract's: **5,935/5,935 records consumed
to byte-exact end, residual histogram `{0: 5935}`** — i.e. zero unexplained
bytes on every record, not "a 20 + 48·numLights tail". **608 of the 5,935
carry a light row**, so the 48-byte `LIGHTINFO` size and the whole post-lights
tail are genuinely exercised rather than trivially satisfied by
`numLights == 0` everywhere (that was worth checking: BN renders the light
allocation as `0x68`-sized at pc:296913, which would have desynchronized 608
records had it been right).

### 1.2 Results — every number reproduces

| Bucket | Contract / test literal | My independent parse |
|---|---|---|
| Total Setups | 5,935 | **5,935** |
| ≥1 CylSphere | 678 | **678** |
| 0 CylSphere, ≥1 Sphere | 3,605 | **3,605** |
| 0 primitives (`ExpectedWithoutAnyPrimitive`) | 1,652 | **1,652** |
| — of those, ≥1 physics-BSP part | 358 | **358** |
| — of those, fully shapeless | 1,294 | **1,294** |
| `Radius > 0.0001` | 4,282 | **4,282** |
| primitive **and** physics-BSP part (AP-152) | 172 | **172** |
| GfxObjs / with physics bit | 15,318 / 1,258 | **15,318 / 1,258** |
| **Fallback reachable** | **0** | **0** |

Third decoder, spot checks — `dotnet run --project tools/SetupInspect`
(DatReaderWriter, a different implementation again) agrees bit-for-bit with my
parser on all three ids the contract cites:

- `0x02000001` humanoid — Radius/Height `0.679 / 1.835`, Spheres 2
  (r = 0.48 at z = 0.475 and 1.35), CylSpheres 0, Parts 34, 0 BSP parts.
- `0x02000C9D` Facility Hub door — Radius/Height `0.000 / 0.000`,
  0 primitives, parts `0x01002936`/`0x01002936`/`0x01002937`, 2 BSP parts.
  This is a member of the 1,294 shapeless bucket **with Radius exactly 0**.
- `0x020019FF` cottage door — Radius/Height `0.141 / 0.200`, 1 Sphere
  (r = 0.100 at z = 0.018), 1 BSP part.

### 1.3 A gap I closed that neither the contract nor the test covers

The test's predicate is `flat.Radius > 0.0001f`, and its comment calls that
**"the exact guard the three deleted copies used"**
(`tests/AcDream.Content.Tests/InstalledSetupCollisionReachabilityTests.cs:69`).
It is not. Site 1 used `setup.Radius > 0.0001f`; **sites 2 and 3 used
`setup.Radius > 0f`** (`LandblockPhysicsPublisher.cs:1043` and
`LandblockPhysicsContentBuilder.cs:696` at `bc4679cd~1`). Over the installed
DAT the two predicates genuinely differ: **4,282 Setups satisfy `> 0.0001`
but 4,283 satisfy `> 0`** — Setup `0x02001657` carries a denormal
`Radius = 1.2988e-39` (bits `0x000E246E`).

I evaluated the strict guard directly: **`!hasCyl && !hasSph && Radius > 0f`
is also 0 of 5,935** (that one denormal Setup carries a primitive). So there
is no hole — but the test does not pin the guard sites 2 and 3 actually used,
and its comment overstates. See Finding C.

**Conclusion: the deletion removes collision from nothing.** The branch was
unreachable dead code under both guards, for every Setup in the installed
`client_portal.dat`.

---

## 2. Is there a fourth copy? — No. Exhaustively.

**By symbol.** `new ShadowShape(` appears exactly **8 times** in `src/`:
2 in `LandblockPhysicsPublisher` (CylSphere loop `:1003`, Sphere loop
`:1030`), 2 in `LandblockPhysicsContentBuilder` (`:658`, `:683`), 4 in
`ShadowShapeBuilder` (`:89` CylSphere, `:108` Sphere, `:147` BSP part, `:231`
`FromLandblockBspParts`). **None of the eight reads `Setup.Radius` or
`Setup.Height`.**

**By concept.** After the deletion the only `setup.Radius`/`setup.Height`
reads left in `src/` are:

- `src/AcDream.Core/Physics/PhysicsDataCache.cs:391-392` — flatten
  pass-through into `FlatSetupCollision`, no synthesis.
- `src/AcDream.App/Physics/LiveEntityMotionRuntimeController.cs:284`
  (`GetSetupCylinder`) — the MoveTo/attack-cone consumer, explicitly
  out of scope and retail-faithful.
- comments.

**By registration entry point.** `RegisterMultiPart` / `ReplaceMultiPartPayload`
have exactly four production callers — the three deleted sites' surviving
registration calls plus `LiveEntityCollisionBuilder.Register:175`. Every
*internal* re-registration (`ShadowObjectRegistry.cs:536`, `:707`, `:1582`,
`:2165`, and `RuntimeRemotePhysicsUpdater`'s re-flood) replays a
previously-built shape list and constructs nothing.
`AcDream.Runtime` and `AcDream.Headless` contain **no** `ShadowShape`
reference at all.

**Adjacent but categorically different — not a fourth copy.**
`src/AcDream.App/Composition/SessionPlayerComposition.cs:544-571` and
`src/AcDream.Runtime/Physics/RuntimeRemotePhysicsUpdater.cs:389-391` derive a
capsule from `GetSetupCylinder` (`Setup.Radius/Height × scale`) with a
hardcoded `0.48 / 1.835` human fallback. That is the **mover's own** sphere/
capsule — the moving object's shape, not the shape other objects collide
*against* — and is separately governed by #184 / TS-46 / AD-25. Correctly out
of AP-22's scope.

---

## 3. The corrected literal `1,652`

**Verified independently, and not derived tautologically.**

- My parse yields `no_primitive = 1,652`, decomposing exactly as the commit
  states: 358 BSP-only + 1,294 shapeless.
- The literal is not a re-derivation from the production code under test:
  its two components are the contract's §3.2 external measurements, and the
  sum is arithmetic. My parse confirms both components and the sum without
  touching `FlatCollisionAssetBuilder`.
- The reason 1,294 was wrong for this test is correct as stated: 1,294 is a
  three-way condition including "0 physics-BSP parts", and the code path
  under test (`FlattenSetup`) resolves no GfxObjs and cannot see that
  dimension.

---

## 4. Test quality — 4 sabotages reproduced, 1 claim disproved

All runs below on a **fully clean** build (all 44 `bin`/`obj` removed first —
see Finding F).

| # | Sabotage | Claimed | Observed |
|---|---|---|---|
| S1 | Restore the deleted block in `LiveEntityCollisionBuilder.Build` | reddens exactly `ShapelessSetupWithRadius_ProducesNoRegistration`, nothing else | **Confirmed** — App.Tests 4,171 passed / **1 failed** / 3 skipped; the single failure is that test |
| S2 | `Assert.Empty(fallbackReachable)` → `Assert.NotEmpty` | reddens | **Confirmed** — `Assert.NotEmpty() Failure: Collection was empty` |
| S3 | Empty the Setup enumeration | fails on the **controls** at `0 != 5935`, not a vacuous pass | **Confirmed** — `Assert.Equal() Failure … Expected: 5935 / Actual: 0` |
| S4 | Flip a `FromPwdBitfield` bit (`IsPK` `0x20`→`0x40`) | reddens `Build_PropagatesExactStateFlagsScaleAndFullSeedCell` | **Confirmed** — that test and only that test |
| **S1b** *(mine)* | Restore the block in **sites 2 and 3** | *(claimed covered by Headless.Tests 89/89)* | **DISPROVED — entire suite stays green** |

**Re-hosting of the deleted test's assertions: genuine, nothing dropped.**
`Build_PropagatesExactStateFlagsScaleAndFullSeedCell`
(`tests/AcDream.App.Tests/Physics/LiveEntityCollisionBuilderTests.cs:37-72`)
carries the old test's live assertions verbatim — `registration.State` vs
`record.FinalPhysicsState`, the four PWD-bitfield flags from `0x28`,
`LandblockId` and `SeedCellId` both the full cell, and the `WorldOffsetX/Y`
pass-through (lines 64-71). Only the two fallback-shape assertions were
replaced, by the CylSphere-derived `0.8 / 2.4`, which preserves the *scale*
coverage the old name advertised (`0.4 × 2`, `1.2 × 2`). S4 proves the flag
decode is load-bearing rather than decoration.

---

## 5. Findings

### DEFECT (record, not code)

**Finding A — "Headless.Tests 89/89 exercises the site-3 copy" is false;
two of the three production deletions are pinned by no test at all.**
`src/AcDream.Content/LandblockPhysicsContentBuilder.cs:694`,
`src/AcDream.App/Streaming/LandblockPhysicsPublisher.cs:1041`.

Evidence, three ways:

1. **Sabotage S1b.** I restored the invented cylinder in *both* static sites
   and rebuilt. Result: App.Tests 4,172/3, Content.Tests 125, **Headless.Tests
   89/89**, Core.Tests 4,263/1 — all green. The invented footprint can be put
   back into the headless-only copy and every gate the commit cites still
   passes.
2. **Grep.** No `.cs` file under `tests/` references
   `LandblockPhysicsContentBuilder`, `PublishStaticCollision`, or
   `StaticCollisionPublication`. `PublishStaticCollision` has exactly one
   caller in the tree: `HeadlessSessionWorldProjection.cs:462`.
3. **The headless suite says so itself.**
   `tests/AcDream.Headless.Tests/HeadlessCollisionNeighborhoodServiceWindowTests.cs:72-77`
   documents that "no lightweight DAT fixture in this test project can drive
   real 3x3 publication through `CenterOn` — its dummy `IDatReaderWriter`
   proxy makes `LandblockLoader.Load` fail for every landblock", and seeds
   `_resident` by reflection instead. `CreatePublication`
   (`HeadlessSessionWorldProjection.cs:367`) returns before line 462 on every
   headless test.

**Failure scenario this permits:** a future edit to the static Setup walk in
either publisher — reordering the Cylinder/Sphere branches, re-adding a
summary-radius shape, changing the `setupShapes.Count` gate — lands green.
The headless bot then collides against an invented footprint, or loses the
Sphere-derived cylinder entirely, and nothing in CI notices; the graphical
client is unaffected, so the divergence is invisible until someone runs a
connected headless route.

The commit's own contract predicted this exactly (§9 trap 4: *"the inverse
trap: treating 'it's dead code' as licence to skip the headless suite"*), and
proof obligation **P5** required *"`AcDream.Headless.Tests` green **and** a
headless connected run"*. Neither half was satisfied: the suite passing proves
site 3 **compiles**, not that it **runs**, and no headless connected run is
reported. **P3** (connected registration-inventory diff) is likewise not
reported.

**Why this is not a FAIL of the code:** the deletion at sites 2 and 3 is
textually the same eleven-line removal as site 1, its guard is provably
unsatisfiable over the installed DAT under both `> 0f` and `> 0.0001f`, and
the surrounding empty-payload handling is unchanged (§6 below). The behavior
is correct; the *claim* is not.

**Required correction:** amend the commit record and the AP-22 retirement text
to say that sites 2 and 3 are covered only by the DAT reachability proof, that
Headless.Tests exercises the site-3 file only by compilation, and that P3/P5
were consciously substituted by the reachability argument. Optionally file a
follow-up for a Content-layer test that drives `PublishStaticCollision` over a
synthetic `LoadedLandblock` + `FlatSetupCollision` — that would pin sites 2/3
cheaply and would have caught S1b.

### LATENT RISK

**Finding E — retail's `CSetup` declares `step_down_height` before
`step_up_height`; ACE and DatReaderWriter read the opposite order.**
`docs/research/named-retail/acclient.h:31129-31132` gives
`float height; float radius; float step_down_height; float step_up_height;`,
while `references/ACE/Source/ACE.DatLoader/FileTypes/SetupModel.cs:93-94`
reads the 3rd serialized float as `StepUpHeight` and the 4th as
`StepDownHeight`; `SetupInspect` (DatReaderWriter) agrees with ACE. For the
humanoid Setup the two values are `0.600` and `1.500`, and physical
plausibility (step **up** 0.6 m, step **down** 1.5 m) favours the
ACE/DatReaderWriter reading — so this is most likely a
declaration-order-vs-serialization-order artifact, not a bug. **It does not
touch AP-22** (`radius` is field #2 and is unambiguous — 0.679 for the
humanoid on all three decoders). But `FlatSetupCollision.StepUpHeight/
StepDownHeight` feed TS-46 remote movement and the MoveTo path, so a swap
would be a real physics divergence. Flagging for a 10-minute settle against
the `0x00521240`-region disassembly; not claiming it is wrong.

**Finding G — the load-bearing reachability test silently passes on a machine
without DATs.** `InstalledSetupCollisionReachabilityTests.cs:47-49`:
`if (datDir is null) return;`. This matches the established suite convention
(`ConformanceDats`, `DatConcurrencyStressTests`, documented as deliberate in
`ContentConformanceDats`'s own doc comment) and is therefore **not** a new
skip or a rule violation. But for the single test the entire zero-risk
argument rests on, a silent green on CI is worth more than convention: `xUnit
v3 Assert.Skip` would make the absence visible in the 4-skip count. Style
preference, not a defect.

### STYLE / ACCURACY

**Finding B — "Release build 0 errors / 0 warnings" is inaccurate.**
`dotnet build AcDream.slnx -c Release` from a fully clean tree reports
**21 warnings / 0 errors**. All 21 are pre-existing and all are in *test*
projects (CS8602 ×4, CS0649 ×3, CS8767 ×3, xUnit2000/2013/2017/1025 ×5+,
Core.Tests and App.Tests). **Zero warnings in `src/`**, and this change adds
none. Presumably the claim meant the production build; it is stated as the
solution build.

**Finding C — the reachability test's guard comment is wrong.**
`InstalledSetupCollisionReachabilityTests.cs:69`: *"The exact guard the three
deleted copies used"*. Two of the three used `Radius > 0f`, not
`> 0.0001f`, and the two predicates differ over the installed DAT by exactly
one Setup (`0x02001657`, denormal radius). The claim happens to be safe — I
verified the strict guard is also 0-reachable — but the comment should say
"site 1's guard; the strict `> 0f` variant used by sites 2 and 3 is also
verified zero", and ideally the test should evaluate both.

**Finding D — the new AP-152 register row conflates a summary radius with a
sphere radius.** It says the cottage door `0x020019FF` has a *"~14 cm base
Sphere"*. Its base Sphere radius is **0.100 m** (bits `0x3DCCCCCD`, origin
z = 0.018); `0.1414` is the Setup's **summary `Radius`** — precisely the field
AP-152's own retail anchor establishes is not collision geometry. Cosmetic,
but it is the same conflation this commit exists to retire.

### PROCESS (affects anyone re-verifying, not the commit)

**Finding F — this worktree's incremental build silently serves stale
referenced assemblies; `--no-incremental` and `-t:Rebuild` are both
insufficient.** My first three Release builds each completed in ~6 s and left
a **stale `AcDream.App.dll`** in `tests/AcDream.App.Tests/bin/Release/net10.0/`
that still contained the deleted fallback. Consequence: on an apparently
"clean rebuilt" tree at `bc4679cd`,
`ShapelessSetupWithRadius_ProducesNoRegistration` failed **deterministically**
(3/3 whole-assembly runs, and also in complete isolation), and
`InstalledSetups_NeverReachTheDeletedRadiusFallback` failed under
full-solution parallelism. I spent a while treating that as a real defect. It
is not: after `rm -rf` of all 44 `bin`/`obj` directories and a fresh build,
everything passes. Note the failure mode is the mirror image of the one the
brief warned about — here a **deleted** block survived in a stale referenced
DLL and reddened the test that proves it is gone. Anyone re-checking this
commit must delete `bin`/`obj` first.

---

## 6. Layering / ownership — clean

Nothing now depends on a shape that may be absent, and the deletion does not
enlarge the set of shapeless entities (that set is *identical* before and
after, because the guard was unsatisfiable).

- Site 1: `if (shapes.Count == 0 && !retainEmptyPayload) return null;`
  (`LiveEntityCollisionBuilder.cs:146`) — unchanged, already the empty case.
- Site 2: `if (setupShapes.Count > 0)` (`LandblockPhysicsPublisher.cs:1044`)
  — unchanged.
- Site 3: `if (setupShapes.Count == 0) { noCollision++; continue; }`
  (`LandblockPhysicsContentBuilder.cs:697`) — unchanged; the `noCollision`
  counter still accounts for it in `StaticCollisionPublication`.
- `retainEmptyPayload: true` has exactly one production caller,
  `LiveEntityAppearanceBinding.cs:100`, and flows only into
  `ReconcileAppearance` → `ReplaceMultiPartPayload`, which handles
  `shapes.Count == 0` explicitly (`ShadowObjectRegistry.cs:705-707`: no prior
  multipart + empty shapes → return).
  `LiveEntityCollisionBuilder.Register` is reachable only from
  `DatLiveEntityProjectionMaterializer.cs:840` with a default-path result,
  which can never be empty.
- The `ShadowShapeBuilder` doc-comment correction is comment-only and does not
  change `FromSetup`'s behaviour; `ShadowShapeBuilderTests` is untouched and
  still pins the additive AP-152 behaviour as the register row says it does.

---

## 7. Rules compliance

- **No workaround / suppression flag / grace period / symptom guard.** The
  change is a pure deletion plus comment corrections. Nothing was added to
  make a symptom go away.
- **No new skips.** 4 skipped before and after (App: `TowerAscent…`,
  `RadarLayoutFixtureGenerator`, `ChatLayoutFixtureGenerator`; Core:
  `Pvs_CottageInterior…`) — all pre-existing.
- **No test weakened to pass.** The one replaced test gained a
  discriminating assertion set (S4 proves it) and the fixture moved from
  DAT-impossible to DAT-possible. The two new tests both fail under sabotage.
- **Known flakes #302/#308/#321 not conflated** — none appeared in any run.
- **Divergence register bookkeeping correct.** AP-22 retired with the
  corrected three-site list and the reachability evidence; AP-152 filed with
  sites, mitigation, risk and retail anchors; #330 filed for the pre-existing
  headless live-entity collision gap. Register row count 127.

## 8. Gates I ran (all on a genuinely clean build)

| Gate | Result |
|---|---|
| `dotnet build AcDream.slnx -c Release` after deleting all bin/obj | 0 errors, 21 pre-existing test-project warnings, 0 in `src/` |
| Complete solution suite | **11,195 passed / 4 skipped / 0 failed** — matches the claim exactly |
| `AcDream.App.Tests` | **4,172 passed / 3 skipped** — matches the claim |
| `AcDream.Content.Tests` | 125 / 0 |
| `AcDream.Headless.Tests` | 89 / 0 |
| `AcDream.Core.Tests` / `Core.Net` / `Runtime` / `UI` / `Bake` / `Cli` | 4,263+1 skip / 764 / 1,217 / 546 / 15 / 4 |
| Independent raw-DAT sweep | 5,935/5,935 byte-exact, 0 fallback-reachable under both guards |
| `tools/SetupInspect` spot checks | agree bit-for-bit on `0x02000001`, `0x02000C9D`, `0x020019FF` |
| Sabotages S1, S2, S3, S4 | all reproduce as claimed |
| Sabotage S1b (mine) | disproves the site-2/site-3 coverage claim |

Tree verified clean (`git diff HEAD` empty) after every sabotage and at the
end of the review.