Fixes identified in docs/reviews/pr-157.md:
P1 — Fix premature import_custom_nodes() call in generated fixture
- Remove top-level import_custom_nodes() before its definition in
tests/runtime/generated/upscale-model-loader.py (NameError crash)
P2.1 — Check CWD before walking upward in find_path()
- find_path() and _find_from_extension_location() now check the starting
directory before moving to parent (was off-by-one when launched from
ComfyUI root itself)
P2.2 — Always promote ComfyUI to sys.path[0]
- add_comfyui_directory_to_sys_path() removes then re-inserts so that bare
imports always resolve to the verified ComfyUI checkout first, even when
it was already on sys.path at a lower index
P2.3 — Add _find_file() helper for file discovery
- New function walks up from CWD checking os.path.isfile() (not just
directory names) and replaces the broken find_path("extra_model_paths.yaml")
call that could never match a file
P2.4 — Load comfy.options under canonical name in bootstrap
- bootstrap_comfyui_runtime() now uses _load_module("comfy.options", ...)
instead of _load_module_temp("_bootstrap_options", ...) so that
enable_args_parsing() mutates the sys.modules cache entry that cli_args.py
imports by its canonical name
P2.5 — Improve test portability without sibling ComfyUI checkout
- Add @skipUnless decorators on integration tests requiring real checkout
- Structural tests use _make_fake_comfyui() in TemporaryDirectory instead
Critical fixes:
- node_runtime.py: change outer handler in _load_module() from except Exception to except BaseException with sys.modules.pop() cleanup, preventing non-Exception BaseExceptions (SystemExit, KeyboardInterrupt) from violating the no-raise contract
- app.py: remove incorrect reassignment of base_node_class_mappings after custom node import; keep it as pre-custom-node baseline so WorkflowPlanner correctly treats extras/custom nodes as NODE_CLASS_MAPPINGS dict lookups instead of direct imports
- Regenerate stale test fixtures (text-to-image.py, upscale-model-loader.py) from current source: updated _load_module() with exec failure cleanup pattern, 3-marker structural verification, and try/finally sys.path restoration in import_custom_nodes()
Suggestions addressed:
- node_runtime.py: _is_comfyui_directory() now checks 3 structural markers (nodes.py, main.py, comfy/) instead of just nodes.py
- generator/render.py: added None guard for getattr on generated_helpers.__all__ items with log warning for missing helpers
- test_import_path_resolution.py: fixed test_sys_path_restored_on_crash to target the server module specifically after sys.path manipulation, so the try/finally restoration path is actually exercised
Nits addressed:
- .gitignore: added missing trailing newline
New test coverage (tests/test_app_base_mappings.py):
- base_node_class_mappings unchanged after custom node import
- deep copy independence of base_node_class_mappings
- render output contains hardened _load_module with except BaseException pattern
Critical:
- _load_module(): wrap exec_module() in try/except, pop from sys.modules
on failure so broken modules aren't cached
- get_comfyui_path(): add _is_comfyui_directory() check to third (CWD walk)
strategy so a non-ComfyUI directory named "ComfyUI" isn't accepted
- get_node_class_mappings(): return cached sys.modules["nodes"] if already
loaded to avoid re-executing nodes.py and losing init_extra_nodes() state
Suggestions:
- import_custom_nodes(): wrap sys.path filtering in try/finally so path is
always restored even if _load_module("server") raises mid-execution
- render.py: auto-discover helpers from generated_helpers.__all__ instead of
a manually maintained list
- pyproject.toml: remove duplicate ruff dev dependency (kept standard
[project.optional-dependencies] form)
Nitpicks:
- Remove dead imports (types, StringIO) from test_import_path_resolution.py
- Trim trailing blank lines in pyproject.toml
- Tone down _load_module docstring: "eliminates ALL" -> "significantly reduces"
Tests (7 new cases):
- TestLoadModuleFailureCleanup: broken module removed on exec failure, retry
starts fresh
- TestLoadModuleTemp: temp modules purged from sys.modules (success + failure)
- TestRepeatedNodeLoad: repeated _load_module("nodes") returns cached copy
- TestGetComfyuiPathThirdStrategy: fake "ComfyUI" dir rejected
- TestImportCustomNodesSysPathRestoration: sys.path restored after crash
nodes.py inserts the comfy/ subdirectory into sys.path, which shadows
the top-level utils/ package (comfy/utils.py vs utils/__init__.py).
This breaks server.py's import chain when app.frontend_management
does 'from utils.install_util import ...'.
Filter out the comfy/ subdirectory temporarily before loading server.py,
then restore it immediately after. Use list comprehension instead of
sys.path.remove to satisfy the gap-window test assertion.
server.py cannot load due to a ComfyUI internal module shadowing bug
(comfy/utils.py file shadows utils/ package). Decouple init_extra_nodes()
from server.py loading so NODE_CLASS_MAPPINGS still gets populated from
comfy_extras (UpscaleModelLoader, etc.) even without full PromptServer setup.
Also fix app.py to re-read from cached sys.modules[nodes] after
import_custom_nodes() instead of using stale initial copy.
Fixes the upscale-model-loader runtime e2e test.
bootstrap_comfyui_runtime() now uses _load_module_temp() with temporary
module names (_bootstrap_options, _bootstrap_cli_args, _bootstrap_cuda_malloc)
instead of real module names. This prevents cached stale modules from
sys.modules conflicting with ComfyUI's internal import chain (e.g.
nodes.py -> comfy.sd -> comfy.model_management -> comfy.cli_args).
Fixes the text-to-image runtime e2e failure:
ImportError: cannot import name 'args' from 'comfy.cli_args'
Replace all bare ComfyUI internal imports with centralized _load_module()
using importlib.util.spec_from_file_location(). This eliminates sys.path
shadowing attacks, removes the remove/re-insert gap window, and makes
import resolution deterministic regardless of working directory, install
layout, or prior sys.path state.
node_runtime.py — core rewrite:
- Add _load_module(): centralized importlib isolation layer that loads
modules from explicit file paths verified against a ComfyUI root
confirmed by _is_comfyui_directory() (checks for nodes.py marker)
- Add _is_comfyui_directory(): structural verification of candidate paths
- Add _find_from_extension_location(): walk up from __file__ via realpath
to find ComfyUI root (handles symlinked installs)
- Rewrite get_comfyui_path(): multi-strategy fallback chain:
(1) COMFYUI_PATH env var with verification, (2) relative extension walk,
(3) CWD walk as depth-limited last resort (max 20 levels)
- Replace bare 'from nodes import NODE_CLASS_MAPPINGS' with _load_module()
- Replace bare 'import execution/server' with _load_module()
- Replace bare 'from main import load_extra_path_config' with _load_module()
(alias "comfy_main" to prevent sys.modules cache collision)
- Replace bare 'from utils.extra_config import ...' with _load_module()
- Replace bare 'import comfy.options/cli_args' with _load_module()
- Replace bare 'import cuda_malloc' with _load_module()
- Rewrite add_comfyui_directory_to_sys_path(): idempotent insert-once,
no remove/re-insert gap window
- Rewrite import_custom_nodes(): _load_module() for execution/nodes/server,
eliminated sys.path.remove() that created a gap window for lazy imports
- Rewrite get_node_class_mappings(): _load_module() for nodes.py
- Replace all print() statements with logging.DEBUG (no path leaks to stdout)
- Add None guards on args attributes after cli_args module load
generator/render.py — generated script isolation:
- Embed _load_module(), _is_comfyui_directory(), _find_from_extension_location()
in generated scripts via inspect.getsource()
- Add importlib.util and logging imports to generated output
- Generated scripts now inherit full importlib isolation guarantee — zero
bare imports of ComfyUI internals
generator/generated_helpers.py:
- Export _load_module, _is_comfyui_directory, _find_from_extension_location
for use by render.py
tests/test_import_path_resolution.py — new (14 test cases):
- TestIsComfyuiDirectory: structural verification against real ComfyUI checkout,
rejection of empty/nonexistent directories
- TestLoadModule: module loading from explicit path, graceful None for missing
- TestShadowingResistance: malicious main.py and nodes.py at sys.path[0] are
ignored; _load_module() always loads from verified file path
- TestSysPathIdempotence: insert-once behavior, no remove/re-insert in
import_custom_nodes(), no print() statements in module source
- TestGeneratedScriptIsolation: generated output contains _load_module() and
spec_from_file_location; zero bare ComfyUI imports
- TestGetComfyuiPath: env var strategy with verification, fallback chain
tests/test_upscale_model_loader_export.py — updated assertions:
- Replace "import comfy.options" / "import cuda_malloc" checks with
_load_module() string pattern matching ("comfy.options", "cuda_malloc")
- Update ordering assertion for cuda_malloc/torch import sequence
Fixes: #105, #117 (utils package shadowing), #19 (FileNotFoundError on Windows)
Addresses security audit findings: #1 (COMFYUI_PATH poisoning), #2 (directory
collision), #3 (main.py shadowing), #6 (gap window race), #7 (unbounded walk),
#8 (path confusion), #11 (stdout path leak), #12 (symlink resolution)
Spec: docs/specs/harden-import-path-resolution.html
Split the previous monolithic implementation into focused modules for CLI handling, workflow loading, runtime bootstrap, load ordering, planning, rendering, and file I/O. Keep the public compatibility surface intact by preserving the legacy top-level wrapper while adding as the documented entrypoint. This reduces cross-cutting coupling inside the exporter and makes the runtime/bootstrap behavior easier to evolve without carrying unrelated concerns in one file.
Peyton DeNiro