components/clp-package-utils/clp_package_utils/general.py (2)

544-572: 🛠️ Refactor suggestion

⚠️ Potential issue

validate_path_for_container_mount — portability & overly broad restrictions

1. path.is_relative_to(...) is Python 3.9+. The rest of the project still targets 3.8 (per pyproject.toml / CI matrix). Falling back to a try/except path.relative_to(...) keeps 3.8 compatibility.

2. Blanket-banning /var prevents many standard Linux layouts (/var/lib/clp, /var/log/clp). Blocking the entire hierarchy may break existing deployments and contradict the PR goal (only avoid remapping host paths that collide with container internals). Narrow the restriction or make it configurable.

3. Missing docstring and RESTRICTED_PREFIXES could be _RESTRICTED_PREFIXES to silence Pylint C0103.

Suggested compatibility shim:

-        if path.is_relative_to(prefix):
+        try:
+            path.relative_to(prefix)
+        except ValueError:
+            continue
+        else:
             raise ValueError(...)

🧰 Tools

🪛 Pylint (3.3.7)

[convention] 544-544: Missing function or method docstring

(C0116)

---

[convention] 545-545: Variable name "RESTRICTED_PREFIXES" doesn't conform to snake_case naming style

(C0103)

---

368-374: 💡 Verification agent

❓ Verification inconclusive

Validate after hostname-suffix is appended

validate_path_for_container_mount runs before the data & logs directories are transformed into node-specific paths (path /= hostname).
If the appended hostname makes the path invalid (e.g., the parent dir passes validation but the suffixed full path exceeds a length restriction, changes its symlink resolution, etc.), the new value is never re-checked, and an invalid mount path may slip through.

Diff illustrating the suggested sequence:

-    validate_path_for_container_mount(clp_config.data_directory)
-    validate_path_for_container_mount(clp_config.logs_directory)
-
     # Make data and logs directories node-specific
     hostname = socket.gethostname()
     clp_config.data_directory /= hostname
     clp_config.logs_directory /= hostname

+    # Re-validate the final absolute paths
+    validate_path_for_container_mount(clp_config.data_directory)
+    validate_path_for_container_mount(clp_config.logs_directory)

---

Re-validate container mount paths after appending hostname

The current code only validates the base directories before adding the hostname suffix. Any changes introduced by appending the hostname (e.g. path-length limits, symlink resolution) won’t be caught. Move the calls to validate_path_for_container_mount to after the hostname is appended:

• File: components/clp-package-utils/clp_package_utils/general.py
Lines: ~368–374

Suggested diff:

-    validate_path_for_container_mount(clp_config.data_directory)
-    validate_path_for_container_mount(clp_config.logs_directory)
-
     # Append hostname to make data and logs directories node-specific
     hostname = socket.gethostname()
     clp_config.data_directory /= hostname
     clp_config.logs_directory /= hostname

+    # Re-validate the final absolute paths
+    validate_path_for_container_mount(clp_config.data_directory)
+    validate_path_for_container_mount(clp_config.logs_directory)

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

components/clp-package-utils/clp_package_utils/general.py (1)

545-577: Path.is_relative_to breaks Py 3.8 compatibility (still unresolved)

This helper still relies on path.is_relative_to, introduced in Python 3.9; CLP currently supports 3.8. On 3.8 the call raises AttributeError, blocking package startup.

Please switch to the safe try/except relative_to pattern, add a minimal doc-string, and fix the stray colon in the “must be absolute” error.

@@
-    if not path.is_absolute():
-        raise ValueError(f"Path: `{path}` must be absolute:")
+    if not path.is_absolute():
+        raise ValueError(f"Path `{path}` must be absolute.")
@@
-        if path.is_relative_to(prefix):
+        try:
+            path.relative_to(prefix)
+        except ValueError:
+            continue
         raise ValueError(
             f"Invalid path: `{path}` cannot be under '{prefix}' which may overlap with a path"
             f" in the container."
         )

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

components/clp-package-utils/clp_package_utils/general.py (2)

568-570: Tighten the “absolute path” error message.

The current string has an unnecessary “Path:” prefix and a trailing colon that makes the message look like a typo.

-    if not path.is_absolute():
-        raise ValueError(f"Path: `{path}` must be absolute:")
+    if not path.is_absolute():
+        raise ValueError(f"Path `{path}` must be absolute.")

---

571-573: Path.is_relative_to breaks Python 3.8 support – use relative_to with try/except.

pathlib.Path.is_relative_to only exists from 3.9 onwards.
The rest of the project still targets 3.8, so importing this module in a 3.8 runtime will raise AttributeError, making the CLI unusable.

-    for prefix in RESTRICTED_PREFIXES:
-        if path.is_relative_to(prefix):
+    for prefix in RESTRICTED_PREFIXES:
+        try:
+            path.relative_to(prefix)
+        except ValueError:
+            continue  # not under this prefix
         raise ValueError(
             f"Invalid path: `{path}` cannot be under '{prefix}' which may overlap with a path"
             f" in the container."
         )

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

components/clp-package-utils/clp_package_utils/general.py (1)

545-576: Python 3.8 incompatibility & minor polish in validate_path_for_container_mount

1. Path.is_relative_to is a Python 3.9 feature – calling it will raise an AttributeError in the 3.8 runtimes we still support.
2. The “must be absolute” error message has an extra trailing colon.
3. Pylint is flagging the missing doc-string.
4. The loop pattern can be kept 3.8-compatible with a try/except relative_to block.

Proposed drop-in patch:

-def validate_path_for_container_mount(path: pathlib.Path) -> None:
-    RESTRICTED_PREFIXES: List[pathlib.Path] = [
+def validate_path_for_container_mount(path: pathlib.Path) -> None:
+    """
+    Validate that a host path is absolute and does not live under a container-reserved
+    prefix that would shadow critical directories once mounted.
+    """
+    # pylint: disable=invalid-name
+    RESTRICTED_PREFIXES: List[pathlib.Path] = [
@@
-    if not path.is_absolute():
-        raise ValueError(f"Path: `{path}` must be absolute:")
+    if not path.is_absolute():
+        raise ValueError(f"Path `{path}` must be absolute.")
@@
-    for prefix in RESTRICTED_PREFIXES:
-        if path.is_relative_to(prefix):
+    for prefix in RESTRICTED_PREFIXES:
+        try:
+            path.relative_to(prefix)
+        except ValueError:
+            continue
         raise ValueError(
             f"Invalid path: `{path}` cannot be under '{prefix}' which may overlap with a path"
             f" in the container."
         )

This restores 3.8 compatibility, silences the style warning, and fixes the typo.

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro