fix: Resolve Docker container HOME environment and add cross-compilation support

[jamesbrink]

Summary

• Fix $HOME not being set in Docker containers causing //.config error
• Add HOME=/root to both CPU and CUDA Docker image environments
• Update config.sh to use COMFY_USER_DIR with proper fallbacks: ${COMFY_USER_DIR:-${HOME:-/root}/.config/comfy-ui}
• Add --cpu flag to CPU Docker image to prevent CUDA initialization crash
• Refactor flake.nix with mkComfyPackages function for reusability
• Add cross-compilation support for building Linux Docker images from macOS
• Add buildDockerLinux and buildDockerLinuxCuda apps for macOS users
• Remove outdated persistence warning from README

Test plan

• Built aarch64-linux Docker image from macOS using remote NixOS builder
• Verified Docker container starts without $HOME error
• Verified CPU image runs with --cpu flag (no CUDA assertion error)
• CI builds Docker images successfully

[claude]

Code Review - PR #10

Thank you for this comprehensive PR! This addresses several important issues and adds valuable cross-compilation support. Here's my detailed review:

---

✅ Strengths

1. Docker HOME Environment Fix

The addition of HOME=/root to Docker environments (lines 205, 279 in flake.nix) properly fixes the //.config error. This is a critical fix that prevents crashes on container startup.

2. Robust Path Handling in config.sh

The COMFY_USER_DIR fallback chain is well-designed:

BASE_DIR="${COMFY_USER_DIR:-${HOME:-/root}/.config/comfy-ui}"

This ensures the application works even when HOME is unset (line 99).

3. CPU Flag for Docker Image

Adding --cpu flag to the CPU Docker image (line 202) prevents CUDA initialization crashes when running on CPU-only systems. Smart defensive programming.

4. Excellent Refactoring with mkComfyPackages

The introduction of the mkComfyPackages function (lines 24-323) is excellent architecture:

• Eliminates code duplication between native and cross-compiled builds
• Makes cross-compilation straightforward
• Improves maintainability significantly
• The forDocker parameter shows forward-thinking design

5. Cross-Compilation Support

The macOS → Linux Docker build support is well-implemented:

• Proper architecture mapping (lines 326-333)
• Conditional app exports (lines 563-613)
• Clear documentation in app descriptions

---

⚠️ Concerns & Suggestions

1. Security: Path Validation in config.sh

Issue: The path validation logic has comprehensive checks, but there's a subtle security consideration.

Lines 112-161: The symlink resolution is good, but consider this edge case:

# What if HOME itself is a symlink to /etc?
export HOME=/tmp/symlink-to-etc

Recommendation: Also validate the final resolved CODE_DIR and COMFY_VENV paths, not just BASE_DIR:

# After line 165
_validate_base_dir "$CODE_DIR"
_validate_base_dir "$COMFY_VENV"

2. Documentation Files (.codex, AGENTS.md)

Issue: The PR adds 5 new documentation files in .codex/prompts/ and AGENTS.md (552 lines total). These appear to be workflow documentation for "deciduous" decision graph tooling.

Concerns:

• These files are not mentioned in the PR description
• They appear to be project-agnostic AI agent instructions
• They add significant maintenance overhead (e.g., build-test.md references Rust/Cargo, but this is a Python/Nix project)
• The .gitignore now excludes .codex/* except prompts (lines 60-63)

Questions:

1. Are these files intended for this repository, or were they accidentally included?
2. If intentional, should they reference this project's actual build commands (Nix) instead of Cargo?
3. Should AGENTS.md content be merged into CLAUDE.md to avoid duplication?

Recommendation: Either:

• Remove these files if they're not essential to the PR's core functionality, OR
• Update them to reference this project's actual tooling (Nix, not Cargo)
• Explain their purpose in the PR description

3. Missing Test Verification

Test Plan Checklist:

• Built aarch64-linux Docker from macOS
• Container starts without $HOME error
• CPU image runs with --cpu flag
• CI builds Docker images successfully ← Still pending

Recommendation: Verify CI passes before merging, especially the new cross-compilation workflows.

4. Potential Performance Impact

Issue: Cross-compilation from macOS requires either:

• A remote Linux builder (as mentioned in test plan), OR
• QEMU emulation (very slow for large builds)

Line 354-365: The linuxPkgs import doesn't specify how cross-compilation should occur.

Recommendation: Add a comment in flake.nix explaining prerequisites:

# For macOS users: Cross-compilation requires either:
# 1. A remote Linux builder configured in nix.conf, OR
# 2. QEMU emulation (slow but works without additional setup)
linuxPkgs = import nixpkgs {

5. Code Style: Inconsistent Comments

Before (old flake.nix): Verbose comments explaining every section
After (new flake.nix): Most comments removed

Example: Lines 170-242 (dockerImage) now have minimal comments compared to the original.

While the code is cleaner, some context was lost. Consider restoring key architectural comments.

---

🐛 Potential Bugs

1. Docker Image Tag Collision

Issue: Both native and cross-compiled Linux images use the same tag comfy-ui:latest (lines 173, 388).

If a macOS user runs:

nix run .#buildDocker        # Builds darwin image, tagged 'latest'
nix run .#buildDockerLinux   # Builds linux image, ALSO tagged 'latest'

The second build overwrites the first.

Recommendation: Use different tags for native vs cross-compiled:

dockerImage = pkgs.dockerTools.buildImage {
  name = "comfy-ui";
  tag = if forDocker then "latest" else "latest-${pkgs.system}";

2. Unused forDocker Parameter

Issue: The forDocker parameter is declared (line 27) but never used in the function body.

Recommendation: Either use it (as suggested above for tagging) or remove it.

---

🧪 Testing Recommendations

1. Test the COMFY_USER_DIR fallback chain:

   # Test 1: All vars set
   HOME=/tmp/test COMFY_USER_DIR=/tmp/custom nix run
   
   # Test 2: HOME unset (Docker scenario)
   env -u HOME COMFY_USER_DIR=/data nix run
   
   # Test 3: Both unset (should use /root)
   env -u HOME -u COMFY_USER_DIR nix run

2. Verify cross-compilation (if you have a Linux machine):

   # On macOS, build Linux image
   nix build .#dockerImageLinux
   
   # Copy to Linux machine and test
   docker load < result
   docker run comfy-ui:latest

3. Test symlink attack prevention:

   ln -s /etc /tmp/evil
   nix run -- --base-directory=/tmp/evil  # Should fail with error

---

📊 Metrics

• Lines changed: +915 / -265 (net +650)
• Files changed: 10
• Documentation added: ~550 lines (mostly .codex files)
• Core functionality: ~100 lines of critical fixes

---

🎯 Overall Assessment

Core Changes: ⭐⭐⭐⭐⭐ (5/5)

• The Docker HOME fix, CPU flag, and cross-compilation support are excellent
• The mkComfyPackages refactor is clean and maintainable

Documentation: ⚠️ (Needs clarification)

• The .codex files need explanation or removal
• Consider updating them to match this project's tooling

Security: ⭐⭐⭐⭐ (4/5)

• Good path validation, minor improvement suggested

Testing: ⚠️ (Pending CI verification)

• Manual testing looks good, waiting on CI

---

✅ Approval Recommendation

Conditional approval pending:

1. Clarification on .codex/AGENTS.md files (remove or update?)
2. CI passing successfully
3. Consider fixing the Docker tag collision issue

The core functionality is solid and addresses real issues. The documentation files are the main question mark.

---

💡 Minor Suggestions

1. README.md: The removed persistence warning is good - consider adding a note about the Docker HOME fix in release notes
2. flake.nix line 99: Consider extracting libPath to a variable at the function level for reusability
3. Code deduplication: The healthcheck config is identical in both Docker images - consider extracting to a shared variable

---

Great work overall! The architectural improvements are particularly impressive. Looking forward to seeing this merged once the documentation questions are resolved.