Fix/dynamic path relocation#14769
Conversation
Samith-NM
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChanges
Sequence Diagram(s)Not applicable. Compact Metadata Related PRs: None identified Suggested Labels: enhancement, needs-review Suggested Reviewers: None identified Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@folder_paths.py`:
- Around line 31-32: Module import should not block on interactive path recovery
in valid_path(). Move the relocation flow out of folder_paths.py import-time
logic so valid_path(), and the callers for base/model/output paths, do not call
input() during import; instead use an explicit recovery/validation step or raise
a clear exception when a path is missing. Update the logic around valid_path and
any related path initialization helpers so they fail fast or are invoked only
from an interactive entrypoint.
- Line 91: The default output path in folder_paths.py is being treated as if it
must already exist, which blocks fresh setups. Update the output_directory
initialization around valid_path(os.path.join(base_path, "output"), "Output
Directory") so the default <base>/output is created or accepted when missing,
while still keeping validation behavior for relocated existing roots. Use the
existing output_directory and valid_path flow to ensure downstream callers
always get a writable destination without prompting for the default case.
- Around line 33-40: The redirect persistence logic in `folder_paths` is using
the `redirects` dictionary where it should use `redirect_path`, so
`os.path.exists()` and the file open calls fail before any data is saved. Update
the load/write flow to check and read from `redirect_path`, then persist
`description -> new_path` back to that same path. In the same block, replace the
broad `except Exception` with specific expected IO/JSON handling so the failure
is not silently swallowed.
- Around line 29-42: The path recovery logic in folder_paths should validate
both the stored redirect and the user-entered new_path before returning them.
Update the redirect lookup/entry flow to ensure the resolved path is absolute,
exists, and is a directory before assigning it for base_path, models_dir, or
output_directory, and raise a clear error instead of silently returning an
invalid path. Use the existing redirect handling block and new_path prompt flow
to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e89dc48-b69e-44d9-bf3f-606b6a712311
📒 Files selected for processing (1)
folder_paths.py
| if description in redirects and os.path.exists(redirects[description]): | ||
| return redirects[description] | ||
| print(f"\n[!] ALERT: {description} not found at: {saved_path}") | ||
| new_path= input(f"Please paste the updated absolute path for '{description}': ").strip() | ||
| try: | ||
| redirects = {} | ||
| if os.path.exists(redirects): | ||
| with open(redirects, "r") as f: redirects = json.load(f) | ||
| redirects[description] = new_path | ||
| with open(redirects, "w") as f: json.dump(redirects, f, indent=4) | ||
| except Exception: | ||
| pass | ||
|
|
||
| return new_path |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate redirected and entered paths before returning them.
Both stored redirects and new_path are returned without enforcing absolute path, existence, or directory-ness. That can leave base_path, models_dir, or output_directory pointing at an empty/relative/nonexistent path after the “recovery” flow.
Proposed guard
- if description in redirects and os.path.exists(redirects[description]):
- return redirects[description]
+ redirected_path = redirects.get(description)
+ if redirected_path and os.path.isabs(redirected_path) and os.path.isdir(redirected_path):
+ return redirected_path
...
- new_path= input(f"Please paste the updated absolute path for '{description}': ").strip()
+ new_path = input(f"Please paste the updated absolute path for '{description}': ").strip()
+ if not os.path.isabs(new_path) or not os.path.isdir(new_path):
+ raise FileNotFoundError(f"{description} must be an existing absolute directory: {new_path}")As per coding guidelines, unsupported paths and bad states should fail with clear errors instead of silently producing worse behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if description in redirects and os.path.exists(redirects[description]): | |
| return redirects[description] | |
| print(f"\n[!] ALERT: {description} not found at: {saved_path}") | |
| new_path= input(f"Please paste the updated absolute path for '{description}': ").strip() | |
| try: | |
| redirects = {} | |
| if os.path.exists(redirects): | |
| with open(redirects, "r") as f: redirects = json.load(f) | |
| redirects[description] = new_path | |
| with open(redirects, "w") as f: json.dump(redirects, f, indent=4) | |
| except Exception: | |
| pass | |
| return new_path | |
| redirected_path = redirects.get(description) | |
| if redirected_path and os.path.isabs(redirected_path) and os.path.isdir(redirected_path): | |
| return redirected_path | |
| print(f"\n[!] ALERT: {description} not found at: {saved_path}") | |
| new_path = input(f"Please paste the updated absolute path for '{description}': ").strip() | |
| if not os.path.isabs(new_path) or not os.path.isdir(new_path): | |
| raise FileNotFoundError(f"{description} must be an existing absolute directory: {new_path}") | |
| try: | |
| redirects = {} | |
| if os.path.exists(redirects): | |
| with open(redirects, "r") as f: redirects = json.load(f) | |
| redirects[description] = new_path | |
| with open(redirects, "w") as f: json.dump(redirects, f, indent=4) | |
| except Exception: | |
| pass | |
| return new_path |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 35-35: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(redirects, "r")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 37-37: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(redirects, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@folder_paths.py` around lines 29 - 42, The path recovery logic in
folder_paths should validate both the stored redirect and the user-entered
new_path before returning them. Update the redirect lookup/entry flow to ensure
the resolved path is absolute, exists, and is a directory before assigning it
for base_path, models_dir, or output_directory, and raise a clear error instead
of silently returning an invalid path. Use the existing redirect handling block
and new_path prompt flow to locate the fix.
Source: Coding guidelines
| print(f"\n[!] ALERT: {description} not found at: {saved_path}") | ||
| new_path= input(f"Please paste the updated absolute path for '{description}': ").strip() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid prompting from module import.
valid_path() is called while folder_paths.py imports, so a missing base/model/output directory can block indefinitely or raise EOFError in headless startup. Route relocation through an explicit validation/recovery step, or fail with a clear exception instead of calling input() here.
Also applies to: 46-48, 91-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@folder_paths.py` around lines 31 - 32, Module import should not block on
interactive path recovery in valid_path(). Move the relocation flow out of
folder_paths.py import-time logic so valid_path(), and the callers for
base/model/output paths, do not call input() during import; instead use an
explicit recovery/validation step or raise a clear exception when a path is
missing. Update the logic around valid_path and any related path initialization
helpers so they fail fast or are invoked only from an interactive entrypoint.
| folder_names_and_paths["detection"] = ([os.path.join(models_dir, "detection")], supported_pt_extensions) | ||
|
|
||
| output_directory = os.path.join(base_path, "output") | ||
| output_directory =valid_path( os.path.join(base_path, "output"),"Output Directory") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t require the default output directory to pre-exist.
output_directory is a writable destination; prompting when <base>/output is missing breaks fresh or relocated instances where the directory can be created. Keep validation for relocated existing roots, but create or tolerate the default output directory before downstream callers use it.
Proposed adjustment
-output_directory =valid_path( os.path.join(base_path, "output"),"Output Directory")
+output_directory = os.path.join(base_path, "output")
+os.makedirs(output_directory, exist_ok=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output_directory =valid_path( os.path.join(base_path, "output"),"Output Directory") | |
| output_directory = os.path.join(base_path, "output") | |
| os.makedirs(output_directory, exist_ok=True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@folder_paths.py` at line 91, The default output path in folder_paths.py is
being treated as if it must already exist, which blocks fresh setups. Update the
output_directory initialization around valid_path(os.path.join(base_path,
"output"), "Output Directory") so the default <base>/output is created or
accepted when missing, while still keeping validation behavior for relocated
existing roots. Use the existing output_directory and valid_path flow to ensure
downstream callers always get a writable destination without prompting for the
default case.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Description
This PR resolves the issue where missing or moved core model/instance directories cause the application to crash or drop into unhandled exception loops during initialization.
Changes
get_notes_pathorget_valid_pathhelper function.#14760 Fixes
Closes #14760