feat: Add proper --base-directory support for custom data locations

[jamesbrink]

Summary

• Parse --base-directory in bash config before setting up paths
• Update Python persistence module to respect user-specified base directory
• Keep app code and venv fixed in ~/.config/comfy-ui/ (managed by nix)
• Allow all data (models, input, output, custom_nodes) to use custom location
• Update README with --base-directory documentation and examples

This enables storing all ComfyUI data on a separate drive (e.g., ZFS volume) while keeping the app installation in a fixed location.

Architecture

Fixed locations (always in ~/.config/comfy-ui/):

• app/ - ComfyUI source code
• venv/ - Python virtual environment

User-configurable via --base-directory:

• models/ - all model files
• input/ - input files
• output/ - generated outputs
• user/ - workflows and settings
• custom_nodes/ - extensions

Test plan

• Run with default settings (no --base-directory)
• Run with --base-directory ~/AI and verify models download to correct location
• Verify model downloader respects base directory setting

🤖 Generated with Claude Code

[claude]

Code Review: Add proper --base-directory support

Thank you for this PR! This is a well-architected feature that cleanly separates app code from user data. Here's my detailed review:

✅ Strengths

1. Clean separation of concerns: Keeping app/ and venv/ fixed while making data directories configurable is an excellent design choice
2. Consistent implementation: The argument parsing is handled in both bash (config.sh) and Python (persistence.py), ensuring consistency
3. Good backwards compatibility: Falls back gracefully to COMFY_USER_DIR env var and default location
4. Documentation: README updates clearly explain the new feature with examples

🐛 Potential Issues

1. Argument Parsing Duplication and Race Condition (scripts/config.sh:38-57)

The bash argument parsing logic is duplicated (lines 38-47 and 49-57), which could lead to maintenance issues. More importantly, there's a potential race condition:

• Lines 38-47: Parse --base-directory=value format
• Lines 49-57: Parse --base-directory value format (space-separated)

If a user provides --base-directory=~/AI, the first loop sets BASE_DIR. Then the second loop might incorrectly match if there's another --base-directory argument.

Recommendation: Consolidate into a single loop:

BASE_DIR="$HOME/.config/comfy-ui"
skip_next=false
for i in "${!_args[@]}"; do
  if [[ "$skip_next" == "true" ]]; then
    skip_next=false
    continue
  fi
  
  case "${_args[$i]}" in
    --base-directory=*)
      BASE_DIR="${_args[$i]#*=}"
      BASE_DIR="${BASE_DIR/#\~\/$HOME}"
      ;;
    --base-directory)
      if [[ $((i+1)) -lt ${#_args[@]} ]]; then
        BASE_DIR="${_args[$((i+1))]}"
        BASE_DIR="${BASE_DIR/#\~\/$HOME}"
        skip_next=true
      fi
      ;;
  esac
done

2. Incomplete Argument Filtering (scripts/config.sh:112-133)

The parse_arguments() function doesn't filter out --base-directory from the ARGS array that gets passed to ComfyUI. This means ComfyUI will receive an unknown argument it doesn't recognize.

Recommendation: Add handling in parse_arguments():

parse_arguments() {
  ARGS=()
  skip_next=false
  for arg in "$@"; do
    if [[ "$skip_next" == "true" ]]; then
      skip_next=false
      continue
    fi
    
    case "$arg" in
      --open)
        OPEN_BROWSER=true
        ;;
      --port=*)
        COMFY_PORT="${arg#*=}"
        ;;
      --base-directory=*)
        # Already handled in config, skip
        ;;
      --base-directory)
        # Skip this and next arg
        skip_next=true
        ;;
      --debug)
        export LOG_LEVEL=$DEBUG
        ;;
      --verbose)
        export LOG_LEVEL=$DEBUG
        ;;
      *)
        ARGS+=("$arg")
        ;;
    esac
  done
}

3. Environment Variable Inconsistency (src/persistence/persistence.py:210-226)

The Python code reads --base-directory from sys.argv, but the environment variable COMFY_USER_DIR is set in config.sh (line 72) to BASE_DIR. This creates a potential inconsistency:

• If user passes --base-directory ~/AI, both bash and Python will see it
• However, COMFY_USER_DIR is also set to BASE_DIR in bash
• The Python code checks --base-directory first, then falls back to COMFY_USER_DIR

This works, but the logic could be cleaner. Since bash already parses the argument and sets COMFY_USER_DIR, the Python code could just trust the environment variable.

Recommendation: Either:

• Remove the --base-directory parsing from Python and just use COMFY_USER_DIR (simpler)
• OR document that both must parse it independently (current approach is fine, just add a comment explaining why)

4. Missing Validation (both files)

Neither bash nor Python validates that:

• The base directory path is valid
• The user has write permissions to the directory
• There's sufficient disk space

Recommendation: Add basic validation in config.sh:

# After parsing BASE_DIR
if [[ ! -d "$(dirname "$BASE_DIR")" ]]; then
  echo "ERROR: Parent directory of BASE_DIR does not exist: $(dirname "$BASE_DIR")"
  exit 1
fi

5. Path Expansion Edge Cases (scripts/config.sh:45, 54)

The tilde expansion ${BASE_DIR/#\~\/$HOME} only handles ~/ but not:

• ~username/path (other users' home directories)
• ~ alone (without trailing slash)

Recommendation: Use eval echo or just rely on the later expansion:

BASE_DIR=$(eval echo "$BASE_DIR")  # Handles all tilde forms

Or simply:

BASE_DIR="${BASE_DIR/#\~/$HOME}"  # Without the slash requirement

🔒 Security Considerations

1. Path Traversal: No validation prevents users from providing paths like --base-directory ../../../../etc. While this is a local application and users can only harm their own system, it's worth documenting that users should provide absolute paths or paths within their home directory.

2. Symlink Attacks: The symlink creation in persistence.py doesn't validate that the target isn't a symlink to a sensitive location. This is probably acceptable for a local application, but worth noting.

⚡ Performance Considerations

No significant performance concerns. The argument parsing happens once at startup and is negligible.

🧪 Test Coverage

Missing tests: There are no automated tests for this feature. Consider adding:

1. Bash tests for config.sh argument parsing:

   • --base-directory=~/AI format
   • --base-directory ~/AI format
   • Tilde expansion works correctly
   • Default value when not specified

2. Python tests for persistence.py:

   • Respects --base-directory in sys.argv
   • Falls back to COMFY_USER_DIR
   • Creates correct directory structure

3. Integration test:

   • Run with --base-directory and verify models are downloaded to correct location

📝 Code Style

Good:

• Follows bash style guide (function-based, error handling)
• Python follows PEP 8
• Clear comments and logging

Minor issues:

• Lines 38-57 in config.sh could be consolidated (see issue Fails to start #1)
• Consider extracting argument parsing into a separate function in config.sh

💡 Suggestions

1. Update CLAUDE.md: The PR updates README.md but should also update CLAUDE.md line 66 which currently says:

   COMFY_USER_DIR: Persistent storage directory (default: ~/.config/comfy-ui)

   Should mention --base-directory as the preferred method.

2. Environment variable deprecation: Consider documenting that --base-directory is now the preferred method and COMFY_USER_DIR is for backwards compatibility.

3. Help text: The README documents the flag, but consider if ComfyUI's --help output should also show it (if you control that).

✅ Summary

This is a solid feature with good architecture. The main issues are:

1. Must fix: Argument parsing duplication and the missing filtering in parse_arguments()
2. Should fix: Path validation and better tilde expansion
3. Nice to have: Tests, updated CLAUDE.md, validation

The current implementation will work but could have edge case issues with argument parsing. I recommend addressing issues #1 and #2 before merging.

Overall assessment: Approve with requested changes ✅

---

Review generated with Claude Code

[claude]

Code Review: Add --base-directory Support

Overview

This PR successfully adds --base-directory support to allow users to store ComfyUI data (models, inputs, outputs, custom_nodes) in a custom location while keeping the app code and venv in a fixed location. The implementation is well-thought-out and follows the project's modular architecture.

✅ Strengths

1. Clean Architecture: Separating fixed locations (app/, venv/) from configurable data locations is a solid design decision
2. Dual Parsing Documentation: The comment in persistence.py:211-214 excellently explains why --base-directory is parsed in both bash and Python
3. Proper Argument Filtering: The bash script correctly filters out --base-directory before passing args to ComfyUI
4. Tilde Expansion: Handles ~ expansion correctly in bash using ${BASE_DIR/#\~/$HOME}
5. Validation: Parent directory validation (line 64-68 in config.sh) prevents cryptic errors

🔍 Code Quality Issues

1. Inconsistent Argument Parsing Logic (Medium Priority)

Location: scripts/config.sh:139-145 and scripts/config.sh:46-59

The argument parsing logic is duplicated in two places with slightly different implementations:

• Early parsing (lines 39-61): Uses array iteration with _skip_next
• parse_arguments() function (lines 124-157): Uses the same pattern but in a function

Issue: Both handle --base-directory=value and --base-directory value forms, but the duplication means bugs fixed in one place might not be fixed in the other.

Recommendation: Consider extracting the base-directory parsing into a dedicated function called from both places:

# Extract base directory from arguments
extract_base_directory() {
  local result="$HOME/.config/comfy-ui"
  local skip_next=false
  for i in "${!@}"; do
    # ... parsing logic
  done
  echo "$result"
}
BASE_DIR=$(extract_base_directory "$@")

2. Argument Parsing Edge Case (Low Priority)

Location: src/persistence/persistence.py:216-223

The Python parsing doesn't handle the --base-directory=value form, only --base-directory value.

Current code:

if "--base-directory" in sys.argv:
    idx = sys.argv.index("--base-directory")
    if idx + 1 < len(sys.argv):
        base_dir = os.path.expanduser(sys.argv[idx + 1])

Issue: If a user passes --base-directory=/path/to/dir, this won't be recognized by the Python code.

Recommendation: Add support for both forms:

base_dir = None
for i, arg in enumerate(sys.argv):
    if arg == "--base-directory" and i + 1 < len(sys.argv):
        base_dir = os.path.expanduser(sys.argv[i + 1])
        break
    elif arg.startswith("--base-directory="):
        base_dir = os.path.expanduser(arg.split("=", 1)[1])
        break

3. Error Handling Gap (Medium Priority)

Location: scripts/config.sh:64-68

The validation only checks if the parent directory exists, but doesn't verify write permissions.

Issue: Users might get cryptic errors later when the script tries to create directories.

Recommendation: Add permission check:

if [[ ! -d "$(dirname "$BASE_DIR")" ]]; then
  echo "ERROR: Parent directory of BASE_DIR does not exist: $(dirname "$BASE_DIR")" >&2
  echo "Please create it first or specify a valid path with --base-directory" >&2
  exit 1
elif [[ ! -w "$(dirname "$BASE_DIR")" ]]; then
  echo "ERROR: No write permission for parent directory: $(dirname "$BASE_DIR")" >&2
  exit 1
fi

🛡️ Security Considerations

1. Path Traversal: ✅ The code uses proper path expansion and doesn't allow arbitrary path injection
2. Symlink Handling: ✅ The Python code properly handles existing symlinks before creating new ones (persistence.py:184-190)
3. Environment Variable Injection: ✅ The COMFY_USER_DIR is properly quoted and escaped

No security vulnerabilities identified.

⚡ Performance Considerations

1. Array Iteration: The argument parsing iterates through all arguments twice (once for BASE_DIR extraction, once in parse_arguments). For typical command lines this is negligible, but consider combining them if performance becomes an issue.
2. Directory Creation: The persistence module creates symlinks lazily, which is good for performance.

Overall performance impact: Minimal

🧪 Test Coverage

Major Gap: No automated tests for the new functionality.

Recommendations:

1. Add bash unit tests for argument parsing edge cases:

   • --base-directory=~/path
   • --base-directory ~/path
   • --base-directory "/path/with spaces"
   • Missing parent directory
   • No write permissions

2. Add Python tests for persistence module:

   • Verify correct directory resolution with --base-directory
   • Test symlink creation when directories exist
   • Test migration of existing data

Test Plan Status: The PR includes a manual test plan but no automated tests. This is acceptable for the initial implementation but should be addressed in a follow-up.

📝 Documentation

Strengths:

• README.md clearly documents the new option
• CLAUDE.md is properly updated with the new architecture
• Inline comments explain the dual parsing rationale

Minor Issue: The README example shows ~/AI but doesn't mention path expansion behavior for paths with spaces or special characters.

Recommendation: Add a note:

**Note**: Paths with spaces should be quoted: `--base-directory "/path/with spaces"`

🎯 Overall Assessment

Code Quality: 8/10

• Well-structured, follows project conventions
• Good separation of concerns
• Minor issues with code duplication and edge cases

Architecture: 9/10

• Clean separation between fixed and configurable locations
• Proper layering between bash and Python

Completeness: 7/10

• Core functionality is solid
• Missing automated tests
• Some edge cases not fully handled

📋 Recommendations

Before Merge:

1. ⚠️ Fix Python argument parsing to handle --base-directory=value form (consistency)

Nice to Have:

1. Add write permission validation
2. Extract base-directory parsing logic to reduce duplication
3. Add documentation about quoting paths with spaces

Follow-up PRs:

1. Add automated tests for argument parsing
2. Add integration tests for persistence module

✅ Approval

Despite the minor issues noted above, this PR:

• Implements the feature correctly
• Follows project conventions
• Has clear documentation
• Introduces no security vulnerabilities
• Has minimal performance impact

Recommendation: Approve with suggestions for follow-up improvements.

The identified issues are minor and don't block the core functionality. They can be addressed in follow-up PRs or before merge at the maintainer's discretion.

---

Review generated by Claude Code

[claude]

Code Review: Add proper --base-directory support

Thank you for this PR! This is a well-structured enhancement that cleanly separates app code from user data. Overall, the implementation is solid, but I've identified several issues that should be addressed before merging.

---

Critical Issues

1. Argument Parsing Race Condition (scripts/config.sh:39-61, persistence.py:216-226)

The same --base-directory argument is parsed twice - once in bash and once in Python. This creates a potential inconsistency:

• Bash parses it early in config.sh to set up paths
• Python re-parses sys.argv to get the same value

Problem: If bash and Python disagree on parsing (edge cases with quotes, spaces, etc.), they'll use different directories.

Recommendation:

• Let bash handle the parsing and pass the value to Python via COMFY_USER_DIR environment variable (which already exists)
• Remove the sys.argv parsing in Python (lines 216-226)
• Python should trust the environment variable set by bash

Example fix for persistence.py:210-232:

# Use COMFY_USER_DIR set by bash, which already parsed --base-directory
base_dir = os.environ.get(
    "COMFY_USER_DIR", os.path.join(os.path.expanduser("~"), ".config", "comfy-ui")
)
logger.info("Using persistent directory: %s", base_dir)

The comment on lines 211-214 acknowledges the duplication but doesn't justify why it's necessary. If bash sets COMFY_USER_DIR, Python doesn't need to re-parse.

---

2. Parent Directory Validation Timing (scripts/config.sh:63-73)

The validation happens before the script sources logger.sh, so error messages bypass the logging system.

Current behavior:

echo "ERROR: Parent directory of BASE_DIR does not exist: $_parent_dir" >&2

Issue: Inconsistent with the project's logging conventions (see CLAUDE.md: "Use the logging functions from logger.sh instead of direct echo statements")

Recommendation:

• Move validation to after logger.sh is sourced, or
• Add a fallback that uses direct echo with a comment explaining why

---

3. Unhandled Edge Case: Relative Paths (scripts/config.sh:48-55, persistence.py:219-225)

The tilde expansion is good, but relative paths are not converted to absolute paths.

Example:

nix run -- --base-directory ./data

This will work initially but could cause issues if the working directory changes or in symlink resolution.

Recommendation: Use realpath to canonicalize the path:

BASE_DIR="$(realpath -m "${_args[]#*=}")"

Note: realpath -m allows the path to not exist yet (important for first run).

---

Security Concerns

4. Path Traversal via User Input (scripts/config.sh:47-58)

User-supplied paths are accepted without sanitization. While directory creation is safe, symlinks could be created pointing outside expected boundaries.

Attack scenario:

nix run -- --base-directory "../../../../etc"

Mitigation:

• Validate that the final path doesn't contain suspicious patterns
• Consider restricting to user's home directory or specific mount points
• At minimum, document the security implications

---

5. sys.argv Injection in Python (persistence.py:290-291)

The code appends to sys.argv if --base-directory isn't present:

if "--base-directory" not in sys.argv:
    sys.argv.extend(["--base-directory", base_dir])

Issues:

1. The check "--base-directory" not in sys.argv is a substring search, not argument parsing. It would match --base-directory-foo or even a value containing that string.
2. Modifying sys.argv after startup can confuse argument parsers and is generally an anti-pattern
3. This seems to be working around the dual parsing issue mentioned earlier

Recommendation: Remove this entirely. If ComfyUI needs the argument, pass it through properly. If not, use environment variables.

---

Code Quality Issues

6. ShellCheck Warnings (scripts/config.sh:120, 122)

SC2034: BASE_PACKAGES appears unused
SC2034: ADDITIONAL_PACKAGES appears unused

These are existing issues, not introduced by this PR, but worth noting. These arrays should be exported or the warnings suppressed with comments.

---

7. Inconsistent Variable Scope (scripts/config.sh:131)

local skip_next=false

This uses local properly, but the earlier parsing loop (lines 39-61) uses module-level variables with _ prefix.

Recommendation: Be consistent - either use local in both places (preferred) or document why one needs module scope.

---

8. Error Handling Gap (scripts/config.sh:53-57)

The code checks if i+1 is within bounds but doesn't validate the next argument isn't another flag:

nix run -- --base-directory --open

This would set BASE_DIR="--open", which then gets created as a directory.

Recommendation: Validate that the next argument doesn't start with --:

if [[ $((i+1)) -lt ${#_args[@]} ]] && [[ ! "${_args[$((i+1))]}" =~ ^-- ]]; then

---

Documentation & Testing

9. Missing Test Plan Items

The PR description includes a test plan, but it's not checked off. Before merging:

• Verify behavior with default settings
• Test with custom --base-directory
• Test model downloader integration
• Test edge cases: paths with spaces, special chars, tilde, relative paths
• Test error cases: non-existent parent, no permissions

10. Documentation Quality

✅ Good: README examples are clear and practical
✅ Good: CLAUDE.md accurately reflects the new architecture
⚠️ Missing: No mention of security implications for user-supplied paths
⚠️ Missing: No guidance on what characters are safe in paths

---

Performance Considerations

11. Redundant Path Expansion

Both bash and Python expand tildes independently. Minor overhead, but unnecessary.

---

Positive Aspects

✅ Clean separation of app code (~/.config/comfy-ui/app) from user data
✅ Backward compatible (defaults work as before)
✅ Well-documented in both README and CLAUDE.md
✅ Follows the project's modular script architecture
✅ Proper use of quote handling in argument parsing

---

Summary

Recommendation: Request changes before merging.

Priority fixes:

1. Remove duplicate argument parsing (Critical)
2. Handle relative paths correctly (High)
3. Validate next argument isn't a flag (High)
4. Remove sys.argv modification (Medium)
5. Use logging system for early errors (Medium)

Non-blocking improvements:

• Add path traversal validation
• Document security implications
• Complete test plan
• Fix variable scoping consistency

The core architecture is sound, but the implementation has several edge cases that could cause subtle bugs. Once the critical issues are addressed, this will be a solid enhancement.

---

Let me know if you'd like help with any of these fixes!

[claude]

Code Review: Support

Thank you for this PR! This is a well-thought-out feature that enables users to store ComfyUI data on separate drives. I've reviewed the changes and have several observations across code quality, security, potential bugs, and testing.

---

✅ Strengths

1. Clean Architecture

• Clear separation between fixed locations (~/.config/comfy-ui/app, venv) and user-configurable data
• Consistent handling across bash and Python layers
• Good use of environment variables to pass configuration from bash to Python

2. Documentation

• Excellent updates to CLAUDE.md and README.md
• Clear examples and use cases
• Well-documented architecture decisions

3. Security Considerations

• Path validation in _validate_base_dir() prevents path traversal attacks
• Parent directory existence and writability checks

---

🔴 Critical Issues

1. Race Condition in Argument Parsing (config.sh:82-86)

Issue: The _parse_base_directory function is called during config.sh sourcing, but the arguments $@ may not be available yet depending on how the script is sourced.

# Current code
BASE_DIR="$HOME/.config/comfy-ui"
_parsed_base_dir="$(_parse_base_directory "$@")"
if [[ -n "$_parsed_base_dir" ]]; then
  BASE_DIR="$_parsed_base_dir"
fi

Problem: When config.sh is sourced from launcher.sh:22, the $@ refers to the arguments passed to the source command (none), not the original script arguments. The actual parsing happens later in parse_arguments() (line 175).

Impact: The --base-directory argument is never actually parsed early, defeating the purpose of this early parsing logic. BASE_DIR will always be the default value when directories are created.

Fix: Pass arguments explicitly when sourcing:

# In launcher.sh
source "$SCRIPT_DIR/config.sh" "$@"

# Or parse arguments before sourcing config.sh
parse_base_dir_early "$@"
source "$SCRIPT_DIR/config.sh"

---

2. Symlink Creation Before Base Directory Validation

Issue: The directory creation in DIRECTORIES associative array (lines 149-161) uses $BASE_DIR, but these directories are created in install.sh without verifying that BASE_DIR was properly set from command-line arguments.

Impact: If the race condition above causes BASE_DIR to use the default value, users who specify --base-directory ~/AI will have directories created in ~/.config/comfy-ui/ instead of ~/AI/, silently ignoring their configuration.

Test: Run nix run -- --base-directory /tmp/test-comfy and check where directories are actually created.

---

🟡 High Priority Issues

3. Path Validation Too Restrictive (config.sh:90-104)

Issue: The validation only allows paths starting with specific prefixes, which may block legitimate use cases.

local allowed_prefixes=("$home_dir" "/tmp" "/var/tmp" "/data" "/mnt" "/media" "/run/media")

Problems:

• Blocks custom mount points like /storage, /ai-models, /scratch, etc.
• Blocks NFS/CIFS mounts that might be in non-standard locations
• The check [[ "$dir" == "$prefix"* ]] is fragile (string prefix matching instead of path canonicalization)

Recommendation:
Instead of allowlisting paths, blocklist dangerous system directories:

_validate_base_dir() {
  local dir="$1"
  local blocked_prefixes=("/etc" "/bin" "/sbin" "/usr" "/lib" "/lib64" "/boot" "/sys" "/proc" "/dev")
  
  for prefix in "${blocked_prefixes[@]}"; do
    if [[ "$dir" == "$prefix"* ]]; then
      echo "ERROR: --base-directory cannot be in system directory: $prefix" >&2
      exit 1
    fi
  done
  
  # Require absolute path
  if [[ "$dir" \!= /* ]]; then
    echo "ERROR: --base-directory must be an absolute path" >&2
    exit 1
  fi
}

---

4. Missing Error Handling in Path Expansion (config.sh:76)

Issue: realpath -m may fail on some systems or with certain paths, and the fallback silently uses the raw path.

realpath -m "$raw_path" 2>/dev/null || echo "$raw_path"

Problems:

• If realpath isn't available (unlikely but possible in minimal environments), errors are silently suppressed
• The fallback echo "$raw_path" doesn't canonicalize the path (e.g., ~/AI/../AI won't be normalized)

Recommendation:

# Expand tilde and resolve path
expanded_path="${raw_path/#\~//home/runner}"
if command -v realpath &>/dev/null; then
  realpath -m "$expanded_path" 2>/dev/null || {
    echo "ERROR: Invalid path: $raw_path" >&2
    exit 1
  }
else
  # Fallback: at least make it absolute
  cd -P "$(dirname "$expanded_path")" 2>/dev/null && echo "$PWD/$(basename "$expanded_path")" || {
    echo "ERROR: Cannot resolve path: $raw_path" >&2
    exit 1
  }
fi

---

5. Inconsistent Argument Handling (config.sh:175-208)

Issue: The parse_arguments() function filters out --base-directory but it's never actually used since BASE_DIR was already set during config.sh sourcing.

Current flow:

1. config.sh is sourced → _parse_base_directory("$@") called with no args → BASE_DIR = default
2. parse_arguments("$@") called → --base-directory is filtered out (but BASE_DIR already set)

Result: The filtering logic is dead code, and BASE_DIR is never set from command-line arguments.

---

🟠 Medium Priority Issues

6. Python Environment Variable Trust (persistence.py:213-215)

Issue: The Python code blindly trusts COMFY_USER_DIR without validation.

base_dir = os.environ.get(
    "COMFY_USER_DIR", os.path.join(os.path.expanduser("~"), ".config", "comfy-ui")
)

Problems:

• If bash validation is bypassed (e.g., someone calls Python directly), unsafe paths could be used
• No verification that the directory is accessible or writable
• The comment says "bash launcher parses --base-directory" but there's no runtime check that this actually happened

Recommendation:
Add basic validation in Python:

base_dir = os.environ.get(
    "COMFY_USER_DIR", os.path.join(os.path.expanduser("~"), ".config", "comfy-ui")
)

# Validate path
if not os.path.isabs(base_dir):
    logger.error("base_dir must be an absolute path: %s", base_dir)
    raise ValueError(f"Invalid base_dir: {base_dir}")

# Basic security check
blocked_dirs = ["/etc", "/bin", "/sbin", "/usr", "/lib", "/boot", "/sys", "/proc", "/dev"]
if any(base_dir.startswith(d) for d in blocked_dirs):
    logger.error("base_dir cannot be in system directory: %s", base_dir)
    raise ValueError(f"Unsafe base_dir: {base_dir}")

---

7. Quotes in Path Handling (config.sh:74-76)

Edge case: User provides --base-directory "/path/with spaces" with quotes included in the value.

# Current code
raw_path="${raw_path/#\~//home/runner}"
realpath -m "$raw_path" 2>/dev/null || echo "$raw_path"

Problem: If the user literally includes quotes in the argument (e.g., via improper escaping), they'll become part of the path.

Test case:

./launcher.sh --base-directory '"/tmp/test"'  # Quotes become part of path

Recommendation: Strip surrounding quotes:

# Remove surrounding quotes if present
raw_path=${raw_path%\"}
raw_path=${raw_path#\"}
raw_path=${raw_path%\'}
raw_path=${raw_path#\'}

---

8. Missing COMFY_APP_DIR Export (config.sh:213-227)

Issue: COMFY_APP_DIR is used in persistence.py:219 but isn't exported in export_config().

export_config() {
  export COMFY_VERSION COMFY_PORT BASE_DIR CODE_DIR COMFY_VENV
  # COMFY_APP_DIR is missing\!

Impact: Python falls back to heuristic detection (lines 221-233), which works but is fragile.

Fix:

export COMFY_VERSION COMFY_PORT BASE_DIR CODE_DIR COMFY_VENV COMFY_APP_DIR
# ... set COMFY_APP_DIR before export
COMFY_APP_DIR="$CODE_DIR"

---

🟢 Low Priority / Suggestions

9. Shellcheck Warnings

The code has some shellcheck disables:

# shellcheck disable=SC2034  # Used in install.sh
BASE_PACKAGES=(pyyaml pillow numpy requests)

While the disables are correct, consider whether these variables could be used in the same file to avoid needing disables.

---

10. Test Coverage

Missing:

• No automated tests for argument parsing
• No tests for path edge cases (spaces, special characters, symlinks, relative paths)
• No validation that directories are created in the correct location

Recommendation:
Add a simple integration test:

#\!/usr/bin/env bash
# test_base_directory.sh

set -euo pipefail

# Test 1: Default location
tmpdir="$(mktemp -d)"
export HOME="$tmpdir"
./launcher.sh --help  # Ensure script runs
[[ -d "$tmpdir/.config/comfy-ui" ]] || { echo "FAIL: default directory not created"; exit 1; }

# Test 2: Custom location
custom_dir="$tmpdir/custom-ai"
./launcher.sh --base-directory "$custom_dir" --help
[[ -d "$custom_dir" ]] || { echo "FAIL: custom directory not created"; exit 1; }

# Test 3: Path with spaces
space_dir="$tmpdir/path with spaces"
./launcher.sh --base-directory "$space_dir" --help
[[ -d "$space_dir" ]] || { echo "FAIL: space directory not created"; exit 1; }

echo "All tests passed"
rm -rf "$tmpdir"

---

11. Documentation: COMFY_USER_DIR Deprecation

The README removes COMFY_USER_DIR documentation entirely, but the environment variable is still supported. Consider adding a deprecation notice:

### Deprecated

- `COMFY_USER_DIR`: Use `--base-directory` instead. This environment variable is still supported for backwards compatibility but may be removed in a future release.

---

🔧 Performance Considerations

The changes are minimal and don't introduce performance concerns. The argument parsing is O(n) in arguments, which is fine for CLI parsing.

---

🔒 Security Summary

Good:

• Path validation prevents obvious path traversal
• Parent directory checks prevent creation in invalid locations

Needs improvement:

• Allowlist is too restrictive (see issue Modernize nix-comfyui with ComfyUI v0.3.76 and flake improvements #3)
• Python layer lacks validation (see issue feat: Code quality overhaul and modernization #6)
• No protection against symlink attacks (e.g., --base-directory /tmp/evil-link → /etc)

Recommendation: Add symlink resolution and validation:

# Resolve symlinks in parent directory
_parent_dir="$(dirname "$BASE_DIR")"
if [[ -L "$_parent_dir" ]]; then
  _parent_dir="$(readlink -f "$_parent_dir")"
  BASE_DIR="$_parent_dir/$(basename "$BASE_DIR")"
fi

---

📋 Summary

Category	Count	Priority
Critical	2	🔴 Must fix before merge
High	3	🟡 Should fix before merge
Medium	3	🟠 Consider fixing
Low	3	🟢 Nice to have

Recommendation: Fix the critical race condition (#1 and #2) before merging. The current implementation likely doesn't work as intended due to argument parsing happening after BASE_DIR is already used.

---

🧪 Testing Checklist

Before merging, please verify:

• nix run -- --base-directory /tmp/test creates directories in /tmp/test (not default location)
• nix run -- --base-directory ~/AI works with tilde expansion
• nix run -- --base-directory "/path/with spaces" handles spaces correctly
• Models downloaded via model_downloader go to the custom base directory
• --base-directory=$PWD/relative/../path is properly canonicalized
• Error handling: --base-directory /etc is rejected with clear error message
• Error handling: --base-directory without argument shows helpful error
• Backwards compatibility: running without --base-directory still uses default location

---

Great work on this feature! The architecture is solid, but the argument parsing needs to be debugged to ensure it actually works at runtime. Happy to provide more detailed feedback on any of these points. 🚀

[claude]

Code Review: --base-directory Support

Thank you for this PR! This is a well-thought-out feature that enables users to store ComfyUI data on separate drives. I've reviewed the changes and have several observations across code quality, security, potential bugs, and testing.

---

✅ Strengths

1. Clean Architecture

• Clear separation between fixed locations (~/.config/comfy-ui/app, venv) and user-configurable data
• Consistent handling across bash and Python layers
• Good use of environment variables to pass configuration from bash to Python

2. Documentation

• Excellent updates to CLAUDE.md and README.md
• Clear examples and use cases
• Well-documented architecture decisions

3. Security Considerations

• Path validation in _validate_base_dir() prevents path traversal attacks
• Parent directory existence and writability checks

---

🔴 Critical Issues

1. Race Condition in Argument Parsing (config.sh:82-86)

Issue: The _parse_base_directory function is called during config.sh sourcing, but the arguments $@ may not be available yet depending on how the script is sourced.

# Current code at config.sh:82-86
BASE_DIR="$HOME/.config/comfy-ui"
_parsed_base_dir="$(_parse_base_directory "$@")"
if [[ -n "$_parsed_base_dir" ]]; then
  BASE_DIR="$_parsed_base_dir"
fi

Problem: When config.sh is sourced from launcher.sh:22, the $@ refers to the arguments passed to the source command (none), not the original script arguments. The actual parsing happens later in parse_arguments() (line 175).

Impact: The --base-directory argument is never actually parsed early, defeating the purpose of this early parsing logic. BASE_DIR will always be the default value when directories are created.

Fix: Pass arguments explicitly when sourcing:

# In launcher.sh
source "$SCRIPT_DIR/config.sh" "$@"

# Or parse arguments before sourcing config.sh
parse_base_dir_early "$@"
source "$SCRIPT_DIR/config.sh"

---

2. Symlink Creation Before Base Directory Validation

Issue: The directory creation in DIRECTORIES associative array (lines 149-161) uses $BASE_DIR, but these directories are created in install.sh without verifying that BASE_DIR was properly set from command-line arguments.

Impact: If the race condition above causes BASE_DIR to use the default value, users who specify --base-directory ~/AI will have directories created in ~/.config/comfy-ui/ instead of ~/AI/, silently ignoring their configuration.

Test: Run nix run -- --base-directory /tmp/test-comfy and check where directories are actually created.

---

🟡 High Priority Issues

3. Path Validation Too Restrictive (config.sh:90-104)

Issue: The validation only allows paths starting with specific prefixes, which may block legitimate use cases.

local allowed_prefixes=("$home_dir" "/tmp" "/var/tmp" "/data" "/mnt" "/media" "/run/media")

Problems:

• Blocks custom mount points like /storage, /ai-models, /scratch, etc.
• Blocks NFS/CIFS mounts that might be in non-standard locations
• The check [[ "$dir" == "$prefix"* ]] is fragile (string prefix matching instead of path canonicalization)

Recommendation:
Instead of allowlisting paths, blocklist dangerous system directories:

_validate_base_dir() {
  local dir="$1"
  local blocked_prefixes=("/etc" "/bin" "/sbin" "/usr" "/lib" "/lib64" "/boot" "/sys" "/proc" "/dev")
  
  for prefix in "${blocked_prefixes[@]}"; do
    if [[ "$dir" == "$prefix"* ]]; then
      echo "ERROR: --base-directory cannot be in system directory: $prefix" >&2
      exit 1
    fi
  done
  
  # Require absolute path
  if [[ "$dir" \!= /* ]]; then
    echo "ERROR: --base-directory must be an absolute path" >&2
    exit 1
  fi
}

---

4. Missing Error Handling in Path Expansion (config.sh:76)

Issue: realpath -m may fail on some systems or with certain paths, and the fallback silently uses the raw path.

realpath -m "$raw_path" 2>/dev/null || echo "$raw_path"

Problems:

• If realpath is not available (unlikely but possible in minimal environments), errors are silently suppressed
• The fallback echo "$raw_path" does not canonicalize the path (e.g., ~/AI/../AI will not be normalized)

Recommendation:

# Expand tilde and resolve path
expanded_path="${raw_path/#\~/$HOME}"
if command -v realpath &>/dev/null; then
  realpath -m "$expanded_path" 2>/dev/null || {
    echo "ERROR: Invalid path: $raw_path" >&2
    exit 1
  }
else
  # Fallback: at least make it absolute
  cd -P "$(dirname "$expanded_path")" 2>/dev/null && echo "$PWD/$(basename "$expanded_path")" || {
    echo "ERROR: Cannot resolve path: $raw_path" >&2
    exit 1
  }
fi

---

5. Inconsistent Argument Handling (config.sh:175-208)

Issue: The parse_arguments() function filters out --base-directory but it is never actually used since BASE_DIR was already set during config.sh sourcing.

Current flow:

1. config.sh is sourced → _parse_base_directory("$@") called with no args → BASE_DIR = default
2. parse_arguments("$@") called → --base-directory is filtered out (but BASE_DIR already set)

Result: The filtering logic is dead code, and BASE_DIR is never set from command-line arguments.

---

🟠 Medium Priority Issues

6. Python Environment Variable Trust (persistence.py:213-215)

Issue: The Python code blindly trusts COMFY_USER_DIR without validation.

base_dir = os.environ.get(
    "COMFY_USER_DIR", os.path.join(os.path.expanduser("~"), ".config", "comfy-ui")
)

Problems:

• If bash validation is bypassed (e.g., someone calls Python directly), unsafe paths could be used
• No verification that the directory is accessible or writable
• The comment says "bash launcher parses --base-directory" but there is no runtime check that this actually happened

Recommendation:
Add basic validation in Python:

base_dir = os.environ.get(
    "COMFY_USER_DIR", os.path.join(os.path.expanduser("~"), ".config", "comfy-ui")
)

# Validate path
if not os.path.isabs(base_dir):
    logger.error("base_dir must be an absolute path: %s", base_dir)
    raise ValueError(f"Invalid base_dir: {base_dir}")

# Basic security check
blocked_dirs = ["/etc", "/bin", "/sbin", "/usr", "/lib", "/boot", "/sys", "/proc", "/dev"]
if any(base_dir.startswith(d) for d in blocked_dirs):
    logger.error("base_dir cannot be in system directory: %s", base_dir)
    raise ValueError(f"Unsafe base_dir: {base_dir}")

---

7. Quotes in Path Handling (config.sh:74-76)

Edge case: User provides --base-directory "/path/with spaces" with quotes included in the value.

# Current code
raw_path="${raw_path/#\~/$HOME}"
realpath -m "$raw_path" 2>/dev/null || echo "$raw_path"

Problem: If the user literally includes quotes in the argument (e.g., via improper escaping), they will become part of the path.

Test case:

./launcher.sh --base-directory '/tmp/test'\

[claude]

Code Review: Add proper --base-directory support for custom data locations

I've reviewed PR #7 and overall this is a solid implementation with good security practices. Below is my detailed feedback:

✅ Strengths

1. Security-First Approach: Excellent defense-in-depth validation in both bash and Python

   • Path validation prevents dangerous system directory usage
   • Symlink resolution prevents symlink attacks (config.sh:132-137)
   • Blocklist approach allows flexibility while maintaining safety

2. Good Separation of Concerns: Fixed locations (~/.config/comfy-ui/app, venv) vs. user-configurable data directories is architecturally sound

3. Comprehensive Argument Parsing: Handles multiple formats (--base-directory=PATH, --base-directory PATH) and edge cases (quotes, tildes)

4. Documentation: CLAUDE.md and README updates are thorough and helpful

🐛 Potential Issues

1. Argument Parsing Duplication (Minor)

Location: scripts/config.sh:43-95 and scripts/config.sh:205-238

The --base-directory argument is parsed twice - once in _parse_base_directory() and again in parse_arguments(). While this works, it creates maintenance burden.

Suggestion: Consider consolidating or adding a comment explaining why both are needed (early parsing for path setup vs. filtering for ComfyUI args).

2. Missing /lib32 in Blocklist (Security)

Location: scripts/config.sh:109 and src/persistence/persistence.py:223

The blocklist includes /lib64 but not /lib32. On multilib systems, /lib32 is also a system directory.

Recommendation:

local blocked_prefixes=("/etc" "/bin" "/sbin" "/usr" "/lib" "/lib32" "/lib64" "/boot" "/sys" "/proc" "/dev" "/root")

3. Potential Race Condition (Low Priority)

Location: scripts/config.sh:139-146

Between checking if parent directory exists/is writable and actually creating the base directory, another process could modify permissions.

Impact: Low - this is unlikely in typical usage, but worth noting for multi-user systems.

4. realpath Fallback Could Be More Robust (Minor)

Location: scripts/config.sh:83-93

The fallback path resolution when realpath isn't available doesn't handle . or .. in paths:

# Input: ./foo/../bar
# Output: /current/dir/./foo/../bar (not canonicalized)

Suggestion: Add path normalization or document this limitation.

5. Python Path Validation Missing /root (Inconsistency)

Location: src/persistence/persistence.py:223

The bash validation blocks /root but Python doesn't:

blocked_dirs = ["/etc", "/bin", "/sbin", "/usr", "/lib", "/boot", "/sys", "/proc", "/dev"]
# Missing: "/root", "/lib32", "/lib64"

🚀 Performance Considerations

1. No Significant Performance Issues: The early parsing of --base-directory is good design - avoids reparsing later
2. Symlink Overhead: Minimal, as symlink dereferencing is a kernel operation

🔒 Security Assessment

Overall: Good security posture with a few minor gaps noted above.

Additional Considerations:

• ✅ Validates absolute paths
• ✅ Blocks system directories
• ✅ Resolves symlinks
• ✅ Checks parent directory permissions
• ⚠️ Could add check for /lib32
• ⚠️ Python validation should match bash blocklist exactly

🧪 Test Coverage

Current State: No automated tests detected for this feature.

Recommended Test Cases:

1. ✅ Default behavior (no --base-directory) - covered in test plan
2. ✅ Custom directory (--base-directory ~/AI) - covered in test plan
3. ✅ Model downloader respects base directory - covered in test plan
4. ⚠️ Missing: Edge cases that should be tested:
   • Paths with spaces: --base-directory "/path/with spaces"
   • Tilde expansion: --base-directory ~/custom/path
   • Blocked paths: --base-directory /etc/comfy (should fail)
   • Non-existent parent: --base-directory /nonexistent/foo/bar (should fail gracefully)
   • Both argument formats: --base-directory=~/AI and --base-directory ~/AI
   • Symlink parent directory: If parent is a symlink, ensure it's resolved correctly

📝 Code Quality

Bash:

• ✅ Follows project conventions (function-based, error handling)
• ✅ Good use of guards and shellcheck disables with comments
• ✅ Clear variable naming
• ⚠️ Could benefit from more inline comments explaining the security rationale

Python:

• ✅ Type hints, proper logging, good error messages
• ✅ Maintains existing code style
• ✅ Clear comments explaining design decisions (e.g., line 210-212)

🎯 Recommendations

Must Fix:

1. Add /lib32 to both bash and Python blocklists
2. Ensure Python blocklist matches bash exactly (add /root, /lib64)

Should Fix:
3. Add test case for paths with spaces
4. Add test case for blocked directories (verify error handling)

Nice to Have:
5. Document the realpath fallback limitation
6. Add comment explaining why --base-directory is parsed twice
7. Consider adding validation that BASE_DIR != CODE_DIR (prevent user from pointing data directory at app directory)

Summary

This is a well-designed feature with good security practices. The main concerns are:

• Minor gaps in the security blocklist consistency between bash and Python
• Lack of automated tests for edge cases

The PR is ready to merge after addressing the blocklist inconsistencies. The other recommendations can be addressed in follow-up PRs if needed.

Overall Assessment: ⭐⭐⭐⭐ (4/5) - Solid implementation with room for minor improvements.

[claude]

Code Review: --base-directory Support

Thank you for this well-thought-out PR! This is a solid implementation that cleanly separates application code from user data. I've reviewed the changes across all commits and have some feedback organized by category.

🎯 Overall Assessment

Strengths:

• Clear separation of concerns (app code vs. user data)
• Strong security validation with defense-in-depth approach
• Good documentation in both README and CLAUDE.md
• Thoughtful handling of edge cases (quoted paths, symlinks, etc.)

Areas for Improvement:

• Some minor code quality and security enhancements needed
• A few potential bugs in edge case handling
• Missing test coverage

---

🔒 Security Concerns

1. Symlink Resolution Logic Issue (scripts/config.sh:132-136)

Severity: Medium

The symlink resolution only checks the parent directory, but doesn't handle the case where the final base directory itself is a symlink that could point to a system directory.

# Current code only checks parent
if [[ -L "$_parent_dir" ]]; then
  _resolved_parent="$(readlink -f "$_parent_dir" 2>/dev/null || echo "$_parent_dir")"
  _validate_base_dir "$_resolved_parent/$(basename "$BASE_DIR")"

Issue: If user does --base-directory /home/user/mylink where mylink -> /etc/shadow, this bypasses validation.

Recommendation:

# Resolve the full BASE_DIR path after parent check
if [[ -L "$BASE_DIR" ]]; then
  _resolved_base="$(readlink -f "$BASE_DIR" 2>/dev/null || echo "$BASE_DIR")"
  _validate_base_dir "$_resolved_base"
  BASE_DIR="$_resolved_base"
fi

2. TOCTOU Race Condition (scripts/config.sh:139-146)

Severity: Low

There's a Time-of-Check-Time-of-Use gap between validation and actual directory creation/use.

Recommendation: This is acceptable for a launcher script, but consider adding a comment explaining the limitation and why it's acceptable in this context.

---

🐛 Potential Bugs

1. Quote Stripping Logic is Fragile (scripts/config.sh:73-77)

Severity: Low

The quote stripping doesn't handle mixed quotes or escaped quotes correctly:

raw_path="${raw_path%\""}"
raw_path="${raw_path#\""}"
raw_path="${raw_path%\'"}"
raw_path="${raw_path#\'"}"

Issue: Paths like "path'with'quotes" or 'path"with"quotes' won't work correctly.

Recommendation: Either remove this quote-stripping (bash's argument parsing already handles quotes), or improve it:

# Only strip if surrounded by matching quotes
if [[ "$raw_path" =~ ^"\(.+\)"$ ]] || [[ "$raw_path" =~ ^'(.+)'$ ]]; then
  raw_path="${BASH_REMATCH[1]}"
fi

2. Inconsistent Error Handling in realpath Fallback (scripts/config.sh:83-93)

Severity: Low

The fallback path construction uses $PWD which may not be what the user expects if they've changed directories.

Recommendation:

# Use HOME as base for relative paths since most users expect that
if [[ "$raw_path" == /* ]]; then
  echo "$raw_path"
else
  echo "$HOME/$raw_path"
fi

3. Missing Validation for Empty Path (scripts/config.sh:99-103)

Severity: Low

If _parse_base_directory returns an empty string due to an error, it silently falls back to default.

Recommendation: Add explicit check:

_parsed_base_dir="$(_parse_base_directory "$@")"
if [[ -n "$_parsed_base_dir" ]]; then
  BASE_DIR="$_parsed_base_dir"
elif [[ "$*" =~ --base-directory ]]; then
  # User provided flag but parsing failed - this is an error
  echo "ERROR: Failed to parse --base-directory argument" >&2
  exit 1
fi

---

📊 Code Quality

1. Duplicate Validation Logic (scripts/config.sh & src/persistence/persistence.py)

Severity: Medium

The blocklist is duplicated between bash and Python with a comment to keep them in sync. This is error-prone.

Recommendation:

• Generate Python blocklist from bash, OR
• Move validation entirely to bash (preferred - single source of truth)
• If keeping both, add a simple integration test that verifies they match

2. Function Naming Inconsistency (scripts/config.sh)

Issue: Mix of private (_function) and public function naming isn't consistently applied.

Recommendation: Apply naming convention consistently:

• _parse_base_directory: Private (good ✓)
• _validate_base_dir: Private (good ✓)
• parse_arguments: Public but should be _parse_arguments since it's only used internally
• export_config: Public (correct usage ✓)

3. Magic Number in Array Index Check (scripts/config.sh:60)

Minor

if [[ $((i+1)) -lt ${#args[@]} ]] && [[ \! "${args[$((i+1))]}" =~ ^-- ]]; then

Recommendation: Extract to variable for clarity:

local next_idx=$((i+1))
if [[ $next_idx -lt ${#args[@]} ]] && [[ \! "${args[$next_idx]}" =~ ^-- ]]; then
  raw_path="${args[$next_idx]}"

4. Logging Before Logger Available (scripts/config.sh:38-39)

Good Practice

The comment explaining why echo is used before logger.sh is sourced is excellent! This is exactly the kind of documentation that helps maintainers.

---

⚡ Performance Considerations

No significant performance issues identified. The argument parsing is O(n) which is appropriate for command-line args.

Minor suggestion: The validation could short-circuit early:

for prefix in "${blocked_prefixes[@]}"; do
  if [[ "$dir" == "$prefix" || "$dir" == "$prefix"/* ]]; then
    echo "ERROR: --base-directory cannot be in system directory: $prefix" >&2
    exit 1  # Already exits, good ✓
  fi
done

---

🧪 Test Coverage

Critical Gap: No automated tests for this security-sensitive feature.

Recommendations:

1. Add bash unit tests (using bats or similar):

   • Test --base-directory parsing (both forms: =value and value)
   • Test all blocked directories are rejected
   • Test symlink attack scenarios
   • Test invalid inputs (missing value, flags as value, etc.)

2. Add integration test:

   • Verify bash and Python blocklists match
   • Test end-to-end with custom directory

3. Add to CI/CD (mentioned in PR test plan but not checked off):

   • Run with default settings ✓
   • Run with custom base directory ✓
   • Verify model downloader respects setting ✓

Example test structure:

#\!/usr/bin/env bats

@test "--base-directory rejects /etc" {
  run source config.sh --base-directory /etc
  [ "$status" -eq 1 ]
  [[ "$output" =~ "cannot be in system directory" ]]
}

@test "--base-directory accepts custom mount" {
  run source config.sh --base-directory /mnt/ai-storage
  [ "$status" -eq 0 ]
}

---

📝 Documentation

Strengths:

• README examples are clear and helpful
• CLAUDE.md updated with new structure
• Inline comments explain non-obvious decisions

Suggestions:

1. Add example of paths with spaces to README (you mention quoting but no example)
2. Document that --base-directory must be created beforehand (or just parent?)
3. Clarify what happens on first run vs. subsequent runs

---

🔍 Architecture Review

Excellent design decisions:

1. Separation of fixed paths (app/venv) from configurable data
2. Early parsing in config.sh before other scripts source
3. Defense-in-depth with both bash and Python validation
4. Single source of truth (bash sets env var, Python reads it)

Minor concern:
The architecture assumes ~/.config/comfy-ui/ is always writable. On some systems (containers, read-only home dirs), this could fail. Consider documenting this requirement or adding validation.

---

🎓 Style & Best Practices

Adheres well to CLAUDE.md guidelines:

• ✅ Modular bash scripts with single responsibility
• ✅ Function-based organization
• ✅ Proper error handling with traps
• ✅ Clear comments and documentation
• ✅ Guard clauses to prevent multiple sourcing

Minor deviations:

• Mixing echo for errors vs. using >&2 (inconsistent, but acceptable given logger.sh isn't available yet)

---

🚀 Recommendations Summary

Must Fix (before merge):

1. Fix symlink resolution to check final BASE_DIR, not just parent
2. Add validation for empty/failed parse when flag is present

Should Fix (high priority):
3. Add automated tests (at minimum: blocklist validation, basic parsing)
4. Improve quote stripping or remove it entirely
5. Ensure bash and Python blocklists stay in sync (test or code generation)

Nice to Have (can be follow-up):
6. Improve relative path handling in realpath fallback
7. Add comprehensive bash unit tests
8. Make parse_arguments private (_parse_arguments)
9. Document container/read-only filesystem limitations

---

✅ Conclusion

This is a well-engineered feature with good security awareness and clean architecture. The separation of concerns is excellent, and the implementation is mostly solid. The main concerns are:

1. Security: Symlink resolution gap (easily fixable)
2. Testing: No automated tests for security-critical code (should add before merge)
3. Maintenance: Duplicated validation logic (minor)

With the symlink fix and basic tests added, this would be ready to merge. The code quality is high, documentation is good, and the feature solves a real user need (storing data on separate drives).

Suggested merge strategy: Fix critical security issue (#1), add basic blocklist validation test (#3), then merge. Address other items in follow-up PR.

Great work overall! 🎉

[claude]

Code Review for PR #7: Add --base-directory support

Thank you for this well-implemented feature! This is a solid implementation that properly separates app code from user data. Below is my detailed review:

---

✅ Strengths

1. Excellent Security Implementation

• ✅ Comprehensive path validation with blocklist of system directories
• ✅ Symlink resolution to prevent symlink attacks
• ✅ Defense-in-depth with validation in both bash and Python
• ✅ Parent directory existence and write permission checks
• ✅ Proper handling of absolute vs relative paths

2. Good Architecture

• ✅ Clean separation: app/venv fixed in ~/.config/comfy-ui/, data configurable
• ✅ Early argument parsing before other scripts are sourced
• ✅ Proper quote handling and tilde expansion
• ✅ Well-documented code with clear comments

3. Code Quality

• ✅ Follows bash style guidelines from CLAUDE.md
• ✅ Proper error handling with meaningful messages
• ✅ Good use of functions and modular design
• ✅ shellcheck disable comments are justified

---

🔍 Issues & Recommendations

Critical Issues

1. Potential Argument Parsing Confusion (scripts/config.sh:220-239)

The skip_next logic in parse_arguments() is technically correct but confusing:

for arg in "$@"; do
  if [[ "$skip_next" == "true" ]]; then
    skip_next=false
    continue  # Skips the value after --base-directory
  fi
  case "$arg" in
    --base-directory)
      skip_next=true
      ;;

Why it works: When --base-directory /path is provided, the first iteration sets skip_next=true, then the second iteration's continue prevents /path from being processed.

Issue: This is hard to follow. The continue at the top of the loop and the case statement interact in a non-obvious way.

Recommendation: Restructure for clarity:

parse_arguments() {
  ARGS=()
  local i=0
  while [[ $i -lt $# ]]; do
    case "${!i}" in
      --base-directory=*)
        # Already handled in config
        ;;
      --base-directory)
        ((i++))  # Skip next arg
        ;;
      --open|--debug|--verbose)
        # Handle flags...
        ;;
      --port=*)
        COMFY_PORT="${arg#*=}"
        ;;
      *)
        ARGS+=("${!i}")
        ;;
    esac
    ((i++))
  done
}

Or at minimum, add a comment explaining the skip_next pattern.

---

High Priority

2. Inconsistent Error Handling (scripts/config.sh:85, 91)

The fallback in _parse_base_directory() silently returns the raw path if realpath fails:

realpath -m "$raw_path" 2>/dev/null || echo "$raw_path"

Issue: If realpath fails due to permission issues or invalid characters, the error is hidden.

Recommendation: Log the failure or validate the path more thoroughly:

if command -v realpath &>/dev/null; then
  local resolved
  if resolved=$(realpath -m "$raw_path" 2>&1); then
    echo "$resolved"
  else
    echo "WARNING: Failed to resolve path: $raw_path (using as-is)" >&2
    echo "$raw_path"
  fi
fi

3. Missing Validation: Empty String Path

Neither bash nor Python validates against empty string paths after processing.

Add to _validate_base_dir():

if [[ -z "$dir" ]]; then
  echo "ERROR: --base-directory path cannot be empty" >&2
  exit 1
fi

Add to Python (persistence.py:217):

if not base_dir or not base_dir.strip():
    logger.error("base_dir cannot be empty")
    raise ValueError("base_dir is empty")

---

Medium Priority

4. TOCTOU (Time-of-Check-Time-of-Use) Consideration (scripts/config.sh:145-149)

The symlink resolution has a theoretical race condition:

if [[ -L "$_parent_dir" ]]; then
  _resolved_parent="$(readlink -f "$_parent_dir" 2>/dev/null || echo "$_parent_dir")"
  _validate_base_dir "$_resolved_parent/$(basename "$BASE_DIR")"
  _parent_dir="$_resolved_parent"
fi

Issue: An attacker could change the symlink between readlink and validation.

Recommendation: This is mostly theoretical (user would race against themselves), but document this limitation or use file descriptors:

# Add comment explaining the limitation
# Note: TOCTOU race condition exists but is not exploitable in single-user context

5. Python Type Annotations (src/persistence/persistence.py:213)

The code uses os.environ.get() which returns str | None, but immediately uses it as str:

base_dir = os.environ.get(
    "COMFY_USER_DIR", os.path.join(os.path.expanduser("~"), ".config", "comfy-ui")
)

Recommendation: This is actually fine because of the default value, but for clarity:

base_dir: str = os.environ.get(
    "COMFY_USER_DIR", os.path.join(os.path.expanduser("~"), ".config", "comfy-ui")
)

6. Documentation: Quoting Examples

README.md mentions quoting paths with spaces, but more examples would help:

# Add to README:
# Paths with spaces (any of these work):
nix run -- --base-directory="/path/with spaces"
nix run -- --base-directory /path/with\ spaces
nix run -- '--base-directory=/path/with spaces'

---

Low Priority / Nice to Have

7. Blocklist vs Allowlist Trade-offs

Current approach uses blocklist which is good for flexibility but could miss edge cases.

Current (good for NixOS):

• Blocks /etc, /bin, /usr, etc.
• Allows custom mounts under any non-system path

Alternative (more restrictive, maybe too much):

• Allow only /home, /mnt, /opt, /data, etc.
• Might break legitimate use cases

Recommendation: Keep current approach, it's the right balance.

8. Enhanced Logging for Security Events

When validation fails, consider logging:

_validate_base_dir() {
  local dir="$1"
  # ... existing checks ...
  for prefix in "${blocked_prefixes[@]}"; do
    if [[ "$dir" == "$prefix" || "$dir" == "$prefix"/* ]]; then
      logger -t comfyui-nix "Blocked dangerous base-directory: $dir" 2>/dev/null || true
      echo "ERROR: --base-directory cannot be in system directory: $prefix" >&2
      exit 1
    fi
  done
}

9. ShellCheck Validation

Run shellcheck scripts/config.sh to catch any additional issues. The code looks clean already.

---

🧪 Testing Recommendations

The test plan in the PR description is good, but consider adding:

Edge Cases:

• --base-directory="" (empty string)
• --base-directory=./relative (should convert to absolute)
• --base-directory=~/path (tilde expansion)
• --base-directory=$HOME/path (variable expansion)
• Paths with special characters: spaces, unicode, etc.
• Very long paths (PATH_MAX boundary)

Security Tests:

• --base-directory=/etc (should be blocked)
• --base-directory=/home/user/../../etc (path traversal, should be normalized then blocked)
• --base-directory=/tmp/link-to-etc where link points to /etc (should be blocked after resolution)
• Symlink loops (e.g., a -> b, b -> a)

Integration Tests:

• Verify models download to custom location
• Verify custom nodes install correctly
• Test with pre-existing ~/.config/comfy-ui/ directory
• Test migration from default location to custom location

Compatibility Tests:

• Run on different shells (bash 4.x, 5.x)
• Test on different platforms (Linux, macOS, NixOS)
• Test with different filesystems (ext4, btrfs, ZFS, NFS)

---

📊 Performance Considerations

• ✅ Argument parsing is O(n), efficient
• ✅ No unnecessary disk I/O during config phase
• ✅ Path validation is fast (string operations only)
• ⚠️ Multiple readlink -f calls could be slow on network filesystems (minor concern)

Recommendation: Performance is good. Network filesystem users might see slight delays during startup, but this is acceptable.

---

🔒 Security Assessment

Overall: Strong ✅

The implementation demonstrates excellent security awareness:

• ✅ Defense-in-depth (validation in both bash and Python)
• ✅ Path traversal prevention
• ✅ Symlink attack mitigation
• ✅ Permission checks
• ⚠️ Minor TOCTOU theoretical issue (not exploitable in practice)

No critical security vulnerabilities identified.

---

📝 Documentation Quality

• ✅ CLAUDE.md updated appropriately
• ✅ README.md has clear examples
• ✅ Code comments explain rationale
• ✅ Architecture documented in PR description
• ⚠️ Could benefit from troubleshooting section

Suggested addition to README:

### Troubleshooting --base-directory

**Error: "Parent directory does not exist"**
- Create the parent directory first: `mkdir -p ~/AI`

**Error: "Cannot be in system directory"**
- Don't use system paths like `/etc`, `/usr`, `/bin`
- Use user-writable locations: `~/AI`, `/mnt/data`, `/opt/comfy-data`

**Path not recognized:**
- Use absolute paths: `/home/user/AI` instead of `./AI`
- Quote paths with spaces: `--base-directory="/path/with spaces"`

---

✨ Summary

This is a well-implemented feature with strong attention to security and architecture. The code quality is high and follows project conventions.

Recommendation: Approve with minor improvements

Should fix before merge:

1. Add empty string validation (both bash and Python)
2. Improve error messages in path resolution
3. Clarify the skip_next logic with comments (or restructure)

Nice to have:
4. Enhanced edge case testing
5. Troubleshooting documentation
6. Security event logging

No blocking issues identified. The current implementation is production-ready.

---

📋 Detailed File Review

scripts/config.sh

• Lines 43-95: Path parsing logic is solid ✅
• Lines 111-128: Validation function is comprehensive ✅
• Lines 131-160: Security checks are thorough ✅
• Lines 220-239: Argument parsing works but could be clearer ⚠️

src/persistence/persistence.py

• Lines 217-230: Good validation, matches bash logic ✅
• Lines 209-212: Good comment explaining design decision ✅
• Type hints: Could be slightly more precise ⚠️

CLAUDE.md & README.md

• Documentation: Clear and accurate ✅
• Examples: Helpful and practical ✅

---

Great work on this PR! The feature is architecturally sound and will be very useful for users wanting to store data on separate drives. 🎉

Estimated review time: ~30 minutes
Code quality: 9/10
Security: 9/10
Documentation: 8/10
Overall: Excellent work! 👏