fix argument splitting on '--' in the command line#9368
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
for more information, see https://pre-commit.ci
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
No issues found across 2 files
Architecture diagram
sequenceDiagram
participant User as CLI / Shell
participant Click as Click Framework
participant CLI as marimo.run()
participant Sys as sys.argv (Raw)
participant Splitter as _split_run_paths_and_args()
User->>Click: marimo run notebook.py -- arg=1
Note over Click: Click strips '--' from arguments
Click->>CLI: run(name="notebook.py", args=("arg=1",))
CLI->>Sys: Access raw process arguments
Sys-->>CLI: ["marimo", "run", "notebook.py", "--", "arg=1"]
alt NEW: Re-inject double dash sentinel
CLI->>CLI: Detect "--" in raw argv but missing in Click args
CLI->>CLI: Reconstruct args as ("--", "arg=1")
end
CLI->>Splitter: CHANGED: pass args with preserved sentinel
Note over Splitter: Logic identifies items after "--" <br/>as notebook params, not file paths
Splitter-->>CLI: return (paths, notebook_args)
CLI->>CLI: Continue execution with correct paths
There was a problem hiding this comment.
4 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pyproject.toml">
<violation number="1" location="pyproject.toml:6">
P0: Package renamed from `marimo` to `marimo-colobas` — this is a fork-specific change that must not be merged. It would break the package identity, PyPI publishing, and all existing installations.</violation>
<violation number="2" location="pyproject.toml:7">
P1: Version changed to `0.23.2.post1` — this fork-specific version bump should be reverted to avoid interfering with the project's release process.</violation>
<violation number="3" location="pyproject.toml:101">
P0: Optional dependency self-references renamed to `marimo-colobas[...]` — these must remain `marimo[...]` to match the actual package name.</violation>
<violation number="4" location="pyproject.toml:603">
P0: Pixi dev dependency renamed to `marimo-colobas` — revert to `marimo` to keep the local development workflow intact.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
|
||
| [tool.pixi.pypi-dependencies] | ||
| marimo = {path = ".", editable = true} | ||
| marimo-colobas = {path = ".", editable = true} |
There was a problem hiding this comment.
P0: Pixi dev dependency renamed to marimo-colobas — revert to marimo to keep the local development workflow intact.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 603:
<comment>Pixi dev dependency renamed to `marimo-colobas` — revert to `marimo` to keep the local development workflow intact.</comment>
<file context>
@@ -600,4 +600,4 @@ python = "3.12.*"
[tool.pixi.pypi-dependencies]
-marimo = {path = ".", editable = true}
+marimo-colobas = {path = ".", editable = true}
</file context>
| marimo-colobas = {path = ".", editable = true} | |
| marimo = {path = ".", editable = true} |
| recommended = [ | ||
| "marimo[sql]", | ||
| "marimo[sandbox]", # For `marimo edit --sandbox DIRECTORY` | ||
| "marimo-colobas[sql]", |
There was a problem hiding this comment.
P0: Optional dependency self-references renamed to marimo-colobas[...] — these must remain marimo[...] to match the actual package name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 101:
<comment>Optional dependency self-references renamed to `marimo-colobas[...]` — these must remain `marimo[...]` to match the actual package name.</comment>
<file context>
@@ -98,8 +98,8 @@ sandbox = [
recommended = [
- "marimo[sql]",
- "marimo[sandbox]", # For `marimo edit --sandbox DIRECTORY`
+ "marimo-colobas[sql]",
+ "marimo-colobas[sandbox]", # For `marimo edit --sandbox DIRECTORY`
"altair>=5.4.0", # Plotting in datasource viewer
</file context>
| "marimo-colobas[sql]", | |
| "marimo[sql]", |
| [project] | ||
| name = "marimo" | ||
| version = "0.23.2" | ||
| name = "marimo-colobas" |
There was a problem hiding this comment.
P0: Package renamed from marimo to marimo-colobas — this is a fork-specific change that must not be merged. It would break the package identity, PyPI publishing, and all existing installations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 6:
<comment>Package renamed from `marimo` to `marimo-colobas` — this is a fork-specific change that must not be merged. It would break the package identity, PyPI publishing, and all existing installations.</comment>
<file context>
@@ -3,8 +3,8 @@ requires = ["uv_build>=0.8.3,<0.12.0"]
[project]
-name = "marimo"
-version = "0.23.2"
+name = "marimo-colobas"
+version = "0.23.2.post1"
description = "A library for making reactive notebooks and apps"
</file context>
| name = "marimo-colobas" | |
| name = "marimo" |
| name = "marimo" | ||
| version = "0.23.2" | ||
| name = "marimo-colobas" | ||
| version = "0.23.2.post1" |
There was a problem hiding this comment.
P1: Version changed to 0.23.2.post1 — this fork-specific version bump should be reverted to avoid interfering with the project's release process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 7:
<comment>Version changed to `0.23.2.post1` — this fork-specific version bump should be reverted to avoid interfering with the project's release process.</comment>
<file context>
@@ -3,8 +3,8 @@ requires = ["uv_build>=0.8.3,<0.12.0"]
-name = "marimo"
-version = "0.23.2"
+name = "marimo-colobas"
+version = "0.23.2.post1"
description = "A library for making reactive notebooks and apps"
# We try to keep dependencies to a minimum, to avoid conflicts with
</file context>
| version = "0.23.2.post1" | |
| version = "0.23.2" |
|
sorry I overwrote the PR branch by mistake because I needed a wheel. I've reverted it back to the original PR state. Apologies |
peter-gy
left a comment
There was a problem hiding this comment.
Hey @colobas, thanks for the contribution, and welcome 👋
I could reproduce the bug. That said, could you please revise the fix to avoid reading from sys.argv inside run()? sys.argv is process-global state, while this command can also be invoked through Click tests, wrappers, or programmatic CLI entrypoints with an explicit args list. In those cases, sys.argv may not match the invocation that Click is actually handling.
We could handle this at the Click parsing boundary and pass the intended separator/argv state into the splitter explicitly. That would keep run() based on the command invocation it received.
📝 Summary
Fixes argument splitting in
marimo runwhen notebook args are passed after--.Problem
clickconsumes the--separator, so_split_run_paths_and_args()does not see it.As a result, trailing args may be interpreted as additional notebook paths (
NAME) and trigger errors like:Invalid NAME - ... is not a Python or Markdown file.Changes
_split_run_paths_and_args()does not receive--from CLI parsing.--sentinel semantics using rawsys.argvinrun()before splitting.--:key=value--key value--key-key valueValidation
📋 Pre-Review Checklist
✅ Merge Checklist