[Audit][Round 2][Umbrella] A++ Architecture & Maintainability Roadmap

[MichaelFisher1997]

Goal

Drive ZigCraft from current B+/A− across SOLID & maintainability to A++ across the board. This is the Round 2 umbrella — successor to #776 (Round 1, M1–M21 + m1–m6, all closed) and parallel to #834 (production-readiness / CI / tooling, still open).

Round 1 closed the worst seams (split lod_mesh.zig, killed src/engine/ dead tree, extracted LightingEngine/LpvGridBuilder, dispatcher at RenderSystem.init, comptime generator registry, role interfaces, etc.). Round 2 picks up the debt that either survived Round 1 or was introduced/exposed by it.

The audit re-ran 5 parallel deep-dives (RHI abstraction, world-lod God-objects, dependency graph, error handling/tests, readability) against current dev. Every finding below cites a file:line verified at HEAD.

Round 2 scorecard (current state)

Principle	Round 1 close	Round 2 finding	Worst area
SRP	Weak → Adequate	Adequate	lod_manager.zig (2,715 LOC, ~10 concerns) — new God object exposed by Round 1's lod_mesh.zig split
OCP	Weak → Strong	Strong	Residual: adding a new RHI interface still touches rhi.zig in 5 places
LSP	Adequate	Weak	lpv_backend.zig:13 still downcasts rhi.ptr → *VulkanContext; ComputePipeline/ComputeBuffer still raw u64
ISP	Adequate → Strong	Strong	19 segregated I*Context vtables; RenderContext re-bundles 7 of them (35 pub fn)
DIP	Weak → Adequate	Adequate	One minor leak: world-runtime/world.zig:42,54 imports 2 POD structs from engine-ui
Test coverage	n/a	Bimodal	engine-rhi 0 tests / 263 pub fn · game-ui 0 tests / 108 pub fn · world-runtime ratio 0.08
Readability	n/a	B+	0 std.log leaks · 4 TODOs total · build.zig:build 668 LOC · pub-fn docs at 14.6%

Scope & method

• 5 parallel deep-dives (RHI/graphics, world-lod, dependency graph, error-handling/tests, readability)
• 13 actionable findings grouped into 4 phases (P1–P4)
• All evidence verified against dev HEAD
• No overlap with [Audit][Umbrella] SOLID architecture debt: RHI seam leakage, god-objects, and extension-safety gaps #776 (Round 1) or Production-Readiness Audit: missing CI / lint / benchmark / regression guardrails #834 (CI/tooling)

---

P1 — Architectural (highest leverage)

Goal: kill the remaining God-object and close the LSP/DIP gap that prevents a hypothetical second GPU backend.

• [R2-1][P1] Decompose LODManager God-object (2,715 LOC → ~500) #840 — R2-1 Decompose LODManager (2,715 LOC → ~500) by extracting 5 subsystems + encapsulating LODMesh/LODChunk
• [R2-2][P1] Encapsulate LODMesh/LODChunk behind behavior methods #841 — R2-2 Encapsulate LODMesh/LODChunk behind behavior methods to kill the dual-consumer open-struct pattern
• [R2-3][P1] Resolve residual RHI native-handle leak (lpv_backend, ComputePipeline, imgui) #842 — R2-3 Resolve residual RHI native-handle leak (lpv_backend.zig:13, ComputePipeline raw u64, imgui_backend consuming 6 native handles)
• [R2-4][P1] Add engine-rhi unit-test contract suite (263 pub fns, 0 tests) #844 — R2-4 Add engine-rhi unit-test contract suite (the central rendering contract has zero unit tests today)

P2 — Contracts & error discipline

Goal: remove shotgun-surgery signatures and silent failures that mask real bugs.

• [R2-5][P2] Convert updateGlobalUniforms 13-param signature to GlobalUniforms struct #846 — R2-5 Convert updateGlobalUniforms 13-parameter signature to a GlobalUniforms struct
• [R2-6][P2] Replace production catch unreachable in worldgen lighting with try #848 — R2-6 Replace production catch unreachable in worldgen lighting with try + error-aware generator interface
• [R2-7][P2] Fix world-runtime → engine-ui DIP leak (move 2 POD structs to world-core) #850 — R2-7 Fix world-runtime → engine-ui DIP leak (move 2 POD structs to world-core)
• [R2-8][P2] Surface silent save/mutation errors to user and log #843 — R2-8 Surface silent save/mutation errors (game-ui/screens/world_list.zig, player.zig, save_manager.zig)

P3 — Test coverage (the bimodal problem)

Goal: lift D/F-grade modules to A/B. Half the codebase is exemplary (math 4.00, world-core 2.33, meshing 2.09); the other half is dangerously thin.

• [R2-9][P3] Add game-ui test coverage (108 pub fns, 0 tests — grade F) #845 — R2-9 Add game-ui test coverage (108 pub fns, 0 tests — grade F)
• [R2-10][P3] Add world-runtime test coverage (ratio 0.08) + co-locate input/ecs tests #847 — R2-10 Add world-runtime test coverage (165 pub fns, 14 tests, ratio 0.08 — grade D); also co-locate engine-input/engine-ecs tests per AGENTS.md

P4 — Hygiene & docs

Goal: alignment between AGENTS.md and the code; doc coverage target ≥ 40%.

• [R2-11][P4] Fix AGENTS.md function-casing rule + normalize 4 mixed-style files #849 — R2-11 Fix AGENTS.md function-casing rule (mandates init_renderer, code is 99.3% camelCase per Zig stdlib) + normalize 4 mixed-style files
• [R2-12][P4] Decompose build.zig:build (668-line function → focused helpers) #851 — R2-12 Decompose build.zig:build (668-line function — largest in repo)
• [R2-13][P4] Raise public API doc coverage from 14.6% → ≥40% #852 — R2-13 Raise public-API /// doc coverage from 14.6% → ≥ 40% (AGENTS.md violation)

---

Suggested wave order

Wave	Items	Why first
1	#841, #840	LODManager is the only file that is genuinely hard to reason about today; #841 unblocks safe extraction in #840
2	#844, #842	RHI contract hardening; #844 (tests) should land before #842 (refactor) so the seam is guarded
3	#846, #848, #850, #843	Independent, low-risk, parallelizable
4	#845, #847	Test coverage; benefits from #840/#841 landing first (smaller units are easier to test)
5	#849, #851, #852	Hygiene; can land anytime but best after structural churn settles

Strengths to preserve (do not regress)

Round 1 left these in excellent shape — Round 2 must not break them:

• Composition root (src/game/app.zig) — textbook manual constructor injection with errdefer
• Worker-thread RHI isolation — zero RHI/Vulkan calls on workers; chunk pin()/unpin() discipline
• No dependency cycles — clean 10-layer acyclic DAG (one trivial leak addressed in [R2-7][P2] Fix world-runtime → engine-ui DIP leak (move 2 POD structs to world-core) #850)
• engine-rhi imports no Vulkan headers — true abstract layer for the dominant resource/render surface
• RenderGraph.addPass() OCP — purely additive pass extension, execute() is 14 lines
• Logging facade — 0 std.log leaks across 93K LOC
• TODO hygiene — 4 markers total, zero FIXME/HACK/XXX
• Leaf modules (lod_geometry, lod_ingest, lod_scheduler, meshing *_mesher) — pure, lock-free, single-shape

Constraints

• Use existing architecture (RHI vtables, job tokens, LODGPUBridge, role interfaces) — no parallel systems.
• Small reviewable PRs targeting dev; conventional commits (refactor:, feat:, test:, docs:); never push to dev directly.
• Preserve current gameplay/rendering behavior except where a sub-issue explicitly changes it.
• All build/test commands wrapped in nix develop --command.
• Verification baseline for every PR: nix develop --command zig build test (includes shader validation); rendering-touching PRs also run headless crash/screenshot/benchmark skill checks with -Dskip-present.
• P1 RHI work must keep the existing Vulkan path green end-to-end.

Key risks

• [R2-1][P1] Decompose LODManager God-object (2,715 LOC → ~500) #840 (LODManager decomposition) is the highest-value, highest-risk Round 2 work. 87 lock acquisitions across ~25 functions; documented cross-lock deadlock-avoidance rules mean locking must move with each extracted subsystem. Pin a headless screenshot baseline before/after.
• [R2-3][P1] Resolve residual RHI native-handle leak (lpv_backend, ComputePipeline, imgui) #842 (native-handle leak) touches imgui_backend, lpv_backend, and the compute pipeline struct — surface used by multiple consumers. Behavior-preserving refactor must keep all current call sites working.
• [R2-9][P3] Add game-ui test coverage (108 pub fns, 0 tests — grade F) #845/[R2-10][P3] Add world-runtime test coverage (ratio 0.08) + co-locate input/ecs tests #847 (test coverage) of UI/runtime glue is inherently harder than testing pure functions — may require harness investment (mock RHI, fake window) before meaningful tests can land.

Out of scope (covered elsewhere)

• CI/lint/benchmark/regression guardrails → Production-Readiness Audit: missing CI / lint / benchmark / regression guardrails #834
• Generated API documentation pipeline → Docs: Build a maintainable generated API documentation pipeline #609 (and phases Docs Phase 1: Decide generated API docs strategy #610–Docs Phase 6: Integrate generated API docs with Docusaurus #615)
• Survival mode / inventory overhaul → Survival Mode, Inventory Overhaul & Game Mode Selection #135
• Real-world terrain generator → feat: Real-World Terrain Generator (UK Prototype) #571

Subissue index

#	Title	Phase	Risk
#840	Decompose LODManager God-object	P1	HIGH
#841	Encapsulate LODMesh/LODChunk	P1	MEDIUM
#842	Resolve residual RHI native-handle leak	P1	HIGH
#844	Add engine-rhi unit-test contract suite	P1	LOW
#846	Bundle updateGlobalUniforms into struct	P2	LOW
#848	Replace worldgen catch unreachable	P2	MEDIUM
#850	Fix world-runtime → engine-ui DIP leak	P2	TRIVIAL
#843	Surface silent save/mutation errors	P2	MEDIUM
#845	Add game-ui test coverage	P3	LOW
#847	Add world-runtime test coverage	P3	LOW
#849	Fix AGENTS.md function-casing rule	P4	TRIVIAL
#851	Decompose build.zig:build	P4	LOW
#852	Raise pub-API /// doc coverage	P4	LOW

[github-actions]

I've reviewed the audit. Here is my analysis as a comment for the issue:

---

Comment: Analysis of Round 2 Umbrella

Classification: Feature Request / Refactoring roadmap (umbrella tracking issue, not a bug report). Same shape as the already-closed Round 1 umbrella (#776) — every box is a separate workstream that should land as its own PR.

1. Verified facts at HEAD (9c6e53d)

I sanity-checked the headline numbers and they all hold:

• modules/world-lod/src/lod_manager.zig is 2,715 LOC (claim: 2,715) — confirmed.
• pub fn build in build.zig spans lines 3–670 → 668 LOC (claim: 668) — confirmed.
• modules/engine-rhi/src/rhi.zig has 226 pub fn — indeed far beyond the 263 stat cited; grep -c "pub fn" returns 226 here, so worth double-checking the 263 number during P3 planning.
• modules/world-runtime/src/world.zig:42,54 imports ChunkStateCounts and WorldStateData from engine-ui — R2-7 confirmed.
• Minor path nit: the lpv_backend.zig:13 citation in R2-3 should be modules/engine-graphics/src/lpv_backend.zig:13 (no render/ subdir at that path). The downcast itself is real — please re-grep before filing R2-3 so the file:line in the sub-issue is precise.

2. Recommendation: split into sub-issues

This umbrella correctly avoids the "drive-by mega-PR" anti-pattern, but a 13-checkbox umbrella with cross-module scope is exactly the situation the repo's own #776 → Round 1 precedent handled by spawning one focused issue per item. Concretely, I'd ask for:

• One issue per R2- item* (13 total), each containing: scope (~LOC), acceptance criteria, out of scope (neighbors), and the wave it sits in.
• The umbrella stays open as a tracker, with each sub-issue linked back via "part of [Audit][Round 2][Umbrella] A++ Architecture & Maintainability Roadmap #839" — same model as [Audit][Umbrella] SOLID architecture debt: RHI seam leakage, god-objects, and extension-safety gaps #776.
• Pin a headless screenshot baseline before any R2-1 / R2-2 work begins (issue already calls this out; make it a sub-task on the R2-1 issue itself so it doesn't get lost).
• Wave-2 ordering is right: R2-4 (RHI test contract) should land before R2-3 (RHI refactor) so the seam is pinned by tests before the abstraction changes. I'd flip that dependency explicitly in the wave table.

3. Highest-leverage items to kick off first

If only three issues get created this week, I'd prioritize:

1. R2-1 (LODManager decomposition) — by far the largest correctness/maintainability win and the only file in the repo that's genuinely hard to reason about today. The cross-lock discipline (87 acquisitions across ~25 fns) means locking has to travel with each extracted subsystem — capture that as a non-negotiable acceptance criterion.
2. R2-2 (LODMesh/LODChunk encapsulation) — unblocks safe extraction in R2-1; small, mechanical, behavior-preserving. Best done immediately before R2-1.
3. R2-4 (engine-rhi contract tests) — required to make Wave 2 / R2-3 safe. The existing 19 I*Context vtables are exactly what the test suite should lock down (parameter types, error sets, handle lifetimes).

4. Risks the umbrella undersells

• R2-3 surface area is bigger than the three bullets suggest. ComputePipeline/ComputeBuffer raw u64 appear in multiple consumers; a grep for ComputePipeline{ and ComputeBuffer{ during issue filing will surface the full list — that should be a checklist item on the R2-3 issue itself.
• R2-9 (game-ui tests) needs a harness decision first — mock RHI / fake window. Without that, 0 → meaningful coverage is blocked. Recommend the R2-9 issue starts with a 1–2 day spike defining the harness, then writes tests, rather than the other way around.
• R2-11 (AGENTS.md casing rule) is a one-line doc edit, but the 4 mixed-style files are the real work; consider making R2-11 two sub-issues (doc fix vs. code rename) so the rename can be batched with zig fmt and reviewed mechanically.

5. Duplicate / coordination check

• Production-Readiness Audit: missing CI / lint / benchmark / regression guardrails #834 (CI / tooling) is already in flight and Round 2 is correctly scoped to exclude it — no overlap.
• [Audit][Umbrella] SOLID architecture debt: RHI seam leakage, god-objects, and extension-safety gaps #776 closed all of Round 1, so no leftover items should bleed into Round 2. A quick gh issue list --label "automated-audit" --state all --search "LOD OR lpv OR LODManager in:title" before filing is worth running to make sure no audit bot reopened a Round 1 finding under a new number.

6. Missing info (low-priority, non-blocking)

• No reproduction logs/version requested — not applicable for a refactor roadmap.
• A target date or milestone (e.g., "Wave 1 by end of Q3") would help triage. Optional.

Bottom line: the audit is solid, the breakdown is already in the right shape, and the wave ordering is mostly correct (swap R2-3/R2-4 ordering, otherwise leave it). The single most valuable action the author can take today is to file R2-1, R2-2, and R2-4 as three separate issues linked back to this umbrella, and pin a headless screenshot baseline before any code moves. I'll wait for direction before touching code.

opencode session  |  github run