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

# AP-152 architecture review — `4abd1b5e`

**Reviewer scope:** blast radius, correctness of reach, test quality. Retail
fidelity is a separate reviewer's.
**Worktree:** `.claude/worktrees/resume-session-e0bd03e1-d5bf45`, branch
`claude/resume-session-e0bd03e1-d5bf45`, HEAD `4abd1b5e`.
**Method:** read-only, inline, no subagents. All `bin`/`obj` deleted (44
directories) before every verdict-deciding build. Five sabotages reproduced;
tree restored and verified clean (`git status --porcelain` empty) after each.

---

## Verdict

**PASS**, with one high-severity latent risk that must not be treated as
landed behaviour until the connected flood-set diff runs, and four
documentation/framing residuals.

No defect found. The two load-bearing claims I was asked to attack — the
headless-neutrality claim and the S4 no-op argument — **both hold**, and I
verified each independently rather than accepting the implementer's evidence.

---

## Gates reproduced

| Gate | Claimed | Measured | |
|---|---|---|---|
| Clean Release build | 0 errors / 21 pre-existing warnings | 0 errors / 21 warnings | ✅ |
| Full solution suite | 11,203 / 4 / 0 | 11,203 passed / 4 skipped / 0 failed | ✅ |
| Headless | 89 / 89 | 89 / 89 | ✅ |
| Delta vs `ec29a732` | +5 (3 Core, 1 App, 1 Content) | +5 `[Fact]` added, **0 removed**; the 5 new facts are exactly the 5 that redden under sabotage B | ✅ |
| Renames 1:1 | 2 renamed, none deleted | `-0 [Fact]` in the test diff | ✅ |
| Register bookkeeping | AP-152 retired, AP-153/154/155 filed | active AP rows 105 → 107 (−1 +3) | ✅ |

Per-project at HEAD, clean build, `--no-build`:
Cli 4, Bake 15, Headless 89, Content 126, UI.Abstractions 546, Core.Net 764,
Runtime 1222, App 4173/3 skip, Core 4264/1 skip. **Σ 11,203 / 4 / 0.**

I did not check out `ec29a732` to re-measure the 11,198 baseline (read-only
worktree, and switching would have disturbed the other agent's assumptions).
The +5 arithmetic is corroborated independently: sabotage B (gate disabled)
reddens exactly five tests solution-wide, and they are exactly the five the
commit says it added.

---

## 1. Reach across hosts — **the neutrality claim is TRUE, and stronger than stated**

I did not verify the claim as written; I enumerated the whole producer set.

`ShadowShape` is constructed at exactly **eight** sites in `src/`:

| Site | Emits |
|---|---|
| `ShadowShapeBuilder.cs:134` (step 1) | Cylinder |
| `ShadowShapeBuilder.cs:155` (step 2) | Sphere |
| `ShadowShapeBuilder.cs:192` (step 3) | BSP |
| `ShadowShapeBuilder.cs:276` (`FromLandblockBspParts`) | BSP only — every path in that loop `continue`s or adds `ShadowCollisionType.BSP` |
| `LandblockPhysicsContentBuilder.cs:658`, `:683` | Cylinder only (`:683` converts a Setup Sphere to a height-capped Cylinder) |
| `LandblockPhysicsPublisher.cs:1003`, `:1030` | Cylinder only (same conversion) |

Steps 1–3 are the only site that could ever produce a heterogeneous list, and
after this change it cannot. So:

- **Content's two registrations are homogeneous by construction.**
  `LandblockPhysicsContentBuilder.cs:619` passes `bspShapes` from
  `FromLandblockBspParts` (all-BSP); `:702` passes `setupShapes`, all
  `ShadowCollisionType.Cylinder`, with no `ShadowCollisionType.Sphere`
  reachable. Confirmed by reading both loops, not by trusting the comment.
- **Every other `RegisterMultiPart` caller replays a stored list.**
  `ShadowObjectRegistry.cs:536` (`ReplaceMultiPartPayload`), `:741`
  (`UpdatePosition`), `:1616` (`RefloodOwnerForLandblock`), `:2199` (mirror)
  all pass `_entityShapes[entityId]`, which was populated by one of the eight
  producers above. Homogeneity propagates.
- **Runtime constructs no shapes.** `grep "new ShadowShape("` in
  `src/AcDream.Runtime` returns nothing; every `ShadowObjects.*` call there is
  `UpdatePosition` / `CommitSetPosition` / `Suspend` / a read.
- **Project references confirm the reach boundary.**
  `AcDream.Headless.csproj` → `AcDream.Runtime` only;
  `AcDream.Runtime.csproj` → Core, Core.Net, Content, Plugin.Abstractions.
  `AcDream.App` is unreachable, so `LiveEntityCollisionBuilder` — the sole
  production caller of `FromSetup` (`LiveEntityCollisionBuilder.cs:126`) —
  cannot execute in headless.

**Empirical confirmation, not just structural:** under sabotage B (the step-0
gate disabled — emission reverts to additive), `AcDream.Headless.Tests` stays
**89/89 green** while Core, App and Content each redden. That is direct
evidence the change does not reach headless, which is the C5b lesson applied
in the correct direction.

**I believe the headless-neutrality claim.**

---

## 2. Effective-GfxObj identity — **correct, and structurally airtight**

The gate (`ShadowShapeBuilder.cs:116-124`) and step 3 (`:171-200`) call the
same helper `EffectivePartGfxObjId` (`:289-302`), over the same index range
(`setup.Parts.Count`), with the same predicate instance (`hasPhysicsBsp`).
Therefore

> `anyPhysicsBspPart == true` ⟺ step 3 emits at least one shape

is an identity, not a tested property. The trap the contract names — gate
suppresses primitives, step 3 emits nothing, `Build` returns null at
`LiveEntityCollisionBuilder.cs:146`, collision silently deleted — is
unreachable by construction.

Downstream identity is the same one: `LiveEntityCollisionBuilder.cs:137`
resolves the real BSP radius from `shape.GfxObjId`, which step 3 set to the
effective id at `ShadowShapeBuilder.cs:193`. The predicate itself
(`LiveEntityCollisionBuilder.cs:56`,
`physicsData.GetFlatGfxObj(id)?.PhysicsBsp.RootIndex >= 0`) is the same
function object passed to the gate. Gate, emission, radius resolution and the
collision-time BSP lookup all key on one id.

**Trap-1 sabotage reproduced.** Replacing the gate body with
`hasPhysicsBsp((uint)setup.Parts[i])`:

```
Failed  ShadowShapeBuilderTests.FromSetup_DispatchGateReadsTheEffectivePartIdentities
Failed! - Failed: 1, Passed: 4263, Skipped: 1, Total: 4265
```

Exactly one test catches it, and it is the one claimed to. Its discriminating
power is real: with a `setup.Parts` gate the swapped case yields
Cylinder + BSP (2 shapes) and `Assert.Single(swapped)` fails, while the
unswapped case still passes — so the test distinguishes *this* wrong gate
from *no* gate.

---

## 3. Test quality — five sabotages reproduced

### S4 (flood reverted to cylinder-first) — **the no-op argument holds**

I reverted `ShadowObjectRegistry.cs:649-652` to cylinder-first and ran the
**whole solution**:

```
Failed  ShadowObjectRegistryMultiPartTests.BuildFloodSpheres_BspBearingOwner_FloodsFromBspNotFromCylinder
Failed! - Failed: 1, Passed: 4263 ... (Core)
Passed! - all 8 other test assemblies, including Headless 89/89
```

`FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint` — the end-to-end fact —
stayed **green**, exactly as claimed. One test out of 11,203 reddens.

Combined with §1's producer enumeration, this is a proof rather than an
anecdote: no production producer can emit a list on which the flood dispatch
observably differs, so **the entire measured membership delta is attributable
to the emission gate alone**, and the flood half is behaviour-identical today.

**Is the flood change justified?** Yes, and it is not a workaround. It is a
faithful port of `calc_cross_cells`' own dispatch (documented at
`ShadowObjectRegistry.cs:600-628`), kept on the same rationale that keeps
`Transition.BspOnlyDispatch`. It is, however, honestly dead against current
inputs — see latent risk #5. The gate alone would produce identical behaviour;
the flood change buys retail-shape correctness for a producer that does not
yet exist.

### S5′ (old `ScaleFactor` body under the same production sabotage) — **reproduced**

Production sabotage: `ShadowShapeBuilder.cs:161`,
`Radius: sph.Radius * entScale` → `Radius: sph.Radius`.

- Corrected test body → **Failed**
  (`FromSetup_ScaleFactor_MultipliesAllRadiiAndOffsets`).
- Old test body restored verbatim under the *same* sabotage → **Passed**.

The old test genuinely covered nothing: its radius/offset assertions sat
inside `if (s.CollisionType == ShadowCollisionType.Cylinder)` on a fixture
with zero CylSpheres. The correction is a strengthening, not a rewrite to
make a failing test pass.

### Sabotage B (step-0 gate disabled → additive emission)

Reddens exactly the five added facts, across three projects:

```
Core     FromSetup_DoorSetup_EmitsBspPartsOnly
Core     FromSetup_DispatchGateReadsTheEffectivePartIdentities
Core     FromSetup_CylSphereAndBspSetup_FloodsTheBspFootprint
App      CylSphereAndPhysicsBspPart_EmitsOnlyTheScaledBspShape
Content  InstalledSetups_WithBothAPrimitiveAndAPhysicsBspPart_EmitOnlyBspShapes
Headless 89/89 GREEN
```

### DAT-sweep control (mine, not on the claimed list)

The installed-DAT sweep completes in ~700 ms, which is fast enough to look
like a vacuous early return at
`InstalledSetupBspPrimitiveDispatchTests.cs:59-60`. I falsified that:
`ExpectedSetups = 5935 → 5936` produces
`Assert.Equal() Failure: Expected 5936, Actual 5935`. The sweep really
enumerates the installed `client_portal.dat`, and its four external bucket
controls are load-bearing rather than derived from the predicate under test.

### Test-quality notes

- No test deleted (`-0 [Fact]` in the diff); both renamed tests are
  strengthened (`Assert.Single` + `Assert.All` + two `DoesNotContain` where
  there were loose counters).
- The `return` on absent DATs is the established Content.Tests convention
  (`ContentConformanceDats.ResolveDatDir`, used identically by six sibling
  tests). Not a new skip.
- No `Skip =`, no `try/catch`, no `Thread.Sleep`/`Task.Delay`, no new
  `GetEnvironmentVariable`, no suppression flag introduced anywhere in the
  diff.
- Known flakes #302/#308/#321 untouched and not conflated.

---

## 4. Downstream consumers of the (now smaller) shape list — clean

- No production site indexes a `FromSetup` list positionally. The only
  positional loops (`LandblockPhysicsPublisher.cs:1083`, `:1098`) walk their
  own homogeneous list.
- No site asserts "at least one primitive". `RegisterMultiPart` handles
  `shapes.Count == 0` by deregistering (`ShadowObjectRegistry.cs:454`), and
  §2 proves the count cannot newly become zero.
- `TransitionTypes.cs:3759 / 3901 / 4089` branch per shape kind, never on the
  presence of a kind.
- `WorldSceneDiagnosticsController.cs:222` is debug wireframe drawing.
- `LiveEntityCollisionBuilder.cs:191-201`'s probe counts `cyl` vs `else`; the
  contract already documents that its `else` mislabels Sphere as `bsp`. That
  matters for reading the un-run connected gate, not for behaviour.

---

## Findings, ranked

### Defect
None.

### Latent risk

**LR-1 [High] — for 99 of the 172 Setups the membership change is a SHRINK, in
the same failure class the commit exists to fix, and it is un-gated.**
`ShadowShapeBuilder.cs:150-164` stops emitting the Setup Sphere for any
Sphere+BSP Setup, so `BuildFloodSpheres` (`ShadowObjectRegistry.cs:654-667`)
now floods only from the BSP parts' bounding spheres — whose production radius
is the flat BSP root bounding sphere (`LiveEntityCollisionBuilder.cs:137`).
Retail's `find_bbox_cell_list` uses a bounding **box** over the whole part
array; acdream approximates it with per-part bounding **spheres**, which
AP-155 correctly registers. Failure scenario: a Sphere+BSP prop whose Setup
sphere is larger than every part's BSP bounding sphere — the contract itself
names `0x02001761` "Boulder" (2.00 m sphere + 1 BSP) as a candidate — loses
shadow cells, and stops blocking when approached from the landcell it dropped
out of while still blocking from another. That is the #98/#168 signature
verbatim. The instrument for this is the contract's §8.2 item 3 keyed
flood-set diff, and it **has not been run**. The commit's "NOT yet gated live"
line covers it; this finding is to make sure the shrink direction, not only
the flip direction, is what the connected session measures, and that the
acceptance criterion "no affected owner ends with an empty set" is checked.

**LR-2 [Medium] — the blast radius is understated: this also changes the
collision shape set, not only membership, wherever the wire flag is absent.**
`Transition.BspOnlyDispatch` (`TransitionTypes.cs:1348-1349`) reads the
*server's* `PhysicsState`, copied at `LiveEntityCollisionBuilder.cs:161`. The
commit's "the collision half was already inert" is conditional on ACE setting
`HAS_PHYSICS_BSP_PS` from `CSetup.HasPhysicsBSP` for every affected Setup. Any
live entity where that bit is absent previously had its primitive tested and
now does not. Failure scenario: an ACE build (or a different server) that
omits the bit for one of the 172 — pre-change the mover collided with the
cylinder, post-change it collides only with the slab BSP, and the two are not
the same shape. This is retail-*correct* (retail derives the flag from the
parts, `CPartArray::CacheHasPhysicsBSP`), and the underlying dependency is
registered as AP-154, so nothing is hidden — but the commit message and the
"NOT yet gated live" note both say "membership", and the honest statement is
"membership, and collision wherever the server omits the bit".

**LR-3 [Medium] — `RegisterMultiPart`'s own XML doc still states the rule this
commit inverted.** `src/AcDream.Core/Physics/ShadowObjectRegistry.cs:432-439`:
"when the object has CylSpheres, they alone drive the flood … otherwise the
BSP parts' bounding spheres stand in for the sorting sphere." That is now
false, and it sits on the **public** method, 165 lines above the corrected
`BuildFloodSpheres` block at `:600-628`. A reader who stops at the public
API doc gets the pre-change rule. This is the same class of defect the commit
was written to correct in AP-152's four false statements, and CLAUDE.md's
"never leave them out of sync" applies.

**LR-4 [Low] — `LiveEntityCollisionBuilder`'s class doc still describes the
additive policy.** `src/AcDream.App/Physics/LiveEntityCollisionBuilder.cs:32`:
"CylSpheres before Spheres, **and** every physics-BSP part." The "and" is
precisely the union this commit removed.

**LR-5 [Low] — the exclusivity invariant is whole-program, not locally
enforced, and the flood guard's only live proof is synthetic.** Nothing in
`AcDream.Core` prevents a future Content/Runtime producer from handing
`RegisterMultiPart` a mixed list; the BSP-first flood branch is the guard, and
S4 shows it is exercised by exactly one synthetic test. That is acceptable
(it is a faithful port, and it is documented as forward insurance), but it
should be understood as inert code with a synthetic-only witness rather than
as covered behaviour.

**LR-6 [Low] — "All eight sabotages run and reported" has no artifact.**
`docs/research/2026-08-06-ap152-contract.md` is the pre-implementation
contract; no closeout records sabotage outcomes. Five hold under independent
reproduction here; the other three rest on the commit message alone.

**LR-7 [Informational] — headless still has no live-entity collision at all
(#330), so graphical and headless now dispatch shapes by different rules for
the same world.** Pre-existing and correctly bounded by the commit ("do not
fix #330 here"), but it is now load-bearing: the fix improves the graphical
host only.

### Style
Covered by LR-3 / LR-4 (both are stale docs on live symbols, so I ranked them
as latent risk rather than style).

---

## AP-153 / AP-154 / AP-155 — honest residuals, not deferral

- **AP-153** (retail caches the dispatch flag once at
  `InitPartArrayObject+0x7e`; acdream's gate is live). A modelling difference
  the fix itself introduces, unreachable against ACE today because humanoid
  part swaps involve no physics-BSP GfxObj on either side. Filing it rather
  than building stale-flag state is the right call — modelling it would be
  speculative machinery for an unreachable case.
- **AP-154** (wire-derived `HAS_PHYSICS_BSP_PS`). **It does not undercut the
  gate's premise.** The gate is now derived from the parts, which is exactly
  what `CPartArray::CacheHasPhysicsBSP` does; AP-154 is about the *query-time*
  guard `BspOnlyDispatch`, which the commit deliberately leaves alone. What it
  does do is name the assumption the "already inert" argument rests on — which
  is why LR-2 is a framing problem rather than a hidden one. Filing it is
  correct; it should not have been resolved inside this commit.
- **AP-155** (static paths emit Setup Spheres as height-capped Cylinders; the
  flood approximates retail's bbox with bounding spheres). Its membership half
  is genuinely closed here; its static half is explicitly left open. The
  second clause is the mechanism behind LR-1, and the row states it plainly
  rather than burying it.

None of the three defers a part of the defect this commit set out to fix.

---

## Rules compliance

No workaround, suppression flag, grace period, retry loop, or symptom guard.
No `if (problematicState) return` at a symptom site — the gate is at the
producer, derived from the same data retail derives it from. No new skips
(4 skipped, unchanged). No test weakened to pass; both corrected tests are
strictly stronger and neither was deleted. Register updated in the same
commit, with count arithmetic checked (105 → 107).

---

## What I checked, so the PASS is auditable

1. Full diff of both production files and all five test files.
2. Every `ShadowShape` producer in `src/` (8 sites) and every
   `RegisterMultiPart` caller (6 production sites) read individually.
3. `.csproj` reference graph for Headless / Runtime / Content / Core.
4. `grep` for `new ShadowShape(` and `ShadowObjects.*` in `AcDream.Runtime`
   (empty / read-only).
5. Downstream consumers keyed on `ShadowCollisionType.Cylinder|Sphere`, on
   `shapes[0]`, and on `shapes.Count`.
6. Clean Release build after deleting all 44 `bin`/`obj` dirs: 0 / 21.
7. Full solution suite twice on clean builds: 11,203 / 4 / 0 both times.
8. Five sabotages reproduced (trap-1 gate identity, S4 flood revert, S5′
   scale-factor pair, B gate-disabled, DAT-sweep count control), each with the
   tree restored and `git status --porcelain` verified empty afterwards.
9. Register row count at HEAD vs `ec29a732`.
10. Diff scanned for `Skip =`, `try/catch`, sleeps, env-var reads, TODO/HACK.

Final tree state: **clean**, no modifications left behind. My single write is
this file.