add git hooks #133
add git hooks #133
Conversation
WalkthroughAdds Makefile targets reorganizing formatting/linting/test workflows, makes Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant Git
participant Hook as .git/hooks/pre-commit
participant Make as Makefile
participant Checks as Pre-commit Checks
Developer->>Git: git commit
Git->>Hook: run pre-commit hook
Hook->>Make: exec make pre-commit
Make->>Checks: run black-check, docstyle, pyright, pylint, ruff, check-types, bandit
Checks-->>Make: success/failure
Make-->>Hook: exit status
Hook-->>Git: allow or abort commit
sequenceDiagram
actor Developer
participant Git
participant Hook as .git/hooks/pre-push
participant Pytest as pytest
Developer->>Git: git push
Git->>Hook: run pre-push hook
Hook->>Pytest: pytest --cov=src --cov-report=term-missing
Pytest-->>Hook: success/failure
Hook-->>Git: allow or abort push
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @githooks/pre-commit:
- Around line 8-13: Replace the direct `uv run` commands with consistent `make`
targets and add the missing security scan: replace `uv run black --check .` and
`uv run pydocstyle -v .` with the corresponding make targets (e.g., `make
black-check` / `make pydocstyle` or use the consolidated `make verify` if it
covers them), ensure `make verify` (or equivalent) is used instead of
duplicating checks, and add a `make bandit` (or call `make security`) step to
run Bandit before other linters; update the githooks/pre-commit sequence to only
invoke make targets (including the new Bandit target) for consistency and
completeness.
In @Makefile:
- Around line 24-26: The git-hooks Makefile target should be made more robust:
update the git-hooks recipe (target name "git-hooks") to iterate a curated list
of hook filenames instead of using a wildcard, check if an existing hook file is
a symlink and warn (and skip or back it up) before overwriting non-symlink
hooks, ensure each hook script is marked executable (chmod +x) before linking,
create a relative symlink into .git/hooks for each approved hook, and print
confirmation for each installed/skipped hook so maintainers get clear feedback.
🧹 Nitpick comments (3)
githooks/pre-push (1)
1-8: Consider adding progress output for better user experience.The hook implementation is correct with appropriate error handling via
set -eo pipefail. However, adding echo statements to show what's running would improve the developer experience, especially when the test suite takes time to execute.💬 Suggested enhancement with progress output
#!/bin/bash # Pre-push hook # If any command fails, exit immediately with that command's exit status set -eo pipefail +echo "Running pre-push checks..." +echo "Executing test suite with coverage..." + # Run all tests uv run python -m pytest tests --cov=src --cov-report term-missing + +echo "✓ Pre-push checks passed!"githooks/pre-commit (2)
15-16: Remove commented-out code.The commented-out pytest command should be removed rather than left as a comment. The commit message or git history already documents that tests were moved to the pre-push hook.
♻️ Proposed cleanup
- -# Tests moved to pre-push hook -# uv run python -m pytest tests --cov=src --cov-report term-missing
1-16: Consider adding progress output for better developer experience.Similar to the pre-push hook, adding echo statements to indicate which checks are running would improve the developer experience, especially since the PR description acknowledges these checks might be slow.
💬 Example with progress output
#!/bin/bash # Pre-commit hook # If any command fails, exit immediately with that command's exit status set -eo pipefail +echo "Running pre-commit checks..." +echo "This may take a moment as checks run against all files..." + # Run all checks for all files make verify make docstyle make pyright make check-types make bandit + +echo "✓ Pre-commit checks passed!"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefilegithooks/pre-commitgithooks/pre-push
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-11T10:05:06.422Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
Applied to files:
githooks/pre-commitMakefile
🔇 Additional comments (1)
Makefile (1)
6-6: Good integration of git-hooks into the install flow.Adding
git-hooksas a dependency ofinstall-toolsensures hooks are automatically installed during setup, which is the correct approach for this workflow.
githooks/pre-commit
Outdated
| uv run black --check . | ||
| uv run pydocstyle -v . | ||
| make pyright | ||
| make pylint | ||
| make ruff | ||
| make check-types |
There was a problem hiding this comment.
Standardize on make targets for consistency and add missing security checks.
The hook mixes direct uv run commands (lines 8-9) with make targets (lines 10-13), creating inconsistency. Additionally, based on learnings, bandit security scanning should be included in pre-commit checks but is currently missing.
♻️ Proposed fix for consistency and completeness
# Run all checks for all files
-uv run black --check .
-uv run pydocstyle -v .
+make verify
+make docstyle
make pyright
-make pylint
-make ruff
make check-types
+make banditThis approach:
- Uses consistent
maketargets throughout - Includes
banditfor security scanning (per learnings) - The
make verifytarget already runs black --check, ruff, and pylint - Consolidates duplicate checks
Based on learnings, which indicate that bandit and other checks should be run before completing code changes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uv run black --check . | |
| uv run pydocstyle -v . | |
| make pyright | |
| make pylint | |
| make ruff | |
| make check-types | |
| make verify | |
| make docstyle | |
| make pyright | |
| make check-types | |
| make bandit |
🤖 Prompt for AI Agents
In @githooks/pre-commit around lines 8 - 13, Replace the direct `uv run`
commands with consistent `make` targets and add the missing security scan:
replace `uv run black --check .` and `uv run pydocstyle -v .` with the
corresponding make targets (e.g., `make black-check` / `make pydocstyle` or use
the consolidated `make verify` if it covers them), ensure `make verify` (or
equivalent) is used instead of duplicating checks, and add a `make bandit` (or
call `make security`) step to run Bandit before other linters; update the
githooks/pre-commit sequence to only invoke make targets (including the new
Bandit target) for consistency and completeness.
There was a problem hiding this comment.
Note, the commands are from our current GitHub CI. The goal is to have pre-commit and Git Hub workflows in sync.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find GitHub workflow files
fd -e yml -e yaml . .github/workflows
# Show contents of workflow files to see what commands are run in CI
echo "=== GitHub Workflow Contents ==="
fd -e yml -e yaml . .github/workflows --exec cat {}
Length of output: 6776
🏁 Script executed:
#!/bin/bash
# Check if there's a make bandit target in the Makefile
echo "=== Checking for bandit target in Makefile ==="
grep -n "bandit" Makefile
echo -e "\n=== Checking what make docstyle does ==="
grep -A 5 "^docstyle:" Makefile
Length of output: 359
@VladimirKadlec, thank you for the clarification! I understand now—keeping the pre-commit hook in sync with your CI workflows makes perfect sense.
I've reviewed your GitHub CI workflows, and the current pre-commit hook is indeed already well-aligned with them. I withdraw my previous suggestion to standardize on make targets, as that would actually diverge from your CI setup.
However, I did notice one difference between the pre-commit hook and your CI:
Bandit security scanner: Your CI includes a bandit workflow (.github/workflows/bandit.yaml) that scans the codebase, but it's not included in the pre-commit hook. The Makefile has a make bandit target available (uv run bandit -r src/lightspeed_evaluation -ll).
Would you like to add make bandit to the pre-commit hook to maintain full sync with CI, or is it intentionally excluded from pre-commit (perhaps to keep the hook faster)?
There's also a minor command style difference for pydocstyle:
- CI uses:
make docstyle - Pre-commit uses:
uv run pydocstyle -v .
Both are functionally equivalent since make docstyle runs the same underlying command. This doesn't affect functionality, just noting it for completeness.
🧠 Learnings used
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
|
Thank you... LGTM. |
githooks/pre-commit
Outdated
|
|
||
| # Run all checks for all files | ||
| uv run black --check . | ||
| uv run pydocstyle -v . |
There was a problem hiding this comment.
I think we need to clean up our CI config and make targets.. Right now these are not in sync + some duplicates. But that is a different topic..
Could you please add make bandit and for docstyle use make docstyle
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Makefile:
- Around line 67-68: The pre-commit Makefile target currently depends on the
black target which mutates files and leaves changes unstaged; change the
pre-commit flow to use a non-mutating verification target instead: add a new
target (e.g., format or black-format) that runs black --fast (or black without
--check) for formatting, and add a separate check-only target (e.g., black-check
or check-black) that runs black --check (or black --diff) and make pre-commit
depend on that check-only target along with pyright, pylint, ruff, check-types,
bandit, etc.; update the pre-commit dependency list to reference the check-only
symbol (black-check) rather than the mutating black/format target so commits
fail when formatting is off but files are not modified by the hook.
🧹 Nitpick comments (1)
Makefile (1)
24-26: Consider warning before overwriting existing hooks.The
ln -sfcommand will silently overwrite any existing git hooks without warning. If developers have custom hooks, they'll be replaced without notice.🔧 Consider checking for existing hooks first
git-hooks: ## Install git hooks @echo "Installing git hooks" + @if [ -e .git/hooks/pre-commit ] && [ ! -L .git/hooks/pre-commit ]; then \ + echo "Warning: Existing pre-commit hook will be overwritten"; \ + fi + @if [ -e .git/hooks/pre-push ] && [ ! -L .git/hooks/pre-push ]; then \ + echo "Warning: Existing pre-push hook will be overwritten"; \ + fi cd .git/hooks ; ln -sf ../../githooks/* ./
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefilegithooks/pre-commit
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-11T10:05:06.422Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
Applied to files:
githooks/pre-commitMakefile
⏰ 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). (2)
- GitHub Check: tests (3.12)
- GitHub Check: tests (3.13)
🔇 Additional comments (6)
githooks/pre-commit (2)
1-5: Good error handling setup.The use of
set -eo pipefailensures the hook fails fast if any check fails, which is the correct behavior for a pre-commit hook.
7-8: Verify thepre-committarget implementation.The hook runs checks on all source files rather than only staged files. While this is noted in the PR description, be aware that this can slow down commits as the codebase grows. The correctness of this hook depends on the
make pre-committarget implementation in the Makefile.Makefile (4)
6-6: Good automation of hook installation.Adding
git-hooksas a dependency ofinstall-toolsensures developers get the hooks set up automatically during initial setup, reducing manual steps.
44-45: LGTM - dedicated Black formatting target.The new
blacktarget provides a clear way to run the Black formatter.
47-47: LGTM - clearer separation of formatting steps.Making
formatdepend onblackexplicitly improves the target's structure and makes the workflow more transparent.
67-67: Verify the scope of checks is intentional.The
pre-committarget's dependencies check different scopes:
- The
pylinttarget (lines 79-81) checks onlysrcandlsc_agent_eval/src- But the
verifytarget (line 53) runspylintonsrc tests lsc_agent_eval/src lsc_agent_eval/tests- The
bandittarget (line 93) only scanssrc/lightspeed_evaluationThis means the pre-commit hook doesn't lint test files. If this is intentional (different standards for tests), consider documenting it. Otherwise, the scopes should be aligned.
Based on learnings, the development workflow includes running
make verifybefore completing changes, which does check tests. Consider whether pre-commit should enforce the same scope asverify.
VladimirKadlec
force-pushed
the
commit-hooks
branch
from
609b98c to
f63fc53
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Makefile:
- Around line 70-72: The pre-commit Makefile target currently lists tools to run
but lacks the install-deps-test prerequisite; update the pre-commit target
declaration so it depends on install-deps-test (similar to the verify target) to
ensure required tools are installed before running black-check, docstyle,
pyright, pylint, ruff, check-types and bandit.
🧹 Nitpick comments (2)
Makefile (1)
24-26: Consider verifying hook executability.The symlink approach works, but could be more robust:
- No verification that the symlinks were created successfully
- Links all files from
githooks/*indiscriminately (may include non-hook files like README)- No check that hook files are executable
♻️ More robust hook installation
git-hooks: ## Install git hooks @echo "Installing git hooks" - cd .git/hooks ; ln -sf ../../githooks/* ./ + @for hook in githooks/pre-commit githooks/pre-push; do \ + [ -f "$$hook" ] && chmod +x "$$hook" && ln -sf "../../$$hook" ".git/hooks/$$(basename $$hook)" && echo "Installed $$(basename $$hook)"; \ + donegithooks/pre-commit (1)
7-8: Consider the pre-commit framework for better performance.As noted in the PR description, this hook runs linters on ALL source files, which can be slow on larger repositories. The hook will add latency to every commit.
You've already identified the pre-commit framework (https://pre-commit.com/) as an alternative that runs checks only on staged files. Consider evaluating whether the current approach becomes too slow in practice, and keep the migration path in mind.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefilegithooks/pre-commit
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
📚 Learning: 2025-12-11T10:05:06.422Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
Applied to files:
Makefilegithooks/pre-commit
⏰ 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). (2)
- GitHub Check: tests (3.12)
- GitHub Check: tests (3.13)
🔇 Additional comments (4)
Makefile (3)
6-6: LGTM: Auto-installing git hooks during tool setup.Adding the
git-hooksdependency ensures hooks are installed when developers runmake install-tools, which aligns well with the development workflow.
44-49: LGTM: Clean separation of Black check and format targets.Separating
black-check(validation) fromblack(formatting) follows good practice and makes the pre-commit target more explicit.
50-51: LGTM: Format target properly chains Black and Ruff formatting.The updated dependencies ensure Black formatting runs before Ruff fixes, creating a logical formatting pipeline.
githooks/pre-commit (1)
1-5: LGTM: Proper error handling for git hook.The
set -eo pipefailensures the hook will fail the commit if any check fails, which is the desired behavior for a pre-commit hook.
| pre-commit: black-check docstyle pyright pylint ruff check-types bandit | ||
| @echo "All checks successful" | ||
|
|
There was a problem hiding this comment.
Add install-deps-test dependency to ensure tools are available.
The pre-commit target runs multiple linting and type-checking tools but doesn't depend on install-deps-test, which could cause failures if the tools aren't installed. Looking at line 53, the verify target correctly includes this dependency.
🔧 Add dependency to ensure tools are installed
-pre-commit: black-check docstyle pyright pylint ruff check-types bandit
+pre-commit: install-deps-test black-check docstyle pyright pylint ruff check-types bandit
@echo "All checks successful"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pre-commit: black-check docstyle pyright pylint ruff check-types bandit | |
| @echo "All checks successful" | |
| pre-commit: install-deps-test black-check docstyle pyright pylint ruff check-types bandit | |
| @echo "All checks successful" | |
🤖 Prompt for AI Agents
In @Makefile around lines 70 - 72, The pre-commit Makefile target currently
lists tools to run but lacks the install-deps-test prerequisite; update the
pre-commit target declaration so it depends on install-deps-test (similar to the
verify target) to ensure required tools are installed before running
black-check, docstyle, pyright, pylint, ruff, check-types and bandit.
Makefile
Outdated
| format: black install-deps-test ## Format the code into unified format | ||
| uv run ruff check . --fix --per-file-ignores=tests/*:S101 --per-file-ignores=scripts/*:S101 --per-file-ignores=lsc_agent_eval/tests/*:S101 | ||
|
|
||
| verify: install-deps-test ## Verify the code using various linters |
There was a problem hiding this comment.
Also this one is combining different checks, but i don't think it is required.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @Makefile:
- Line 4: The .PHONY line is incomplete; update the Makefile's .PHONY
declaration to list every non-file-producing target so make won't confuse them
with files—add targets like git-hooks, black-check, black-format, pre-commit,
verify, install-tools, install-deps, install-deps-test, pylint, pyright,
docstyle, ruff, bandit (and any other recipe-only targets defined in the
Makefile) to the .PHONY entry so all these targets are declared phony.
- Around line 67-68: The pre-commit Makefile target currently runs checks but
does not run the format target, so developers must run make format manually;
update the pre-commit target to invoke the existing format target (or call it
via $(MAKE) format) before running black-check/docstyle/etc. to ensure code is
auto-formatted on commit; reference the Makefile target names "pre-commit" and
"format" (and note the separate "pre-push" -> "pytest" flow remains unchanged).
- Around line 44-48: Add the missing make target and update .PHONY: create a new
format target that depends on or invokes the existing black-format target (so
running make format runs Black), and add black-check and black-format (and
format) to the .PHONY declaration so all three are treated as phony targets;
ensure the new target is named format and tied to black-format and that .PHONY
lists format verify help test black-check black-format.
- Around line 24-26: Update the git-hooks Makefile target to validate
directories and avoid running ln if cd fails: in the "git-hooks" target check
that .git/hooks and githooks exist before attempting to link, replace the `cd
.git/hooks ; ln -sf ../../githooks/* ./` pattern with a guarded sequence that
only runs ln when the directory change succeeds (use && semantics), emit clear
error messages if either directory is missing or the link step fails, and print
a confirmation on success so the user knows hooks were installed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
📚 Learning: 2025-12-11T10:05:06.422Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
Applied to files:
Makefile
🪛 checkmake (0.2.2)
Makefile
[warning] 4-4: Missing required phony target "all"
(minphony)
[warning] 4-4: Missing required phony target "clean"
(minphony)
⏰ 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). (1)
- GitHub Check: tests (3.13)
🔇 Additional comments (2)
Makefile (2)
6-6: Good practice to auto-install hooks with dev tools.Automatically installing git hooks when setting up the development environment ensures all contributors have consistent pre-commit and pre-push checks.
64-65: LGTM! Test target properly configured.The
testtarget correctly depends oninstall-deps-testand runs pytest on the appropriate test directories. This aligns with the expected workflow.
| black-check: | ||
| uv run black . --check | ||
|
|
||
| black-format: | ||
| uv run black . |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the complete Makefile structure around black targets and .PHONY
cat -n Makefile | head -60Repository: lightspeed-core/lightspeed-evaluation
Length of output: 3084
🏁 Script executed:
#!/bin/bash
# Search for .PHONY declaration and black-related targets
rg -n "\.PHONY|black-" MakefileRepository: lightspeed-core/lightspeed-evaluation
Length of output: 222
🏁 Script executed:
#!/bin/bash
# Check for the format target and its implementation
rg -n "^format:" Makefile -A 2Repository: lightspeed-core/lightspeed-evaluation
Length of output: 63
🏁 Script executed:
#!/bin/bash
# Get file length and search for format target in entire file
wc -l Makefile
echo "---"
rg -n "format" MakefileRepository: lightspeed-core/lightspeed-evaluation
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Read the entire Makefile to see complete structure
cat -n Makefile | tail -50Repository: lightspeed-core/lightspeed-evaluation
Length of output: 2151
Add black-check and black-format to .PHONY, and create the missing format target.
The black-check and black-format targets are not in the .PHONY declaration and should be added. Additionally, the format target is declared in .PHONY (line 4) but has no implementation, which will cause make format to fail when developers follow the learnings requirement to run it.
Create a format target that invokes black-format:
format: black-format ## Format code with Black
Then update .PHONY to include all targets:
.PHONY: format verify help test black-check black-format
🤖 Prompt for AI Agents
In @Makefile around lines 44 - 48, Add the missing make target and update
.PHONY: create a new format target that depends on or invokes the existing
black-format target (so running make format runs Black), and add black-check and
black-format (and format) to the .PHONY declaration so all three are treated as
phony targets; ensure the new target is named format and tied to black-format
and that .PHONY lists format verify help test black-check black-format.
| pre-commit: black-check docstyle pyright pylint ruff check-types bandit | ||
| @echo "All checks successful" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if githooks directory and hook files exist
if [ -d githooks ]; then
echo "=== githooks directory contents ==="
ls -la githooks/
echo -e "\n=== pre-commit hook content ==="
cat githooks/pre-commit
echo -e "\n=== pre-push hook content ==="
cat githooks/pre-push
else
echo "Error: githooks directory not found"
fiRepository: lightspeed-core/lightspeed-evaluation
Length of output: 780
🏁 Script executed:
# Check Makefile for all check-related targets
rg "^(test|format|verify):" MakefileRepository: lightspeed-core/lightspeed-evaluation
Length of output: 200
🏁 Script executed:
# Get the complete Makefile to understand all available targets
cat -n Makefile | head -100Repository: lightspeed-core/lightspeed-evaluation
Length of output: 4289
Comprehensive pre-commit checks; note test and format placement.
The pre-commit target runs essential code quality checks (black-check, docstyle, pyright, pylint, ruff, check-types, bandit). The git hooks are properly configured: the pre-commit hook invokes make pre-commit, while the pre-push hook runs tests separately via pytest. However, the format target is not automatically invoked by any hook—developers must run make format manually before committing.
🤖 Prompt for AI Agents
In @Makefile around lines 67 - 68, The pre-commit Makefile target currently runs
checks but does not run the format target, so developers must run make format
manually; update the pre-commit target to invoke the existing format target (or
call it via $(MAKE) format) before running black-check/docstyle/etc. to ensure
code is auto-formatted on commit; reference the Makefile target names
"pre-commit" and "format" (and note the separate "pre-push" -> "pytest" flow
remains unchanged).
e130204 to
bb19fbd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Makefile:
- Line 4: Update the .PHONY declaration to reflect current Makefile targets:
remove the obsolete targets format and verify, and add the new targets
git-hooks, black-check, black-format, and pre-commit (so .PHONY lists only the
actual phony targets used such as test, help, git-hooks, black-check,
black-format, pre-commit, etc.); edit the line containing ".PHONY: format verify
help test" to match the current target names.
🧹 Nitpick comments (2)
Makefile (2)
24-26: Consider adding a help comment and guarding against missing.gitdirectory.The target lacks a
##help comment, so it won't appear inmake helpoutput. Additionally, the command will fail silently if.git/hooksdoesn't exist (e.g., when working with a tarball instead of a git clone).Suggested improvement
-git-hooks: ## Install git hooks - @echo "Installing git hooks" - cd .git/hooks ; ln -sf ../../githooks/* ./ +git-hooks: ## Install git hooks (symlinks from .git/hooks to githooks/) + @if [ -d .git/hooks ]; then \ + echo "Installing git hooks"; \ + cd .git/hooks && ln -sf ../../githooks/* ./; \ + else \ + echo "Skipping git hooks (not a git repository)"; \ + fi
44-48: Consider adding help comments for discoverability.These new targets lack
##help comments, so they won't appear inmake helpoutput. Users may want to runblack-formatmanually.Suggested improvement
-black-check: +black-check: ## Check code formatting with Black uv run black . --check -black-format: +black-format: ## Format code with Black uv run black .
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
📚 Learning: 2025-12-11T10:05:06.422Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
Applied to files:
Makefile
🪛 checkmake (0.2.2)
Makefile
[warning] 4-4: Missing required phony target "all"
(minphony)
[warning] 4-4: Missing required phony target "clean"
(minphony)
⏰ 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). (2)
- GitHub Check: tests (3.13)
- GitHub Check: tests (3.12)
🔇 Additional comments (2)
Makefile (2)
6-6: LGTM!Adding
git-hooksas a dependency ensures hooks are automatically installed during initial setup. Good developer experience.
62-63: LGTM!The composite target correctly chains all linting and static analysis checks. Excluding
testmakes sense since tests run on pre-push instead (as per the PR description).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Makefile:
- Around line 24-27: The git-hooks target currently chains commands with a
semicolon, which can run ln -sf even if cd .git/hooks fails; update the
git-hooks recipe (target name: git-hooks) to chain the cd and ln commands with
&& so ln -sf only runs when cd succeeds (or alternatively use a
subshell/pushd/popd approach), ensuring the ln -sf invocation executes in the
intended .git/hooks directory.
🧹 Nitpick comments (1)
Makefile (1)
4-4: Expand .PHONY to include all non-file targets.The
.PHONYdeclaration only listshelpandtest, but many other targets don't correspond to files:install-tools,git-hooks,install-deps,install-deps-test,update-deps,check-types,black-check,black-format,pre-commit,verify-packages-completeness,distribution-archives,pylint,pyright,docstyle,ruff,bandit, anduv-lock-check. If files with these names exist in the repository, Make will incorrectly skip these targets.📝 Proposed fix
-.PHONY: help test +.PHONY: help test install-tools git-hooks install-deps install-deps-test update-deps check-types black-check black-format pre-commit verify-packages-completeness distribution-archives pylint pyright docstyle ruff bandit uv-lock-check
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
📚 Learning: 2025-12-11T10:05:06.422Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
Applied to files:
Makefile
🪛 checkmake (0.2.2)
Makefile
[warning] 4-4: Missing required phony target "all"
(minphony)
[warning] 4-4: Missing required phony target "clean"
(minphony)
🔇 Additional comments (4)
Makefile (4)
6-6: LGTM!Adding
git-hooksas a dependency toinstall-toolsensures hooks are automatically installed during developer setup, which improves onboarding.
44-46: LGTM!Renaming to
black-checkand separating check-only from formatting improves clarity and aligns with the pre-commit workflow.
47-48: LGTM!The
black-formattarget provides a convenient way for developers to format code manually, complementing theblack-checktarget used in pre-commit hooks.
62-64: LGTM! Excellent consolidation of pre-commit checks.The
pre-committarget effectively consolidates all linting and type-checking steps (black-check,docstyle,pyright,pylint,ruff,check-types,bandit) into a single command, which aligns with best practices and the retrieved learning's intent.Note: This change updates the development workflow. The old
make formatandmake verifycommands are replaced by:
make black-format(for formatting)make pre-commit(for all verification checks)Based on learnings, the workflow has evolved from individual commands to this consolidated approach.
VladimirKadlec
force-pushed
the
commit-hooks
branch
from
bb19fbd to
66a9fa2
Compare
|
@VladimirKadlec readme/doc changes are required for this to use new/updated targets. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @README.md:
- Line 33: Fix the typo in the README heading by changing the line containing
"Additonal steps for local development" to "Additional steps for local
development" so the section title is spelled correctly.
🧹 Nitpick comments (1)
Makefile (1)
4-4: Consider declaring all non-file-producing targets as phony.The
.PHONYdeclaration should include all targets that don't create files with the target's name. Currently missing:git-hooks,black-check,black-format,pre-commit,install-tools,install-deps,install-deps-test,update-deps,check-types,pylint,pyright,docstyle,ruff,bandit,uv-lock-check,verify-packages-completeness, anddistribution-archives.While conflicts are rare in practice, declaring these as phony is a Makefile best practice.
♻️ Proposed fix
-.PHONY: help test +.PHONY: help test git-hooks black-check black-format pre-commit install-tools install-deps install-deps-test update-deps check-types pylint pyright docstyle ruff bandit uv-lock-check verify-packages-completeness distribution-archives
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
MakefileREADME.md
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
📚 Learning: 2025-12-11T10:05:06.422Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Update docs/, README.md, and AGENTS.md when modifying functionality or adding new features
Applied to files:
README.md
📚 Learning: 2025-12-11T10:05:06.422Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-evaluation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T10:05:06.422Z
Learning: Run make format, make verify, make check-types, make pyright, make docstyle, make bandit, and make test before completing code changes
Applied to files:
README.mdMakefile
🪛 checkmake (0.2.2)
Makefile
[warning] 4-4: Missing required phony target "all"
(minphony)
[warning] 4-4: Missing required phony target "clean"
(minphony)
⏰ 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). (2)
- GitHub Check: tests (3.12)
- GitHub Check: tests (3.13)
🔇 Additional comments (5)
README.md (1)
385-405: Well-documented workflow updates.The documentation accurately reflects the new Makefile targets and provides clear guidance for developers on the updated workflow. This aligns well with the PR's goal of adding git hooks and reorganizing the development tooling.
Based on learnings, documentation updates for functionality changes are important, and this update satisfies that requirement.
Makefile (4)
6-6: Excellent integration of git hooks into the setup workflow.Making
install-toolsdepend ongit-hooksensures that developers automatically get the pre-commit and pre-push hooks when setting up their development environment. This aligns well with the PR's goal of enforcing code quality through git hooks.
44-48: Clear and straightforward Black formatting targets.The split between
black-check(for CI/validation) andblack-format(for local formatting) is intuitive and follows common conventions. This replaces the previousformat/verifytargets mentioned in the learnings.
62-63: Well-designed composite pre-commit target.This target effectively aggregates all quality checks that should run before committing code. The design aligns perfectly with the new
githooks/pre-commithook and ensures consistency between local checks and CI validation.Based on learnings, this consolidates the previously separate
format,verify, and other quality check commands into a unified workflow.
24-26: Git hooks installation looks correct.The symlink approach ensures that updates to
githooks/are automatically reflected without re-running the installation. The-fflag correctly handles overwriting existing hooks. The relative path../../githooksproperly resolves from.git/hooksto the repository root.
VladimirKadlec
force-pushed
the
commit-hooks
branch
from
a8a9db2 to
8bc8d05
Compare
3 participants
Description
Add pre-commit (running all the linters) and pre-push (running all the tests) git hooks.
It runs the linters for all source files. If we consider this too slow/annoying we can switch to pre-commit and run them only on committed files.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.