Run pytest on Ubuntu, macOS, and Windows in CI#293
Merged
wpak-ai merged 4 commits intoJun 16, 2026
Conversation
leostar0412
requested review from
jonathanMLDev,
snowfox1003 and
wpak-ai
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR extracts pytest execution into a reusable GitHub composite action, replaces the single CI test job with Ubuntu/macOS/Windows platform-specific jobs, adds a PostgreSQL verification script, updates repository docs to describe the new cross-platform test workflow and platform notes, and bumps core dependency versions for security. ChangesCross-platform CI matrix and workflow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/actions.yml (1)
90-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit least-privilege
permissionsfor the new test jobs.These jobs currently inherit default token scopes. Pinning to read-only where possible reduces CI token blast radius.
🔒 Suggested hardening
test-ubuntu: + permissions: + contents: read runs-on: ubuntu-latest timeout-minutes: 15 @@ test-macos: + permissions: + contents: read runs-on: macos-latest timeout-minutes: 25 @@ test-windows: + permissions: + contents: read runs-on: windows-latest timeout-minutes: 25🤖 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 @.github/workflows/actions.yml around lines 90 - 159, Add explicit least-privilege `permissions` configuration to each of the three test jobs: test-ubuntu, test-macos, and test-windows. These jobs currently inherit default token scopes which poses a security risk. Set the `permissions` field for each job to an empty object or specify only the minimal permissions needed (typically none for test-only jobs). This reduces the CI token blast radius by explicitly restricting what each job can do with its GitHub token.Source: Linters/SAST tools
🧹 Nitpick comments (1)
requirements.in (1)
6-6: ⚡ Quick winAutomate the
plyvelmarker check instead of relying on manual verification.This safeguard is important for Windows legs, so it should be enforced by CI (or pre-commit) rather than a comment-only reminder.
♻️ Minimal guard you can add (example CI check)
+ - name: Verify lockfiles keep Windows marker for plyvel + run: | + rg -n 'plyvel.*sys_platform != "win32"' requirements.lock requirements-dev.lock🤖 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 `@requirements.in` at line 6, The manual verification comment for ensuring plyvel maintains the `; sys_platform != "win32"` marker in lock files should be replaced with an automated check. Create a CI or pre-commit script that verifies both lock files contain the plyvel dependency with the correct platform marker for Windows exclusion. This ensures the Windows build safeguard is enforced automatically rather than relying on manual developer verification of the comment reminder.
🤖 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.
Inline comments:
In `@README.md`:
- Around line 175-186: The README claims "A failure on any test job blocks
merge" but the branch protection rules do not actually enforce required status
checks for the test-ubuntu, test-macos, or test-windows jobs. Fix this
discrepancy by either: (1) configuring the three test jobs as required status
checks in GitHub's branch protection settings for both develop and main
branches, or (2) revising the final sentence in the CI section of README.md to
clarify that test failures should block merge once required checks are
configured, rather than stating they currently do. Choose the approach that
aligns with your team's policy and implement accordingly.
---
Outside diff comments:
In @.github/workflows/actions.yml:
- Around line 90-159: Add explicit least-privilege `permissions` configuration
to each of the three test jobs: test-ubuntu, test-macos, and test-windows. These
jobs currently inherit default token scopes which poses a security risk. Set the
`permissions` field for each job to an empty object or specify only the minimal
permissions needed (typically none for test-only jobs). This reduces the CI
token blast radius by explicitly restricting what each job can do with its
GitHub token.
---
Nitpick comments:
In `@requirements.in`:
- Line 6: The manual verification comment for ensuring plyvel maintains the `;
sys_platform != "win32"` marker in lock files should be replaced with an
automated check. Create a CI or pre-commit script that verifies both lock files
contain the plyvel dependency with the correct platform marker for Windows
exclusion. This ensures the Windows build safeguard is enforced automatically
rather than relying on manual developer verification of the comment reminder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30e84736-c13e-4104-bbff-8544f07b96d5
⛔ Files ignored due to path filters (2)
requirements-dev.lockis excluded by!**/*.lockrequirements.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/actions/run-pytest/action.yml.github/workflows/actions.ymlCONTRIBUTING.mdREADME.mddocs/Celery_test.mdrequirements.inscripts/verify_postgres_connection.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In @.github/workflows/actions.yml:
- Around line 112-115: The workflow file contains three instances of the
actions/checkout action (at lines 112-115, 133-136, and 157-160) that lack the
persist-credentials: false setting. This allows the workflow token to be written
to local git config, creating unnecessary token-exposure surface for test jobs
that don't require push authentication. Add persist-credentials: false to the
with section of the actions/checkout action at all three locations to disable
credential persistence and improve security.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32771549-e30d-4606-a007-64562c18afbe
📒 Files selected for processing (2)
.github/actions/run-pytest/action.yml.github/workflows/actions.yml
💤 Files with no reviewable changes (1)
- .github/actions/run-pytest/action.yml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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.
Inline comments:
In @.github/workflows/actions.yml:
- Around line 112-115: The workflow file contains three instances of the
actions/checkout action (at lines 112-115, 133-136, and 157-160) that lack the
persist-credentials: false setting. This allows the workflow token to be written
to local git config, creating unnecessary token-exposure surface for test jobs
that don't require push authentication. Add persist-credentials: false to the
with section of the actions/checkout action at all three locations to disable
credential persistence and improve security.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32771549-e30d-4606-a007-64562c18afbe
📒 Files selected for processing (2)
.github/actions/run-pytest/action.yml.github/workflows/actions.yml
💤 Files with no reviewable changes (1)
- .github/actions/run-pytest/action.yml
🛑 Comments failed to post (1)
.github/workflows/actions.yml (1)
112-115:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable persisted checkout credentials in all test jobs.
Line 112, Line 133, and Line 157 use
actions/checkoutwithoutpersist-credentials: false, so the workflow token is written to local git config by default. These are test jobs and don’t need push auth, so this is avoidable token-exposure surface.🔐 Suggested hardening diff
- name: Checkout uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: fetch-depth: 0 + persist-credentials: falseAlso applies to: 133-136, 157-160
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 112-115: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/actions.yml around lines 112 - 115, The workflow file contains three instances of the actions/checkout action (at lines 112-115, 133-136, and 157-160) that lack the persist-credentials: false setting. This allows the workflow token to be written to local git config, creating unnecessary token-exposure surface for test jobs that don't require push authentication. Add persist-credentials: false to the with section of the actions/checkout action at all three locations to disable credential persistence and improve security.Source: Linters/SAST tools
…1 for security fixes; adjust requirements files accordingly. Enhance CI workflow to set CPPFLAGS and LDFLAGS for leveldb installation.
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/actions.yml (1)
90-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden token exposure for test jobs.
The workflow runs test code without explicit least-privilege token permissions, and checkout keeps credentials in git config by default. Tighten both to reduce credential abuse risk during CI execution.
🔧 Suggested hardening patch
name: CI on: @@ concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true + +permissions: + contents: read jobs: @@ - name: Checkout uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: fetch-depth: 0 + persist-credentials: false @@ - name: Checkout uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: fetch-depth: 0 + persist-credentials: false @@ - name: Checkout uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 with: fetch-depth: 0 + persist-credentials: false🤖 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 @.github/workflows/actions.yml around lines 90 - 177, Add explicit least-privilege token permissions to restrict credential exposure in the test job workflows. For each of the three test jobs (test-ubuntu, test-macos, test-windows), add a permissions block at the job level with only the minimum required permissions (typically empty or just contents: read for checkout). Additionally, update each Checkout step using actions/checkout to disable credential persistence by adding with: persist-credentials: false, which prevents git credentials from being stored in the git config and reduces the attack surface if the checkout token is exposed during test execution.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In @.github/workflows/actions.yml:
- Around line 90-177: Add explicit least-privilege token permissions to restrict
credential exposure in the test job workflows. For each of the three test jobs
(test-ubuntu, test-macos, test-windows), add a permissions block at the job
level with only the minimum required permissions (typically empty or just
contents: read for checkout). Additionally, update each Checkout step using
actions/checkout to disable credential persistence by adding with:
persist-credentials: false, which prevents git credentials from being stored in
the git config and reduces the attack surface if the checkout token is exposed
during test execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d1b76c4-1403-4d24-9a1b-df5352b3e348
⛔ Files ignored due to path filters (2)
requirements-dev.lockis excluded by!**/*.lockrequirements.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/actions/run-pytest/action.yml.github/workflows/actions.ymlCONTRIBUTING.mdREADME.mddocs/Celery_test.mdrequirements.inscripts/verify_postgres_connection.py
…ements and enable required status checks for CI jobs on protected branches.
jonathanMLDev
approved these changes
Jun 16, 2026
wpak-ai
approved these changes
Jun 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CI previously ran the full pytest suite only on
ubuntu-latest, while the README documents macOS and Windows support. This PR splits the singletestjob into three parallel jobs (test-ubuntu,test-macos,test-windows) so path handling, dependency install behavior, and platform-specific code paths are exercised before merge.Changes:
.github/actions/run-pytestcomposite action (checkout, uv install, DB verify, pytest with--cov-fail-under=90, OS-suffixed artifacts).test-ubuntu: existing Dockerservices.postgres+ apt (pandoc, leveldb).test-macos/test-windows:ikalnytskyi/action-setup-postgres@v8(Postgres 16) + OS-specific pandoc/leveldb setup.scripts/verify_postgres_connection.py(replaces bash heredoc; works on Windows PowerShell).plyvelWindows exclusion in lock files:plyvel==1.5.1 ; sys_platform != "win32".docs/Celery_test.mdto describe the new CI layout.lint,pyright, andcompose-smokeremain Ubuntu-only. Nocontinue-on-erroron any test leg.Follow-up after merge: update branch protection required checks from
testtotest-ubuntu,test-macos, andtest-windows.Apps touched
Test plan
python -m pytest(or scoped:python -m pytest <app>/tests) — full suite passed locally on macOS (3218 passed, 4 skipped, 90.42% coverage)uv run pyright(if typed code changed) — N/A (no application code changes)lint-imports(if imports or cross-app coupling changed) — N/ADocs / coupling
python scripts/generate_service_docs.pyrun (ifservices.pyorcore/protocols.pychanged)docs/updated (if behavior or ops changed)Closes #289
Summary by CodeRabbit
pytestwith coverage (HTML/XML), enforces a 90% minimum, and uploads test/coverage artifacts.plyvelmarker verification).