fix(packages): handle spaces in package extras - #10686
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
No issues found across 4 files
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
There was a problem hiding this comment.
🟡 Changes recommended
The new extras-aware tokenizer uses repeated string joins inside a per-character loop (potentially quadratic) which undermines the stated linear-parsing goal and should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes backend package parsing so valid PEP 508 requirements with whitespace inside extras (e.g. pydantic-ai[duckduckgo, web-fetch]) are preserved end-to-end, preventing malformed upgrade/version pins and ensuring notebook script metadata is only updated after successful operations.
Changes:
- Update package endpoints to only mutate notebook script metadata after a successful install/uninstall.
- Enhance the shared
split_packagestokenizer to treat full PEP 508 requirements atomically and to avoid splitting within extras that contain whitespace. - Add regression tests covering spaced extras, command generation, and failure paths that must not update metadata.
File summaries
| File | Description |
|---|---|
| tests/_server/api/endpoints/test_packages.py | Adds endpoint tests for spaced extras and ensures metadata isn’t updated on failed add/remove. |
| tests/_runtime/packages/test_pypi_package_manager.py | Verifies uv install command preserves spaced extras as a single requirement argument. |
| tests/_runtime/packages/test_package_utils.py | Expands tokenizer test coverage for valid PEP 508 requirements, spaced extras, and bracket edge cases. |
| marimo/_server/api/endpoints/packages.py | Gates update_notebook_script_metadata behind success for add/remove operations. |
| marimo/_runtime/packages/utils.py | Adds PEP 508 detection and an extras-aware tokenizer used by package managers and metadata updates. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
**This pull request was authored by a coding agent.** ## 📝 Summary Valid PEP 508 requirements may contain whitespace between extras. The package panel could pass a requirement such as `pydantic-ai[duckduckgo, web-fetch]` to the backend, where whitespace splitting produced a malformed version pin during upgrade. This change preserves complete PEP 508 requirements and makes the existing package-list tokenizer aware of extras, including lists such as `matplotlib pydantic-ai[duckduckgo, web-fetch]`. Parsing remains linear and shared by UI and non-UI callers. Notebook metadata is also updated only after a successful package operation, so failed installs or removals cannot change the notebook declaration. ### Original error When I tried to click upgrade: ```text error: Failed to parse: `pydantic-ai[duckduckgo,==2.33.0` Caused by: Expected an alphanumeric character starting the extra name, found `=` pydantic-ai[duckduckgo,==2.33.0 ```  ## 📋 Pre-Review Checklist - [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (not applicable; no public API change) - [ ] Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it. - [ ] Video or media evidence is provided for any visual changes (not applicable; no visual changes) ## ✅ Merge Checklist - [ ] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [x] Documentation has been updated where applicable, including docstrings for API changes. - [x] Tests have been added for the changes made. > Written by gpt-5 on Codex
This pull request was authored by a coding agent.
📝 Summary
Valid PEP 508 requirements may contain whitespace between extras. The package panel could pass a requirement such as
pydantic-ai[duckduckgo, web-fetch]to the backend, where whitespace splitting produced a malformed version pin during upgrade.This change preserves complete PEP 508 requirements and makes the existing package-list tokenizer aware of extras, including lists such as
matplotlib pydantic-ai[duckduckgo, web-fetch]. Parsing remains linear and shared by UI and non-UI callers. Notebook metadata is also updated only after a successful package operation, so failed installs or removals cannot change the notebook declaration.Original error
When I tried to click upgrade:
📋 Pre-Review Checklist
✅ Merge Checklist