fix: make pre-commit hooks portable on Windows - #6881
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughLocal pre-commit hooks now call Ruff, Ruff Format, Mypy, and pip-audit through direct ChangesPre-commit hook execution
Suggested reviewers: Priority: ➖ Normal — Impact reflects medium issue severity. Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Local hooks now use platform-independent uv commands, with configuration coverage for the removed Unix activation path. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/crewai/tests/test_pre_commit_config.py (1)
23-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the intended tool for each hook.
The current assertions only check the
uv runprefix. An entry could invoke the wrong tool for its hook ID and still pass. Assert command prefixes forruff,ruff-format,mypy, andpip-auditby hook ID.Proposed test strengthening
assert local_hooks assert all(hook["entry"].startswith("uv run ") for hook in local_hooks) assert all(".venv/bin/activate" not in hook["entry"] for hook in local_hooks) + expected_prefixes = { + "ruff": "uv run ruff check --config pyproject.toml", + "ruff-format": "uv run ruff format --config pyproject.toml", + "mypy": "uv run mypy --config-file pyproject.toml", + "pip-audit": "uv run pip-audit --skip-editable", + } + hooks_by_id = {hook["id"]: hook for hook in local_hooks} + assert set(expected_prefixes) <= set(hooks_by_id) + for hook_id, prefix in expected_prefixes.items(): + assert hooks_by_id[hook_id]["entry"].startswith(prefix)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/tests/test_pre_commit_config.py` around lines 23 - 25, Strengthen the assertions in the pre-commit hook test by mapping each hook ID to its expected command prefix and validating the corresponding hook["entry"] for ruff, ruff-format, mypy, and pip-audit. Retain the existing checks for local hooks, the uv run prefix, and absence of .venv/bin/activate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/crewai/tests/test_pre_commit_config.py`:
- Around line 23-25: Strengthen the assertions in the pre-commit hook test by
mapping each hook ID to its expected command prefix and validating the
corresponding hook["entry"] for ruff, ruff-format, mypy, and pip-audit. Retain
the existing checks for local hooks, the uv run prefix, and absence of
.venv/bin/activate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2cfc5518-d11e-48cc-bd79-f4186b07be67
📒 Files selected for processing (2)
.pre-commit-config.yamllib/crewai/tests/test_pre_commit_config.py
|
@coderabbitai review |
|
Vidit-Ostwal
left a comment
Vidit-Ostwal
left a comment
There was a problem hiding this comment.
Thanks — this is the right fix. Dropping source .venv/bin/activate and the bash -c wrapper is the portable version (Git Bash is no longer required). pass_filenames: true still appends paths onto uv run ruff/mypy ..., so that behavior is preserved.
Please rebase onto main before we can merge (mergeStateStatus is CONFLICTING). The pip-audit ignore list on main has grown (e.g. PYSEC-2026-139, GHSA-2wm9-hf6c-p5cr, GHSA-36p7-vc44-83pf, GHSA-xph7-9rjv-w5fr). Keep those entries; only change the entry wrapper from bash -c 'source .venv/bin/activate && uv run ...' to uv run ....
CodeRabbit's nit on the regression test is optional: asserting each hook id maps to the matching tool (ruff, ruff format, mypy, pip-audit) would make the test fail if an entry were copy-pasted onto the wrong hook. Not a merge blocker.
The parents[3] walk from lib/crewai/tests/ to the repo root is a bit brittle if this file ever moves, but it's fine for this check.
78c8fff to
571dd8a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
70c6021 to
c1051ad
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Hi, just checking in on PR #6881. I rebased it onto the latest |
|
Hi @Vidit-Ostwal, the required GitHub Actions runs for this fork PR are expiring while waiting for maintainer approval. CodeRabbit has passed and there are no new code comments; the branch is already rebased onto the latest |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Hi @Vidit-Ostwal, I pushed commit a604363 with a small test-isolation fix. The failed test matrix was caused by test_bedrock_completion_module_is_imported removing the Bedrock completion module from sys.modules without restoring it; it now uses pytest's monkeypatch cleanup. The original Windows pre-commit changes are unchanged. Could you please approve the new fork workflow runs so CI can execute? Thanks! |
Vidit-Ostwal
left a comment
Vidit-Ostwal
left a comment
There was a problem hiding this comment.
Keep Unix source .venv/bin/activate and add a Windows $OSTYPE branch instead of dropping activation.
…recommit # Conflicts: # .github/workflows/vulnerability-scan.yml # .pre-commit-config.yaml # pyproject.toml
2 participants
Summary
Fixes #6863.
The local pre-commit hooks sourced
.venv/bin/activatethrough Bash, which fails on Windows before Ruff, Ruff Format, mypy, or pip-audit can run. The hooks now invokeuv rundirectly, allowinguvto select the project environment on every supported platform.A regression test verifies that every local hook uses
uv runand does not depend on the Unix-only activation path.Validation
pytest lib/crewai/tests/test_pre_commit_config.py -q -n 0— passedpre-commit run --files lib/crewai/tests/test_pre_commit_config.py— Ruff and Ruff Format passedpre-commit validate-config— passedThe repository-wide all-files run still reports unrelated existing Ruff/mypy issues outside this change.