Fix bundled setup script fallback#187
Merged
Merged
Conversation
Dependency ReviewThe following issues were found:
License Issuessrc/holoscan_cli/setup_scripts/requirements.template.txt
OpenSSF Scorecard
Scanned Files
|
Bundle template setup requirements with the setup scripts and allow container extra-script layers to use setup script directories outside the active source project. This keeps the packaged fallback usable for downstream projects that do not vendor utilities/setup. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
Clarify that release-candidate fixes must merge to main before being cherry-picked to the release branch and used for a new RC workflow or Kitmaker release. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
wyli
force-pushed
the
fix/rc3-setup-script-fallback
branch
from
9ef0b2f to
caa72ee
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPackages bundled setup-scripts with pinned requirements, updates template.sh to use the bundled requirements, adjusts Dockerfile execution and HoloscanContainer build to support external/bundled script directories, adds tests and wheel-content checks, and documents an updated RC-fix workflow requiring main-first merges. ChangesBundled Setup Scripts with Packaged Requirements
Release Candidate Fix Workflow Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Use the requirements.template.txt file colocated with the bundled setup scripts as the only supported lookup path. Downstream projects that override setup scripts can copy the setup-script directory as one self-contained unit. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/CI.md (1)
133-138: ⚡ Quick winConsider consistent terminology for clarity.
Line 138 uses "has been picked onto" while line 134 uses "cherry-pick." For consistency and clarity, consider using "has been cherry-picked onto" in line 138.
📝 Suggested wording improvement
-Do not direct-push unreviewed fixes to `release/X.Y.0`, and do not submit an RC -workflow or Kitmaker release before the fix has merged to `main` and has been -picked onto the release branch. +Do not direct-push unreviewed fixes to `release/X.Y.0`, and do not submit an RC +workflow or Kitmaker release before the fix has merged to `main` and has been +cherry-picked onto the release branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/CI.md around lines 133 - 138, The wording is inconsistent: replace the phrase "has been picked onto the release branch" in the sentence that begins "Do not submit an RC workflow or Kitmaker release before the fix..." with "has been cherry-picked onto the release branch" so it matches the earlier use of "cherry-pick" and keeps terminology consistent; search for the sentence containing "has been picked onto" to locate and update it.src/holoscan_cli/container/core.py (1)
506-514: ⚡ Quick winValidate
extra_scriptsnames before building paths.Line 506 currently accepts tokens with path separators (for example
../foo), which can escape the intended setup-script namespace and target unintended files. Enforce basename-only script identifiers before resolving paths.Suggested patch
if extra_scripts: setup_scripts_dir = get_holohub_setup_scripts_dir() for script in extra_scripts: + if Path(script).name != script or script in {".", ".."}: + fatal( + f"Invalid script name {script!r}. " + "Use a setup script basename without path separators." + ) script_path = setup_scripts_dir / f"{script}.sh" if not script_path.exists(): fatal(f"Script {script}.sh not found in {setup_scripts_dir}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/holoscan_cli/container/core.py` around lines 506 - 514, The code currently builds script paths from potentially unsafe tokens in extra_scripts; before constructing script_path (where script is used), validate that script is a basename-only identifier (no path separators, no parent traversals, and not an absolute path). Concretely, in the block that uses script, add a check (e.g. ensure Path(script).name == script and not Path(script).is_absolute() and not any(sep in script for sep in ("/","\\")) ) and call fatal(...) if validation fails; then continue to compute script_path using setup_scripts_dir and resolve relative paths against HoloscanContainer.HOLOHUB_ROOT as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/CI.md:
- Around line 133-138: The wording is inconsistent: replace the phrase "has been
picked onto the release branch" in the sentence that begins "Do not submit an RC
workflow or Kitmaker release before the fix..." with "has been cherry-picked
onto the release branch" so it matches the earlier use of "cherry-pick" and
keeps terminology consistent; search for the sentence containing "has been
picked onto" to locate and update it.
In `@src/holoscan_cli/container/core.py`:
- Around line 506-514: The code currently builds script paths from potentially
unsafe tokens in extra_scripts; before constructing script_path (where script is
used), validate that script is a basename-only identifier (no path separators,
no parent traversals, and not an absolute path). Concretely, in the block that
uses script, add a check (e.g. ensure Path(script).name == script and not
Path(script).is_absolute() and not any(sep in script for sep in ("/","\\")) )
and call fatal(...) if validation fails; then continue to compute script_path
using setup_scripts_dir and resolve relative paths against
HoloscanContainer.HOLOHUB_ROOT as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33a0e8c2-83af-4a24-ae78-49e087068d1e
📒 Files selected for processing (8)
.github/CI.md.github/scripts/assert_wheel_contents.shsrc/holoscan_cli/container/core.pysrc/holoscan_cli/setup_scripts/Dockerfile.utilsrc/holoscan_cli/setup_scripts/requirements.template.txtsrc/holoscan_cli/setup_scripts/template.shtests/unit/test_container_core.pytests/unit/test_package_data.py
tbirdso
approved these changes
Jun 10, 2026
wyli
added a commit
that referenced
this pull request
Jun 11, 2026
* Fix bundled setup script fallback Bundle template setup requirements with the setup scripts and allow container extra-script layers to use setup script directories outside the active source project. This keeps the packaged fallback usable for downstream projects that do not vendor utilities/setup. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com> * Document PR-first RC fix flow Clarify that release-candidate fixes must merge to main before being cherry-picked to the release branch and used for a new RC workflow or Kitmaker release. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com> * Simplify bundled template requirements lookup Use the requirements.template.txt file colocated with the bundled setup scripts as the only supported lookup path. Downstream projects that override setup scripts can copy the setup-script directory as one self-contained unit. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Wenqi Li <wenqil@nvidia.com> --------- Signed-off-by: Wenqi Li <wenqil@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
AI-assisted: Created with Codex/GPT at the user's request.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests