Conversation
|
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. 📝 WalkthroughWalkthroughTemplate sync adds a pinned Changes
Sequence Diagram(s)(Skipped — changes are primarily configuration, tests, and Makefile additions; no new multi-component runtime control flow requiring sequence diagrams.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
🤖 Fix all issues with AI agents
In @.github/workflows/rhiza_mypy.yml:
- Around line 32-34: Restore the "Run mypy" step instead of leaving it commented
out and gate its execution behind a repository variable; specifically, re-enable
the step named "Run mypy" (the step that runs `make -f .rhiza/rhiza.mk mypy`)
and add a conditional like `if: ${{ vars.RUN_MYPY == 'true' }}` (or use an
equivalent repo/secret/env variable) so the step only runs when the variable is
set to true, allowing toggling without editing the workflow file.
In @.github/workflows/rhiza_sync.yml:
- Around line 83-86: The git URL rewrite currently runs even when PAT_TOKEN is
empty; wrap the git config --global url."https://x-access-token:${{
secrets.PAT_TOKEN }}@github.com/".insteadOf "https://github.com/" command in a
guard so it only executes when a PAT is present (e.g., add a separate step with
an if: ${{ secrets.PAT_TOKEN != '' }} or guard inside the run script using [ -n
"${{ secrets.PAT_TOKEN }}" ]); target the exact command string git config
--global url."https://x-access-token:${{ secrets.PAT_TOKEN
}}@github.com/".insteadOf "https://github.com/" to ensure no rewrite occurs when
PAT_TOKEN is missing.
In `@tests/test_rhiza/test_rhiza_workflows.py`:
- Line 20: The test imports helpers from conftest which pytest doesn't export as
a regular module; remove the line "from conftest import run_make,
setup_rhiza_git_repo, strip_ansi" and instead duplicate the helper
implementations locally in tests/test_rhiza/test_rhiza_workflows.py (copy the
run_make, setup_rhiza_git_repo, and strip_ansi functions from test_makefile.py)
so the test uses local functions rather than importing conftest.
- Around line 148-156: The test test_summarise_sync_skips_in_rhiza_repo
currently calls run_make(..., dry_run=True) but asserts on a runtime skip
message that only appears when the target actually runs; either change the
invocation to run_make(logger, ["summarise-sync"], dry_run=False) to execute the
target and keep the existing assert against proc.stdout, or keep dry_run=True
and instead assert on the dry-run command-check output (e.g., the
conditional/make command that would detect a rhiza repo) by updating the
assertion to look for that conditional pattern in proc.stdout; update the test
accordingly referencing test_summarise_sync_skips_in_rhiza_repo, run_make, the
"summarise-sync" target, and proc.stdout.
In `@tests/tests.mk`:
- Around line 26-42: The check for SOURCE_FOLDER uses [ -d ${SOURCE_FOLDER} ]
which fails when SOURCE_FOLDER is empty; change the condition to guard for
emptiness and quote the variable, e.g. replace the test with a combined check
like [ -n "${SOURCE_FOLDER}" ] && [ -d "${SOURCE_FOLDER}" ] (and ensure other
uses of SOURCE_FOLDER in that conditional are quoted) so the fallback branch
that runs tests without coverage executes when SOURCE_FOLDER is unset or empty.
♻️ Duplicate comments (2)
tests/test_rhiza/test_marimushka_target.py (1)
72-74: Same skip pattern as above.tests/test_rhiza/test_book.py (1)
24-29: Same rmtree guard applies here.
🧹 Nitpick comments (7)
tests/tests.mk (1)
61-74: Alignsecurity/mutatewith other targets by checking SOURCE_FOLDER.Now that failures are surfaced, a missing/empty
SOURCE_FOLDERwill makebandit/mutmuterror out. Consider the same guard used intest/typecheck.♻️ Suggested refactor
security: install ## run security scans (pip-audit and bandit) `@printf` "${BLUE}[INFO] Running pip-audit for dependency vulnerabilities...${RESET}\n" @${UVX_BIN} pip-audit `@printf` "${BLUE}[INFO] Running bandit security scan...${RESET}\n" - @${UVX_BIN} bandit -r ${SOURCE_FOLDER} -ll -q + `@if` [ -d "${SOURCE_FOLDER}" ]; then \ + ${UVX_BIN} bandit -r ${SOURCE_FOLDER} -ll -q; \ + else \ + printf "${YELLOW}[WARN] Source folder ${SOURCE_FOLDER} not found, skipping bandit${RESET}\n"; \ + fi mutate: install ## run mutation testing with mutmut (slow, for CI or thorough testing) `@printf` "${BLUE}[INFO] Running mutation testing with mutmut...${RESET}\n" `@printf` "${YELLOW}[WARN] This may take a while...${RESET}\n" - @${UVX_BIN} mutmut run --paths-to-mutate=${SOURCE_FOLDER} + `@if` [ -d "${SOURCE_FOLDER}" ]; then \ + ${UVX_BIN} mutmut run --paths-to-mutate=${SOURCE_FOLDER}; \ + else \ + printf "${YELLOW}[WARN] Source folder ${SOURCE_FOLDER} not found, skipping mutmut${RESET}\n"; \ + fi @${UVX_BIN} mutmut resultstests/test_rhiza/test_book.py (1)
11-14: Guard rmtree in case the book folder is already missing.
This keeps the test resilient across repos that don't shipbook/.Suggested tweak
- shutil.rmtree(git_repo / "book") - assert not (git_repo / "book").exists() + book_dir = git_repo / "book" + if book_dir.exists(): + shutil.rmtree(book_dir) + assert not book_dir.exists()tests/test_rhiza/conftest.py (1)
29-56: Signature mismatch:run_makein conftest.py lacksenvparameter.The
run_makefunction in conftest.py does not include theenvparameter, but the same function intest_makefile.py(lines 87-92) has been extended withenv: dict[str, str] | None = None. Sincetest_rhiza_workflows.pyimportsrun_makefromconftest, this inconsistency may cause issues if tests need to pass environment variables.Consider adding the
envparameter for consistency:♻️ Proposed fix
def run_make( - logger, args: list[str] | None = None, check: bool = True, dry_run: bool = True + logger, args: list[str] | None = None, check: bool = True, dry_run: bool = True, + env: dict[str, str] | None = None, ) -> subprocess.CompletedProcess: """Run `make` with optional arguments and return the completed process. Args: logger: Logger used to emit diagnostic messages during the run args: Additional arguments for make check: If True, raise on non-zero return code dry_run: If True, use -n to avoid executing commands + env: Optional environment variables to pass to the subprocess """ cmd = [MAKE] if args: cmd.extend(args) # Use -s to reduce noise, -n to avoid executing commands flags = "-sn" if dry_run else "-s" cmd.insert(1, flags) logger.info("Running command: %s", " ".join(cmd)) - result = subprocess.run(cmd, capture_output=True, text=True) + result = subprocess.run(cmd, capture_output=True, text=True, env=env)tests/test_rhiza/test_makefile.py (2)
154-167: Test logic relies on fixture parameter that may not be available.The test uses
tmp_pathto check for.python-version, but this path points to the fixture's temp directory. Sincesetup_tmp_makefilechanges the working directory totmp_path, you could usePath.cwd()orPath(".python-version")instead for consistency.♻️ Suggested improvement
- python_version_file = tmp_path / ".python-version" - if python_version_file.exists(): - python_version = python_version_file.read_text().strip() + python_version_file = Path(".python-version") + if python_version_file.exists(): + python_version = python_version_file.read_text().strip()
245-258: Potential race condition: environment may inheritPYTHON_VERSIONfrom parent.Copying
os.environand only removingPYTHON_VERSIONmay still inherit other variables that could influence the test. Consider using a more minimal environment if you need strict isolation, or verify this approach works reliably in CI.tests/test_rhiza/test_rhiza_workflows.py (2)
23-70: Fixture duplication withtest_makefile.py.This
setup_tmp_makefilefixture is very similar to the one intest_makefile.py(lines 41-84), with some differences:
- This version copies
.rhiza-version(line 39-40)- This version creates
template.ymlandpyproject.toml(lines 48-52)- This version calls
setup_rhiza_git_repo()(line 60)- This version creates
srcandtestsdirectories (lines 63-64)Since both fixtures serve similar purposes but with different setup requirements, consider either:
- Extracting common setup logic to a shared helper
- Making the fixture in
conftest.pyconfigurable to handle both casesThis is acceptable for now given the different test requirements.
170-185: Unusedloggerparameter flagged by static analysis.The
loggerparameter is unused intest_workflow_version_reading_patternandtest_workflow_version_fallback_pattern. Since these tests don't callrun_make, the logger isn't needed.♻️ Proposed fix
- def test_workflow_version_reading_pattern(self, logger, tmp_path): + def test_workflow_version_reading_pattern(self, tmp_path): """Test the pattern used in workflow to read Rhiza version.""" # ... - def test_workflow_version_fallback_pattern(self, logger, tmp_path): + def test_workflow_version_fallback_pattern(self, tmp_path): """Test the fallback pattern when .rhiza-version doesn't exist.""" # ...Also applies to: 187-207
| # to brutal for now | ||
| # - name: Run mypy | ||
| # run: make -f .rhiza/rhiza.mk mypy |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for mypy configuration files
rg -n --hidden -g 'pyproject.toml' -g '.mypy.ini' -g 'mypy.ini' -g 'setup.cfg' 'mypy' .
echo "---"
# Check for mypy references in the codebase (excluding common paths)
rg -n --hidden -g '!**/.venv/**' -g '!**/dist/**' '\bmypy\b' .Repository: tschm/jquantstats
Length of output: 1703
Avoid leaving mypy fully disabled—gate it instead.
Mypy is configured in the project (pinned to 1.19.1, runs in strict mode via pyproject.toml, and has dedicated makefile targets and tests). Commenting out the CI step removes type-checking entirely from automated checks. Instead of disabling it completely, gate the step behind a repo variable so it can be re-enabled without editing the workflow:
Suggested change
- # to brutal for now
- # - name: Run mypy
- # run: make -f .rhiza/rhiza.mk mypy
+ # Temporarily gated; enable via repo variable ENABLE_MYPY=true
+ - name: Run mypy
+ if: ${{ vars.ENABLE_MYPY == 'true' }}
+ run: make -f .rhiza/rhiza.mk mypyThis preserves the type-checking infrastructure while allowing you to toggle it without 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.
| # to brutal for now | |
| # - name: Run mypy | |
| # run: make -f .rhiza/rhiza.mk mypy | |
| # Temporarily gated; enable via repo variable ENABLE_MYPY=true | |
| - name: Run mypy | |
| if: ${{ vars.ENABLE_MYPY == 'true' }} | |
| run: make -f .rhiza/rhiza.mk mypy |
🤖 Prompt for AI Agents
In @.github/workflows/rhiza_mypy.yml around lines 32 - 34, Restore the "Run
mypy" step instead of leaving it commented out and gate its execution behind a
repository variable; specifically, re-enable the step named "Run mypy" (the step
that runs `make -f .rhiza/rhiza.mk mypy`) and add a conditional like `if: ${{
vars.RUN_MYPY == 'true' }}` (or use an equivalent repo/secret/env variable) so
the step only runs when the variable is set to true, allowing toggling without
editing the workflow file.
| git config user.name "github-actions[bot]" | ||
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | ||
| git config --global url."https://x-access-token:${{ secrets.PAT_TOKEN }}@github.com/".insteadOf "https://github.com/" | ||
|
|
There was a problem hiding this comment.
Guard the URL rewrite when PAT_TOKEN is missing.
As written, an empty PAT_TOKEN still rewrites GitHub URLs and can cause confusing auth behavior when relying on the default token.
🔧 Proposed fix
- git config --global url."https://x-access-token:${{ secrets.PAT_TOKEN }}@github.com/".insteadOf "https://github.com/"
+ if [ -n "${{ secrets.PAT_TOKEN }}" ]; then
+ git config --global url."https://x-access-token:${{ secrets.PAT_TOKEN }}@github.com/".insteadOf "https://github.com/"
+ fi📝 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.
| git config user.name "github-actions[bot]" | |
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | |
| git config --global url."https://x-access-token:${{ secrets.PAT_TOKEN }}@github.com/".insteadOf "https://github.com/" | |
| git config user.name "github-actions[bot]" | |
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | |
| if [ -n "${{ secrets.PAT_TOKEN }}" ]; then | |
| git config --global url."https://x-access-token:${{ secrets.PAT_TOKEN }}@github.com/".insteadOf "https://github.com/" | |
| fi | |
🤖 Prompt for AI Agents
In @.github/workflows/rhiza_sync.yml around lines 83 - 86, The git URL rewrite
currently runs even when PAT_TOKEN is empty; wrap the git config --global
url."https://x-access-token:${{ secrets.PAT_TOKEN }}@github.com/".insteadOf
"https://github.com/" command in a guard so it only executes when a PAT is
present (e.g., add a separate step with an if: ${{ secrets.PAT_TOKEN != '' }} or
guard inside the run script using [ -n "${{ secrets.PAT_TOKEN }}" ]); target the
exact command string git config --global url."https://x-access-token:${{
secrets.PAT_TOKEN }}@github.com/".insteadOf "https://github.com/" to ensure no
rewrite occurs when PAT_TOKEN is missing.
| def test_summarise_sync_skips_in_rhiza_repo(self, logger): | ||
| """Summarise-sync target should skip execution in rhiza repository.""" | ||
| # setup_rhiza_git_repo() is already called by fixture | ||
|
|
||
| proc = run_make(logger, ["summarise-sync"], dry_run=True) | ||
| # Should succeed but skip the actual summarise | ||
| assert proc.returncode == 0 | ||
| # Verify the skip message is in the output | ||
| assert "Skipping summarise-sync in rhiza repository" in proc.stdout |
There was a problem hiding this comment.
Test assertion may be too strict.
The test asserts that "Skipping summarise-sync in rhiza repository" appears in stdout during a dry-run. However, dry-run (make -n) shows the commands that would be executed, not the actual output of those commands. The skip message would only appear if the target actually runs, not during dry-run.
Consider either:
- Running with
dry_run=Falseif you want to test the actual skip behavior - Adjusting the assertion to check for the conditional check pattern in the dry-run output
🤖 Prompt for AI Agents
In `@tests/test_rhiza/test_rhiza_workflows.py` around lines 148 - 156, The test
test_summarise_sync_skips_in_rhiza_repo currently calls run_make(...,
dry_run=True) but asserts on a runtime skip message that only appears when the
target actually runs; either change the invocation to run_make(logger,
["summarise-sync"], dry_run=False) to execute the target and keep the existing
assert against proc.stdout, or keep dry_run=True and instead assert on the
dry-run command-check output (e.g., the conditional/make command that would
detect a rhiza repo) by updating the assertion to look for that conditional
pattern in proc.stdout; update the test accordingly referencing
test_summarise_sync_skips_in_rhiza_repo, run_make, the "summarise-sync" target,
and proc.stdout.
| @if [ -d ${TESTS_FOLDER} ]; then \ | ||
| mkdir -p _tests/html-coverage _tests/html-report; \ | ||
| ${VENV}/bin/python -m pytest ${TESTS_FOLDER} \ | ||
| --ignore=${TESTS_FOLDER}/benchmarks \ | ||
| --cov=${SOURCE_FOLDER} \ | ||
| --cov-report=term \ | ||
| --cov-report=html:_tests/html-coverage \ | ||
| --cov-fail-under=90 \ | ||
| --cov-report=json:_tests/coverage.json \ | ||
| --html=_tests/html-report/report.html; \ | ||
| if [ -d ${SOURCE_FOLDER} ]; then \ | ||
| ${VENV}/bin/python -m pytest ${TESTS_FOLDER} \ | ||
| --ignore=${TESTS_FOLDER}/benchmarks \ | ||
| --cov=${SOURCE_FOLDER} \ | ||
| --cov-report=term \ | ||
| --cov-report=html:_tests/html-coverage \ | ||
| --cov-fail-under=$(COVERAGE_FAIL_UNDER) \ | ||
| --cov-report=json:_tests/coverage.json \ | ||
| --html=_tests/html-report/report.html; \ | ||
| else \ | ||
| printf "${YELLOW}[WARN] Source folder ${SOURCE_FOLDER} not found, running tests without coverage${RESET}\n"; \ | ||
| ${VENV}/bin/python -m pytest ${TESTS_FOLDER} \ | ||
| --ignore=${TESTS_FOLDER}/benchmarks \ | ||
| --html=_tests/html-report/report.html; \ | ||
| fi \ |
There was a problem hiding this comment.
Guard the SOURCE_FOLDER check against empty values.
If SOURCE_FOLDER is unset/empty, [ -d ${SOURCE_FOLDER} ] expands to [ -d ] and the shell errors, so the fallback “run tests without coverage” path never executes.
✅ Suggested fix
- if [ -d ${SOURCE_FOLDER} ]; then \
+ if [ -n "${SOURCE_FOLDER}" ] && [ -d "${SOURCE_FOLDER}" ]; then \🤖 Prompt for AI Agents
In `@tests/tests.mk` around lines 26 - 42, The check for SOURCE_FOLDER uses [ -d
${SOURCE_FOLDER} ] which fails when SOURCE_FOLDER is empty; change the condition
to guard for emptiness and quote the variable, e.g. replace the test with a
combined check like [ -n "${SOURCE_FOLDER}" ] && [ -d "${SOURCE_FOLDER}" ] (and
ensure other uses of SOURCE_FOLDER in that conditional are quoted) so the
fallback branch that runs tests without coverage executes when SOURCE_FOLDER is
unset or empty.
Add mypy configuration with ignore_missing_imports and fix return type annotations by adding explicit casts and list() wrappers. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extract helper functions (run_make, setup_rhiza_git_repo, strip_ansi) to a separate helpers.py module to avoid import conflict between test_rhiza/conftest.py and test_jquantstats/conftest.py. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
This pull request synchronizes the repository with its template.
Changes were generated automatically using rhiza.
Summary by CodeRabbit
New Features
Improvements
Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.