Copier update: Sphinx #59
Conversation
|
Warning Rate limit exceeded@ejfine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 32 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 (1)
WalkthroughUpdates development tooling, devcontainer, CI workflow/action versions, pre-commit and pyright excludes, replaces windows host helper repo-sync logic with an rsync-based approach, tweaks a hashing script's exception handling, and adds minimal test logging/pytest configuration scaffolding. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/hash_git_files.py (1)
57-70: Clarify/streamline directory & symlink handling in checksum loopCatching only
IsADirectoryErroris a good tightening vs a blanketException, but the handler will now silently skip any directory that ends up infiles, not just Windows symlink oddities. To make the intent clearer (and avoid relying on exception control-flow), consider an explicit directory check:- try: - with file_path.open("rb") as f: - while True: - chunk = f.read(4096) - if not chunk: - break - checksum = zlib.adler32(chunk, checksum) - except IsADirectoryError: - # Ignore symlinks that on windows sometimes get confused as being directories - continue + if file_path.is_dir(): + # Ignore directories (including Windows symlink directory cases) + continue + + with file_path.open("rb") as f: + while True: + chunk = f.read(4096) + if not chunk: + break + checksum = zlib.adler32(chunk, checksum)This keeps unexpected I/O failures visible while making the behavior for directories/symlinks explicit and aligned with the comment.
.devcontainer/windows-host-helper.sh (1)
10-15:set -umakes the BASH_VERSION guard brittle (but only if someone mis-invokes the script)With
set -u, referencing$BASH_VERSIONin a non‑bash shell will error before your friendly message runs. Given the shebang already enforces/bin/bash, this guard is mostly redundant.You can either drop the BASH_VERSION check or move
set -uafter it and use a${BASH_VERSION-}expansion if you want to keep the explicit error path.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.copier-answers.yml(1 hunks).devcontainer/devcontainer.json(3 hunks).devcontainer/install-ci-tooling.py(1 hunks).devcontainer/windows-host-helper.sh(1 hunks).github/actions/install_deps/action.yml(2 hunks).github/actions/update-devcontainer-hash/action.yml(1 hunks).github/workflows/ci.yaml(2 hunks).github/workflows/get-values.yaml(2 hunks).github/workflows/hash_git_files.py(1 hunks).github/workflows/pre-commit.yaml(3 hunks).github/workflows/publish.yaml(6 hunks).github/workflows/publish_to_staging.yaml(2 hunks).gitignore(1 hunks).pre-commit-config.yaml(6 hunks)docs/requirements.txt(1 hunks)pyproject.toml(1 hunks)pyrightconfig.json(1 hunks)tests/unit/conftest.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/conftest.py (1)
tests/unit/snapshot.py (1)
snapshot_xml(18-23)
🪛 actionlint (1.7.9)
.github/workflows/pre-commit.yaml
72-72: shellcheck reported issue in this script: SC2086:info:6:8: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (19)
.gitignore (1)
80-80: Approved: Improved macOS DS_Store pattern consolidation.The recursive
**/.DS_Storepattern is a clean improvement that consolidates the separate patterns into a single, more comprehensive pattern that matches DS_Store files at all directory depths..github/workflows/hash_git_files.py (1)
75-88: Reverse iteration refactor is correct and keeps behavior unchangedUsing
for i in reversed(range(len(lines))):preserves the original bottom‑up scan semantics for locating the closing}and optional hash comment; the index arithmetic and return values remain consistent withupdate_devcontainer_context_hash. No functional issues here..github/workflows/pre-commit.yaml (2)
36-36: ✓ Checkout action upgrade looks good.Upgrading to
actions/checkout@v6.0.1is part of the broader workflow modernization and is a safe change.Also applies to: 43-43
62-62: Cache action upgrade is safe. The upgrade fromactions/cache@v4.2.4tov4.3.0contains no breaking changes—v4.3.0 is a maintenance release with no feature changes that would affect cache key generation or restoration behavior..github/workflows/publish_to_staging.yaml (1)
47-48: Checkout action bump is consistent with other workflowsBoth jobs now pin
actions/checkout@v6.0.1, matching the rest of the repo and reducing surprise from upstream major changes. Please just confirm in CI that there are no regressions from the v5 → v6 upgrade.Also applies to: 63-64
.github/workflows/ci.yaml (1)
56-57: Aligningactions/checkoutto v6.0.1 looks goodCI now uses
actions/checkout@v6.0.1in both the test matrix and docs build, consistent with other workflows. Let the full CI run validate that there are no behavior changes from the v5 series.Also applies to: 101-102
pyrightconfig.json (1)
13-18: Pyright excludes for generated and template artifacts make senseExcluding
**/generated/graphql,**/venv, and**/copier_template_resourcesfrom Pyright keeps noise from generated/env files out of type checking and matches how other tools typically treat these paths..github/actions/install_deps/action.yml (1)
59-62: Tooling action version bumps are straightforwardBumping
actions/setup-python,actions/setup-node, andaws-actions/configure-aws-credentialsto newer minor versions keeps the composite action current without changing behavior here. Please confirm against their changelogs that there are no breaking defaults that would affect your CI environments.Also applies to: 65-68, 76-79
docs/requirements.txt (1)
1-2: Sphinx 9.0.4 pin looks consistent; verify extension compatibilityPinning Sphinx to
9.0.4here matches the dev dependency inpyproject.toml, which is good for reproducible docs builds. Please confirm that your Sphinx extensions (notablyautodoc_pydantic==2.2.0and any others enabled inconf.py) remain compatible with Sphinx 9.pyproject.toml (1)
36-42: Dev dependency bumps are reasonable; check plugin ecosystemUpdating to
pytest>=9.0.2andsphinx==9.0.4is in line with the rest of the tooling updates. Please verify that your pytest plugins (pytest-cov,pytest-randomly, any others) and Sphinx extensions still support these versions and that the full test/docs pipeline passes under both Python 3.12 and 3.13..devcontainer/install-ci-tooling.py (1)
10-11: UV and PNPM version bumps are localized; confirm availabilityUpdating
UV_VERSIONto0.9.17andPNPM_VERSIONto10.25.0keeps the devcontainer tooling current, and the values are only used in the installer commands below. Please confirm that these exact versions exist and are stable in your CI/dev environments (e.g., by running the script in a fresh container and ensuring the installs succeed)..github/workflows/publish.yaml (1)
27-31: Publish workflow action bumps are consistent and scoped
actions/checkoutis now pinned tov6.0.1andactions/setup-pythontov6.1.0across all relevant jobs (get-values, tests, build, staging/primary install checks, tag creation). This keeps the release pipeline aligned with CI and the sharedinstall_depsaction. Please validate with a dry-run publish (or on this PR) that these newer action versions don’t change auth or environment behavior for your packaging steps.Also applies to: 73-75, 90-91, 163-167, 236-240, 297-301
.copier-answers.yml (1)
2-5: Copier answers update looks consistentThe updated
_commitand the addedconfigure_python_asyncio/configure_vcrpyflags are consistent with disabling those template features. As long as you don’t rely on Copier to manage asyncio/VCR scaffolding, this looks fine.tests/unit/conftest.py (1)
10-15: vcr logger level tweak is reasonable—verify logger name matches your vcrpy versionUsing
pytest_configureto bumplogging.getLogger("vcr.cassette")to WARNING is a straightforward way to reduce cassette noise and should be safe for tests, assuming that is still the correct logger name for the vCRpy version in use.Please double‑check against your installed vCRpy version that the logger name is indeed
"vcr.cassette"and not e.g."vcr"or similar..github/workflows/get-values.yaml (2)
12-14: Newpr-short-numworkflow output and checkout bump look goodExposing
pr-short-numviaworkflow_call.outputsand wiring it tojobs.get-values.outputs.pr-short-numis correct, and the job output matchessteps.find-pr-num.outputs.number. The bump toactions/checkout@v6.0.1is also aligned with the rest of the PR.If you haven’t already, verify that downstream workflows consuming
pr-short-numhandle the empty-string case when this workflow runs on events without a PR/merge_group.Also applies to: 27-29, 38-38
54-61: The bash substring syntax is valid and works correctly, but add explicit error handlingThe line
short_number=${full_number:${#full_number}<2?0:-2}is valid bash; parameter expansion offsets do support arithmetic expressions with ternary operators, and it correctly returns either the full value (when length ≤ 2) or the last two digits (when length > 2).However, the code lacks explicit error handling for cases where
full_numberis unset or extraction fails. The current implementation silently uses an empty string, which could cause issues downstream. Additionally, the nestedecho $(echo $short_number)is inefficient and the unquoted variables should be quoted:+ if [[ -z "${full_number:-}" ]]; then + echo "Failed to determine PR number from event context" >&2 + exit 1 + fi + short_number=${full_number:${#full_number}<2?0:-2} - - echo number=$(echo $short_number) >> $GITHUB_OUTPUT - echo "PR number extracted as $full_number and truncated to $short_number" + echo "number=${short_number}" >> "$GITHUB_OUTPUT" + echo "PR number extracted as ${full_number} and truncated to ${short_number}"Likely an incorrect or invalid review comment.
.pre-commit-config.yaml (1)
45-46: Tooling rev bumps and newgenerated/graphqlexcludes look coherentPinned SHAs for typos, prettier, check-jsonschema, ruff, pylint, and the updated excludes targeting
.*/generated/graphql/.*across ruff, pylint, and pyright are internally consistent and match the move away fromgraphql_codegenpaths.Please re-run
pre-commit run --all-fileslocally to confirm:
- The new hook revisions (esp. ruff/pylint/pyright) behave as expected.
- The new
generated/graphqldirectory structure matches these exclude patterns, and there are no remaining stale patterns pointing at old locations.Also applies to: 111-111, 197-199, 252-253, 260-261, 270-271, 277-279, 282-283, 289-292, 305-307
.github/actions/update-devcontainer-hash/action.yml (1)
29-35: Checkout update wired correctly toinputs.branchUsing
actions/checkout@v6.0.1and explicitly settingref: ${{ inputs.branch }}matches the input contract for this composite action and ensures the hash update runs on the intended branch.Confirm in a Dependabot PR run that the action still checks out the expected branch tip before computing and committing the devcontainer hash.
.devcontainer/devcontainer.json (1)
9-10: Devcontainer feature and extension updates look intentional
- AWS CLI feature version bump and the negative extension entry
"-AmazonWebServices.aws-toolkit-vscode"align with the comment about that extension causing problems.- The coderabbit VS Code extension bump and devcontainer context hash refresh fit with the broader tooling update.
After rebuilding the devcontainer, verify:
- AWS Toolkit is no longer installed in VS Code inside the container.
- The pinned extension versions (including
coderabbit.coderabbit-vscode@0.16.1) are available and install without warnings.Also applies to: 22-23, 25-26, 67-68
| sudo rm -rf "./$repoName" || true | ||
| sudo rm -rf "./$repoName/*.md" | ||
| mkdir -p "./$repoName" | ||
| sudo chown -R "$(whoami):$(whoami)" "./$repoName" # TODO: see if this alone is enough to fix everything | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .devcontainer/windows-host-helper.shRepository: LabAutomationAndScreening/pyalab
Length of output: 2451
🏁 Script executed:
find .devcontainer -name "*.json" -o -name "*.yaml" -o -name "*.yml" | head -20Repository: LabAutomationAndScreening/pyalab
Length of output: 164
🏁 Script executed:
if [ -f .devcontainer/devcontainer.json ]; then cat .devcontainer/devcontainer.json; fiRepository: LabAutomationAndScreening/pyalab
Length of output: 3107
🏁 Script executed:
cat .devcontainer/docker-compose.ymlRepository: LabAutomationAndScreening/pyalab
Length of output: 769
🏁 Script executed:
find . -maxdepth 2 -name "package.json" -o -name "requirements.txt" | head -10Repository: LabAutomationAndScreening/pyalab
Length of output: 99
🏁 Script executed:
git ls-files | grep -E "(package\.json|requirements\.txt|pnpm|venv)" | head -20Repository: LabAutomationAndScreening/pyalab
Length of output: 97
Remove rm -rf "./$repoName" or replace with selective cleanup—rsync --exclude does not preserve deleted content
The script deletes the entire repository directory on the Windows host (lines 30–31), then claims to preserve .venv by excluding it from the rsync (lines 46–50, 55–56). This is misleading: if .venv exists on the host filesystem as a directory, rm -rf destroys it, and rsync --exclude cannot restore it—exclusion only prevents overwriting, it does not recover deleted content.
In the current setup, .venv is managed as a Docker named volume (python_venv), so the practical risk is low. However, the script logic is flawed. If node_modules, .pnpm-store, or .venv were ever present on the host filesystem, they would be lost.
To genuinely preserve these directories, either:
- Remove the
rm -rf "./$repoName"andrm -rf "./$repoName/*.md"lines entirely, lettingrsyncoverwrite only necessary files. - Or use
rsync --deletewith appropriate--excludepatterns to selectively clean tracked files while preserving excluded directories.
Also, the excludes for node_modules and .pnpm-store are defensive: this project does not use them (only .venv via Python). Clarify the exclusion list to match actual project dependencies.
Also applies to: 46–50, 55–56
🤖 Prompt for AI Agents
.devcontainer/windows-host-helper.sh around lines 30–34 (and related rsync
excludes at 46–50 and 55–56): the script currently force-deletes the whole repo
directory with sudo rm -rf "./$repoName" and ./ $repoName/*.md which can
irreversibly remove host-side directories like .venv/node_modules/.pnpm-store
that rsync --exclude cannot restore; either remove those rm -rf lines entirely
so rsync can update files in-place, or replace them with a selective rsync-based
cleanup (use rsync --delete together with explicit --exclude patterns) to remove
only tracked files while preserving excluded dirs; also tighten the exclude list
to only the actual host dirs we intend to preserve (e.g., .venv) and remove
unused excludes for node_modules/.pnpm-store if the project doesn't use them;
apply the same change to the other exclude sections referenced (lines 46–50 and
55–56).
| run: | | ||
| pre-commit run -a || PRE_COMMIT_EXIT_CODE=$? | ||
| if [ -n "$PRE_COMMIT_EXIT_CODE" ]; then | ||
| echo "Pre-commit failed with exit code $PRE_COMMIT_EXIT_CODE" | ||
| echo "Showing git diff:" | ||
| git --no-pager diff | ||
| exit $PRE_COMMIT_EXIT_CODE | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Address shell linting warning in pre-commit step.
The refactoring improves error diagnostics by capturing the pre-commit exit code and displaying git diff on failure. However, static analysis flagged an SC2086 warning (double-quote to prevent globbing/word splitting). Additionally, the exit code capture pattern using || PRE_COMMIT_EXIT_CODE=$? is less idiomatic than using conditional execution.
Consider this more straightforward refactor:
- name: Run pre-commit
- run: |
- pre-commit run -a || PRE_COMMIT_EXIT_CODE=$?
- if [ -n "$PRE_COMMIT_EXIT_CODE" ]; then
- echo "Pre-commit failed with exit code $PRE_COMMIT_EXIT_CODE"
- echo "Showing git diff:"
- git --no-pager diff
- exit $PRE_COMMIT_EXIT_CODE
- fi
+ run: |
+ set -e
+ pre-commit run -a || {
+ EXIT_CODE=$?
+ echo "Pre-commit failed with exit code $EXIT_CODE"
+ echo "Showing git diff:"
+ git --no-pager diff
+ exit "$EXIT_CODE"
+ }Alternatively, if you prefer the current structure, ensure the exit code variable is consistently quoted: exit "$PRE_COMMIT_EXIT_CODE".
📝 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.
| run: | | |
| pre-commit run -a || PRE_COMMIT_EXIT_CODE=$? | |
| if [ -n "$PRE_COMMIT_EXIT_CODE" ]; then | |
| echo "Pre-commit failed with exit code $PRE_COMMIT_EXIT_CODE" | |
| echo "Showing git diff:" | |
| git --no-pager diff | |
| exit $PRE_COMMIT_EXIT_CODE | |
| fi | |
| run: | | |
| set -e | |
| pre-commit run -a || { | |
| EXIT_CODE=$? | |
| echo "Pre-commit failed with exit code $EXIT_CODE" | |
| echo "Showing git diff:" | |
| git --no-pager diff | |
| exit "$EXIT_CODE" | |
| } |
🧰 Tools
🪛 actionlint (1.7.9)
72-72: shellcheck reported issue in this script: SC2086:info:6:8: Double quote to prevent globbing and word splitting
(shellcheck)
🤖 Prompt for AI Agents
.github/workflows/pre-commit.yaml lines 72-79: the shell snippet that captures
and exits with the pre-commit exit code triggers shellcheck SC2086 due to
unquoted variable usage and uses a less idiomatic capture pattern; update the
script to either use a simple conditional that captures the exit code into a
quoted variable (e.g., run pre-commit and on failure set
PRE_COMMIT_EXIT_CODE="$?" then exit "$PRE_COMMIT_EXIT_CODE") or refactor to use
conditional execution (if pre-commit run -a; then :; else
PRE_COMMIT_EXIT_CODE="$?"; echo "Pre-commit failed with exit code
$PRE_COMMIT_EXIT_CODE"; echo "Showing git diff:"; git --no-pager diff; exit
"$PRE_COMMIT_EXIT_CODE"; fi), ensuring PRE_COMMIT_EXIT_CODE is always quoted
when referenced.
| import logging | ||
|
|
||
| _imported_fixtures = (snapshot_xml,) | ||
| import pytest | ||
|
|
||
| from .snapshot import snapshot_xml # noqa: F401 # this is a fixture we need in conftest scope | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused logger binding
logger = logging.getLogger(__name__) is currently unused. Either wire it into log calls or drop it to avoid future lint warnings.
🤖 Prompt for AI Agents
In tests/unit/conftest.py lines 1-8, the module-level assignment logger =
logging.getLogger(__name__) is unused; either remove that line to avoid lint
warnings or actually use the logger for any module-level logging (e.g., replace
prints or add debug/info calls inside fixtures). Edit the file to delete the
unused binding if no logs are needed, or wire the logger into existing fixture
code where appropriate so the variable is referenced.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/conftest.py (1)
7-7: Unusedloggerbinding (duplicate from previous review)The module-level
loggeris still unused. Consider removing it or wiring it into fixture code if logging is needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.copier-answers.yml(1 hunks)tests/unit/conftest.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/conftest.py (1)
tests/unit/snapshot.py (1)
snapshot_xml(18-23)
⏰ 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: test (windows-2025, 3.12.7)
- GitHub Check: test (windows-2025, 3.13.2)
🔇 Additional comments (2)
.copier-answers.yml (2)
2-2: Copier template commit bump looks consistent with an upstream refreshPinning
_committov0.0.57-19-g4d3b5bcis expected when updating from the upstream template and keeps futurecopier updateruns reproducible. No issues from my side here.
4-5: Confirm that disabling asyncio and vcrpy integration is intentionalThe new template options
configure_python_asyncio: falseandconfigure_vcrpy: falsewill prevent the template from scaffolding asyncio helpers and vcrpy test tooling. Given the branch/PR context (“cop-vcr” / VCR-related work), please double‑check thatconfigure_vcrpyin particular is meant to stayfalseand that you’re not expecting template‑managed VCR support here.
| def pytest_configure( | ||
| config: pytest.Config, | ||
| ): | ||
| """Configure pytest itself, such as logging levels.""" |
There was a problem hiding this comment.
Empty pytest_configure with misleading docstring
The docstring promises to configure logging levels, but the function body is empty. Either implement the configuration, remove the unused hook, or update the docstring to reflect that no configuration is currently needed.
If logging configuration is intended, apply this diff:
def pytest_configure(
config: pytest.Config,
):
"""Configure pytest itself, such as logging levels."""
+ logging.basicConfig(
+ level=logging.DEBUG,
+ format="%(asctime)s - %(name)s - %(levelname)s - %(message)s"
+ )Or, if no configuration is needed, remove the hook:
-def pytest_configure(
- config: pytest.Config,
-):
- """Configure pytest itself, such as logging levels."""📝 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.
| def pytest_configure( | |
| config: pytest.Config, | |
| ): | |
| """Configure pytest itself, such as logging levels.""" | |
| def pytest_configure( | |
| config: pytest.Config, | |
| ): | |
| """Configure pytest itself, such as logging levels.""" | |
| logging.basicConfig( | |
| level=logging.DEBUG, | |
| format="%(asctime)s - %(name)s - %(levelname)s - %(message)s" | |
| ) |
| def pytest_configure( | |
| config: pytest.Config, | |
| ): | |
| """Configure pytest itself, such as logging levels.""" |
🤖 Prompt for AI Agents
In tests/unit/conftest.py around lines 10 to 13, the pytest_configure hook has
an empty body but a docstring claiming it configures logging levels; either
implement the logging configuration or remove/update the hook. If you intend to
configure logging, add code to set up pytest logging (e.g., configure logging
level/format, call logging.basicConfig or pytest's logging plugin settings) and
keep the docstring; otherwise remove the pytest_configure function entirely or
change the docstring to state no configuration is necessary.
Pull in upstream changes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.