fix: 'marimo edit' for absolute directory #7291
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mscolnick
requested review from
Copilot and
dmadisetti
and removed request for
dmadisetti
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where marimo edit /absolute/path/to/dir would fail to open notebooks. The fix adds path resolution logic to handle both absolute and relative file paths when a router is configured with an absolute directory.
- Adds a new
get_file_manageroverride inLazyListOfFilesAppFileRouterto resolve relative file paths against the router's directory - Implements security checks to prevent accessing files outside the allowed directory
- Includes comprehensive test coverage for the absolute directory path scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| tests/_server/test_file_manager_absolute_path.py | New comprehensive test file covering absolute directory path scenarios, including security tests for path traversal prevention |
| marimo/_server/file_router.py | Adds get_file_manager method override to LazyListOfFilesAppFileRouter class to handle path resolution for absolute directories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| files = router.files | ||
| assert len(files) > 0 | ||
| file_info = files[0] | ||
| print(f"File path from router.files: {file_info.path}") |
There was a problem hiding this comment.
Debug print statements should not be committed to the codebase. Remove this print statement or replace it with proper logging if this output is needed for debugging purposes.
| # Now simulate the frontend sending just the basename | ||
| # (This is what might be happening in the bug) | ||
| basename = "notebook.py" | ||
| print(f"Attempting to open with basename: {basename}") |
There was a problem hiding this comment.
Debug print statements should not be committed to the codebase. Remove this print statement or replace it with proper logging if this output is needed for debugging purposes.
| file_manager_from_basename = router.get_file_manager(basename) | ||
| assert file_manager_from_basename is not None | ||
| assert file_manager_from_basename.is_notebook_named | ||
| print("SUCCESS: Opened file with basename") |
There was a problem hiding this comment.
Debug print statements should not be committed to the codebase. Remove this print statement or replace it with proper logging if this output is needed for debugging purposes.
| print("SUCCESS: Opened file with basename") | |
| # Successfully opened file with basename |
| assert file_manager_from_basename.is_notebook_named | ||
| print("SUCCESS: Opened file with basename") | ||
| except HTTPException as e: | ||
| print(f"FAILED: {e.detail}") |
There was a problem hiding this comment.
Debug print statements should not be committed to the codebase. Remove this print statement or replace it with proper logging if this output is needed for debugging purposes.
| print(f"FAILED: {e.detail}") |
|
|
||
| # Try to open with relative path | ||
| relative_path = "subdir/notebook.py" | ||
| print(f"Attempting to open with relative path: {relative_path}") |
There was a problem hiding this comment.
Debug print statements should not be committed to the codebase. Remove this print statement or replace it with proper logging if this output is needed for debugging purposes.
| print(f"Attempting to open with relative path: {relative_path}") |
| file_manager = router.get_file_manager(relative_path) | ||
| assert file_manager is not None | ||
| assert file_manager.is_notebook_named | ||
| print("SUCCESS: Opened file with relative path") |
There was a problem hiding this comment.
Debug print statements should not be committed to the codebase. Remove this print statement or replace it with proper logging if this output is needed for debugging purposes.
| print("SUCCESS: Opened file with relative path") |
| assert file_manager.is_notebook_named | ||
| print("SUCCESS: Opened file with relative path") | ||
| except HTTPException as e: | ||
| print(f"FAILED: {e.detail}") |
There was a problem hiding this comment.
Debug print statements should not be committed to the codebase. Remove this print statement or replace it with proper logging if this output is needed for debugging purposes.
| print(f"FAILED: {e.detail}") |
|
|
||
| # All paths should be absolute | ||
| for file_info in files: | ||
| print(f"File: {file_info.name}, Path: {file_info.path}") |
There was a problem hiding this comment.
Debug print statements should not be committed to the codebase. Remove this print statement or replace it with proper logging if this output is needed for debugging purposes.
| print(f"File: {file_info.name}, Path: {file_info.path}") |
marimo edit /absolute/path/to/dirwould fail to open any notebooks. A relative directory likemarimo edit path/to/dirwould work, but not an absolute directory. This fixes that case.