ci(checks): run the test suite on Linux and Windows - #234
Conversation
`checks.yml` runs clippy, fmt, license checks and cargo audit, but never `cargo test`, while `release.yml` compiles, signs and ships a Windows binary. A test that only fails on Windows can therefore reach a release unnoticed, which is what Piebald-AI#226 turned out to be. The new `test` job uses the matrix rather than the Linux-only version, per the preference stated in Piebald-AI#231. `fail-fast: false` matters here: without it a failure on one OS cancels the other, and telling platform-specific failures apart is the whole point of the job. `test` is added to the `needs` list of `all-checks`. Without that the job would run and report, but a failure would not block the merge.
📝 WalkthroughWalkthroughThe checks workflow adds a matrix test job for Ubuntu and Windows. The job installs nightly Rust with Cranelift and runs ChangesCI testing
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/checks.yml:
- Around line 63-65: Restrict the test job permissions by adding a job-level
permissions block under the test job, granting only contents: read for checkout
and leaving all other permissions unavailable.
- Around line 74-75: Update the tests job’s actions/checkout@v4 step to set
persist-credentials to false, ensuring checkout credentials are not retained for
subsequent repository-controlled commands.
- Around line 76-82: Update the workflow’s Rust test configuration around the
“Install Rust toolchain” and “Run tests” steps to opt Cargo into the installed
rustc-codegen-cranelift-preview backend and configure the test profile to use
Cranelift. Ensure cargo test --locked explicitly runs with Cranelift rather than
falling back to LLVM.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8324778c-08c0-4a37-9567-4020957cd06a
📒 Files selected for processing (1)
.github/workflows/checks.yml
| test: | ||
| name: Test (${{ matrix.os }}) | ||
| runs-on: ${{ matrix.os }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
if [ -f .github/workflows/checks.yml ]; then
nl -ba .github/workflows/checks.yml | sed -n '1,120p'
else
echo ".github/workflows/checks.yml not found"
fi
echo
echo "== permissions references in workflow =="
rg -n 'permission[s]?:|permissions\s*:' .github/workflows/checks.yml || true
echo
echo "== repository/organization defaults references =="
rg -n 'default_permissions|permissions|contents:' .github/workflows .github 2>/dev/null || trueRepository: Piebald-AI/splitrail
Length of output: 221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
if [ -f .github/workflows/checks.yml ]; then
nl -ba .github/workflows/checks.yml | sed -n '1,120p'
else
echo ".github/workflows/checks.yml not found"
fi
echo
echo "== permissions references in workflow =="
rg -n 'permission[s]?:|permissions\s*:' .github/workflows/checks.yml || true
echo
echo "== permissions/default defaults references =="
rg -n 'default_permissions|permissions|contents:' .github/workflows .github 2>/dev/null || trueRepository: Piebald-AI/splitrail
Length of output: 221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow exists =="
if [ -f .github/workflows/checks.yml ]; then
echo "yes"
echo
echo "== workflow excerpt =="
awk 'NR<=120{printf "%6d %s\n", NR, $0}' .github/workflows/checks.yml
else
echo "no"
fi
echo
echo "== permissions references in workflow =="
grep -EniE 'permissions?\s*:' .github/workflows/checks.yml || true
echo
echo "== permissions/default references in .github =="
grep -REniE 'default_permissions|permissions\s*:' .github/workflows .github 2>/dev/null || true
echo
echo "== workflow outline/jobs =="
python3 - <<'PY'
from pathlib import Path
p=Path('.github/workflows/checks.yml')
if p.exists():
lines=p.read_text().splitlines()
print([line.strip() for i,line in enumerate(lines,1) if i<=80 and line.strip().startswith('jobs:')] + [line.strip() for i,line in enumerate(lines,1) if i<=80 and line.startswith(' ') and line[1:].split(':',1)[0] in {'on','jobs','name','test','default'}])
PYRepository: Piebald-AI/splitrail
Length of output: 4811
Restrict the test job token permissions.
This job runs repository-controlled code with no job-level permissions, so it can inherit broader repository or organization defaults. Add metrics: contents: read only if needed for checkout; otherwise keep the proposed fix.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 63-82: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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/checks.yml around lines 63 - 65, Restrict the test job
permissions by adding a job-level permissions block under the test job, granting
only contents: read for checkout and leaving all other permissions unavailable.
Source: Linters/SAST tools
| - name: Checkout source code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## workflow context"
if [ -f .github/workflows/checks.yml ]; then
nl -ba .github/workflows/checks.yml | sed -n '1,140p'
else
echo ".github/workflows/checks.yml not found"
fi
echo
echo "## checkout usages"
rg -n "actions/checkout|persist-credentials|cargo|cargo test|cargo build|test" .github/workflows/checks.yml || trueRepository: Piebald-AI/splitrail
Length of output: 150
🌐 Web query:
actions/checkout v4 persist-credentials default true README
💡 Result:
In the actions/checkout action, including version 4, the persist-credentials input defaults to true [1][2]. When set to true, this option configures the authentication token or SSH key with the local git configuration [1][2]. This allows subsequent git commands in your workflow steps to run as an authenticated user [3][4]. The credential is removed automatically during the post-job cleanup process [3][4]. You can opt out of this behavior by explicitly setting persist-credentials to false in your workflow file [3][4]. Note that while the default is true, recent versions of the action (v6 and later) have improved the security of this process by storing credentials in a separate file under the $RUNNER_TEMP directory, rather than directly modifying the.git/config file as was done in earlier versions [4][5].
Citations:
- 1: https://github.com/actions/checkout/blob/v4/action.yml
- 2: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 3: https://github.com/actions/checkout/blob/v4/README.md
- 4: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 5: https://github.com/actions/checkout/tree/v6.0.0
Disable persisted checkout credentials in the tests job.
actions/checkout@v4 defaults persist-credentials to true, so the checkout token remains available to later steps. Since this workflow runs repository-controlled build/test commands, set persist-credentials: false on the checkout action.
The static-analysis warning identifies this credential persistence.
Proposed fix
- name: Checkout source code
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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.
| - name: Checkout source code | |
| uses: actions/checkout@v4 | |
| - name: Checkout source code | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 74-75: 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/checks.yml around lines 74 - 75, Update the tests job’s
actions/checkout@v4 step to set persist-credentials to false, ensuring checkout
credentials are not retained for subsequent repository-controlled commands.
Source: Linters/SAST tools
| - name: Install Rust toolchain | ||
| uses: dtolnay/rust-toolchain@master | ||
| with: | ||
| toolchain: nightly | ||
| components: rustc-codegen-cranelift-preview | ||
| - name: Run tests | ||
| run: cargo test --locked |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Expect: an explicit Cranelift profile setting and -Zcodegen-backend opt-in.
rg -n --glob '*.toml' --glob '*.yml' --glob '*.yaml' \
'rustc-codegen-cranelift-preview|codegen-backend|CARGO_PROFILE_.*CODEGEN_BACKEND|cargo(\s+\+nightly)?\s+test' .Repository: Piebald-AI/splitrail
Length of output: 150
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'tracked TOML/YML files:\n'
git ls-files '*.toml' '*.yml' '*.yaml' | sed -n '1,120p'
printf '\ncargo/rust-related config files:\n'
git ls-files | rg '(^\.cargo/|Cargo\.toml$|\.cargo\.toml$|\.github/workflows/.*\.ya?ml$)' | sed -n '1,200p'
printf '\nquick grep for Cranelift/backend/test keywords (config/workflow files):\n'
rg -n --glob='Cargo.toml' --glob='*.toml' --glob='*.yml' --glob='*.yaml' \
'rustc-codegen-cranelift-preview|codegen-backend|CARGO_PROFILE_.*CODEGEN_BACKEND|cargo(\s+\+nightly)?\s+test|nightly' . \
--glob='!target/**' --glob='!.git/**' || trueRepository: Piebald-AI/splitrail
Length of output: 150
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'tracked TOML/YML files (first 120):\n'
git ls-files '*.toml' '*.yml' '*.yaml' \
2>/dev/null | LC_ALL=C sort \
| head -n 120
printf '\nrelevant tracked files:\n'
git ls-files \
2>/dev/null \
| LC_ALL=C sort \
| rg '(^\.cargo/|Cargo\.toml$|\.cargo\.toml$|\.github/workflows/.*\.ya?ml$|\.rust-toolchain\.toml$|rust-toolchain\.toml$)' \
| head -n 200
printf '\nquick grep for Cranelift/backend/test keywords (config/workflow files, no target):\n'
rg -n --glob='Cargo.toml' --glob='*.toml' --glob='*.yml' --glob='*.yaml' \
'rustc-codegen-cranelift-preview|codegen-backend|CARGO_PROFILE_.*CODEGEN_BACKEND|cargo(\s+\+nightly)?\s+test|nightly' . \
--glob='!target/**' --glob='!.git/**' || trueRepository: Piebald-AI/splitrail
Length of output: 150
Make cargo test use Cranelift.
The workflow installs rustc-codegen-cranelift-preview, but cargo test --locked does not select that backend without profile configuration and the Cargo opt-in. This job can run tests with LLVM instead of Cranelift.
One documented workflow form
- name: Run tests
- run: cargo test --locked
+ env:
+ CARGO_PROFILE_DEV_CODEGEN_BACKEND: cranelift
+ run: cargo test --locked -Zcodegen-backend📝 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.
| - name: Install Rust toolchain | |
| uses: dtolnay/rust-toolchain@master | |
| with: | |
| toolchain: nightly | |
| components: rustc-codegen-cranelift-preview | |
| - name: Run tests | |
| run: cargo test --locked | |
| - name: Install Rust toolchain | |
| uses: dtolnay/rust-toolchain@master | |
| with: | |
| toolchain: nightly | |
| components: rustc-codegen-cranelift-preview | |
| - name: Run tests | |
| env: | |
| CARGO_PROFILE_DEV_CODEGEN_BACKEND: cranelift | |
| run: cargo test --locked -Zcodegen-backend |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 63-82: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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/checks.yml around lines 76 - 82, Update the workflow’s
Rust test configuration around the “Install Rust toolchain” and “Run tests”
steps to opt Cargo into the installed rustc-codegen-cranelift-preview backend
and configure the test profile to use Cranelift. Ensure cargo test --locked
explicitly runs with Cranelift rather than falling back to LLVM.
|
CodeRabbit flagged three things, and all three apply to the jobs already on The missing job-level The Cranelift point is correct on the facts. I have left all three alone. Fixing them only in the job I am adding would make it the one job that differs from its neighbours, which seems worse than leaving them consistent. I can open a separate PR covering all of them together if you want that. |
|
@NickAme03 I'll just fix them in a seperate PR, you don't need to. Merging. |
I'll fix them in a seperate PR.
2 participants
Closes #231.
Adds a
testjob tochecks.yml, in the matrix version you preferred rather than the Linux-only one.fail-fast: falseis deliberate: without it a failure on one OS cancels the other, and telling platform-specific failures apart is the whole point of running both.testis also added to theneedslist ofall-checks. Without that the job would run and report, but a failure would not block a merge, which is the state the issue describes.Verified on my fork before opening this, since the
pushtrigger here only coversmain. Both platforms green, andall-checkspicks up the new dependency: run 30719515514.The Windows row is the one worth checking: the suite passes there today, so this job goes in green rather than red.
Summary by CodeRabbit