uv download with SHA check and download retry on failure #11
Conversation
|
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a checksum-verified, retryable uv download/install flow to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Penv as penv.download_and_install_uv
participant Prep as _prepare_download_dirs
participant GH as GitHub Checksums
participant Net as Downloader
participant Ver as verify_download
participant Proc as _process_downloaded_archive
participant FS as Filesystem
Caller->>Penv: download_and_install_uv(cache_dir, retries, retry_delay)
Penv->>Prep: create temp/cache dirs
alt checksum available
Penv->>GH: fetch_uv_checksums_from_github()
GH-->>Penv: checksums
Penv->>Penv: get_expected_checksum()
else no checksum
Penv->>Penv: proceed without checksum
end
loop up to retries
Penv->>Net: download uv archive (with timeout)
Net-->>Penv: archive file
Penv->>Ver: verify_download(archive, checksum?)
alt verified or no checksum
Penv->>Proc: extract & install uv
Proc->>FS: place UV_EXE, chmod
Proc-->>Penv: path to UV_EXE
Penv-->>Caller: return UV_EXE
else checksum mismatch or failure
Penv->>FS: discard archive
Note over Penv: wait retry_delay then retry
end
end
alt retries exhausted
Penv-->>Caller: raise installation error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
pioinstaller/penv.py (7)
172-174: Prefer logging.exception inside except blocks.You’re in an exception handler; include traceback for faster diagnosis.
- log.error("Failed to verify checksum for %s: %s", - os.path.basename(file_path), e) + log.exception("Failed to verify checksum for %s", + os.path.basename(file_path))
100-117: Make musl detection more portable (no hardcoded x86_64 path, avoid partial executable path).Use shutil.which for ldd and glob for ld-musl across arches.
-def detect_musl(): +def detect_musl(): @@ - methods = [ - lambda: b"musl" in subprocess.check_output( - ["ldd", "--version"], stderr=subprocess.STDOUT - ), - lambda: os.path.exists("/lib/libc.musl-x86_64.so.1"), - lambda: "musl" in os.environ.get("LD_LIBRARY_PATH", ""), - ] + ldd = shutil.which("ldd") + methods = [ + lambda: ldd and b"musl" in subprocess.check_output( + [ldd, "--version"], stderr=subprocess.STDOUT + ), + # match any arch, e.g. /lib/ld-musl-aarch64.so.1 + lambda: any(fn for fn in glob.glob("/lib/ld-musl-*.so.1")), + lambda: "musl" in os.environ.get("LD_LIBRARY_PATH", ""), + ]Add imports:
+import glob(near the other imports)
237-270: Retry flow: don’t delete the archive preemptively; delete only on failure.Removing the file before every attempt disables util.download_file’s HEAD/cache optimization. Delete it only after a failed verify or exception.
- # Remove potentially corrupted file from previous attempt - if os.path.exists(config.archive_path): - os.remove(config.archive_path) util.download_file(config.uv_url, config.archive_path) @@ - raise exception.PIOInstallerException( - "Downloaded uv archive failed checksum verification" - ) + try: + os.remove(config.archive_path) + except OSError: + pass + raise exception.PIOInstallerException( + "Downloaded uv archive failed checksum verification" + ) @@ - ) as e: - log.debug("Attempt %d/%d failed: %s", attempt, retries, str(e)) + ) as e: + log.debug("Attempt %d/%d failed: %s", attempt, retries, str(e)) + try: + if os.path.exists(config.archive_path): + os.remove(config.archive_path) + except OSError: + pass return False
301-309: Surface a clear error when all retries fail.Either raise or at least log at error level so callers/users get context.
- # Clean up on final failure + # Clean up on final failure if os.path.exists(archive_path): try: os.remove(archive_path) except OSError: pass - - return None + log.error("Failed to download/install uv after %d attempts", retries) + return None
386-428: Emit traceback on unexpected venv-creation errors.Switch to logging.exception in the broad handler.
- except Exception as e: - log.error("Unexpected error creating venv with uv: %s", str(e)) - raise + except Exception: + log.exception("Unexpected error creating venv with uv") + raise
466-480: Include traceback when probing Python version fails.Improves diagnosability.
- except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: - log.error("Failed to get python version: %s", e) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired): + log.exception("Failed to get python version") raise exception.PIOInstallerException( "Could not determine python version") from e
504-510: Minor: prefer logging.exception on JSON load errors (optional).Not required, but consistent with other handlers.
- except (OSError, json.JSONDecodeError) as e: - raise exception.PIOInstallerException( - f"Could not load state file: {e}" - ) from e + except (OSError, json.JSONDecodeError) as e: + log.exception("Failed to load state file %s", state_path) + raise exception.PIOInstallerException( + f"Could not load state file: {e}" + ) from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pioinstaller/penv.py(13 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
📚 Learning: 2025-08-29T14:31:26.756Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:02.793Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:16.386Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T13:36:47.065Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Applied to files:
pioinstaller/penv.py
🧬 Code graph analysis (1)
pioinstaller/penv.py (3)
pioinstaller/exception.py (1)
PIOInstallerException(16-24)pioinstaller/util.py (2)
safe_remove_dir(149-155)download_file(89-113)pioinstaller/core.py (1)
check(224-293)
🪛 Ruff (0.12.2)
pioinstaller/penv.py
104-104: Starting a process with a partial executable path
(S607)
136-136: Consider moving this statement to an else block
(TRY300)
170-170: Consider moving this statement to an else block
(TRY300)
172-173: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
185-186: Avoid specifying long messages outside the exception class
(TRY003)
194-195: Avoid specifying long messages outside the exception class
(TRY003)
224-226: Avoid specifying long messages outside the exception class
(TRY003)
231-231: os.chmod setting a permissive mask 0o755 on file or directory
(S103)
253-255: Abstract raise to an inner function
(TRY301)
253-255: Avoid specifying long messages outside the exception class
(TRY003)
259-259: Consider moving this statement to an else block
(TRY300)
277-280: Avoid specifying long messages outside the exception class
(TRY003)
426-426: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
452-453: Avoid specifying long messages outside the exception class
(TRY003)
456-457: Avoid specifying long messages outside the exception class
(TRY003)
468-468: subprocess call: check for execution of untrusted input
(S603)
477-477: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
478-479: Avoid specifying long messages outside the exception class
(TRY003)
508-510: Avoid specifying long messages outside the exception class
(TRY003)
520-520: Consider moving this statement to an else block
(TRY300)
522-524: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (macos-latest, 3.9)
- GitHub Check: build (macos-13, 3.13)
- GitHub Check: build (macos-13, 3.10)
- GitHub Check: build (macos-13, 3.9)
🔇 Additional comments (4)
pioinstaller/penv.py (4)
94-97: LGTM: musl-aware platform selection.Replacing -gnu with -musl when detected is correct and matches uv asset names. (docs.wakemeops.com)
177-197: LGTM: basic traversal guard retained.Path prefix checks for zip/tar are fine for this trusted upstream and align with accepted project trade-offs.
430-458: LGTM: install uv (PyPI package) into the venv via system uv.Matches the project’s intent to keep the venv self-contained without depending on the bootstrap CLI.
34-46: LGTM: cohesive platform constants and penv plumbing.
- PYTHON_EXE/BIN_DIR/UV_EXE simplify cross-platform path handling.
- get_penv_bin_dir changes align with core.py usage.
- get_uv_executable correctly prefers PATH, then cache, then download.
- Download/extract/placement helpers are clean and predictable.
Also applies to: 343-349, 379-383, 311-333, 208-214, 216-235
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pioinstaller/penv.py (1)
181-201: Normalize checksum comparison to accept “sha256:” and case differencesPrevents false mismatches when the expected digest includes the algorithm prefix or different casing.
- calculated_hash = sha256_hash.hexdigest() - is_valid = calculated_hash == expected_sha256 + calculated_hash = sha256_hash.hexdigest().lower() + expected = (expected_sha256 or "").strip().lower() + if ":" in expected: + expected = expected.split(":", 1)[1] + is_valid = calculated_hash == expected
🧹 Nitpick comments (4)
pioinstaller/penv.py (4)
119-137: Return lowercased digest from parser to keep comparisons consistentMinor normalization; complements the verify change.
- return hash_value.strip() + return hash_value.strip().lower()
102-108: Avoid invoking a bare 'ldd' path; gate on availabilityPrevents S607 and avoids needless exceptions on systems without ldd.
-def detect_musl(): +def detect_musl(): """Detect musl libc more robustly.""" - methods = [ - lambda: b"musl" in subprocess.check_output( - ["ldd", "--version"], stderr=subprocess.STDOUT - ), + ldd_path = shutil.which("ldd") + methods = [ + lambda: bool(ldd_path) + and b"musl" in subprocess.check_output([ldd_path, "--version"], stderr=subprocess.STDOUT), lambda: os.path.exists("/lib/libc.musl-x86_64.so.1"), lambda: "musl" in os.environ.get("LD_LIBRARY_PATH", ""), ] - for method in methods: + for method in methods: try: if method(): return True except (subprocess.CalledProcessError, FileNotFoundError, OSError): continue return FalseAlso applies to: 110-115
203-205: Prefer logging.exception for caught exceptions; keep messages in exception typesSmall logging/style polish aligning with Ruff TRY400/TRY003.
- log.error("Failed to verify checksum for %s: %s", - os.path.basename(file_path), e) + log.exception("Failed to verify checksum for %s", os.path.basename(file_path))- log.error("Unexpected error creating venv with uv: %s", str(e)) + log.exception("Unexpected error creating venv with uv")- log.error("Failed to get python version: %s", e) - raise exception.PIOInstallerException( - "Could not determine python version") from e + log.exception("Failed to get python version") + raise exception.PIOInstallerException("Could not determine python version") from e- except (OSError, json.JSONDecodeError) as e: - raise exception.PIOInstallerException( - f"Could not load state file: {e}" - ) from e + except (OSError, json.JSONDecodeError) as e: + raise exception.PIOInstallerException(f"Could not load state file: {e}") from e- except OSError as e: - raise exception.PIOInstallerException( - f"Could not save state file: {e}" - ) from e + except OSError as e: + raise exception.PIOInstallerException(f"Could not save state file: {e}") from eAlso applies to: 457-458, 508-511, 539-541, 553-555
426-433: Optional: capture uv output on failure for easier diagnosticsRight now all output is suppressed; capturing stderr on failure would speed up triage.
- subprocess.run( - cmd, - check=True, - timeout=300, # 5 minutes timeout - stdout=subprocess.DEVNULL, - stderr=subprocess.STDOUT, - ) + proc = subprocess.run( + cmd, + check=False, + timeout=300, # 5 minutes timeout + capture_output=True, + text=True, + ) + if proc.returncode != 0: + log.debug("uv venv failed (rc=%s): %s", proc.returncode, proc.stderr or proc.stdout) + proc.check_returncode()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pioinstaller/penv.py(13 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
📚 Learning: 2025-08-29T14:31:26.756Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:16.386Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:02.793Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T13:36:47.065Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Applied to files:
pioinstaller/penv.py
🧬 Code graph analysis (1)
pioinstaller/penv.py (4)
pioinstaller/exception.py (1)
PIOInstallerException(16-24)pioinstaller/util.py (2)
safe_remove_dir(149-155)download_file(89-113)pioinstaller/core.py (1)
check(224-293)pioinstaller/python.py (1)
check(126-169)
🪛 Ruff (0.12.2)
pioinstaller/penv.py
104-104: Starting a process with a partial executable path
(S607)
166-166: Consider moving this statement to an else block
(TRY300)
201-201: Consider moving this statement to an else block
(TRY300)
203-204: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
216-217: Avoid specifying long messages outside the exception class
(TRY003)
225-226: Avoid specifying long messages outside the exception class
(TRY003)
255-257: Avoid specifying long messages outside the exception class
(TRY003)
262-262: os.chmod setting a permissive mask 0o755 on file or directory
(S103)
284-286: Abstract raise to an inner function
(TRY301)
284-286: Avoid specifying long messages outside the exception class
(TRY003)
290-290: Consider moving this statement to an else block
(TRY300)
308-311: Avoid specifying long messages outside the exception class
(TRY003)
457-457: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
483-484: Avoid specifying long messages outside the exception class
(TRY003)
487-488: Avoid specifying long messages outside the exception class
(TRY003)
499-499: subprocess call: check for execution of untrusted input
(S603)
508-508: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
509-510: Avoid specifying long messages outside the exception class
(TRY003)
539-541: Avoid specifying long messages outside the exception class
(TRY003)
551-551: Consider moving this statement to an else block
(TRY300)
553-555: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
pioinstaller/penv.py (1)
139-171: Unify UV asset URL and checksum retrieval in one API call
Fetch bothbrowser_download_urland SHA-256 digest from the same release snapshot to eliminate release-flip races. Replacefetch_uv_checksums_from_github()withfetch_uv_release_assets()(returning{"checksum", "url"}), updateget_expected_checksum()and theDownloadConfiginstantiation (with fallback) as in the suggested diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pioinstaller/penv.py (1)
183-204: Normalize checksum comparison to avoid false mismatches.Lowercase both sides and accept optional “sha256:” prefix.
- calculated_hash = sha256_hash.hexdigest() - is_valid = calculated_hash == expected_sha256 + calculated_hash = sha256_hash.hexdigest().lower() + expected = (expected_sha256 or "").strip().lower() + if ":" in expected: + expected = expected.split(":", 1)[1] + is_valid = calculated_hash == expected if not is_valid: log.error("Checksum mismatch for %s: expected %s, got %s", - os.path.basename(file_path), expected_sha256, + os.path.basename(file_path), expected, calculated_hash)
🧹 Nitpick comments (6)
pioinstaller/penv.py (6)
102-119: Broaden musl detection for non-x86_64 and Alpine.Current checks miss common cases like
/etc/alpine-releaseandld-musl-*on non-x86_64. Add a couple of cheap probes.def detect_musl(): @@ - methods = [ + methods = [ lambda: b"musl" in subprocess.check_output( ["ldd", "--version"], stderr=subprocess.STDOUT ), - lambda: os.path.exists("/lib/libc.musl-x86_64.so.1"), + # common across arches: ld-musl-<arch>.so.1 + lambda: any( + f.startswith("ld-musl-") and f.endswith(".so.1") + for f in os.listdir("/lib") + ), + # x86_64 libc path (kept for backward compatibility) + lambda: os.path.exists("/lib/libc.musl-x86_64.so.1"), + # Alpine indicator + lambda: os.path.exists("/etc/alpine-release"), lambda: "musl" in os.environ.get("LD_LIBRARY_PATH", ""), ]
249-268: Clean up extraction directory after installing uv.Leaving
uv-extractaround grows the cache over time.uv_dest = os.path.join(cache_dir, UV_EXE) shutil.copy2(uv_binary, uv_dest) if not util.IS_WINDOWS: os.chmod(uv_dest, 0o755) log.debug("uv installed at %s", uv_dest) + # best-effort cleanup + util.safe_remove_dir(extract_dir) return uv_dest
270-303: Fail closed when checksum is unavailable (env-guarded).Given the PR’s goal (“with SHA check”), proceeding unverified by default weakens the guarantee. Recommend failing by default with an override.
- if config.is_checksum_available(): - if not verify_download(config.archive_path, - config.expected_checksum): - raise exception.PIOInstallerException( - "Downloaded uv archive failed checksum verification" - ) - else: - log.warning("No checksum available for verification") + if config.is_checksum_available(): + if not verify_download(config.archive_path, config.expected_checksum): + raise exception.PIOInstallerException( + "Downloaded uv archive failed checksum verification" + ) + else: + if os.environ.get("PLATFORMIO_UV_ALLOW_UNVERIFIED", "").lower() not in ( + "1", + "true", + "yes", + ): + raise exception.PIOInstallerException( + "No checksum available for verification; set PLATFORMIO_UV_ALLOW_UNVERIFIED=1 to proceed unverified" + ) + log.warning( + "No checksum available for verification; proceeding unverified due to PLATFORMIO_UV_ALLOW_UNVERIFIED" + )
315-317: Allow pinning a specific uv tag for reproducibility.Support an env override to avoid “latest” drift, useful for CI and rollbacks.
- uv_url = UV_URL.format(platform=uv_platform, ext=ext) + tag = os.environ.get("PLATFORMIO_UV_TAG") + if tag: + uv_url = f"https://github.com/astral-sh/uv/releases/download/{tag}/uv-{uv_platform}.{ext}" + else: + uv_url = UV_URL.format(platform=uv_platform, ext=ext)
430-435: Capture subprocess output and use absolute path for uv.Improves debuggability and satisfies the partial-path lint.
- cmd = [uv_exe, "venv", "--python", python_exe, penv_dir] + uv_exe = os.path.abspath(uv_exe) + cmd = [uv_exe, "venv", "--python", python_exe, penv_dir] subprocess.run( cmd, check=True, - timeout=300, # 5 minutes timeout - stdout=subprocess.DEVNULL, - stderr=subprocess.STDOUT, + timeout=300, # 5 minutes timeout + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + text=True, ) @@ - except subprocess.CalledProcessError as e: - log.debug("Failed to create venv with uv: %s", str(e)) + except subprocess.CalledProcessError as e: + log.debug("Failed to create venv with uv: %s\n%s", str(e), e.stdout or "") return NoneAlso applies to: 450-461
510-514: Prefer log.exception to preserve traceback.Keeps context when python version probing fails.
- except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: - log.error("Failed to get python version: %s", e) + except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: + log.exception("Failed to get python version: %s", e) raise exception.PIOInstallerException( "Could not determine python version") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pioinstaller/penv.py(13 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
📚 Learning: 2025-08-29T14:31:26.756Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:02.793Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:16.386Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T13:36:47.065Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Applied to files:
pioinstaller/penv.py
🧬 Code graph analysis (1)
pioinstaller/penv.py (3)
pioinstaller/exception.py (1)
PIOInstallerException(16-24)pioinstaller/util.py (2)
safe_remove_dir(149-155)download_file(89-113)pioinstaller/core.py (1)
check(224-293)
🪛 Ruff (0.12.2)
pioinstaller/penv.py
106-106: Starting a process with a partial executable path
(S607)
168-168: Consider moving this statement to an else block
(TRY300)
203-203: Consider moving this statement to an else block
(TRY300)
204-204: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
218-219: Avoid specifying long messages outside the exception class
(TRY003)
227-228: Avoid specifying long messages outside the exception class
(TRY003)
257-259: Avoid specifying long messages outside the exception class
(TRY003)
264-264: os.chmod setting a permissive mask 0o755 on file or directory
(S103)
286-288: Abstract raise to an inner function
(TRY301)
286-288: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Consider moving this statement to an else block
(TRY300)
310-313: Avoid specifying long messages outside the exception class
(TRY003)
460-460: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
486-487: Avoid specifying long messages outside the exception class
(TRY003)
490-491: Avoid specifying long messages outside the exception class
(TRY003)
502-502: subprocess call: check for execution of untrusted input
(S603)
511-511: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
512-513: Avoid specifying long messages outside the exception class
(TRY003)
542-544: Avoid specifying long messages outside the exception class
(TRY003)
554-554: Consider moving this statement to an else block
(TRY300)
556-558: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (macos-latest, 3.9)
- GitHub Check: build (macos-latest, 3.10)
- GitHub Check: build (macos-13, 3.13)
🔇 Additional comments (3)
pioinstaller/penv.py (3)
94-99: Musl remapping for ARMv7 fixed — good catch.The additional
-gnueabihf→musleabihfmapping closes a real gap on Alpine armv7. Looks good.
141-169: Digest fetch + headers implemented correctly.Using the GitHub API headers and parsing string digests addresses the prior breakage. Nice.
478-491: Good: bounded install with timeout + clear error path.Installing Python “uv” inside the venv via the system uv is consistent with previous decisions and keeps the venv self-contained.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pioinstaller/penv.py (2)
94-99: Musl mapping fix is correct (adds gnueabihf → musleabihf).This resolves Alpine ARMv7 targeting issues.
183-210: Log normalized expected digest to avoid confusion.Comparison uses normalized value but error logs print the raw input.
Apply:
- if not is_valid: - log.error("Checksum mismatch for %s: expected %s, got %s", - os.path.basename(file_path), expected_sha256, - calculated_hash) - else: - log.debug("Checksum verified for %s", os.path.basename(file_path)) + if not is_valid: + log.error( + "Checksum mismatch for %s: expected %s, got %s", + os.path.basename(file_path), + expected, + calculated_hash, + ) + else: + log.debug("Checksum verified for %s", os.path.basename(file_path))
🧹 Nitpick comments (7)
pioinstaller/penv.py (7)
48-66: Consider a dataclass for DownloadConfig.Cuts boilerplate, makes intent clearer, and adds repr/eq.
Apply:
+from dataclasses import dataclass ... -class DownloadConfig: - """Configuration class for download parameters.""" - - def __init__(self, uv_url, archive_path, uv_platform, ext): - self.uv_url = uv_url - self.archive_path = archive_path - self.uv_platform = uv_platform - self.ext = ext - self.expected_checksum = None +@dataclass +class DownloadConfig: + """Configuration class for download parameters.""" + uv_url: str + archive_path: str + uv_platform: str + ext: str + expected_checksum: str | None = None
102-119: Broaden musl detection, reduce arch specificity.Checking only x86_64’s libc path may miss other arches; glob the musl soname instead.
Apply:
def detect_musl(): @@ - methods = [ + methods = [ lambda: bool(ldd_path) and b"musl" in subprocess.check_output([ldd_path, "--version"], stderr=subprocess.STDOUT), - lambda: os.path.exists("/lib/libc.musl-x86_64.so.1"), + lambda: any( + os.path.exists(p) + for p in ( + "/lib/libc.musl-x86_64.so.1", + "/lib/ld-musl-x86_64.so.1", + "/lib/ld-musl-aarch64.so.1", + "/lib/ld-musl-armhf.so.1", + "/lib/ld-musl-arm.so.1", + ) + ), lambda: "musl" in os.environ.get("LD_LIBRARY_PATH", ""), ]
252-271: Clean up temp files/dirs after successful install.Avoids cache bloat; wrap with try/finally or remove after copy.
Apply:
def _process_downloaded_archive(archive_path, extract_dir, cache_dir): """Process downloaded archive: extract and install binary.""" - util.safe_remove_dir(extract_dir) - os.makedirs(extract_dir, exist_ok=True) - _extract_uv_archive(archive_path, extract_dir) + util.safe_remove_dir(extract_dir) + os.makedirs(extract_dir, exist_ok=True) + _extract_uv_archive(archive_path, extract_dir) @@ - log.debug("uv installed at %s", uv_dest) - return uv_dest + log.debug("uv installed at %s", uv_dest) + # Best-effort cleanup + try: + util.safe_remove_dir(extract_dir) + if os.path.exists(archive_path): + os.remove(archive_path) + except OSError: + pass + return uv_dest
273-306: Narrow exception scope in _attempt_download.tar/zip errors can’t occur here (only download + hash). Keep it focused.
Apply:
- except ( - requests.RequestException, - tarfile.TarError, - zipfile.BadZipFile, - OSError, - exception.PIOInstallerException, - ) as e: + except (requests.RequestException, OSError, exception.PIOInstallerException) as e:
308-346: Optional: add jitter to backoff.Reduces synchronized retries when many instances run concurrently.
Apply:
- actual_delay = retry_delay * (2 ** (attempt - 1)) + import random + base = retry_delay * (2 ** (attempt - 1)) + actual_delay = base + random.uniform(0, base * 0.2) - log.debug("Retrying in %d seconds...", actual_delay) + log.debug("Retrying in %.1f seconds...", actual_delay)
441-451: Target the venv explicitly when installing the Python ‘uv’ package.Pass the venv’s Python to “uv pip” for determinism; VIRTUAL_ENV alone can be flaky.
Apply:
- expected_python = os.path.join(get_penv_bin_dir(penv_dir), PYTHON_EXE) + expected_python = os.path.join(get_penv_bin_dir(penv_dir), PYTHON_EXE) @@ - cmd = [system_uv_exe, "pip", "install", "uv"] + venv_python = os.path.join(get_penv_bin_dir(penv_dir), PYTHON_EXE) + cmd = [system_uv_exe, "pip", "install", "--python", venv_python, "uv"]Also applies to: 475-485
534-547: Nit: error message grammar.Consider “Could not find state.json file…” on Line 539.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pioinstaller/penv.py(13 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
📚 Learning: 2025-08-29T14:31:26.756Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:16.386Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T13:36:47.065Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Applied to files:
pioinstaller/penv.py
🧬 Code graph analysis (1)
pioinstaller/penv.py (4)
pioinstaller/exception.py (1)
PIOInstallerException(16-24)pioinstaller/util.py (2)
safe_remove_dir(149-155)download_file(89-113)pioinstaller/core.py (1)
check(224-293)pioinstaller/python.py (1)
check(126-169)
🪛 Ruff (0.12.2)
pioinstaller/penv.py
107-107: subprocess call: check for execution of untrusted input
(S603)
168-168: Consider moving this statement to an else block
(TRY300)
206-206: Consider moving this statement to an else block
(TRY300)
221-222: Avoid specifying long messages outside the exception class
(TRY003)
230-231: Avoid specifying long messages outside the exception class
(TRY003)
260-262: Avoid specifying long messages outside the exception class
(TRY003)
267-267: os.chmod setting a permissive mask 0o755 on file or directory
(S103)
289-291: Abstract raise to an inner function
(TRY301)
289-291: Avoid specifying long messages outside the exception class
(TRY003)
295-295: Consider moving this statement to an else block
(TRY300)
313-316: Avoid specifying long messages outside the exception class
(TRY003)
489-490: Avoid specifying long messages outside the exception class
(TRY003)
493-494: Avoid specifying long messages outside the exception class
(TRY003)
505-505: subprocess call: check for execution of untrusted input
(S603)
515-516: Avoid specifying long messages outside the exception class
(TRY003)
545-547: Avoid specifying long messages outside the exception class
(TRY003)
557-557: Consider moving this statement to an else block
(TRY300)
559-561: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
pioinstaller/penv.py (7)
34-37: Good: Centralized URLs for uv binary and API.Stable, readable, and easy to maintain.
38-46: Good: Sensible defaults and cross-platform constants.Retry defaults and BIN/PYTHON/UV_EXE constants look correct.
121-139: Checksum string parsing looks solid.Handles prefix, casing, and whitespace correctly.
141-174: GitHub API usage and asset digest handling LGTM.Headers and digest parsing are correct; logging is helpful.
213-233: Archive extraction path checks are acceptable for this project.Matches prior agreement to keep traversal checks basic for trusted uv source.
356-363: Good: reuse cached uv when present.Simple and fast path with executable check.
550-562: Good: state I/O with UTF-8 and exception wrapping.Clear errors and stable encoding.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pioinstaller/penv.py (2)
121-139: Digest string parser is correct and robust.Handles "sha256:", trims/normalizes case, and ignores non-sha256. Matches GitHub’s format.
141-169: GitHub API call + headers and checksum harvesting are implemented correctly.Using Accept + API-Version + UA, iterating assets, and capturing digests only for archive assets is aligned with the new API behavior.
🧹 Nitpick comments (7)
pioinstaller/penv.py (7)
190-206: Minor perf: increase hash read chunk size.Larger chunks speed up hashing of multi‑MB archives without changing behavior.
Apply:
- with open(file_path, "rb") as f: - for chunk in iter(lambda: f.read(4096), b""): + with open(file_path, "rb") as f: + for chunk in iter(lambda: f.read(1024 * 1024), b""): sha256_hash.update(chunk)
221-223: Include entry name in “Unsafe path in archive” errors.A tiny context boost for user diagnostics; doesn’t alter behavior.
Apply:
- raise exception.PIOInstallerException( - "Unsafe path in archive") + raise exception.PIOInstallerException( + f"Unsafe path in archive entry: {m.filename}")- raise exception.PIOInstallerException( - "Unsafe path in archive") + raise exception.PIOInstallerException( + f"Unsafe path in archive entry: {m.name}")Also applies to: 230-232
264-270: Make install atomic and ensure cache_dir exists.Avoids partial updates if multiple installers race or the process is interrupted mid‑copy.
Apply:
- uv_dest = os.path.join(cache_dir, UV_EXE) - shutil.copy2(uv_binary, uv_dest) - if not util.IS_WINDOWS: - os.chmod(uv_dest, 0o755) + uv_dest = os.path.join(cache_dir, UV_EXE) + os.makedirs(cache_dir, exist_ok=True) + uv_dest_tmp = uv_dest + ".tmp" + shutil.copy2(uv_binary, uv_dest_tmp) + if not util.IS_WINDOWS: + os.chmod(uv_dest_tmp, 0o755) + os.replace(uv_dest_tmp, uv_dest)
286-295: Reduce log noise when checksums are unavailable.Warn once; subsequent retries can be debug level.
Apply:
- else: - log.warning("No checksum available for verification") + else: + if attempt == 1: + log.warning("No checksum available for verification") + else: + log.debug("No checksum available for verification")
344-345: Actionable failure message on final retry.Give users clear next steps when download fails.
Apply:
- log.error("Failed to download/install uv after %d attempts", retries) + log.error( + "Failed to download/install uv after %d attempts. " + "Check network/proxy, GitHub availability, or install uv manually " + "and ensure it is on PATH.", + retries, + )
48-65: Use a dataclass for DownloadConfig.Removes boilerplate and adds lightweight type hints.
Apply:
-class DownloadConfig: - """Configuration class for download parameters.""" - - def __init__(self, uv_url, archive_path, uv_platform, ext): - self.uv_url = uv_url - self.archive_path = archive_path - self.uv_platform = uv_platform - self.ext = ext - self.expected_checksum = None - - def set_checksum(self, checksum): - """Set the expected checksum for verification.""" - self.expected_checksum = checksum - - def is_checksum_available(self): - """Check if checksum is available for verification.""" - return self.expected_checksum is not None +from dataclasses import dataclass + +@dataclass +class DownloadConfig: + """Configuration for uv download.""" + uv_url: str + archive_path: str + uv_platform: str + ext: str + expected_checksum: str | None = None + + def set_checksum(self, checksum: str | None) -> None: + self.expected_checksum = checksum + + def is_checksum_available(self) -> bool: + return self.expected_checksum is not NoneNote: add the import if not already present.
538-540: Fix grammar in error message.Tiny polish for user-facing text.
Apply:
- raise exception.PIOInstallerException( - "Could not found state.json file in `%s`" % state_path - ) + raise exception.PIOInstallerException( + "Could not find state.json file at `%s`" % state_path + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pioinstaller/penv.py(13 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
📚 Learning: 2025-08-29T14:31:26.756Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:16.386Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:02.793Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T13:36:47.065Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Applied to files:
pioinstaller/penv.py
🧬 Code graph analysis (1)
pioinstaller/penv.py (4)
pioinstaller/exception.py (1)
PIOInstallerException(16-24)pioinstaller/util.py (2)
safe_remove_dir(149-155)download_file(89-113)pioinstaller/core.py (1)
check(224-293)pioinstaller/python.py (1)
check(126-169)
🪛 Ruff (0.12.2)
pioinstaller/penv.py
107-107: subprocess call: check for execution of untrusted input
(S603)
168-168: Consider moving this statement to an else block
(TRY300)
206-206: Consider moving this statement to an else block
(TRY300)
221-222: Avoid specifying long messages outside the exception class
(TRY003)
230-231: Avoid specifying long messages outside the exception class
(TRY003)
260-262: Avoid specifying long messages outside the exception class
(TRY003)
267-267: os.chmod setting a permissive mask 0o755 on file or directory
(S103)
289-291: Abstract raise to an inner function
(TRY301)
289-291: Avoid specifying long messages outside the exception class
(TRY003)
295-295: Consider moving this statement to an else block
(TRY300)
313-316: Avoid specifying long messages outside the exception class
(TRY003)
489-490: Avoid specifying long messages outside the exception class
(TRY003)
493-494: Avoid specifying long messages outside the exception class
(TRY003)
505-505: subprocess call: check for execution of untrusted input
(S603)
515-516: Avoid specifying long messages outside the exception class
(TRY003)
545-547: Avoid specifying long messages outside the exception class
(TRY003)
557-557: Consider moving this statement to an else block
(TRY300)
559-561: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
pioinstaller/penv.py (2)
94-99: Musl platform mapping fix looks good.Covers both gnu and gnueabihf → musl variants. This should pick correct assets on Alpine x86_64 and ARMv7.
423-461: Venv creation flow with timeouts/readability is solid.Timeouts and explicit logging around subprocess calls are appropriate; fallback across candidate Pythons is clear.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pioinstaller/penv.py (1)
141-174: Digest parsing: also accept dict shape; keep connect/read timeout splitGitHub has returned both str and dict digests in the wild. Handle both to future‑proof. Minor: use (connect, read) timeouts.
- response = requests.get(UV_API_URL, headers=headers, timeout=30) + response = requests.get(UV_API_URL, headers=headers, timeout=(5, 30)) @@ - if asset_name.endswith((".tar.gz", ".zip")): - # GitHub digest is a string like "sha256:abc123..." - digest_str = asset.get("digest") - digest_hash = _parse_digest_string(digest_str) - if digest_hash: - checksums[asset_name] = digest_hash + if asset_name.endswith((".tar.gz", ".zip")): + digest = asset.get("digest") + digest_hash = None + if isinstance(digest, str): + digest_hash = _parse_digest_string(digest) + elif isinstance(digest, dict): + val = digest.get("sha256") + if val: + digest_hash = val.strip().lower() + if digest_hash: + checksums[asset_name] = digest_hash
🧹 Nitpick comments (7)
pioinstaller/penv.py (7)
48-61: Use a dataclass for DownloadConfig to cut boilerplate and clarify intentSimplifies init, typing, and default handling.
Apply:
+from dataclasses import dataclass @@ -class DownloadConfig: - """Configuration class for download parameters.""" - - def __init__(self, uv_url, archive_path, uv_platform, ext): - self.uv_url = uv_url - self.archive_path = archive_path - self.uv_platform = uv_platform - self.ext = ext - self.expected_checksum = None - - def set_checksum(self, checksum): - """Set the expected checksum for verification.""" - self.expected_checksum = checksum - - def is_checksum_available(self): - """Check if checksum is available for verification.""" - return self.expected_checksum is not None +@dataclass +class DownloadConfig: + """Configuration class for download parameters.""" + uv_url: str + archive_path: str + uv_platform: str + ext: str + expected_checksum: str | None = None + + def set_checksum(self, checksum: str | None) -> None: + self.expected_checksum = checksum + + def is_checksum_available(self) -> bool: + return self.expected_checksum is not NoneAdd the import at the top of the file:
from dataclasses import dataclass
102-119: Harden musl detection with an Alpine heuristicBusyBox ldd and minimal images can evade current checks. Add a cheap Alpine fingerprint.
def detect_musl(): @@ - methods = [ + methods = [ lambda: bool(ldd_path) and b"musl" in subprocess.check_output([ldd_path, "--version"], stderr=subprocess.STDOUT), lambda: os.path.exists("/lib/libc.musl-x86_64.so.1"), lambda: "musl" in os.environ.get("LD_LIBRARY_PATH", ""), + lambda: os.path.exists("/etc/alpine-release"), ]
221-223: Include the offending member name in the “Unsafe path in archive” errorHelps diagnostics without changing behavior.
- raise exception.PIOInstallerException( - "Unsafe path in archive") + raise exception.PIOInstallerException( + f"Unsafe path in archive: {m.filename}") @@ - raise exception.PIOInstallerException( - "Unsafe path in archive") + raise exception.PIOInstallerException( + f"Unsafe path in archive: {m.name}")Also applies to: 230-231
252-271: Clean up temp extract dir (and optionally the archive) on successPrevents cache/tmp bloat across runs.
uv_dest = os.path.join(cache_dir, UV_EXE) shutil.copy2(uv_binary, uv_dest) if not util.IS_WINDOWS: os.chmod(uv_dest, 0o755) log.debug("uv installed at %s", uv_dest) - return uv_dest + # Best-effort cleanup + try: + util.safe_remove_dir(extract_dir) + os.remove(archive_path) + except OSError: + log.debug("Cleanup of temp files failed: %s, %s", extract_dir, archive_path) + return uv_dest
283-294: Skip HEAD when we always force a re-download in retriesYou delete the file before downloading; disabling cache avoids the extra HEAD round‑trip per attempt.
- util.download_file(config.uv_url, config.archive_path) + util.download_file(config.uv_url, config.archive_path, cache=False)
431-436: Capture uv venv output for better diagnostics on failure (still silent on success)Currently errors lose stderr/stdout details. Capture them and emit on exception.
- subprocess.run( - cmd, - check=True, - timeout=300, # 5 minutes timeout - stdout=subprocess.DEVNULL, - stderr=subprocess.STDOUT, - ) + subprocess.run( + cmd, + check=True, + timeout=300, # 5 minutes timeout + capture_output=True, # silence on success, available on error + text=False, + ) @@ - except subprocess.CalledProcessError as e: - log.debug("Failed to create venv with uv: %s", str(e)) + except subprocess.CalledProcessError as e: + out = (e.stdout or b"") + (e.stderr or b"") + log.debug("Failed to create venv with uv (rc=%s): %s", e.returncode, out.decode(errors="ignore")) return NoneAlso applies to: 451-456
503-511: Minor: use text=True to avoid manual decode/stripSaves a decode and is clearer.
- python_version = ( - subprocess.check_output( - [python_exe, "-c", version_code], - stderr=subprocess.PIPE, - timeout=30 - ) - .decode() - .strip() - ) + python_version = subprocess.check_output( + [python_exe, "-c", version_code], + stderr=subprocess.PIPE, + timeout=30, + text=True, + ).strip()Also applies to: 512-516
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pioinstaller/penv.py(13 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
📚 Learning: 2025-08-29T14:31:26.756Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:16.386Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T13:36:47.065Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Applied to files:
pioinstaller/penv.py
🧬 Code graph analysis (1)
pioinstaller/penv.py (3)
pioinstaller/exception.py (1)
PIOInstallerException(16-24)pioinstaller/util.py (2)
safe_remove_dir(149-155)download_file(89-113)pioinstaller/core.py (1)
check(224-293)
🪛 Ruff (0.12.2)
pioinstaller/penv.py
107-107: subprocess call: check for execution of untrusted input
(S603)
168-168: Consider moving this statement to an else block
(TRY300)
206-206: Consider moving this statement to an else block
(TRY300)
221-222: Avoid specifying long messages outside the exception class
(TRY003)
230-231: Avoid specifying long messages outside the exception class
(TRY003)
260-262: Avoid specifying long messages outside the exception class
(TRY003)
267-267: os.chmod setting a permissive mask 0o755 on file or directory
(S103)
289-291: Abstract raise to an inner function
(TRY301)
289-291: Avoid specifying long messages outside the exception class
(TRY003)
295-295: Consider moving this statement to an else block
(TRY300)
311-314: Avoid specifying long messages outside the exception class
(TRY003)
477-477: subprocess call: check for execution of untrusted input
(S603)
488-489: Avoid specifying long messages outside the exception class
(TRY003)
492-493: Avoid specifying long messages outside the exception class
(TRY003)
504-504: subprocess call: check for execution of untrusted input
(S603)
514-515: Avoid specifying long messages outside the exception class
(TRY003)
544-546: Avoid specifying long messages outside the exception class
(TRY003)
556-556: Consider moving this statement to an else block
(TRY300)
558-560: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
pioinstaller/penv.py (4)
94-99: Musl mapping fix (incl. ARMv7) looks goodCovers both gnu and gnueabihf suffixes. This should resolve Alpine/ARMv7 asset selection mismatches.
183-206: Checksum verification normalization is correctLowercasing and optional “sha256:” stripping avoids false negatives. Good error messaging on mismatch.
390-401: PATH update for uv dir is a nice touchEnsures helper calls to “uv” work even if only the cached binary exists.
473-490: Installing the Python “uv” into the venv via system uv matches project intentAligned with prior decision to self-contain the venv. Timeouts and error surfacing look good.
Refactor DownloadConfig class to use dataclass for cleaner initialization and type hints.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pioinstaller/penv.py (1)
141-174: GitHub API headers + digest parsing fix are on pointThis addresses the API digest shape and ensures stable responses.
🧹 Nitpick comments (8)
pioinstaller/penv.py (8)
49-65: Use slots on DownloadConfigSmall memory/readability win; no behavior change.
-@dataclass +@dataclass(slots=True) class DownloadConfig:
102-119: Cache musl detection (avoids repeated subprocess checks)Musl probing can be relatively expensive; cache once per process.
+from functools import lru_cache @@ -def detect_musl(): +@lru_cache(maxsize=1) +def detect_musl(): """Detect musl libc more robustly."""
141-174: Memoize checksum fetch to reduce API traffic and rate-limit riskResults rarely change during a run; cache them.
@@ -def fetch_uv_checksums_from_github(): +from functools import lru_cache + +@lru_cache(maxsize=1) +def fetch_uv_checksums_from_github(): """Fetch SHA256 checksums from GitHub API."""
252-271: Clean up extracted temp directory after install (and optionally the archive)Avoids leaving ~cache/tmp/uv-extract on disk.
uv_dest = os.path.join(cache_dir, UV_EXE) shutil.copy2(uv_binary, uv_dest) if not util.IS_WINDOWS: os.chmod(uv_dest, 0o755) log.debug("uv installed at %s", uv_dest) + # Cleanup extraction directory + util.safe_remove_dir(extract_dir) return uv_destOutside this hunk (in download_and_install_uv), also delete the archive after success:
- return _process_downloaded_archive(archive_path, extract_dir, - cache_dir) + uv_path = _process_downloaded_archive(archive_path, extract_dir, cache_dir) + try: + os.remove(archive_path) + except OSError: + pass + return uv_path
306-344: Add jitter to backoff and clean tmp on final failureReduces thundering herd and tidies leftovers.
@@ - actual_delay = retry_delay * (2 ** (attempt - 1)) - log.debug("Retrying in %d seconds...", actual_delay) - time.sleep(actual_delay) + actual_delay = retry_delay * (2 ** (attempt - 1)) + # Add ~10% jitter + import random as _rnd # local import to avoid global dependency + jitter = int(max(1, actual_delay * 0.1)) + sleep_for = actual_delay + _rnd.randint(0, jitter) + log.debug("Retrying in %d seconds...", sleep_for) + time.sleep(sleep_for) @@ - if os.path.exists(archive_path): + if os.path.exists(archive_path): try: os.remove(archive_path) except OSError: pass + # Also remove any extraction leftovers + util.safe_remove_dir(extract_dir)
356-356: Windows: relax executable check for cached uvOn Windows, X_OK can be misleading; .exe existence is enough.
- if os.path.isfile(cached_uv) and os.access(cached_uv, os.X_OK): + if os.path.isfile(cached_uv) and (util.IS_WINDOWS or os.access(cached_uv, os.X_OK)):
431-436: Optional: capture uv venv output only on failureHelps diagnose failures without spamming stdout in the happy path.
- subprocess.run( - cmd, - check=True, - timeout=300, # 5 minutes timeout - stdout=subprocess.DEVNULL, - stderr=subprocess.STDOUT, - ) + completed = subprocess.run( + cmd, timeout=300, capture_output=True, text=True, check=False + ) + if completed.returncode != 0: + log.debug("uv venv failed: %s", completed.stdout or completed.stderr) + return NoneAlso applies to: 451-463
549-561: Write state.json atomicallyPrevents partial writes on crash/kill.
- try: - with open(state_path, "w", encoding="utf-8") as fp: - json.dump(state, fp, indent=2) - return state_path + try: + tmp_path = state_path + ".tmp" + with open(tmp_path, "w", encoding="utf-8") as fp: + json.dump(state, fp, indent=2) + os.replace(tmp_path, state_path) + return state_path
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pioinstaller/penv.py(14 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/pack/packer.py:28-33
Timestamp: 2025-08-30T12:37:02.793Z
Learning: In the pioarduino-core-installer project, the pack command and create_wheels function are used in development/CI contexts where uv is already available (e.g., via `uv run make pack`). The uv availability is ensured by the workflow context rather than needing explicit checks in the pack function itself.
📚 Learning: 2025-08-29T14:31:26.756Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#7
File: pioinstaller/penv.py:198-206
Timestamp: 2025-08-29T14:31:26.756Z
Learning: In the pioarduino-installer project, the `install_uv_in_venv_with_system_uv` function intentionally installs the Python "uv" package from PyPI into the virtual environment using `uv pip install uv`. This is not the Rust CLI tool but rather a Python package that provides uv functionality within the venv, allowing the environment to be self-contained without dependency on the bootstrap uv CLI.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T12:37:16.386Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/penv.py:71-89
Timestamp: 2025-08-30T12:37:16.386Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in _extract_uv_archive for uv downloads because astral-sh/uv is considered a trusted source, and additional security hardening against path traversal via symlinks/hardlinks is not required for this use case.
Applied to files:
pioinstaller/penv.py
📚 Learning: 2025-08-30T13:36:47.065Z
Learnt from: Jason2866
PR: Jason2866/pioarduino-core-installer#9
File: pioinstaller/util.py:116-127
Timestamp: 2025-08-30T13:36:47.065Z
Learning: In the pioarduino-installer project, Jason2866 accepts the current archive extraction implementation in unpack_archive function with basic path traversal protection. Additional security hardening against symlinks/hardlinks/device files is not required for this use case, following the principle that if it doesn't fail, it's good enough.
Applied to files:
pioinstaller/penv.py
🧬 Code graph analysis (1)
pioinstaller/penv.py (4)
pioinstaller/exception.py (1)
PIOInstallerException(16-24)pioinstaller/util.py (2)
safe_remove_dir(149-155)download_file(89-113)pioinstaller/core.py (1)
check(224-293)pioinstaller/python.py (1)
check(126-169)
🪛 Ruff (0.12.2)
pioinstaller/penv.py
107-107: subprocess call: check for execution of untrusted input
(S603)
168-168: Consider moving this statement to an else block
(TRY300)
206-206: Consider moving this statement to an else block
(TRY300)
221-222: Avoid specifying long messages outside the exception class
(TRY003)
230-231: Avoid specifying long messages outside the exception class
(TRY003)
260-262: Avoid specifying long messages outside the exception class
(TRY003)
267-267: os.chmod setting a permissive mask 0o755 on file or directory
(S103)
289-291: Abstract raise to an inner function
(TRY301)
289-291: Avoid specifying long messages outside the exception class
(TRY003)
295-295: Consider moving this statement to an else block
(TRY300)
311-314: Avoid specifying long messages outside the exception class
(TRY003)
477-477: subprocess call: check for execution of untrusted input
(S603)
488-489: Avoid specifying long messages outside the exception class
(TRY003)
492-493: Avoid specifying long messages outside the exception class
(TRY003)
504-504: subprocess call: check for execution of untrusted input
(S603)
514-515: Avoid specifying long messages outside the exception class
(TRY003)
544-546: Avoid specifying long messages outside the exception class
(TRY003)
556-556: Consider moving this statement to an else block
(TRY300)
558-560: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (11)
pioinstaller/penv.py (11)
35-47: Good constants factoringCentralizing URL, retry, and platform-specific exe/bin names looks solid.
94-99: musl mapping fix is correctThe gnueabihf→musleabihf handling closes the ARMv7 gap.
121-139: Robust digest-string parserHandles "sha256:" cleanly and normalizes case. LGTM.
176-181: LGTM: expected checksum lookupSimple filename match against fetched assets is fine.
183-211: LGTM: checksum verificationNormalization and streaming hash are correct.
221-223: Path traversal guard in archive extraction is goodThe prefix check prevents writing outside extract_dir.
Also applies to: 230-232
244-250: LGTM: temp paths creationPredictable tmp layout under cache_dir is fine.
273-304: LGTM: single-attempt download with verificationRemoves stale partials and gates success on checksum when available.
371-384: LGTM: dir helpers docstringsClear and consistent with new constants.
473-475: LGTM: install uv (Python package) into venv via system uvMatches the accepted workflow for a self-contained penv.
Also applies to: 480-494
496-516: LGTM: timed python version probe with clear errorTimeout and exception mapping are appropriate.
Summary by CodeRabbit
New Features
Bug Fixes