[SOLID] [M16] Fix or delete split-brain RenderDevice; honor bindShader contract (covers M14, m5)

[MichaelFisher1997]

Part of #776 — SOLID architecture debt umbrella.
Phase: P3 · Finding: M16

Problem

Three related contract gaps:

• M16: modules/engine-rhi/src/render_device.zig — RenderDevice.createBuffer returns a handle never wired to the backend; backend_data always null. The real IResourceFactory.createBuffer ignores RenderDevice. Split-brain.
• M14: rhi_vulkan.zig:764-769 bindShader is a silent no-op in Vulkan but in the public IGraphicsCommandEncoder contract.
• m5: render_device.zig:14 doc-comment advertises a backend_type parameter the real signature lacks.

Evidence

rhi_vulkan.zig:764-769:

fn bindShader(ctx_ptr, handle) void {
  _ = ctx_ptr;
  if (handle != rhi.InvalidShaderHandle) {
    log.log.warn("Vulkan RHI bindShader({}) ignored: fixed pipelines...", .{handle});
  }
}

Fix

1. RenderDevice: wire it to the backend or delete it. The split-brain confuses every reader.
2. bindShader: either honor the contract or remove the method. A silent no-op is the worst option.
3. Doc lie: implement the backend_type parameter or fix the doc.

Verification

• nix develop --command zig build test (includes shader validation)
• If rendering-touching: capture headless screenshot baseline before/after (-Dskip-present)
• If behavior-preserving refactor: golden-output test must stay green

Constraints

• Small reviewable PR targeting dev; conventional commit (refactor: / feat:)
• Preserve current behavior unless this issue explicitly changes it
• Keep worker-thread RHI isolation intact (no RHI calls off main thread)

Notes

M14 may be resolved implicitly by C2 (native handles elimination) — check before implementing.

Tracking: #776

[github-actions]

Thanks — this is a clean audit finding, and the evidence cited is accurate. I validated each claim against the working tree and walked the call sites; below is what I found plus a recommended sequencing for the PR.

Validation

M16 (split-brain RenderDevice) — confirmed:

• modules/engine-rhi/src/render_device.zig:14 doc says init(allocator, backend_type) but the real signature at render_device.zig:37 is init(allocator: Allocator) !RenderDevice (no backend_type).
• render_device.zig:148,188,261,314,326,405,446,455 set backend_data = null on every code path. The pool never gets a backend handle.
• The "real" IResourceFactory.createBuffer lives at modules/engine-rhi/src/rhi.zig:112-127 and is plumbed straight to the Vulkan backend (rhi_vulkan.zig:959-1067). RenderDevice is not on that path.
• Smoking gun: modules/engine-graphics/src/render_system.zig:128 calls rhi_vulkan.createRHI(..., null, ...) and rhi.init(allocator, null) (render_system.zig:132). Production never instantiates RenderDevice; it is dead infrastructure kept alive only by the ?*RenderDevice plumbing slots, one defensive stats read at modules/engine-ui/src/ui_system_manager.zig:73, and the shader cache backend_data field that is also never populated.

M14 (bindShader no-op) — confirmed:

• modules/engine-rhi/src/rhi.zig:558 requires bindShader in IGraphicsCommandEncoder.VTable.
• modules/engine-graphics/src/rhi_vulkan.zig:764-769 is the silent-no-op+warn stub cited. Wired into the vtable at rhi_vulkan.zig:947. Exposed publicly via rhi.zig:318-319, rhi.zig:570-572, and rhi.zig:1195-1197.

m5 (doc lie) — same line as M16s doc lie (render_device.zig:14).

Coordinate before implementing

Two siblings in the same umbrella already claim pieces of this surface. Please check with them before opening code so we do not collide or duplicate:

• [SOLID] [C2] Eliminate INativeHandlesContext; move sky/water/shadow draws behind the RHI #784 (C2 — eliminate INativeHandlesContext) explicitly states: "Resolves M14 (bindShader no-op is a related contract gap — handle here or in M16)." If C2s plan removes bindShader from IGraphicsCommandEncoder.VTable as part of collapsing the native-handles seam, M14 is already done and this PR should not touch it. Confirm with whoever owns [SOLID] [C2] Eliminate INativeHandlesContext; move sky/water/shadow draws behind the RHI #784. Note: an earlier [Audit][High] bindShader RHI function is a no-op stub that silently ignores shader binding calls #566 raised the same bindShader problem and was closed (2026-04-28) without a code fix — worth understanding why before reopening.
• [SOLID] [M10] Add backend dispatcher at RenderSystem.init #798 (M10 — backend dispatcher) explicitly states: "Either implement the backend_type parameter the doc promises, or fix the doc." That is the same doc fix as m5. Either land it under M10 or fold it into this PR — pick one, not both.

Recommended PR shape

Once the above coordination is settled, this issue reduces to mostly M16 and is best landed as two small PRs rather than one:

PR 1 — M16 delete RenderDevice (recommended)
Since production never instantiates it, delete is cleaner than wiring:

1. Delete modules/engine-rhi/src/render_device.zig.
2. Drop the unused import and RenderDevice field from rhi.zig:59, 1026, 1029, 1250 (replace device: ?*RenderDevice with device: ?*anyopaque or remove the parameter entirely if no backend keeps a reference).
3. Drop the ?*RenderDevice parameter from modules/engine-graphics/src/rhi_vulkan.zig:28, 1098 and the mirror tree modules/engine-graphics/src/vulkan/rhi_context_factory.zig:5,20,31,202, rhi_context_types.zig:5,195, rhi_init_deinit.zig:5,23,25. Remove the now-redundant src/engine/graphics/render_device.zig and the src/engine/graphics/vulkan/* mirrors if [SOLID] [M10] Add backend dispatcher at RenderSystem.init #798 or the C1 follow-up has not already done so.
4. Update modules/engine-ui/src/timing_overlay.zig:6,158 and modules/engine-ui/src/ui_system_manager.zig:11,73 to drop the stats import (the UI overlay already falls back to zeroes — see line 73 — so behavior is preserved).
5. Behavior-preserving refactor: nix develop --command zig build test must stay green; golden-output tests unaffected because RenderDevice never carried GPU state.

PR 2 (only if #784 does not already do it) — M14 remove bindShader

1. Drop bindShader from IGraphicsCommandEncoder.VTable (rhi.zig:558) and the wrappers (rhi.zig:318-319, 570-572, 1195-1197).
2. Remove the impl rhi_vulkan.zig:764-769 and the vtable slot rhi_vulkan.zig:947.
3. Remove the Mock impl rhi_tests.zig:21, the vtable slot rhi_tests.zig:469, and the test call rhi_tests.zig:496.
4. Grep for any remaining callers (grep found only the rhi_tests site + the vtable entries; none in engine-graphics or world-runtime).

m5 — doc fix
Trivial one-liner: either drop the backend_type word from render_device.zig:14 (preferred if PR 1 deletes the file) or add a backend_type parameter + plumbing if #798 plans to wire it. Coordinate with #798 to avoid two PRs touching the same doc line.

Verification

• nix develop --command zig build test (shader validation included).
• nix develop --command zig build run -Dskip-present -Dsmoke-test to confirm no runtime regression in the graphics init path.
• Not rendering-touching (no GPU state on the RenderDevice side), so a headless screenshot baseline is not required for M16/m5. Required only if PR 2 (bindShader removal) lands and any code path actually called bindShader at runtime — the grep suggests none do, but worth confirming.

Scope verdict

Appropriately scoped if split into the two PRs above. Do not try to also fix M10 (#798) or C2 (#784) here — both are tracked and this PR should not silently absorb their surface.

[github-actions]

Comment posted to #802: #802 (comment)

Summary of what I posted:

• Classification: Refactor / architecture cleanup with bug undertones (the m5 doc-comment is a documentation bug; M14 and M16 are contract/architecture smells).
• Validation: Walked every cited line; confirmed all three claims. Smoking gun for M16 is render_system.zig:128 passing null for RenderDevice, plus the backend_data = null literal on every code path of the pools.
• Scope guidance: Recommended splitting into two small PRs (delete RenderDevice, remove bindShader) rather than one combined PR, and explicitly flagged that:
  • #784 (C2) already claims M14 — coordinate before touching bindShader.
  • #798 (M10) already claims m5 — coordinate before touching the doc-comment.
  • #566 was a previously-closed duplicate of M14 worth investigating.
• Next steps: Concrete file:line list for deletion sites, plus verification commands (zig build test and the smoke-test variant).

opencode session  |  github run