# AP-156 fix review — closure record Closes the six findings in [`2026-08-06-ap156-fix-review.md`](2026-08-06-ap156-fix-review.md) (both lenses PASS; this is cleanup before merge, not a rescue). Worktree `.claude/worktrees/resume-session-e0bd03e1-d5bf45`, base `e2b2d04c`. All 44 `bin`/`obj` deleted before every verdict-deciding build, and every test run gated on a grep for `Build succeeded` in the same invocation — the trap the review documented (a sabotage that trips `CS0219` under `TreatWarningsAsErrors` fails the build and lets the *previous* sabotage's DLL report the wrong failure) was not hit. --- ## R1 — the containment oracle now discriminates **Done.** `InstalledSetupBspPrimitiveDispatchTests` no longer compares the emitted flood sphere against a hand-rebuilt copy of the same sphere. It transforms every **physics-polygon vertex** (`GfxObj.PhysicsPolygons` → `GfxObj.VertexArray.Vertices`) of every physics-BSP part into the entity frame and asserts some emitted flood sphere covers it. Vertices are a different DAT field from the bounding sphere, so the two sides of the comparison are no longer the same expression. Renamed to `InstalledSetups_BspFloodSpheres_ContainTheirOwnPhysicsPolygons`. New population + defect controls: 973 physics-BSP parts, 530 BSP-bearing Setups, 376 off-centre parts, **91,689** physics vertices, 428 pre-fix failures, deepest part array 49. Every one measured first by a scratch DatReaderWriter console program **outside the repo** that references no acdream assembly, then reproduced exactly through the production builder. **Sabotage-verified, three ways, each after a full clean and a verified `Build succeeded`:** | # | Sabotage | Result | |---|---|---| | S1 | `ShadowShape.Bsp` drops the bounds centre (the AP-156 defect itself) | **RED — 428 Setups, worst 35.869 m on 0x0200129A** (matches the independent sweep exactly) | | S2 | `ShadowShape.Bsp` applies the scale to the radius but not the centre | **RED — 326 Setups, worst 15.288 m** | | S3 | the TEST's own `Bounds()` oracle corrupted (`bs.Origin` negated) | **RED — 467 Setups, worst 72.134 m** | | S1b | same sabotage, run against Core rather than Content | **RED — 5 tests**: the 4 the review's sabotage A found, plus `ShadowRegistrationOverflowTests.FromLandblockBspParts_CarriesTheScaledRootSphereCentre`, which sabotage A could not reach because it only touched `BuildFloodSpheres`. The factory refactor makes both BSP producers share one scaling path, so one sabotage now covers both. | | — | restored | **GREEN — 127/127 Content, 4,268/1 skip Core** | S3 is the one that matters for R1. Under the shipped oracle that same corruption was invisible by algebra: `truth` was `(partOrigin + rotate(O, R))·s` and `flood` was `LocalPosition + rotate(BoundsCenter, R)` = `partOrigin·s + rotate(O·s, R)`, so negating `O` moved both sides identically and the shortfall stayed ≡ 0. The vertex oracle catches it. That is the tenth green-test-covering-nothing this campaign, and this is the direction of sabotage that proves it is closed — breaking the production code was never the hard half. ## R2 — population corrected, and refined **Done, with a refinement the review will want.** The review says 412, not "170 of 172". Both figures are right about different things, and neither is the number to quote to the user: | Question | Answer | |---|---| | Setups carrying both a primitive and a physics-BSP part (AP-152's DISPATCH population) | **172** of 5,935 | | BSP-bearing Setups (AP-156's population) | **530** | | …whose flood sphere actually MOVES because of the fix | **525** | | …failing vertex-level containment before the fix, 1 mm tolerance | **428** | | …failing at a 1 cm tolerance | **412** ← the review's figure | | …failing after the fix, at any tolerance down to zero | **0** | The 412/428 gap is 16 Setups whose pre-fix shortfall lands between 1.4 mm and 10 mm — real geometry, three orders of magnitude above float noise at these radii, not a measurement artifact. The test keeps its existing 1 mm tolerance and therefore pins **428**; the register records both and names the tolerance, because a bare "412" is unreproducible without it. **The number to tell the user is 525** — Setups whose behaviour changes — not 172, and not 412. Corrected in the AP-156 row, the section-3 header, the C5c handoff, and the two stale test docstrings. The dated review documents (`2026-08-06-ap152-review-retail.md`, and the fix review itself) are left as written: "170 of 172" was correct for what those reviews measured (root-sphere containment over the 172), and rewriting a dated evidence artifact to match a later measurement is how provenance gets lost. The commit's own record cannot be corrected in place — `b52967de` is HEAD~1 and amending it would rewrite published history. This document plus the register row is that correction. ## A1 — the hole is closed at the type **Done, and deliberately stronger than the review's recommendation.** The review suggested dropping the `= default` and passing `Vector3.Zero` explicitly at six call sites. That does **not** close the review's own failure scenario: someone adding a BSP branch to `LandblockPhysicsContentBuilder` by copying the adjacent Cylinder call would then write `Vector3.Zero` explicitly and get the same 20 m mis-placement, still green. Instead `ShadowShape`'s constructor is now **private**, and the three factories are the only way to build one: ```csharp ShadowShape.Bsp(gfxObjId, localPosition, localRotation, scale, FlatCollisionSphere localBounds) ShadowShape.Cylinder(gfxObjId, localPosition, localRotation, scale, radius, cylHeight) ShadowShape.Sphere(gfxObjId, localPosition, localRotation, scale, radius) ``` `Bsp` takes the radius and the centre as **one `FlatCollisionSphere` value** and scales them together inside the factory. There is no longer an expression a caller can write that carries one and drops the other — which is the commit body's own claim ("the radius cannot be taken while the origin is dropped"), now true of the type and not only of the `FromSetup` seam. 22 construction sites converted; S1 and S2 above are exactly the two ways the factory could still be got wrong, and both are red. Residual, recorded rather than fixed: `default(ShadowShape)` remains constructible because C# structs always have a parameterless constructor. It yields `CollisionType = BSP, Radius = 0` — inert, and no production path produces one. ## A2 — #333 updated, AP-158 filed, and the retail claim independently verified **Done.** I disassembled `CObjCell::find_obj_collisions` @`0x0052b750` myself from the PDB-paired binary (`check_exe_pdb.py` → **MATCH**, CodeView GUID `9e847e2f-777c-4bd9-886c-22256bb87f32`, linker 2013-09-06T00:17:56Z) rather than inheriting the claim, because Binary Ninja drops flag tests and that has bitten this campaign repeatedly: ``` 0x0052b759 cmp dword [ebx+0x174], 2 ; sphere_path.insert_type 0x0052b765 je 0x52b7a0 ; INITIAL_PLACEMENT_INSERT -> return OK_TS 0x0052b773 mov ecx,[edi+0xc8] ; shadow_object_list.data 0x0052b77f mov edx,[ecx+0x40] ; physobj->parent -> skip if set 0x0052b786 cmp ecx,[ebx] ; self -> skip 0x0052b78b call 0x50f050 ; FindObjCollisions — UNCONDITIONAL 0x0052b79e jb 0x52b773 ; loop, bound [edi+0xc4] ``` The single early-out is a transition-state test, not a distance test — the whole function is 0x54 bytes and there is no float comparison in it. Confirms the review: **retail has no distance pre-filter at all**, so acdream's `sphereRadius + obj.Radius + movement.Length() + 2f` is an invention. `[ebx+0x174]` resolves to `sphere_path.insert_type` against `acclient.h`'s `CTransition` layout and the pseudo-C at 308916-308940 agrees. Blast radius reproduced independently: **118 of the 477** unique installed physics-BSP GfxObjs exceed the ~2.5 m budget, **46** exceed 5 m. Filed as **AP-158** — the filter had no register row at all — carrying the disassembly, the `F_EPSILON` = 1.9999999e-4 m contrast (R5), the measured blast radius, and the "this can mask AP-156's entire visible benefit" consequence. #333 updated: its research question is struck through and answered, and what remains is a judgement call (keep the filter with a correct measurement point, or delete it and walk the list as retail does — the retail-faithful option, which should be costed first). ## The consequence that had to be recorded prominently **Tall props may show no visible change at all until #333 lands, and a null result at the connected gate is EXPECTED, not evidence against AP-156.** Recorded in three places a reader will actually hit: the AP-156 register row's `WHAT REMAINS OPEN`, the AP-158 row's risk column, and item 7 of the C5c handoff's §3.1 gate list — alongside the other half of the same warning, that "a prop stopped blocking" is only a bug if the object's own BSP geometry still reaches the cell it stopped blocking from. ## LOW items **R3 — `int.MaxValue` on the sorting-sphere branch.** *Comment corrected; code deliberately unchanged, and I think the review's implied fix is wrong.* The review is right that the cited evidence (`0x0052b9d6 push 1`) argues for a cap of 1, not `int.MaxValue`, and the comment block claimed to justify the whole line when it justified only the BSP half — that is fixed, and the comment now says plainly which half is a port and which is not. But capping at 1 would take `Spheres[0]`, and retail's one sphere is `CSetup::sorting_sphere`, a **different DAT field**. Capping would not move toward retail; it would keep the wrong field and additionally make the substitution under-inclusive, which is the #98/#168 direction. `int.MaxValue` keeps it over-inclusive until AP-157 ports the real field, which is the honest state. Inert over installed data either way — max 5 Spheres on any Setup (0x020016F7). **R4 — acdream scales the flood sphere, retail does not.** Recorded as a second residual on the AP-156 row. `find_transit_cells` @0x0052cae0 reads only `CPhysicsPart::pos` (`+0x30`) and never `gfxobj_scale` (`+0x24`), while `CPhysicsPart::find_obj_collisions` @0x0050d8d0 does thread `gfxobj_scale.z` into `SPHEREPATH::cache_localspace_sphere`. Direction: over-inclusive for scale > 1, under-inclusive for scale < 1. Radius scaling predates the commit; centre scaling did not, which is why it needed the row. **R5 — retail's slack is 0.0002 m, not 2 m.** Carried into AP-158's anchor column and into #333, where whoever fixes the filter will hit it. **A3 — per-call delegate allocation.** Fixed: `LiveEntityCollisionBuilder` now holds `private readonly Func _hasPhysicsBsp`, initialised once in the single real constructor and still derived from `_physicsBspBounds`, so the single-resolver invariant is intact and `Build` allocates no closure. **A6 — the Content test's "cap control" does not test the cap.** Comment corrected: the loop is the test's own uncapped re-implementation and cannot observe a cap regression; the cap is covered by `BuildFloodSpheres_CapsCylSpheresAtTenButNeverTheBspParts`, which reddens under both cap sabotages. What `mostBspShapesOnOneSetup == 49` genuinely proves — that the containment claim reaches Setups past the retired clamp rather than stopping short of them — is now what the comment claims. **A5 — sabotage A reddens 4 Core tests, not 3.** Noted; `b52967de`'s message cannot be amended. Coverage is one better than advertised, which is the benign direction. --- ## Gates | Gate | Baseline `e2b2d04c` | Observed | |---|---|---| | Release build, all 44 `bin`/`obj` deleted | 0 errors, 21 warnings | **0 errors, 21 warnings** | | Complete suite, `-m:1`, `ACDREAM_PAK_PATH` set | 11,208 / 4 skipped / 0 failed | **11,208 / 4 / 0** — reconciles exactly; one test RENAMED, none added or removed | | Content | 127 / 0 skips | **127 / 0 skips** | | Active AP register rows | 109 | **110** (AP-158 filed) | No new skips. Nothing was conflated with the known load-sensitive flakes #302 / #308 / #321.