Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 9 additions & 47 deletions marimo/_cli/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a faulty if -> elif cascade. Refactored this code and added tests.

# 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
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand Down
56 changes: 56 additions & 0 deletions marimo/_cli/sandbox.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,62 @@ 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:
from marimo._cli.sandbox import maybe_prompt_run_in_sandbox

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 ("==", ">=", "<=", ">", "<", "~"))

Expand Down
77 changes: 77 additions & 0 deletions tests/_cli/test_sandbox.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Loading