fix: destructive-boundary guard for clear-cache#198
Merged
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
wyli
force-pushed
the
fix/clear-cache-safety-boundary
branch
2 times, most recently
from
July 4, 2026 19:30
8d6707e to
6635a5e
Compare
`clear-cache` fed shutil.rmtree() with directories derived from env-overridable roots (HOLOSCAN_CLI_BUILD_PARENT_DIR / _DATA_DIR) and repo-root globs, with no safety boundary. A hostile or fat-fingered value such as `HOLOSCAN_CLI_BUILD_PARENT_DIR=/` made the command report `Would remove: /` and, without --dryrun under root, would wipe the filesystem. Add `_is_safe_to_remove()`: canonicalize each candidate and refuse it unless it is (1) not a critical anchor (`/`, $HOME, the repo root) or an ancestor of one, and (2) at or under an approved cache root (the repo tree, the build parent dir, or the data dir). Refused paths print a `Refusing to remove:` notice and are skipped. Also fix the policy regression where `test --clear-cache` forwarded the test namespace directly: with build/data/install unset, clear-cache treated it as "clear everything" including downloaded data. Select build/install explicitly, matching historical HoloHub behavior. Add destructive-boundary tests covering /, $HOME, the repo root, an ancestor of the repo root, the happy path, and data preservation on `test --clear-cache`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
wyli
force-pushed
the
fix/clear-cache-safety-boundary
branch
from
July 4, 2026 19:38
6635a5e to
a89374f
Compare
WalkthroughThis PR adds path-safety checks to ChangesClear-cache path-safety guardrails
test_cmd --clear-cache Namespace wiring
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
tests/unit/test_cli_behaviors.py (1)
400-405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBare
except Exception: passflagged by Ruff (S110/BLE001).Consider narrowing the caught exception type or at least logging/asserting on it so a real regression in
handle_test(unrelated to the intentionally-incomplete namespace) doesn't silently pass the test.♻️ Proposed tweak
try: test_cmd.handle_test(cli, args) - except Exception: + except Exception as exc: # noqa: BLE001 - namespace intentionally incomplete past clear-cache branch # Downstream steps may need more of the namespace than this focused # test supplies; the clear-cache selection is already captured. - pass + assert "clear_cache" not in str(exc).lower()🤖 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 `@tests/unit/test_cli_behaviors.py` around lines 400 - 405, The test in test_cli_behaviors around test_cmd.handle_test is swallowing all exceptions with a bare except Exception: pass, which hides real failures. Narrow the exception handling to the specific expected incomplete-namespace error for this scenario, or assert/log the exception before ignoring it, so handle_test regressions are not silently masked.Source: Linters/SAST tools
.github/workflows/main.yaml (1)
132-170: 🩺 Stability & Availability | 🔵 TrivialGating
buildon an external repo's test suite adds a flakiness/availability risk to the release path.
buildnow requiresholohub-integrationto succeed, which checks out and runsnvidia-holoscan/holohub's own wrapper test suite. If that upstream repo is unavailable, has an unrelated breakage, or its test suite regresses independently of this repo, every push/PR here gets blocked from producing a build artifact even thoughholoscan-cliitself is fine. Consider whether this reverse-integration check should becontinue-on-erroror otherwise decoupled from the release-critical path, or whether it should only be required onmain/release/*.🤖 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/workflows/main.yaml around lines 132 - 170, Decouple the external HoloHub reverse-integration check from the release-critical build path in the holohub-integration job and the build job’s needs list. Keep the HoloHub wrapper test run in main.yaml, but make it non-blocking for artifact production by using continue-on-error, moving it out of build dependencies, or scoping it to main/release workflows only. Reference the holohub-integration job and the build job dependency chain when updating the workflow..github/workflows/release.yaml (1)
253-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting shared smoke-test steps to reduce cross-workflow duplication.
The wheel/create-extra/sdist smoke sequence here is now duplicated verbatim between
main.yamlandrelease.yaml. A composite action would keep both in sync going forward, though this is optional given the current size of the duplication.🤖 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/workflows/release.yaml around lines 253 - 296, The smoke-test sequence in the release workflow duplicates the same wheel, create-extra, and sdist validation steps already present elsewhere, so refactor those repeated steps into a shared composite action or reusable workflow. Update the smoke-test job in release.yaml to call the shared unit identified by the current install/test steps (for example, the wheel install, Validate create extra from wheel, Install sdist in clean venv, and Source-distribution smoke test blocks) so both workflows stay in sync with one implementation.src/holoscan_cli/commands/run.py (1)
44-45: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueInconsistent quoting for
--build-withvs--configure-args.
_local_build_commandusesshlex.quote()for--configure-args(line 55) but naive double-quote interpolation for--build-with. Ifargs.with_operatorsever contains a", it would break the generated command string executed viabash -cin the builder container. This mirrors a pre-existing pattern elsewhere in the file, so it's not a regression, but worth tightening while touching this code.♻️ Suggested fix
if getattr(args, "with_operators", None): - command += f' --build-with "{args.with_operators}"' + command += f" --build-with {shlex.quote(args.with_operators)}"🤖 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/commands/run.py` around lines 44 - 45, The _local_build_command logic in run.py builds `--build-with` using naive double-quote interpolation, unlike the `--configure-args` handling, which can break the bash command when `args.with_operators` contains quotes or other shell-sensitive characters. Update the command construction in `_local_build_command` to quote `args.with_operators` safely using the same approach as `--configure-args` (for example via shlex.quote) so the generated string remains valid when executed by bash -c.
🤖 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.
Inline comments:
In @.github/workflows/main.yaml:
- Around line 134-165: The holohub-integration job is missing an explicit
permissions block and currently inherits broader default GITHUB_TOKEN access
than needed. Add a permissions setting for that job with contents: read only,
keeping the rest of the job steps unchanged and using the holohub-integration
job name as the anchor for the update.
- Around line 138-147: Both checkout steps in the workflow job are leaving git
credentials persisted by default, which is unnecessary here. Update the two
actions/checkout usages in the main workflow job to explicitly disable
credential persistence by setting persist-credentials to false for both the
holoscan-cli checkout and the HoloHub checkout, since no later step in this job
needs push or tag access.
In `@src/holoscan_cli/commands/clear_cache.py`:
- Around line 46-48: Guard the anchor resolution in clear_cache by handling
Path.home() as a best-effort lookup instead of assuming it always succeeds. In
the clear_cache flow around _resolve and the anchors set, catch the RuntimeError
from Path.home() and omit that anchor when the home directory cannot be
resolved, while still building anchors from _resolve("/") and
_resolve(cli.HOLOHUB_ROOT). This keeps clear-cache from aborting before the
removal checks run.
In `@src/holoscan_cli/commands/setup_cmd.py`:
- Around line 49-56: Always clear PYTHONHOME in the environment builder for
setup scripts, since the current _build_env logic in setup_cmd.py only removes
it inside the virtualenv branch and can leave a stale value behind under system
Python. Update the _build_env function so PYTHONHOME is removed unconditionally
from the copied env before returning it, while keeping the existing PATH and
VIRTUAL_ENV handling in place.
In `@src/holoscan_cli/utils/host_setup.py`:
- Around line 229-235: The apt-key pipeline in the host setup logic uses
PATH-based lookups for wget and gpg, which should be replaced with absolute
executable paths. Update the subprocess calls in the key download/dearmor flow
within the host setup routine so the commands reference fixed binaries directly
while preserving the existing pipeline behavior. Use the existing subprocess.run
calls around the kitware key retrieval and dearmor steps as the place to make
this change.
In `@src/holoscan_cli/utils/io.py`:
- Around line 233-240: The environment-preservation logic in the sudo command
builder is too permissive because the helper currently adds sudo -E alongside
the explicit NAME=value allowlist. Update the command construction in the io
utility so only the whitelisted variables from preserve_env are passed through
via /usr/bin/env, and remove the -E flag from both the execution and display
prefixes while keeping the existing allowlist loop intact.
---
Nitpick comments:
In @.github/workflows/main.yaml:
- Around line 132-170: Decouple the external HoloHub reverse-integration check
from the release-critical build path in the holohub-integration job and the
build job’s needs list. Keep the HoloHub wrapper test run in main.yaml, but make
it non-blocking for artifact production by using continue-on-error, moving it
out of build dependencies, or scoping it to main/release workflows only.
Reference the holohub-integration job and the build job dependency chain when
updating the workflow.
In @.github/workflows/release.yaml:
- Around line 253-296: The smoke-test sequence in the release workflow
duplicates the same wheel, create-extra, and sdist validation steps already
present elsewhere, so refactor those repeated steps into a shared composite
action or reusable workflow. Update the smoke-test job in release.yaml to call
the shared unit identified by the current install/test steps (for example, the
wheel install, Validate create extra from wheel, Install sdist in clean venv,
and Source-distribution smoke test blocks) so both workflows stay in sync with
one implementation.
In `@src/holoscan_cli/commands/run.py`:
- Around line 44-45: The _local_build_command logic in run.py builds
`--build-with` using naive double-quote interpolation, unlike the
`--configure-args` handling, which can break the bash command when
`args.with_operators` contains quotes or other shell-sensitive characters.
Update the command construction in `_local_build_command` to quote
`args.with_operators` safely using the same approach as `--configure-args` (for
example via shlex.quote) so the generated string remains valid when executed by
bash -c.
In `@tests/unit/test_cli_behaviors.py`:
- Around line 400-405: The test in test_cli_behaviors around
test_cmd.handle_test is swallowing all exceptions with a bare except Exception:
pass, which hides real failures. Narrow the exception handling to the specific
expected incomplete-namespace error for this scenario, or assert/log the
exception before ignoring it, so handle_test regressions are not silently
masked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbdad0f6-fe95-49a7-b92c-4345a5aa89c6
📒 Files selected for processing (26)
.github/CI.md.github/workflows/main.yaml.github/workflows/release.yamlsrc/holoscan_cli/commands/clear_cache.pysrc/holoscan_cli/commands/run.pysrc/holoscan_cli/commands/setup_cmd.pysrc/holoscan_cli/commands/test_cmd.pysrc/holoscan_cli/container/core.pysrc/holoscan_cli/container/parsers.pysrc/holoscan_cli/setup_scripts/Dockerfile.utilsrc/holoscan_cli/setup_scripts/benchmarking.shsrc/holoscan_cli/setup_scripts/debug.shsrc/holoscan_cli/setup_scripts/sccache.shsrc/holoscan_cli/setup_scripts/xvfb.shsrc/holoscan_cli/system_check.pysrc/holoscan_cli/utils/env_info.pysrc/holoscan_cli/utils/host_setup.pysrc/holoscan_cli/utils/io.pytests/unit/test_cli_behaviors.pytests/unit/test_container_core.pytests/unit/test_env_info.pytests/unit/test_host_setup.pytests/unit/test_io.pytests/unit/test_lifecycle_commands.pytests/unit/test_package_data.pytests/unit/test_setup_cmd.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_cli_behaviors.py (1)
296-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the caught exception type.
pytest.raises(Exception)is flagged by Ruff (B017) as a blind exception catch. The test relies oncontainer.verbose = args.verboseraisingAttributeError(the passedNamespaceomitsverbose); asserting that specific type documents intent and prevents the test from silently passing if an unrelated exception fires earlier for the wrong reason.♻️ Proposed fix
- with pytest.raises(Exception): # downstream local run needs more of the namespace + with pytest.raises(AttributeError): # downstream local run needs more of the namespace test_cmd.handle_test(cli, args)🤖 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 `@tests/unit/test_cli_behaviors.py` around lines 296 - 299, Update the test in test_cli_behaviors by narrowing the broad pytest.raises(Exception) around test_cmd.handle_test to the specific AttributeError raised when container.verbose is accessed from the incomplete Namespace. This makes the intent explicit and keeps the test from passing on unrelated failures; locate it via handle_test and the Namespace setup with clear_cache/local.Source: Linters/SAST tools
🤖 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 `@tests/unit/test_cli_behaviors.py`:
- Around line 296-299: Update the test in test_cli_behaviors by narrowing the
broad pytest.raises(Exception) around test_cmd.handle_test to the specific
AttributeError raised when container.verbose is accessed from the incomplete
Namespace. This makes the intent explicit and keeps the test from passing on
unrelated failures; locate it via handle_test and the Namespace setup with
clear_cache/local.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04b50610-789e-4abe-8c9b-22d243744bb5
📒 Files selected for processing (3)
src/holoscan_cli/commands/clear_cache.pysrc/holoscan_cli/commands/test_cmd.pytests/unit/test_cli_behaviors.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/holoscan_cli/commands/test_cmd.py
- src/holoscan_cli/commands/clear_cache.py
Path.home() raises RuntimeError when the home directory cannot be determined; treat it as a best-effort anchor lookup so clear-cache still runs its removal checks with the remaining anchors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… tests Move the path canonicalizer into holoscan_cli.utils.io as a public helper, condense the guard docstrings/comments, collapse the four-way dangerous-root parametrize into one loop, and make the test_cmd forwarding test stop deterministically via a SystemExit sentinel instead of a catch-all pytest.raises(Exception). Co-Authored-By: Claude Fable 5 <noreply@anthropic.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.
Problem
The cache roots are user-overridable (
HOLOSCAN_CLI_BUILD_PARENT_DIR/HOLOSCAN_CLI_DATA_DIR) andclear-cachepassed them straight toshutil.rmtree():Separately,
test --clear-cacheforwarded the raw test namespace, which clear-cache treats as "clear everything" — deleting the downloaded data cache instead of only build/install artifacts.Fix
clear-cachecanonicalizes each candidate (newholoscan_cli.utils.io.resolve()) and refuses to remove/,$HOME, the repo root, an ancestor of any of these, or anything outside the configured cache roots. Refused paths printRefusing to remove: <path>and are skipped. This is a backstop against the worst targets, not full validation of arbitrary overrides.test --clear-cachenow selects build/install explicitly; data is preserved.$HOMEtolerated, and the build/install-only selection.Testing
pytest tests/unitpasses; CI green on Python 3.10–3.13.🤖 Generated with Claude Code