build: migrate dev environment and CI from nix flake to devenv

[MichaelFisher1997]

Summary

Migrates the entire developer environment and CI from a hand-rolled Nix flake (flake.nix + flake-utils) to devenv. The four C/C++ derivations (zig 0.16.0, cimgui, rmlui, rmlui-bridge) and the ZIGCRAFT_DYNAMIC_LINKER / ZIGCRAFT_RUNTIME_LIBRARY_PATH runtime-loader mechanism are ported verbatim, so the binaries produced are identical.

Key decisions (decided up-front in planning)

• Profiles, not shells — base devenv.nix exposes the common foundation; three additive profiles mirror the old devShells:
  • unit (lean CPU: kcov, shellcheck — no mesa/weston/zls)
  • graphics (mesa/Lavapipe + weston)
  • default (full local-dev: zls, mesa, weston, kcov — auto-activated by .envrc)
• nix build -L → devenv task — the zigcraft:build task replaces packages.default; produces the Debug x86_64-linux binary with the nixpkgs rpath baked in via patchelf.
• direnv auto-activation — .envrc uses use devenv --profile default.
• Docs use devenv shell <cmd> form (CI uses devenv shell --profile <unit|graphics> --).

Commits (8, ordered)

1. feat: add devenv shell configuration alongside nix flake — devenv.nix/devenv.yaml/devenv.lock/.envrc
2. ci: add setup-devenv action, migrate weston/zig-cache, add zigcraft task
3. ci: migrate build.yml (nix build -L → devenv tasks run zigcraft:build)
4. ci: migrate remaining workflows (11 workflows + labeler.yml)
5. fix: use namespaced devenv task name zigcraft:build — bare zigcraft failed devenv shell validation; devenv task names require namespace:name
6. refactor: migrate dev scripts, pre-push hook, shader error strings
7. docs: migrate all docs, skills, and prompts (23 files)
8. chore: remove nix flake and setup-nix action

Scope touched

• Config: devenv.nix (+624 lines), devenv.yaml, devenv.lock, .envrc, .gitignore
• CI: new .github/actions/setup-devenv/; all 12 workflows; start-weston, setup-zig-cache, labeler.yml; removed setup-nix
• Runtime: 4 scripts, .githooks/pre-push, 3 Zig shader error strings
• Docs: AGENTS.md, README.md, CONTRIBUTING.md, 9 docs/*, 5 agent skills, pr-autopilot skill, 2 opencode commands, 3 prompts

Verification

• devenv info evaluates; ZIGCRAFT_DYNAMIC_LINKER/ZIGCRAFT_RUNTIME_LIBRARY_PATH resolve to the nixpkgs glibc loader + 9-library path.
• Profiles differentiate correctly: unit has no weston/mesa, graphics has weston, default has zls.
• IN_NIX_SHELL=impure is set by devenv, so scripts/run_benchmark.sh needed no logic change (only its fallback wrapper updated).
• zig build links zigcraft/benchmark/robust-demo end-to-end in devenv shell --profile unit (exit 0).
• actionlint full-repo scan: clean.
• grep across all tracked files: zero residual nix develop / nix flake / setup-nix / flake-utils references.

Notes for reviewers

• One-time cache invalidation: the Nix-store and Zig-compiler cache keys change (now keyed on devenv.nix/devenv.yaml/devenv.lock), so the first CI run after merge will be cold.
• Local dev setup: contributors now need devenv installed (nix profile add nixpkgs#devenv) in addition to Nix; direnv is optional but recommended. CONTRIBUTING prerequisites updated.
• Ad-hoc nix run nixpkgs#gitleaks|trivy / nix shell nixpkgs#imagemagick in security.yml/visual-test.yml are intentionally retained — Nix remains installed under devenv.
• platform-build (Windows/macOS) is unchanged; it never used Nix.

Migration notes

Remaining "Nix" mentions in docs are the accurate "Nix-based" / "required by devenv" qualifiers — devenv still runs on a Nix daemon.

🤖 Generated with assistance from opencode

[github-actions]

kcov coverage

Line coverage ran for this PR and uploaded a non-blocking report artifact named kcov-report. Codecov upload is configured as non-blocking while the project captures a stable baseline.

[github-actions]

I have completed my analysis. Here is my review.

📋 Summary

Linked issues: None. The PR description does not reference any issues via Closes #, Fixes #, or Resolves #. (Note: PR is labeled ci, build, documentation, engine — not automated-test — so the automated-test-writer full coverage criteria do not apply.)

This PR migrates ZigCraft's developer environment and CI from a hand-rolled Nix flake to devenv, porting the four C/C++ derivations (zig 0.16.0, cimgui, rmlui, rmlui-bridge) verbatim and re-wiring all 12 workflows, the runtime scripts, pre-push hook, docs, skills, and prompts to devenv shell --profile <unit|graphics>. The migration sweep is mechanically clean (zero residual nix develop/flake-utils/setup-nix references), actionlint passes, and the build/fmt/unit-test-matrix jobs are green. However, the integration-test and benchmark CI gates are failing on the head commit because the Vulkan validation layers used by setup-lavapipe are pulled unpinned from nixpkgs and have drifted to 1.4.350.0, surfacing 86 Vulkan validation errors. The author's devenv.lock pinning fix only covers packages consumed via the devenv profile, not the ad-hoc nix build nixpkgs#... calls.

📌 Review Metadata

• Reviewed Commit SHA: e2cb133c69ef4f1a1acb0600bb4f0b19b47824f3
• Reviewed PR: build: migrate dev environment and CI from nix flake to devenv #951

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (no previous automated reviews existed).

None identified at the critical severity — the failing CI gates are classified below as High because they are recoverable with a small pinning fix to setup-lavapipe rather than a fundamental flaw in the migration.

⚠️ High Priority Issues (Should Fix)

[HIGH] .github/actions/setup-lavapipe/action.yml:11-12 - integration-test and benchmark CI gates fail on the head commit because Vulkan validation layers are not pinned to devenv.lock
Confidence: High
Description: The head commit e2cb133 was supposed to pin nixpkgs to 0bb7ec54... so that "the migration changes only the shell tooling, not the dependency versions," and the commit message claims this was verified. But the actual CI run for e2cb133 still fails integration-test (3m29s, exit 1) and benchmark (6m28s, exit 1). The integration log shows Integration test finished with 86 Vulkan validation errors / expected 0, found 86 / FAIL (TestExpectedEqual) and the benchmark fails with error.LodGpuBudgetBreach. Root cause: setup-lavapipe/action.yml:11-12 resolves Lavapipe and the Khronos validation layers via nix build --no-link --print-out-paths nixpkgs#mesa.drivers and nix build --no-link --print-out-paths nixpkgs#vulkan-validation-layers, which use the unpinned flake-registry nixpkgs (currently pulling vulkan-validation-layers-1.4.350.0). The devenv.yaml nixpkgs pin does not flow into these two ad-hoc nix build calls. dev's last green integration run was 2026-07-08; the layers drifted forward in the 17-day gap and now flag pre-existing latent issues (PIE binaries, viewport 0×0, srcAccessMask mismatches between VkRenderPass 0x11d and pipeline built against 0x10a, descriptor-set invalidation across recorded command buffers).
Impact: Two of the required CI gates are red on the merge candidate. Merging advertises dev as healthy while its primary graphics correctness signal is broken. The PR's stated invariant — "the binaries produced are identical" — does not hold for the validation layer that gates correctness.
Suggested Fix: Pin both ad-hoc nix build references to the same revision devenv.lock pins, so the validation layers match the rest of the environment:

- name: Resolve Vulkan paths
  shell: bash
  run: |
    set -euo pipefail
    # Match devenv.yaml's pinned nixpkgs so validation-layer drift doesn't
    # surface latent errors unrelated to a PR under review.
    nixpkgs_rev="${NIXPKGS_PIN:-0bb7ec54c8483066ec9d7720e780a5caa71f8612}"
    LVP_PATH=$(nix build --no-link --print-out-paths "github:NixOS/nixpkgs/${nixpkgs_rev}#mesa.drivers")/share/vulkan/icd.d/lvp_icd.x86_64.json
    LAYER_PATH=$(nix build --no-link --print-out-paths "github:NixOS/nixpkgs/${nixpkgs_rev}#vulkan-validation-layers")/share/vulkan/explicit_layer.d
    ...

(Alternatively, surface mesa.drivers and vulkan-validation-layers as devenv profile packages so they share devenv.lock's pin and the ad-hoc nix build calls can be dropped.) Until one of these is in place, the integration-test and benchmark gates will remain red on this PR and on any subsequent dev push.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] devenv.nix:424 - patchelf failure diagnostics are silenced
Confidence: Medium
Description: The best-effort rpath bake in tasks."zigcraft:build".exec redirects patchelf's stderr to /dev/null (if patchelf --add-rpath ${artifact_runtime_rpath} "$out/bin/zigcraft" 2>/dev/null; then). The PR description notes patchelf hits rewriteSectionsExecutable assertions on zig's PIE output. Swallowing stderr means the actual assertion text is not in the build log, making it hard to tell whether the binary is relocatable or just unpinned for that build.
Impact: Harder to diagnose future regressions; if zig stops emitting PIE for some targets the rpath bake could silently start succeeding or hit a different failure.
Suggested Fix: Capture stderr to a temp file and print it only on failure, e.g. if patchelf ... 2>"$tmp/patchelf.log"; then ... else echo "patchelf failed:"; cat "$tmp/patchelf.log"; fi.

[MEDIUM] .github/workflows/build.yml:147-161 - artifact directory double-lists the binary
Confidence: High
Description: The zigcraft:build task writes to dist/bin/zigcraft. The "Prepare Artifact" step then copies it to dist/zigcraft-linux, and "Upload Artifact" uses path: dist/. The uploaded artifact therefore contains both dist/bin/zigcraft and the duplicate dist/zigcraft-linux.
Impact: Doubled artifact size and an ambiguous binary path for consumers. Minor, but the previous flake packages.default produced a single result/bin/zigcraft symlink.
Suggested Fix: Either upload only dist/zigcraft-linux (the prepared copy) or upload the bin directory explicitly with path: dist/bin/.

ℹ️ Low Priority Suggestions (Optional)

[LOW] .github/actions/start-weston/action.yml:50 - redundant profile realization
Confidence: Low
Description: The action runs devenv shell --profile "${{ inputs.devenv-profile }}" -- true purely to realize the profile before launching weston. Each devenv shell invocation re-evaluates the configuration; in CI this typically adds 5–15s per call.
Impact: Minor extra wall-clock time on every graphics job. Not incorrect.
Suggested Fix: devenv shell --profile ... -- weston ... will realize the profile on first use anyway; the warmup -- true is only useful if you want to surface realization failures distinctly from weston startup failures. If that's the intent, document it; otherwise drop the warmup line.

[LOW] devenv.nix:11-25 - nix_runtime_library_path includes pkgs.mesa unconditionally
Confidence: Medium
Description: The runtime library path always includes pkgs.mesa, but the unit profile (used by the zigcraft:build task and unit tests) does not include mesa in its packages. This is ported verbatim from the old flake ci-unit shell (which also omitted mesa from buildInputs while nix_runtime_library_path listed it), so behavior is preserved — but the makeLibraryPath call will resolve a mesa store path that may not actually be reachable in the unit profile.
Impact: No functional issue today (the path entry is harmless if the lib isn't loaded), but the implicit mesa dependency makes the unit profile less hermetic than it appears.
Suggested Fix: Either add mesa to the unit profile's packages (so the path is real) or make the runtime path computation profile-aware.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	7/10	devenv.nix mixes derivations, env, task, and profiles, but this is the canonical devenv layout and the concerns are clearly sectioned.
Open/Closed	8/10	The additive profiles.{default,unit,graphics} design makes it trivial to add new profiles without touching the base.
Liskov Substitution	7/10	Profiles compose predictably on top of the common foundation; no surprising overrides.
Interface Segregation	8/10	Three narrow profiles cleanly segregate CPU-only vs graphics vs full local dev.
Dependency Inversion	5/10	The migration correctly inverts the devShell mechanism, but setup-lavapipe still depends on the unpinned nixpkgs registry for the validation layers (see High issue).
Average	7.0

🎯 Final Assessment

Overall Confidence Score: 58%

Confidence Breakdown:

• Code Quality: 80% (clean mechanical sweep, well-commented Nix, faithful port of derivations)
• Completeness: 55% (migration is functionally complete but the integration/benchmark gates are red on the head commit and the pinning fix is incomplete)
• Risk Level: 45% (a broken integration-test gate on dev would mask real graphics regressions for subsequent PRs)
• Test Coverage: 50% (no new tests; existing CI is the verification surface and two key jobs are failing)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE WITH FIXES — The migration itself is thorough and well-executed, but pinning vulkan-validation-layers (and mesa.drivers) in setup-lavapipe/action.yml to the same revision devenv.lock uses is required before merge so the integration-test and benchmark gates return to green.

Machine Readable Verdict

{
  "reviewed_sha": "e2cb133c69ef4f1a1acb0600bb4f0b19b47824f3",
  "critical_issues": 0,
  "high_priority_issues": 1,
  "medium_priority_issues": 2,
  "overall_confidence_score": 58,
  "recommendation": "MERGE_WITH_FIXES"
}

opencode session  |  github run

[github-actions]

I have enough information to write the review. The previously reported HIGH issue's "fix" (commit bde5bcf) introduced a new critical regression: the benchmark CI gate now fails at the Setup Lavapipe Vulkan step itself (1m31s into the run) because pinning via github:NixOS/nixpkgs/<rev>#vulkan-validation-layers triggers a source build that fails (git not in sandbox). The integration-test gate uses the same action and will fail identically.

📋 Summary

Linked issues: None. The PR description references no issues via Closes #/Fixes #/Resolves #. The PR is labeled ci, build, documentation, engine — not automated-test — so the automated-test-writer full coverage criteria do not apply.

This PR migrates ZigCraft's developer environment and CI from a hand-rolled Nix flake to devenv, porting the four C/C++ derivations (zig 0.16.0, cimgui, rmlui, rmlui-bridge) verbatim and re-wiring all 12 workflows, runtime scripts, pre-push hook, docs, skills, and prompts to devenv shell --profile <unit|graphics>. The migration sweep itself is mechanically clean and the previously reported MEDIUM issues (patchelf diagnostics, artifact double-listing) are properly resolved. However, the latest commit bde5bcf — intended to fix the previously reported HIGH issue (unpinned Lavapipe/validation layers) — introduces a new critical regression: the pinning change forces Nix to source-build vulkan-validation-layers, which fails because the derivation's update_deps.py invokes git clone and git is not in the build sandbox. The benchmark gate fails at the Setup Lavapipe Vulkan step on the head commit; the integration-test gate uses the same action and will fail identically once build completes.

📌 Review Metadata

• Reviewed Commit SHA: bde5bcf704233242d033cbe35f14259924cfa8ef
• Reviewed PR: build: migrate dev environment and CI from nix flake to devenv #951

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ Previous critical issues status: No critical issues were reported in the prior review (it had only HIGH/MEDIUM/LOW). However, the latest commit's attempted fix of the prior HIGH issue introduced a new critical regression below.

[CRITICAL] .github/actions/setup-lavapipe/action.yml:18-20 - Pinning vulkan-validation-layers via github:NixOS/nixpkgs/<rev>#vulkan-validation-layers triggers a source build that fails, breaking the Lavapipe setup step on every graphics job
Confidence: High
Description: The fix commit bde5bcf reads nixpkgs_rev from devenv.lock and then calls nix build --no-link --print-out-paths "github:NixOS/nixpkgs/${nixpkgs_rev}#vulkan-validation-layers". Empirically, this address form does not get a binary-cache hit on the actual vulkan-validation-layers-1.4.350.0 derivation output (the bare nixpkgs#vulkan-validation-layers shorthand used by dev does, because it resolves through the nixpkgs flake registry). Nix therefore falls through to a source build of the layer package. That source build fails because the upstream derivation runs scripts/update_deps.py, which invokes git clone to fetch Vulkan-Utility-Libraries, and git is not present in the build sandbox:

benchmark  Setup Lavapipe Vulkan  2026-07-25T11:06:02.7881220Z building '/nix/store/rlskik9fpi31rky450vfyipsrzvz5kk9-vulkan-validation-layers-1.4.350.0.drv'...
benchmark  Setup Lavapipe Vulkan  2026-07-25T11:06:03.3364362Z ❌ github:NixOS/nixpkgs/0bb7ec54c8483066ec9d7720e780a5caa71f8612#vulkan-validation-layers
benchmark  Setup Lavapipe Vulkan  2026-07-25T11:06:03.3364792Z error: Cannot build '/nix/store/rlskik9fpi31rky450vfyipsrzvz5kk9-vulkan-validation-layers-1.4.350.0.drv'.
benchmark  Setup Lavapipe Vulkan  2026-07-25T11:06:03.3457609Z        > FileNotFoundError: [Errno 2] No such file or directory: 'git'
benchmark  Setup Lavapipe Vulkan  2026-07-25T11:06:03.3457810Z        > CMake Error at scripts/CMakeLists.txt:133 (message):
benchmark  Setup Lavapipe Vulkan  2026-07-25T11:06:03.3458328Z        >   Could not run update_deps.py which is necessary to download dependencies.
benchmark  Setup Lavapipe Vulkan  2026-07-25T11:06:03.3462009Z ##[error]Process completed with exit code 1.

The benchmark job on bde5bcf failed at this step after 1m31s (run 30155603524) — before the benchmark binary was even built. The integration-test gate in build.yml:267-269 calls the exact same setup-lavapipe action and will fail identically once its upstream build/unit-test jobs complete.

Impact: Two of the required correctness/performance CI gates (integration-test, benchmark) are red on the merge candidate because they cannot even install the validation layers. Merging advertises dev as healthy while its primary graphics correctness signal is dead. The fix is also strictly worse than the prior state on e2cb133 (which at least ran the integration test and surfaced validation errors) — now the test cannot start at all.

Suggested Fix: Use a flake URL form that preserves the binary-cache hit. The cleanest option is to surface mesa.drivers and vulkan-validation-layers as devenv profile packages in devenv.nix so they share devenv.lock's pin and go through the same fetch path as everything else (the graphics profile already pulls mesa):

graphics.module = { pkgs, ... }: {
  packages = [
    pkgs.mesa
    pkgs.mesa.drivers            # for the Lavapipe ICD JSON
    pkgs.vulkan-validation-layers
    pkgs.weston
    pkgs.shellcheck
  ];
};

Then have setup-lavapipe/action.yml resolve the paths out of the already-realized profile (no nix build call at all):

- name: Resolve Vulkan paths
  shell: bash
  run: |
    set -euo pipefail
    # mesa.drivers and vulkan-validation-layers come from the graphics profile
    # (pinned via devenv.lock) -- no separate nix build needed.
    LVP_PATH="$(devenv shell --profile graphics -- bash -c 'echo "${pkgs.mesa.drivers}"')/share/vulkan/icd.d/lvp_icd.x86_64.json"
    ...

If a self-contained ad-hoc nix build is still preferred, switch the address form so it hits cache.nixos.org:

nixpkgs_rev=$(jq -r '.nodes.nixpkgs.locked.rev' devenv.lock)
# legacyPackages keeps the registry alias path that cache.nixos.org indexes;
# 'packages.<system>.foo' or 'github:owner/repo/rev#foo' can miss the binary cache.
LVP_PATH=$(nix build --no-link --print-out-paths "nixpkgs/${nixpkgs_rev}#legacyPackages.x86_64-linux.mesa.drivers")/share/vulkan/icd.d/lvp_icd.x86_64.json
LAYER_PATH=$(nix build --no-link --print-out-paths "nixpkgs/${nixpkgs_rev}#legacyPackages.x86_64-linux.vulkan-validation-layers")/share/vulkan/explicit_layer.d

Either way, the choice must be verified by re-running the benchmark and integration-test gates on the new commit before merge.

⚠️ High Priority Issues (Should Fix)

✅ [FIXED] Previous HIGH issue: setup-lavapipe/action.yml not pinning vulkan-validation-layers/mesa.drivers to devenv.lock rev. A pinning change has been applied (.github/actions/setup-lavapipe/action.yml:11-20), but the chosen approach introduces the critical regression above. The previous HIGH itself is considered resolved-intent, with the caveat that the resolution mechanism is broken — tracked separately as Critical.

No other new HIGH issues identified.

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous MEDIUM issue: devenv.nix:424 patchelf diagnostics silenced. The tasks."zigcraft:build".exec now captures stderr to a mktemp file (patchelf_log) and cats it on failure (devenv.nix:424-431). Future patchelf regressions will be diagnosable.

✅ [FIXED] Previous MEDIUM issue: build.yml artifact directory double-listed the binary. The "Upload Artifact" step now uses path: dist/zigcraft-linux (the prepared copy) instead of path: dist/ (build.yml:160), so the artifact no longer contains both dist/bin/zigcraft and dist/zigcraft-linux.

No new MEDIUM issues identified.

ℹ️ Low Priority Suggestions (Optional)

[LOW] .github/actions/start-weston/action.yml:49-50 - Redundant profile realization (carried over from previous review)
Confidence: Low
Description: The action still runs devenv shell --profile ... -- true purely to realize the profile before launching weston (now with an explanatory echo). This re-evaluates the devenv config, typically adding 5–15s per graphics job. The warmup is only useful to surface realization failures distinctly from weston startup failures; that intent is now documented, so this is acceptable as-is, but it remains an extra wall-clock cost on every graphics job.
Suggested Fix: None required now that the intent is documented; drop the warmup line if future profiling shows it dominates cold-start time.

[LOW] devenv.nix:22-25 - nix_runtime_library_path includes pkgs.mesa unconditionally (carried over from previous review)
Confidence: Medium
Description: The runtime library path always includes pkgs.mesa, but the unit profile (used by zigcraft:build and unit tests) does not include mesa in its packages. This is ported verbatim from the old flake ci-unit shell, so behavior is preserved — but the makeLibraryPath call will resolve a mesa store path that may not actually be reachable in the unit profile.
Suggested Fix: Either add pkgs.mesa to the unit profile's packages (so the path is real) or make the runtime path computation profile-aware. Not blocking.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	7/10	devenv.nix still mixes derivations, env, task, and profiles, but this is the canonical devenv layout and concerns are clearly sectioned.
Open/Closed	8/10	The additive profiles.{default,unit,graphics} design makes adding new profiles trivial without touching the base.
Liskov Substitution	7/10	Profiles compose predictably on top of the common foundation.
Interface Segregation	8/10	Three narrow profiles cleanly segregate CPU-only vs graphics vs full local dev.
Dependency Inversion	4/10	The pinning fix in setup-lavapipe re-introduces a fragile dependency on the GitHub-rev address form hitting cache.nixos.org (it doesn't), so the migration's "single source of truth" claim is broken in practice (see Critical).
Average	6.8

🎯 Final Assessment

Overall Confidence Score: 35%

Confidence Breakdown:

• Code Quality: 75% (clean mechanical sweep, faithful port of derivations, MEDIUM fixes are clean)
• Completeness: 30% (the migration code is complete, but the Lavapipe-pinning fix is broken and two required CI gates cannot even start their core test/benchmark)
• Risk Level: 25% (a broken integration-test gate on dev would mask real graphics regressions for every subsequent PR; the fix made things strictly worse than e2cb133)
• Test Coverage: 25% (existing CI is the verification surface, and the benchmark gate fails at environment setup before any test runs)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE WITH FIXES — The migration itself is well-executed and the previous MEDIUM issues are properly resolved, but the head commit's fix for the prior HIGH issue (Lavapipe pinning) is broken: the benchmark gate fails at Setup Lavapipe Vulkan itself and the integration-test gate uses the same action, so the pinning approach must be reworked (prefer devenv profile packages or legacyPackages address form) and the benchmark/integration-test gates re-verified before merge.

Machine Readable Verdict

{
  "reviewed_sha": "bde5bcf704233242d033cbe35f14259924cfa8ef",
  "critical_issues": 1,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 35,
  "recommendation": "MERGE_WITH_FIXES"
}

opencode session  |  github run

[github-actions]

I have enough information to complete the review. The head commit f7f8888 cleanly reverts the previously-broken Lavapipe pinning and the previously-fixed MEDIUM issues are retained.

📋 Summary

Linked issues: None. The PR description references no issues via Closes #/Fixes #/Resolves #. The PR is labeled ci, build, documentation, engine — not automated-test — so the automated-test-writer full-coverage criteria do not apply.

This PR migrates ZigCraft's developer environment and CI from a hand-rolled Nix flake to devenv, porting the four C/C++ derivations (zig 0.16.0, cimgui, rmlui, rmlui-bridge) verbatim and re-wiring all 12 workflows, runtime scripts, pre-push hook, docs, skills, and prompts to devenv shell --profile <unit|graphics>. The head commit (f7f8888) cleanly reverts the previously-broken Lavapipe-pinning attempt with a sound root-cause analysis, the previously-fixed MEDIUM issues are retained, and the migration sweep is mechanically clean (actionlint passes; zero residual nix develop/flake-utils/setup-nix references).

📌 Review Metadata

• Reviewed Commit SHA: f7f888893a6a49fbb32b58e302619dda14e9616f
• Reviewed PR: build: migrate dev environment and CI from nix flake to devenv #951

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ [FIXED] Previous CRITICAL issue (commit bde5bcf): pinning vulkan-validation-layers via github:NixOS/nixpkgs/<rev>#vulkan-validation-layers forced a from-source build that failed (FileNotFoundError: git in the sandbox), breaking the Lavapipe setup step on every graphics job. The head commit reverts setup-lavapipe/action.yml:19-20 back to the floating nixpkgs#mesa.drivers / nixpkgs#vulkan-validation-layers shorthand — which substitutes from the well-populated nixpkgs-unstable binary cache — and adds a clear comment (action.yml:11-18) documenting why pinning is not viable at this rev. No source build is triggered.

✅ All previously reported critical issues have been resolved.

No new critical issues identified.

⚠️ High Priority Issues (Should Fix)

✅ [FIXED] Previous HIGH issue (setup-lavapipe not pinning validation layers / causing drift). The head commit's revised root-cause analysis is sound and well-documented: the validation-layer version (1.4.350.0) was identical across the pinned and floating nixpkgs, so layer drift was not the differentiator. The actual regression was the SDL3 bump (3.4.10 → 3.4.12) that the floated devenv.lock pulled in, which surfaced 86 validation errors (0×0 viewport under headless weston). The fix is the devenv.yaml nixpkgs pin to 0bb7ec54… (devenv.yaml:9), which restores SDL3 3.4.10 — matching dev's last green integration-test (2026-07-08), which also used floating nixpkgs# validation layers. Keeping the layers floating therefore reproduces dev's known-good configuration exactly.

No new HIGH issues identified. (Caveat: this is an empirical claim that the in-progress integration-test and benchmark gates on f7f8888 must confirm — see Final Assessment.)

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous MEDIUM issue: devenv.nix:424-431 patchelf diagnostics. The best-effort rpath bake now captures stderr to a mktemp file (patchelf_log) and cats it on failure instead of redirecting to /dev/null. Retained in the head commit.

✅ [FIXED] Previous MEDIUM issue: build.yml artifact double-listing. "Upload Artifact" still uses path: dist/zigcraft-linux (the prepared copy, build.yml:160) rather than path: dist/, so the artifact no longer contains both dist/bin/zigcraft and dist/zigcraft-linux. Retained in the head commit.

No new MEDIUM issues identified.

ℹ️ Low Priority Suggestions (Optional)

[LOW] .github/actions/start-weston/action.yml:49-50 - Redundant profile realization (carried over; now documented)
Confidence: Low
Description: The action runs devenv shell --profile ... -- true purely to realize the profile before launching weston, re-evaluating the devenv config (~5–15s per graphics job). The intent (surface realization failures distinctly from weston startup failures) is now documented with an explanatory echo.
Suggested Fix: None required; intent is documented. Drop the warmup if future profiling shows it dominates cold-start time.

[LOW] devenv.nix:20 - nix_runtime_library_path includes pkgs.mesa unconditionally (carried over)
Confidence: Medium
Description: The runtime library path always includes pkgs.mesa, but the unit profile (used by zigcraft:build and unit tests) does not include mesa in its packages. Ported verbatim from the old flake ci-unit shell, so behavior is preserved; the makeLibraryPath entry is harmless when the lib isn't loaded.
Suggested Fix: Either add pkgs.mesa to the unit profile's packages (so the path is real) or make the runtime path computation profile-aware. Not blocking.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	7/10	devenv.nix mixes derivations, env, task, and profiles, but this is the canonical devenv layout and concerns are clearly sectioned.
Open/Closed	8/10	Additive profiles.{default,unit,graphics} design makes adding new profiles trivial without touching the base.
Liskov Substitution	7/10	Profiles compose predictably on top of the common foundation.
Interface Segregation	8/10	Three narrow profiles cleanly segregate CPU-only vs graphics vs full local dev.
Dependency Inversion	7/10	Single nixpkgs pin via devenv.yaml/devenv.lock for all profile packages and the SDL3/vulkan-loader versions that govern correctness; ad-hoc Lavapipe fetch intentionally stays floating to match the binary cache.
Average	7.4

🎯 Final Assessment

Overall Confidence Score: 80%

Confidence Breakdown:

• Code Quality: 85% (clean mechanical sweep, faithful verbatim port of derivations, well-commented Nix, MEDIUM fixes retained)
• Completeness: 80% (migration functionally complete; previous CRITICAL cleanly reverted with sound root-cause analysis; pending only empirical CI confirmation)
• Risk Level: 75% (configuration now matches dev's known-good 2026-07-08 green state; the in-progress integration-test/benchmark gates are the final confirmation that SDL3 — not layer version — was the regression)
• Test Coverage: 80% (existing CI is the verification surface; fmt/build/changes already green on f7f8888; integration/benchmark running)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• [~] Tests present and passing — fmt/build/changes green; integration-test/benchmark gates still in_progress on f7f8888 at review time and must be confirmed green as the merge condition

Verdict:

MERGE — The previously-reported CRITICAL (broken Lavapipe pinning) is cleanly resolved by reverting to floating nixpkgs# resolution, the previously-fixed MEDIUMs are retained, and the configuration now matches dev's last known-good integration state (pinned SDL3 3.4.10 + floating validation layers); confirm the in-progress integration-test and benchmark gates are green on f7f8888 as the merge condition.

Machine Readable Verdict

{
  "reviewed_sha": "f7f888893a6a49fbb32b58e302619dda14e9616f",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 80,
  "recommendation": "MERGE"
}

opencode session  |  github run