erik/acdream
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2
acdream/docs/research/2026-08-06-ap22-review-architecture.md
Erik 619de97ad1 fix(test): evaluate BOTH deleted guards; correct AP-22's overstated coverage claim 
From the AP-22 dual review (both lenses PASS). No production change.

THE RECORD WAS WRONG. bc4679cd claimed "Headless.Tests 89/89 exercises the
site-3 copy". The architecture review disproved it by sabotage: restoring the
invented cylinder in BOTH static sites left the entire suite green. No test
anywhere references PublishStaticCollision, and the headless suite's dummy DAT
proxy makes LandblockLoader.Load fail for every landblock, so CreatePublication
returns before reaching it. Two of the three deletions — including the
headless-only one — are pinned by the installed-DAT reachability proof ALONE.
The deletion is still correct; the evidence claim was not, and a successor
trusting it would think those sites had regression cover they do not have.

THE TEST NOW COVERS WHAT IT CLAIMED. Its comment said "the exact guard the
three deleted copies used", but site 1 guarded on `Radius > 0.0001f` while
sites 2 and 3 used the strictly wider `Radius > 0f`. Those are not the same
predicate: the review measured that they differ over the installed DAT by
exactly one Setup, 0x02001657, whose radius is the denormal 1.3e-39. The test
now evaluates BOTH and asserts each is empty, so the wider guard the
headless-reachable deletion actually used is no longer asserted by proxy.

Sabotage-verified: widening the new guard to `>= 0f` reddens it (1,652
zero-radius Setups appear), so the assertion is live rather than vacuously
empty over real DAT data.

AP-22's row also corrected for two precisions the reviews surfaced: the
load-bearing fact is that all 1,652 no-primitive Setups carry Radius exactly 0
(not the 1,294 first cited), and retail's `report_object_collision` DOES read
GetHeight for the quadrant field — recorded so a future reader does not mistake
it for a refutation of "never collision geometry", which is a claim about
FindObjCollisions' shape dispatch only.

Reachability now independently reproduced by four decoders — the contract's
sweep, the implementer's parser, and both reviewers' from-scratch parsers —
plus tools/SetupInspect agreeing bit-for-bit on the cited ids.

Content.Tests 125/125. No new skips; #302/#308/#321 did not fire.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-08-06 08:54:31 +02:00

19 KiB
Raw Permalink Blame History

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.