Repository navigation
Harden run ZIP import and fix review findings - #451
t0mdavid-m wants to merge 3 commits into
Conversation
Run ZIP import (RunManager.import_zip): - a run zipped while still running is imported as failed, not finished - refuse more than 100,000 files, a run.json over 1 MB and a params.json over 64 MB before anything is read or written - keep "created"/"finished" only when they are ISO timestamp strings - damaged member data (CRC, deflate) raises RunImportError instead of an uncaught zipfile/zlib error - accept a run with no result files (zip() leaves out an empty results/) - refuse Windows device names (NUL, CON, LPT1, ...) in member paths - the name with its " (n)" suffix stays within MAX_NAME_LENGTH Run import widgets: the message after an import says whether the run is shown on the results pages, and the results pages take it too, so it no longer waits for the Download page. The uploader names the upload limit. "All parameters" dialog: list values from pyOpenMS (bytes) and from params.json (newline-separated text) become lists of str, and the list editor is chosen from the parameter's type, not from its default. build-image.yml strips YAML quotes from the overlay's newName/newTag. package.sh notarizes with the API key only when key, key id and issuer are all set; otherwise it uses the Apple ID, or fails naming what is missing. The get_branding() docstring says what app_icon is for. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkEGengFwqzeVYN7rGGrrZ
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 minutes. View limit details
 |
| Layer / File(s) | Summary |
|---|---|
Archive validation and discovery src/workflow/RunManager.py, tests/test_run_import.py |
The importer enforces member-count and metadata-size limits, validates ZIP paths, and accepts archives identified by results/ or run.json. Damaged ZIP and unpacking errors produce distinct import errors. Tests cover archive limits, unsafe paths, discovery cases, and damaged member data. |
Imported run metadata and names src/workflow/RunManager.py, tests/test_run_import.py |
The importer validates metadata, timestamps, and parameter objects; maps running to failed; and creates unique names within the length limit. Tests cover statuses, timestamps, and duplicate overlong names. |
Import notices and UI integration src/workflow/run_widgets.py, src/workflow/WorkflowManager.py, tests/test_run_import_widgets.py |
The workflow displays a one-time success or warning notice, selects an imported run only when it is finished, and includes the upload-size limit in the uploader caption. Tests cover notice behavior. |
List Parameter Handling
| Layer / File(s) | Summary |
|---|---|
Normalize list parameter values src/workflow/StreamlitUI.py, tests/test_run_import_widgets.py |
A shared helper converts list and tuple items to strings and splits scalar values on newlines. The parameter dialog and row widgets use it for list values. |
Image Retagging
| Layer / File(s) | Summary |
|---|---|
Parse overlay image values .github/workflows/build-image.yml |
The retagging step strips single and double quotes from the overlay image name and tag before the existing empty-value check. |
macOS Notarization
| Layer / File(s) | Summary |
|---|---|
Validate notarization credentials electron/scripts/package.sh |
The API-key path requires key content, key ID, and issuer. When key content is present but a required credential is missing and no Apple ID is configured, the script fails with an error. |
Branding Documentation
| Layer / File(s) | Summary |
|---|---|
Clarify icon documentation src/common/common.py |
The app_icon documentation says app pages can use it, the template does not display it, and it is the default for logo_icon. |
Possibly related PRs
- OpenMS/streamlit-template#426: Adds a notarization hook that consumes the credential variables whose selection this pull request updates.
- OpenMS/streamlit-template#438: Adds the image name and tag lookups in the workflow that this pull request modifies.
- OpenMS/streamlit-template#433: Adds the macOS notarization credential-selection logic that this pull request updates.
Suggested reviewers: claude
Change: Bug fix
Merge Risk: 
89b93
-
Some list parameters may appear differently from the values passed to a tool, and certain unreadable ZIPs may show a traceback instead of an import error. These are bounded issues to address or accept before merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main change: hardening the run ZIP import in RunManager.import_zip. It also covers the review fixes. The summary and objectives both center on the import hardening, which makes up … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: hardening the run ZIP import in RunManager.import_zip. It also covers the review fixes. The summary and objectives both center on the import hardening, which makes up most of the changeset. The remaining edits (notarization, image tagging, parameter list handling, docstring) are secondary and not named, which is acceptable for a title.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 22 functions across 8 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each ZIP with care,
And finds the run metadata there.
List values line up, names stay neat,
The workflow shares a notice sweet.
New tags shed quotes before they meet.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
_as_str_list() split away empty entries and turned 0 into an empty list, so the "All parameters" dialog could show a value as unchanged while run_topp() passes the extra empty argument. It now splits like run_topp() does. import_zip() turns RuntimeError (encrypted members, unsupported compression, over-nested JSON) into RunImportError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkEGengFwqzeVYN7rGGrrZ
…st yet kubectl wait on a pod selector fails at once with "no matching resources found" when it runs before the controller Deployment has created its pod, which made test-nginx fail now and then (seen on arm64). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkEGengFwqzeVYN7rGGrrZ
|
Generated by Claude Code |
🤖 Generated with Claude Code
https://claude.ai/code/session_01FkEGengFwqzeVYN7rGGrrZ
Fixes the CodeRabbit findings from the template sync in OpenMS/umetaflow#41 that hold up on template
main. They are all in template code, so they reach every app through the next sync.Run ZIP import (
RunManager.import_zip)runningis imported asfailed, notfinished(its results may be partial).run.jsonover 1 MB, aparams.jsonover 64 MB.created/finishedare kept only when they are ISO timestamp strings. A number there brokelist_runs()sorting andformat_time().RunImportError("The ZIP is damaged") instead of an uncaughtBadZipFile/zlib.error.zip()leaves out an emptyresults/, so arun.jsonalone now counts as a run.NUL,CON,LPT1,aux.txt, ...) in member paths are refused.(n)collision suffix, stays withinMAX_NAME_LENGTH.Run import widgets
show_results_section()takes the message too (run_widgets.show_import_notice), so after an import from the results page it no longer waits for the Download page.server.maxUploadSize). Streamlit 1.51 has no per-widgetmax_upload_size, so the limit itself stays a deployment setting."All parameters" dialog (
StreamlitUI)_as_str_list), so an unchanged list no longer showsb'a'and selections keep every item.row["is_list"]), not from the type of its default.CI and docs
build-image.yml: quotes are stripped from the overlay'snewName/newTagbeforedocker tag.package.sh: notarizes with the API key only when key, key ID and issuer are all set. Otherwise it uses the Apple ID, or fails and names what is missing.get_branding()docstring:app_iconis for an app's own pages; the template shows it nowhere itself.Not changed
openms.yml): a package built for a commit is a valid build of that commit, as answered on umetaflow#41.params.jsoninParameterManager: real, but it needs a locking design across all writers. Better as its own change.Tests
tests/test_run_import.py: new cases for each import change above.tests/test_run_import_widgets.py: the import message (AppTest) and_as_str_list.python -m pytest tests/ test_gui.py: 470 passed. The 5 failures areraw_data_viewer.py, which needs pyopenms-viz'sms_plotlybackend; it is missing from the local environment, and the tests fail the same way onmainthere.Generated by Claude Code
Summary by CodeRabbit