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 continues 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.