Add stylua to supported tools and move off pre-commit#95
Conversation
kdeal
changed the title
|
Warning Rate limit exceeded
⌛ 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. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughReplaces pre-commit based linting and CI setup with a unified install-and-check flow: Buildkite and GitHub Actions now run Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2bee7b5b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| lint: | ||
| uv run ruff check --fix scripts tests | ||
| ruff check --fix scripts tests |
There was a problem hiding this comment.
just check is now the gate in both CI workflows, but lint runs ruff check --fix without --exit-non-zero-on-fix, so fixable violations are auto-rewritten and still return success. Ruff’s own help says --exit-non-zero-on-fix is required to fail when files were modified, which means a PR containing only fixable lint/format issues can pass CI while the checkout was mutated during the run instead of being rejected.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/cmd_install.py (1)
522-528: Consider catchingzipfile.BadZipFileexplicitly.Now that
extract_archivehandles ZIP files,zipfile.BadZipFileexceptions could occur but aren't explicitly caught. While it inherits fromExceptionand the code would still fail gracefully, adding explicit handling provides clearer error messages.♻️ Suggested change
except ( FileNotFoundError, OSError, tarfile.TarError, ValueError, subprocess.CalledProcessError, + zipfile.BadZipFile, ) as exc:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cmd_install.py` around lines 522 - 528, Update the exception handling around the archive extraction in the install routine to explicitly include zipfile.BadZipFile in the except tuple so ZIP-specific errors are logged with a clear message; locate the try/except that currently catches FileNotFoundError, OSError, tarfile.TarError, ValueError, and subprocess.CalledProcessError (the block using variable exc after calling extract_archive) and add zipfile.BadZipFile to that list so the same error handling/logging logic covers corrupted ZIP files explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/cmd_install.py`:
- Around line 217-236: The ZIP extraction uses zip_file.extractall() which is
vulnerable to ZipSlip; modify extract_archive to avoid extractall by iterating
zip_file.infolist(), compute the target path for each member as (destination /
member.filename).resolve(), verify that it is within destination.resolve()
(reject or skip any entries that do not start with destination.resolve()),
create parent directories as needed, then extract files by opening member with
zip_file.open(member) and writing to the verified target path (preserve
directories and file modes like the existing external_attr handling and chmod
logic). Replace the zip_file.extractall(destination) call with this safe
extraction loop so member.filename and permission handling (stored_mode,
user_executable_bit) remain applied only after verifying the path is safe.
---
Nitpick comments:
In `@scripts/cmd_install.py`:
- Around line 522-528: Update the exception handling around the archive
extraction in the install routine to explicitly include zipfile.BadZipFile in
the except tuple so ZIP-specific errors are logged with a clear message; locate
the try/except that currently catches FileNotFoundError, OSError,
tarfile.TarError, ValueError, and subprocess.CalledProcessError (the block using
variable exc after calling extract_archive) and add zipfile.BadZipFile to that
list so the same error handling/logging logic covers corrupted ZIP files
explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a506beaf-e3f9-456c-83bc-26142e063519
📒 Files selected for processing (8)
.buildkite/pipeline.yml.github/workflows/ci.yml.pre-commit-config.yamlcmd_install.tomljustfilescripts/cmd_install.pytests/test_cmd_install.pytests/test_verify_cmd_install_shas.py
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
| def extract_archive(archive: Path, destination: Path) -> None: | ||
| *_, final_suffix = tuple(archive.suffixes) | ||
| if final_suffix != ".zip": | ||
| shutil.unpack_archive(archive, destination, filter="data") | ||
| return | ||
|
|
||
| with zipfile.ZipFile(archive) as zip_file: | ||
| zip_file.extractall(destination) | ||
| for member in zip_file.infolist(): | ||
| if member.is_dir(): | ||
| continue | ||
|
|
||
| match suffixes: | ||
| case (*_, ".tar", ".gz") | (*_, ".tgz"): | ||
| mode = "r:gz" | ||
| case (*_, ".tar", ".bz2") | (*_, ".tbz2"): | ||
| mode = "r:bz2" | ||
| case (*_, ".tar", ".xz") | (*_, ".txz"): | ||
| mode = "r:xz" | ||
| case (*_, ".tar"): | ||
| mode = "r:" | ||
| case _: | ||
| raise ValueError(f"Unsupported tar archive extension for {archive.name}") | ||
| stored_mode = member.external_attr >> 16 | ||
| user_executable_bit = stored_mode & stat.S_IXUSR | ||
| if user_executable_bit == 0: | ||
| continue | ||
|
|
||
| with tarfile.open(archive, mode=mode) as tar: | ||
| tar.extractall(destination, filter="data") | ||
| extracted_path = destination / member.filename | ||
| current_mode = extracted_path.stat().st_mode | ||
| extracted_path.chmod(current_mode | user_executable_bit) |
There was a problem hiding this comment.
Potential path traversal vulnerability (ZipSlip) in ZIP extraction.
zipfile.ZipFile.extractall() does not sanitize member paths. A malicious ZIP archive containing entries like ../../../etc/passwd could write files outside the destination directory.
Unlike shutil.unpack_archive with filter="data" used for non-ZIP archives (which provides path safety), the zipfile.extractall() call lacks this protection.
🛡️ Proposed fix to validate member paths before extraction
def extract_archive(archive: Path, destination: Path) -> None:
*_, final_suffix = tuple(archive.suffixes)
if final_suffix != ".zip":
shutil.unpack_archive(archive, destination, filter="data")
return
with zipfile.ZipFile(archive) as zip_file:
+ for member in zip_file.namelist():
+ member_path = (destination / member).resolve()
+ if not member_path.is_relative_to(destination.resolve()):
+ raise ValueError(f"ZIP member '{member}' would extract outside destination")
zip_file.extractall(destination)
for member in zip_file.infolist():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cmd_install.py` around lines 217 - 236, The ZIP extraction uses
zip_file.extractall() which is vulnerable to ZipSlip; modify extract_archive to
avoid extractall by iterating zip_file.infolist(), compute the target path for
each member as (destination / member.filename).resolve(), verify that it is
within destination.resolve() (reject or skip any entries that do not start with
destination.resolve()), create parent directories as needed, then extract files
by opening member with zip_file.open(member) and writing to the verified target
path (preserve directories and file modes like the existing external_attr
handling and chmod logic). Replace the zip_file.extractall(destination) call
with this safe extraction loop so member.filename and permission handling
(stored_mode, user_executable_bit) remain applied only after verifying the path
is safe.
Codex pointed out that you need this to exit non zero when something is automatically fixed which is important in ci.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
justfile (1)
10-13: Consider adding check mode for CI to catch unformatted Lua/Fish files.Unlike the
linttarget which fails on fixes, theformattarget silently applies formatting without failing. When CI runsjust check, unformattedstyluaorfish_indentfiles would be formatted in the CI environment but the build would still pass, allowing improperly formatted code to be merged.Consider using check modes (
stylua --check .andfish_indent --check) for CI, or alternatively adding a git dirty check after formatting:git diff --exit-code🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@justfile` around lines 10 - 13, The current just target named format silently mutates files (target: format) so CI running just check won't fail on unformatted Lua/Fish; change the recipe so CI uses check-mode instead (use stylua --check for Lua and fish_indent --check for Fish) or add a post-format git dirty check (git diff --exit-code) in a separate check-format target invoked by CI; update or add targets named format and check-format (or add a CI-only flag) to ensure stylua and fish_indent run in --check mode (or that a git diff check runs) so the job fails when files are not properly formatted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@justfile`:
- Around line 10-13: The current just target named format silently mutates files
(target: format) so CI running just check won't fail on unformatted Lua/Fish;
change the recipe so CI uses check-mode instead (use stylua --check for Lua and
fish_indent --check for Fish) or add a post-format git dirty check (git diff
--exit-code) in a separate check-format target invoked by CI; update or add
targets named format and check-format (or add a CI-only flag) to ensure stylua
and fish_indent run in --check mode (or that a git diff check runs) so the job
fails when files are not properly formatted.
Rather than each command do the check, do a final check for diffs at the end.
1 participant
Summary by CodeRabbit
Chores
Developer Tools
Tests