Feature/refactor readability#159
Open
pydn wants to merge 29 commits into
Open
Conversation
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: pydn#105, pydn#117 (utils package shadowing), pydn#19 (FileNotFoundError on Windows)
Addresses security audit findings: #1 (COMFYUI_PATH poisoning), pydn#2 (directory
collision), pydn#3 (main.py shadowing), pydn#6 (gap window race), pydn#7 (unbounded walk),
pydn#8 (path confusion), pydn#11 (stdout path leak), pydn#12 (symlink resolution)
Spec: docs/specs/harden-import-path-resolution.html
- Add ruff as a dev dependency (pyproject.toml + uv.lock) for formatting hooks on Python files - Ignore docs/, .agents/, .codex/, AGENTS.md in .gitignore
… conflicts 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'
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.
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.
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
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
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
add_extra_model_paths() uses _find_file() which must be available in generated standalone scripts. Add it to generated_helpers __all__ so the render step embeds it alongside the other runtime helpers.
Replace _load_module_temp with __import__ in bootstrap_comfyui_runtime() so parsed CLI args persist in sys.modules across the full import chain. This ensures flags like --cpu take effect before model_management.py triggers CUDA init on GPU-less systems. Key changes: - Add _bootstrap_import helper for namespace-package-safe imports that keep modules cached in sys.modules (options, cli_args) - Add _filter_comfyui_args to strip non-ComfyUI flags from subprocess argv, preventing argparse crashes while preserving valid ComfyUI flags - Call bootstrap_comfyui_runtime() from get_node_class_mappings() so the export path also respects CLI args - Auto-detect unavailable CUDA and force CPU mode even without --cpu flag - Regenerate test fixtures with updated bootstrap code Verified: 73 unit tests pass, 5/5 fast tier validation pass, runtime tier generates image on CPU (output write blocked by read-only FS only).
- Add missing 'import warnings' to generated script static imports, fixing NameError crash in cleanup_comfyui_runtime() when a hook fails - Replace _bootstrap_import(__import__) with _load_module() for comfy.options and comfy.cli_args — maintains importlib isolation for ALL ComfyUI modules including namespace package contents - Remove _bootstrap_import from generated_helpers exports (no longer needed) - Fix E402 lint violations in render.py (move logger after imports) - Add test_generated_script_includes_warnings_import assertion
…ger used after replacing with _load_module() for namespace package\ncontents. Eliminates bare __import__() from the codebase entirely.
…egression tests Add back _bootstrap_import helper to properly handle ComfyUI's namespace packages (comfy/ has no __init__.py). This fixes the export path where _load_module with file paths fails on namespace packages. Changes: - Add _bootstrap_import using __import__() for namespace-package-safe imports - Use _bootstrap_import in bootstrap_comfyui_runtime() for comfy.options and comfy.cli_args - Fix _filter_comfyui_args to include value args for known flags (e.g., --reserve-vram 4096 was dropping the 4096) - Guard all args access with 'if args is not None' in bootstrap to prevent crashes during export path when cli_args fails to load - Add _bootstrap_import to generated_helpers.__all__ for embedding - Regenerate test fixtures with updated bootstrap code Add CLI args propagation regression tests (16 new tests): - TestLoadModuleCachesInSysModules: verify _load_module caches in sys.modules - TestLoadModuleTempRemovesFromSysModules: document _load_module_temp behavior - TestFilterComfyuiArgs: comprehensive flag filtering tests - TestBootstrapUsesBootstrapImport: source-level regression checks - TestGeneratedScriptEmbedsBootstrap: verify generated script structure
…tion The old _filter_comfyui_args() maintained a hardcoded frozenset of ~70 recognized CLI flags. This list was already missing 35 ComfyUI flags (--listen, --port, --bf16-unet, etc.) that would be silently dropped. Replace with _discover_comfyui_cli_options() that inspects comfy.cli_args.parser._actions at runtime to discover all recognized options and their value-taking behavior automatically. Benefits: - Zero maintenance - always in sync with ComfyUI's actual parser - No silent dropping of new flags added by ComfyUI updates - ~130 lines of hardcoded flag sets eliminated - Graceful fallback (empty sets) when ComfyUI is unavailable
…overrides Three fixes for generated standalone scripts: 1. Add _GENERATED_GLOBALS mechanism so module-level declarations (e.g. _DISCOVERED_OPTIONS = None) are emitted into generated output. Previously the generator only copied function bodies via inspect.getsource(), omitting required global variable initializers. 2. Clean ComfyUI modules from sys.modules after CLI option discovery. _discover_comfyui_cli_options() imported comfy.cli_args with fake argv (["_discover"]), and the cached module was reused by bootstrap so real CLI flags (--cpu, --output-directory) were never parsed. Now discovery removes newly-loaded ComfyUI modules so bootstrap re-imports fresh. 3. Apply --output-directory, --input-directory, --user-directory overrides in bootstrap_comfyui_runtime(), mirroring main.py behavior. This allows generated scripts to redirect output away from read-only mounts.
Restructure comfyui_to_python/node_runtime.py from a monolithic 560-line file into three purpose-driven submodules plus a thin facade: - runtime/path_discovery.py — ComfyUI root/file location (get_comfyui_path, find_path, _is_comfyui_directory) - runtime/module_loader.py — importlib-based module loading (_load_module, _bootstrap_import with allowlist validation) - runtime/bootstrap.py — CLI option discovery and argv filtering The facade re-exports all symbols so existing callers require zero import changes. Security hardening (from Red Team audit): - Added inline allowlist to _bootstrap_import() blocking unauthorized module names - Fixed _filter_comfyui_args edge case where known flags could be consumed as values - Improved docstring coherence across all functions All 98 tests pass. Generated scripts verified for correct function embedding.
…s test_runtime_validation_harness import error
…cli_options Move 'from .module_loader import _bootstrap_import' from inside _discover_comfyui_cli_options() to module-level in bootstrap.py. When inspect.getsource() extracts the function for embedding into generated standalone scripts, the inline relative import was captured and would fail with 'ImportError: attempted relative import with no known parent package' when the script runs as __main__. - Module-level import stays invisible to inspect.getsource() (function-only extraction) - _bootstrap_import is already embedded as a standalone helper in generated scripts - Updated test mocks to patch bootstrap._bootstrap_import (new namespace location) - Added regression test suite (3 tests) verifying no relative imports in generated output
… modules
Replace manual generated_helpers.__all__ curation with automatic
module-level source embedding. Instead of using inspect.getsource() on
individual functions (which misses cross-call dependencies like
_apply_device_settings), the new embedded_modules module:
1. Reads source files from all contributing runtime modules in dependency order
2. Strips import statements via AST analysis
3. Concatenates clean definitions into a single embeddable block
This guarantees ALL helper functions are always present in generated
scripts, eliminating NameError when new internal helpers are added.
New files:
- generator/embedded_modules.py: auto-discovery and embedding logic
with verify_no_missing_cross_calls() for CI validation
- tests/test_embedded_modules.py: unit tests for strip_imports,
get_embedded_helpers, list_embedded_names, cross-call verification
Updated:
- generator/render.py: simplified to call get_embedded_helpers()
- tests/test_generated_script_standalone.py: updated for new architecture
…tection, stale check Generated code fixes: - Add 'import gc' to static imports in render.py (fixes NameError in cleanup_comfyui_runtime) - Remove premature 'import torch as _torch' from bootstrap (fixes Torch-early-import warning) E2E runtime validation improvements: - Add bootstrap smoke test (validate_bootstrap): imports generated script and runs bootstrap phase in a subprocess, catching import-order segfaults without GPU/models - Signal crash detection: returncode > 128 classified with specific signal name (SIGSEGV etc.) - Add --check-stale flag: regenerates fixtures and diffs against committed output - execute_generated_python uses writable temp output dir with --output-directory flag - All subprocess paths pass --cpu to avoid CUDA init in non-GPU environments - Internal-export handler sets sys.argv for correct CLI arg parsing Test results: 119 unit tests OK, fast tier 5/5 pass, runtime text-to-image passes end-to-end
- Remove _GENERATED_GLOBALS from runtime/bootstrap.py (never consumed) - Remove re-export from node_runtime.py and generator/generated_helpers.py - Add __all__ to node_runtime.py for explicit public API surface - Remove unused typing.Any import from bootstrap.py - Add allowlist enforcement test (test_bootstrap_import_allowlist.py) - Regenerate committed scripts (no _GENERATED_GLOBALS in output)
bootstrap.py: - Extract _parse_parser_actions() from _discover_comfyui_cli_options() (reduces function from ~80 to ~45 lines) - Move _base_option() to module-level as _get_base_option() for embedding compatibility - Add _EMPTY_OPTIONS constant to reduce repetition node_runtime.py: - Extract _load_custom_node_modules() and _init_extra_nodes() helpers from import_custom_nodes() (reduces function from ~50 to ~20 lines) - All new helpers are at module level for embedding compatibility
module_loader.py:
- Consolidate dual exception handlers in _load_module() into single try/except
(eliminates redundant sys.modules.pop call path)
- Reorder log.debug before sys.modules.pop for consistency
node_runtime.py:
- Consolidate three _bootstrap_import("folder_paths") calls into one in
_apply_directory_overrides() (idempotent via sys.modules cache)
- Add early return when folder_paths_mod is None to reduce nesting
tests:
- Update test_app_base_mappings assertion to match updated except clause
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/pydn/ComfyUI-to-Python-Extension/blob/34729a830bb655ba0c4fae23ece23e937b1fb3ad/tests/__init__.py#L1
Adding this package marker makes pytest treat tests as part of the repository package and import the parent __init__.py during collection; that module imports ComfyUI runtime dependencies like aiohttp and server at import time, so python -m pytest -q now fails before any tests run in a normal development environment without the full ComfyUI server stack. Keeping tests as a plain test directory avoids pulling in the custom-node entrypoint just to collect unit tests.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if i + 1 < len(argv) and not argv[i + 1].startswith("--"): | ||
| i += 1 | ||
| else: | ||
| result.append(token) |
There was a problem hiding this comment.
When bootstrap runs under a test runner or notebook wrapper, positional arguments such as tests/test_node_runtime.py are preserved here and then passed on to ComfyUI's comfy.cli_args, which calls parser.parse_args() and rejects unknown positionals. For example, with only --cpu discovered as a ComfyUI flag, _filter_comfyui_args(['pytest', 'tests/test_node_runtime.py', '--cpu']) returns the test path unchanged, so bootstrap still exits before the generated workflow can run in exactly the embedded/test-runner context this filter is meant to support.
Useful? React with 👍 / 👎.
pydn-hermes-agent
force-pushed
the
feature/refactor-readability
branch
from
34729a8 to
29cbbcc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens generated ComfyUI scripts so they resolve the real ComfyUI runtime more reliably, preserve ComfyUI CLI arguments, and avoid import shadowing failures in standalone/runtime execution. This also refactors the runtime helper code into focused modules and expands regression coverage around import path resolution, custom node loading, generated script embedding, and runtime validation.
Open Issues Addressed
utils.json_utilshadowing by loading ComfyUI internals from verified file paths instead of bare imports.utilspackage shadowing failure as #105, covered by the hardened import path loader.sys.argvso generated scripts can define their own arguments.--cpu,--highvram,--lowvram,--normalvram, and value-taking CLI options.What Changed
node_runtime.pyinto focused runtime modules for path discovery, module loading, and CLI bootstrap behavior.COMFYUI_PATH, extension-relative discovery, and CWD fallback.importlibloading from verified files.comfy.optionsandcomfy.cli_argsso parsed CLI flags survive ComfyUI’s later imports.NODE_CLASS_MAPPINGSafter custom node initialization while preserving the pre-custom-node baseline for planner decisions.Validation
uv run python -m unittest discover testsuv run python tests/runtime/run_runtime_validation.py --tier fastCOMFYUI_PATH=/opt/ComfyUI uv run python tests/runtime/run_runtime_validation.py --check-staleCOMFYUI_PATH=/opt/ComfyUI uv run python tests/runtime/run_runtime_validation.py --tier runtime