replay: compile-enforce the command chokepoint

Privatize the Simulation player-action mutators (tryPlaceBuilding, demolish,
applySchematicChoice) and the mutable subsystem accessors (now buildingsMutable/
beltsMutable; only the const buildings()/belts() stay public). Production's only
handle to a subsystem is through Simulation, and every production buildings()/
belts() call is a const query, so this fully locks gameplay out of any
command-bypassing mutation path -- such code now fails to compile instead of
silently desyncing replays.

Tests reach the private mutators through SimulationTestAccess, a friend struct
under src/test (off the lib/ui/app include path), so they keep calling the real
mutators and keep their return values -- no id-by-position recovery needed. The
BuildingSystem subsystem mutators stay public (BuildingTest unit-tests a bare
subsystem; unreachable from production anyway).

Upgrades the Phase 1 "convention only" decision to structural enforcement.
All targets build; 354 cases / 3418 assertions pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DUsFgd2Ga6pmLz8giS8WUn

docs/replay_design.md

@@ -113,20 +113,36 @@ types), but the sim-mutating command path is a **dedicated, ordered queue**, not
 chokepoint. Any path that mutates the sim directly would not be recorded and would silently
 desync the replay.
 
-This was originally intended to be enforced **structurally** (make the `Simulation` mutators
-non-public so the only way to reach them is `apply(command)`). 
+This is enforced **structurally**: the `Simulation` player-action mutators are **private**, so
+the only way production code can reach them is `apply(command)`.
 
-> **Implementation decision (Phase 1).** The structural-enforcement plan was **dropped in
-> favour of convention**, because the test suite legitimately drives the same mutators
-> directly (`sim.tryPlaceBuilding(...)` and its returned id, `buildings().setRecipe(...)`,
-> `applySchematicChoice`, `reset`, `placeImmediate`, …) and relies on their return values —
-> making them non-public would break ~30 test call sites, and `apply()` cannot hand a new
-> `BuildingId` back to a caller. So the mutators stay **public**; the rule "every UI mutation
-> goes through a command" is upheld by convention and a documented chokepoint comment on
-> `Simulation::apply`. A `[command]` Catch2 suite asserts `apply(...)` produces byte-identical
-> state to the direct mutator path, guarding the equivalence the replay relies on. Tests are
-> not gameplay (they never record), so direct mutator use there does not affect replay
-> correctness.
+> **Implementation decision (Phase 1, revised post-Phase 4).** Structural enforcement was
+> initially deferred in favour of convention, because the test suite legitimately drives the
+> same mutators directly and relies on their return values (notably the `BuildingId` from
+> placement, which `apply()` cannot hand back to a caller). It was later restored once a key
+> observation made the change cheap: **the UI's only handle to a mutable subsystem is through
+> `Simulation`** — no production code in `ui`/`app`/`balancing` holds a `BuildingSystem`/
+> `BeltSystem` directly, and every production `buildings()`/`belts()` call is a const query.
+> So:
+>
+> - `Simulation::tryPlaceBuilding`, `demolish`, and `applySchematicChoice` are **private**.
+> - The mutable subsystem accessors are private and renamed `buildingsMutable()` /
+>   `beltsMutable()`; only `const BuildingSystem& buildings() const` / `belts() const` are
+>   public (queries). UI query sites bind to the const overload unchanged.
+> - `Simulation::apply` still mutates through the private members directly, so the chokepoint
+>   itself is unaffected.
+> - Tests reach the private mutators through `SimulationTestAccess` (src/test, a `friend struct`
+>   of `Simulation`), so they keep calling the real mutators **and keep getting return values**
+>   — no id-by-position recovery needed. This header is not on the lib/ui/app include path, so
+>   only test translation units can use it.
+>
+> The `BuildingSystem` subsystem mutators (`place`, `setRecipe`, `placeImmediate`,
+> `forEachBuilding`, …) stay **public**: `BuildingTest` unit-tests a bare `BuildingSystem` with
+> no `Simulation`/command layer, and that surface is unreachable from production anyway (you
+> cannot obtain a mutable subsystem without the private accessor). A `[command]` Catch2 suite
+> still asserts `apply(...)` produces byte-identical state to the direct mutator path, guarding
+> the equivalence the replay relies on. Tests are not gameplay (they never record), so direct
+> mutator use there does not affect replay correctness.
 
 Recording happens **at the apply chokepoint**, not at the UI gesture — so only commands that
 actually reached the sim are recorded, and they replay through the identical apply path.
@@ -366,9 +382,10 @@ Reshape mutations to flow through one path; behaviour unchanged.
   `ClearBeltTiles`, `ApplySchematicChoice`, `Reset`) in `lib`, each with a `playerId` (always 0
   now). `PlaceBuilding` is atomic (carries optional config — see the refinement note above).
 - Added `CommandManager` (FIFO queue, `enqueue`/`drain`) in `lib`, holding a `Simulation&`.
-- Added `Simulation::apply(const Command&)` dispatching by `CommandKind` to the existing
-  mutators — the single documented chokepoint. (Mutators stay public; enforced by convention,
-  see the decision note above.)
+- Added `Simulation::apply(const Command&)` dispatching by `CommandKind` to the underlying
+  mutators — the single chokepoint. The `Simulation` player-action mutators are **private**
+  (compile-time enforced; tests reach them via the `SimulationTestAccess` friend) — see the
+  decision note above.
 - Wired the drain: `GameWorldView::onFrame` calls `CommandManager::drain()` once per frame,
   before the tick batch (runs even at 0× → build-while-paused preserved). A drained `Reset`
   triggers the view reset.
@@ -379,8 +396,9 @@ Reshape mutations to flow through one path; behaviour unchanged.
   `Simulation.{h,cpp}` (`apply`); `GameWorldView.{h,cpp}`, `MainWindow.cpp`,
   `SelectedBuildingPanel.cpp`; new `CommandTest.cpp`.
 - **Exit criteria:** game plays identically (including build-while-paused); determinism test
-  still passes; `[command]` equivalence tests pass; no UI call site mutates the sim directly
-  (verified by grep — convention, not compile-enforced).
+  still passes; `[command]` equivalence tests pass; no production call site can mutate the sim
+  directly (compile-enforced: the `Simulation` mutators are private, tests excepted via
+  `SimulationTestAccess`).
 
 ### Phase 2 — Recording — DONE