diff --git a/marimo/_cli/cli.py b/marimo/_cli/cli.py index b6531baa3a2..7679b41024b 100644 --- a/marimo/_cli/cli.py +++ b/marimo/_cli/cli.py @@ -513,6 +513,8 @@ def edit( name: Optional[str], args: tuple[str, ...], ) -> None: + from marimo._cli.sandbox import run_in_sandbox, should_run_in_sandbox + # We support unix-style piping, e.g. cat notebook.py | marimo edit if name is None and (stdin_contents := _get_stdin_contents()) is not None: temp_dir = tempfile.TemporaryDirectory() @@ -535,45 +537,9 @@ def edit( ) return - # Dangerous sandbox can be forced on by setting an environment variable; - # this allows our VS Code extension to force sandbox regardless of the - # marimo version. - if sandbox and os.getenv("MARIMO_DANGEROUS_SANDBOX"): - dangerous_sandbox = True - - if dangerous_sandbox and (name is None or os.path.isdir(name)): - sandbox = True - click.echo( - click.style( - "Warning: Using sandbox with multi-notebook edit servers is dangerous.\n", - fg="yellow", - ) - + "Notebook dependencies may not be respected, may not be written, and may be overwritten.\n" - + "Learn more: https://github.com/marimo-team/marimo/issues/5219l.\n", - err=True, - ) - - if sandbox is None: - # When the sandbox flag is omitted we infer whether to - # to start in sandbox mode by examining the notebook file and - # prompting the user. - from marimo._cli.sandbox import maybe_prompt_run_in_sandbox - - sandbox = maybe_prompt_run_in_sandbox(name) - elif ( - sandbox - and not dangerous_sandbox - and (name is None or os.path.isdir(name)) + if should_run_in_sandbox( + sandbox=sandbox, dangerous_sandbox=dangerous_sandbox, name=name ): - raise click.UsageError( - """marimo's package sandbox requires a notebook name: - - * marimo edit --sandbox my_notebook.py - - Multi-notebook sandboxed servers (marimo edit --sandbox) are not supported. - Follow this issue at: https://github.com/marimo-team/marimo/issues/2598.""" - ) - elif sandbox: from marimo._cli.sandbox import run_in_sandbox # TODO: consider adding recommended as well @@ -1009,6 +975,8 @@ def run( name: str, args: tuple[str, ...], ) -> None: + from marimo._cli.sandbox import run_in_sandbox, should_run_in_sandbox + # If file is a url, we prompt to run in docker # We only do this for remote files, # but later we can make this a CLI flag @@ -1023,15 +991,9 @@ def run( ) return - # Set default, if not provided - if sandbox is None: - from marimo._cli.sandbox import maybe_prompt_run_in_sandbox - - sandbox = maybe_prompt_run_in_sandbox(name) - - if sandbox: - from marimo._cli.sandbox import run_in_sandbox - + if should_run_in_sandbox( + sandbox=sandbox, dangerous_sandbox=None, name=name + ): run_in_sandbox(sys.argv[1:], name=name) return diff --git a/marimo/_cli/sandbox.py b/marimo/_cli/sandbox.py index 0e16a3e7b7d..fdd8df89aa7 100644 --- a/marimo/_cli/sandbox.py +++ b/marimo/_cli/sandbox.py @@ -75,6 +75,60 @@ def maybe_prompt_run_in_sandbox(name: str | None) -> bool: return False +def should_run_in_sandbox( + sandbox: bool | None, dangerous_sandbox: bool | None, name: str | None +) -> bool: + """Return whether the named notebook should be run in a sandbox. + + Prompts the user if sandbox is None and the notebook has sandbox metadata. + + The `sandbox` arg is whether the user requested sandbox. Even + if running in sandbox was requested, it may not be allowed + if the target is a directory (unless overridden by `dangerous_sandbox`). + """ + + # Dangerous sandbox can be forced on by setting an environment variable; + # this allows our VS Code extension to force sandbox regardless of the + # marimo version. + if sandbox and os.getenv("MARIMO_DANGEROUS_SANDBOX"): + dangerous_sandbox = True + + if dangerous_sandbox and (name is None or os.path.isdir(name)): + sandbox = True + click.echo( + click.style( + "Warning: Using sandbox with multi-notebook edit servers is dangerous.\n", + fg="yellow", + ) + + "Notebook dependencies may not be respected, may not be written, and may be overwritten.\n" + + "Learn more: https://github.com/marimo-team/marimo/issues/5219l.\n", + err=True, + ) + + # When the sandbox flag is omitted we infer whether to + # to start in sandbox mode by examining the notebook file and + # prompting the user. + if sandbox is None: + sandbox = maybe_prompt_run_in_sandbox(name) + + # Validation: we don't yet support multi-notebook sandboxed servers. + if ( + sandbox + and not dangerous_sandbox + and (name is None or os.path.isdir(name)) + ): + raise click.UsageError( + """marimo's package sandbox requires a notebook name: + + * marimo edit --sandbox my_notebook.py + + Multi-notebook sandboxed servers (marimo edit --sandbox) are not supported. + Follow this issue at: https://github.com/marimo-team/marimo/issues/2598.""" + ) + + return sandbox + + def _is_versioned(dependency: str) -> bool: return any(c in dependency for c in ("==", ">=", "<=", ">", "<", "~")) diff --git a/tests/_cli/test_sandbox.py b/tests/_cli/test_sandbox.py index 61f5e6a0c25..6e64f5e192a 100644 --- a/tests/_cli/test_sandbox.py +++ b/tests/_cli/test_sandbox.py @@ -3,9 +3,13 @@ from typing import TYPE_CHECKING, Any from unittest.mock import patch +import click +import pytest + from marimo._cli.sandbox import ( _normalize_sandbox_dependencies, construct_uv_command, + should_run_in_sandbox, ) from marimo._utils.inline_script_metadata import PyProjectReader @@ -406,3 +410,76 @@ def test_markdown_sandbox_and_header(tmp_path: Path) -> None: with open(req_file_path) as f: requirements = f.read() assert "numpy" in requirements + + +def test_should_run_in_sandbox_user_confirms(tmp_path: Path) -> None: + """Test that should_run_in_sandbox returns True when user types 'y'.""" + # Create a file with dependencies + script_path = tmp_path / "test.py" + script_path.write_text( + """ +# /// script +# dependencies = ["numpy"] +# /// +import marimo + """ + ) + + # Mock the prompt to return True (simulating user typing 'y') + with patch("marimo._cli.sandbox.click.confirm", return_value=True): + with patch( + "marimo._cli.sandbox.DependencyManager.which", + return_value="/usr/bin/uv", + ): + with patch( + "marimo._cli.sandbox.sys.stdin.isatty", return_value=True + ): + result = should_run_in_sandbox( + sandbox=None, + dangerous_sandbox=None, + name=str(script_path), + ) + assert result + + +def test_should_run_in_sandbox_explicit_flag() -> None: + """Test that should_run_in_sandbox returns True when sandbox=True.""" + result = should_run_in_sandbox( + sandbox=True, + dangerous_sandbox=None, + name="test.py", + ) + assert result + + +def test_should_run_in_sandbox_explicit_false() -> None: + """Test that should_run_in_sandbox returns False when sandbox=False.""" + result = should_run_in_sandbox( + sandbox=False, + dangerous_sandbox=None, + name="test.py", + ) + assert not result + + +def test_should_run_in_sandbox_dangerous_sandbox(tmp_path: Path) -> None: + """Test that dangerous_sandbox allows sandbox for directories.""" + dir_path = tmp_path / "notebooks" + dir_path.mkdir() + + # Without dangerous_sandbox, should raise an error + with pytest.raises(click.UsageError): + should_run_in_sandbox( + sandbox=True, + dangerous_sandbox=False, + name=str(dir_path), + ) + + # With dangerous_sandbox, should work + with patch("click.echo"): + result = should_run_in_sandbox( + sandbox=True, + dangerous_sandbox=True, + name=str(dir_path), + ) + assert result