CI Improvements - Parallelize integration tests, cache ghcup, fix tag caching#1664
Open
CI Improvements - Parallelize integration tests, cache ghcup, fix tag caching#1664
Conversation
Contributor
WalkthroughAdds a top-level concurrency rule and a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/integrations-test.yml (1)
120-135: Make the matrix coverage self-verifying.These
matchexpressions are now the only thing deciding which integration specs run, but nothing checks that every spec is matched exactly once. A small validation step would keep future spec additions or renames from silently dropping out of CI or running in multiple groups.Also applies to: 212-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integrations-test.yml around lines 120 - 135, Add a CI validation job that reads the repository's integration spec filenames and verifies the matrix match patterns (the include entries for groups "jvm", "compiled", and "scripting-and-other") cover each spec exactly once; implement the check as a step (bash or node) that expands the three regex-like match strings, collects matched spec paths, and fails with a non-zero exit code if any spec is unmatched or matched multiple times so the workflow cannot silently drop or duplicate specs. Ensure the script references the same match strings used in the matrix (the match values for groups jvm, compiled, scripting-and-other) so the validation stays in sync and add this validation job as a required/early job in the workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/integrations-test.yml:
- Around line 197-202: The workflow currently re-runs vendor_download.sh in each
matrix job, which causes inconsistent tool snapshots and extra network
dependency; instead, run vendor_download.sh once in the build job to create the
vendor-bins directory, upload it as an artifact from the build job (use
actions/upload-artifact), and in the matrix test jobs replace the vendor-bins
creation step with an artifact download step (use actions/download-artifact) to
restore vendor-bins; update references to vendor-bins and vendor_download.sh so
build produces and uploads the artifact and downstream jobs only download and
use it.
---
Nitpick comments:
In @.github/workflows/integrations-test.yml:
- Around line 120-135: Add a CI validation job that reads the repository's
integration spec filenames and verifies the matrix match patterns (the include
entries for groups "jvm", "compiled", and "scripting-and-other") cover each spec
exactly once; implement the check as a step (bash or node) that expands the
three regex-like match strings, collects matched spec paths, and fails with a
non-zero exit code if any spec is unmatched or matched multiple times so the
workflow cannot silently drop or duplicate specs. Ensure the script references
the same match strings used in the matrix (the match values for groups jvm,
compiled, scripting-and-other) so the validation stays in sync and add this
validation job as a required/early job in the workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 155800b7-36f6-4c2f-b9ce-75828e2ba1c2
📒 Files selected for processing (1)
.github/workflows/integrations-test.yml
zlav
changed the base branch from
master
to
zach/simplify-integration-cache-keys
zlav
force-pushed
the
zach/parallel-integration-tests
branch
3 times, most recently
from
36f593b to
af5c8c0
Compare
zlav
force-pushed
the
zach/parallel-integration-tests
branch
from
086b6ac to
0b1c744
Compare
zlav
changed the title
zlav
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/integrations-test.yml (1)
183-188:⚠️ Potential issue | 🟠 MajorDon't redownload vendored binaries in every matrix job.
vendor_download.shstill resolves releases at runtime here, so thebuildjob and each test group can end up compiling against different vendored snapshots. It also adds another network-dependent failure point afterneeds: buildhas already succeeded. Producevendor-binsonce inbuildand pass it to the matrix jobs as an artifact instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integrations-test.yml around lines 183 - 188, The workflow currently runs vendor_download.sh inside each matrix job ("Update vendored binaries" step), causing inconsistent vendored snapshots and extra network dependence; instead, run ./vendor_download.sh once in the build job to create vendor-bins, upload vendor-bins as an artifact from the build job (use the existing GITHUB_TOKEN/secrets as needed), then remove the vendor_download.sh invocation from the matrix jobs and add an artifact download step in those matrix jobs to retrieve the vendor-bins artifact before tests compile; update the steps named "Update vendored binaries" (in build) to use upload-artifact and the ones in the matrix groups to use download-artifact so all jobs consume the same vendored snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-all.yml:
- Around line 73-83: The ghcup cache key only varies by GHC and OS (key: ${{
matrix.os-name }}-ghcup-${{ matrix.ghc }}) but the cache includes cabal's
symlink (~/.ghcup/bin), so update the Cache ghcup step (actions/cache@v5) to
include the cabal version from the matrix (e.g. append -${{ matrix.cabal-version
}} or the actual matrix variable name used for cabal) to the key so changes to
the cabal-version invalidate the cache.
In @.github/workflows/integrations-test.yml:
- Around line 190-200: Remove the redundant "cabal update" invocation from the
integration test step named "Run integration tests (${{ matrix.group }})": edit
the multi-line run block that currently begins with "cabal update" and delete
that first line so the job relies on the restored cabal store/dist-newstyle
caches and does not update the index again, leaving the remaining loop that
builds MATCH_ARGS and the final "cabal test ..." invocation intact.
---
Duplicate comments:
In @.github/workflows/integrations-test.yml:
- Around line 183-188: The workflow currently runs vendor_download.sh inside
each matrix job ("Update vendored binaries" step), causing inconsistent vendored
snapshots and extra network dependence; instead, run ./vendor_download.sh once
in the build job to create vendor-bins, upload vendor-bins as an artifact from
the build job (use the existing GITHUB_TOKEN/secrets as needed), then remove the
vendor_download.sh invocation from the matrix jobs and add an artifact download
step in those matrix jobs to retrieve the vendor-bins artifact before tests
compile; update the steps named "Update vendored binaries" (in build) to use
upload-artifact and the ones in the matrix groups to use download-artifact so
all jobs consume the same vendored snapshot.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53b47ff5-da65-420a-9311-b4bfa59d8c66
📒 Files selected for processing (3)
.github/workflows/bench.yml.github/workflows/build-all.yml.github/workflows/integrations-test.yml
zlav
force-pushed
the
zach/parallel-integration-tests
branch
from
0b1c744 to
166fccc
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.github/workflows/integrations-test.yml (2)
186-198:⚠️ Potential issue | 🟠 MajorRemove redundant
cabal updatefrom test jobs.The build job already runs
cabal update(line 96) and populates the caches. Running it again in each test job can advance the package index between build and test phases, potentially causing dependency resolution drift. Since the caches are already populated and index state should be locked, this update is unnecessary.Suggested fix
- name: Run integration tests (${{ matrix.group }}) run: | - cabal update MATCH_ARGS="" for pattern in ${{ matrix.matches }}; do MATCH_ARGS="$MATCH_ARGS --test-option=--match --test-option=$pattern" done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integrations-test.yml around lines 186 - 198, Remove the redundant "cabal update" invocation at the top of the "Run integration tests (${{ matrix.group }})" job; keep the rest of the job (the MATCH_ARGS loop, the cabal test invocation with --project-file=cabal.project.ci.linux and the integration-tests target) intact so tests use the cached package index populated by the build job and avoid advancing the index between build and test phases.
179-184:⚠️ Potential issue | 🟠 MajorDon't re-run
vendor_download.shin each matrix job.Each of the three test jobs re-executes
vendor_download.sh, which:
- Fetches latest GitHub releases at runtime, so different jobs may get different tool versions
- Creates race conditions when jobs run concurrently on the same runner (the script runs
rm -f vendor-bins/*and writes to shared paths)- Adds unnecessary network calls and failure points after build already succeeded
Run
vendor_download.shonce in the build job and uploadvendor-bins/as an artifact for test jobs to download.Suggested approach
In the build job (after line 90), add:
- uses: actions/upload-artifact@v4 with: name: vendor-bins path: vendor-bins/In the integration-test job, replace lines 179-184 with:
- uses: actions/download-artifact@v4 with: name: vendor-bins path: vendor-bins/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integrations-test.yml around lines 179 - 184, Replace repeated runtime invocation of vendor_download.sh in each matrix test job by producing and consuming a single artifact: in the build job, run vendor_download.sh once to populate vendor-bins/ and then upload that directory using actions/upload-artifact (name it vendor-bins); in the integration-test matrix jobs, remove the mkdir + ./vendor_download.sh block and instead download the uploaded artifact using actions/download-artifact with name vendor-bins and path vendor-bins/. This avoids concurrent writes, non-deterministic release fetches, and extra network calls while preserving the same vendor-bins contents for all test jobs..github/workflows/build-all.yml (1)
73-83:⚠️ Potential issue | 🟡 MinorInclude
cabal-versionin the ghcup cache key.The cache key only varies by OS and GHC version, but
~/.ghcup/bincontains cabal symlinks. If the cabal version at line 90 (3.10.3.0) changes, the cache won't invalidate, leading to version mismatches.Suggested fix
- key: ${{ matrix.os-name }}-ghcup-${{ matrix.ghc }} + key: ${{ matrix.os-name }}-ghcup-${{ matrix.ghc }}-cabal-3.10.3.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-all.yml around lines 73 - 83, The ghcup cache key only includes OS and GHC but not the cabal version, causing stale ~/.ghcup/bin symlink mismatches; update the workflow matrix to expose the cabal version (e.g., add a matrix entry like cabal-version: '3.10.3.0' or similar) and change the actions/cache key in the ghcup block (currently "${{ matrix.os-name }}-ghcup-${{ matrix.ghc }}") to include that value (e.g., "${{ matrix.os-name }}-ghcup-${{ matrix.ghc }}-cabal-${{ matrix.cabal-version }}") so the cache invalidates when cabal changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build-all.yml:
- Around line 73-83: The ghcup cache key only includes OS and GHC but not the
cabal version, causing stale ~/.ghcup/bin symlink mismatches; update the
workflow matrix to expose the cabal version (e.g., add a matrix entry like
cabal-version: '3.10.3.0' or similar) and change the actions/cache key in the
ghcup block (currently "${{ matrix.os-name }}-ghcup-${{ matrix.ghc }}") to
include that value (e.g., "${{ matrix.os-name }}-ghcup-${{ matrix.ghc
}}-cabal-${{ matrix.cabal-version }}") so the cache invalidates when cabal
changes.
In @.github/workflows/integrations-test.yml:
- Around line 186-198: Remove the redundant "cabal update" invocation at the top
of the "Run integration tests (${{ matrix.group }})" job; keep the rest of the
job (the MATCH_ARGS loop, the cabal test invocation with
--project-file=cabal.project.ci.linux and the integration-tests target) intact
so tests use the cached package index populated by the build job and avoid
advancing the index between build and test phases.
- Around line 179-184: Replace repeated runtime invocation of vendor_download.sh
in each matrix test job by producing and consuming a single artifact: in the
build job, run vendor_download.sh once to populate vendor-bins/ and then upload
that directory using actions/upload-artifact (name it vendor-bins); in the
integration-test matrix jobs, remove the mkdir + ./vendor_download.sh block and
instead download the uploaded artifact using actions/download-artifact with name
vendor-bins and path vendor-bins/. This avoids concurrent writes,
non-deterministic release fetches, and extra network calls while preserving the
same vendor-bins contents for all test jobs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2762af6d-54a4-4852-bf71-7702ef0c0bae
📒 Files selected for processing (3)
.github/workflows/bench.yml.github/workflows/build-all.yml.github/workflows/integrations-test.yml
zlav
commented
Mar 11, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/integrations-test.yml (2)
186-198:⚠️ Potential issue | 🟠 MajorDrop the second
cabal updatefrom the test jobs.The
buildjob already updates the index and populates the shared cabal/dist caches. Runningcabal updateagain here adds another network dependency to every matrix leg, and any rebuild in this phase can now happen against a newer package index than the cache was built with.Suggested fix
- name: Run integration tests (${{ matrix.group }}) run: | - cabal update MATCH_ARGS="" for pattern in ${{ matrix.matches }}; do MATCH_ARGS="$MATCH_ARGS --test-option=--match --test-option=$pattern" done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integrations-test.yml around lines 186 - 198, Remove the redundant network call by deleting the second "cabal update" inside the integration test job block named "Run integration tests (${{ matrix.group }})"; keep the MATCH_ARGS loop and the cabal test invocation (including --project-file=cabal.project.ci.linux and integration-tests with +RTS flags) unchanged so the job uses the index populated by the earlier build job instead of updating again.
179-184:⚠️ Potential issue | 🟠 MajorDon't redownload
vendor-binsin each matrix leg.
buildalready resolves and uses these vendored tools. Downloading them again here means the test jobs can run against a different external snapshot than the one that populated the shared cache, and it adds three extra network-dependent failure points. Uploadvendor-binsonce frombuildand download it here instead.Suggested fix
build: @@ - name: Update vendored binaries run: | mkdir vendor-bins ./vendor_download.sh env: GITHUB_TOKEN: ${{ secrets.FOSSABOT_ANALYSIS_VENDOR_DOWNLOAD_TOKEN }} + + - uses: actions/upload-artifact@v4 + with: + name: vendor-bins + path: vendor-bins @@ - - name: Update vendored binaries - run: | - mkdir vendor-bins - ./vendor_download.sh - env: - GITHUB_TOKEN: ${{ secrets.FOSSABOT_ANALYSIS_VENDOR_DOWNLOAD_TOKEN }} + - uses: actions/download-artifact@v4 + with: + name: vendor-bins + path: vendor-bins🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integrations-test.yml around lines 179 - 184, The workflow step "Update vendored binaries" currently re-creates vendor-bins by running ./vendor_download.sh in each matrix job; instead remove that step and fetch the previously uploaded vendor-bins artifact/cache produced by the build job (i.e., stop re-downloading in the matrix leg), so the matrix jobs use the same vendored snapshot as the build; replace the mkdir + ./vendor_download.sh usage with the artifact/cache download action referencing the upload from the build job (ensure names match the upload in the build job and that subsequent steps use the downloaded vendor-bins directory).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/integrations-test.yml:
- Around line 132-134: The integration-test job recomputes the latest tag
independently which can differ from the build job; change the job to consume the
build job's output instead of re-running the git tag step: remove or disable the
step with id/latest-tag inside the integration-test job and reference
needs.build.outputs.latest-tag (the `latest-tag` output produced by the build
job) when composing the dist-newstyle cache key and any other places that
previously used the local latest-tag step, making sure the build job emits that
output (id `latest-tag`) as shown so needs.build.outputs.latest-tag is available
to the integration-test job.
---
Duplicate comments:
In @.github/workflows/integrations-test.yml:
- Around line 186-198: Remove the redundant network call by deleting the second
"cabal update" inside the integration test job block named "Run integration
tests (${{ matrix.group }})"; keep the MATCH_ARGS loop and the cabal test
invocation (including --project-file=cabal.project.ci.linux and
integration-tests with +RTS flags) unchanged so the job uses the index populated
by the earlier build job instead of updating again.
- Around line 179-184: The workflow step "Update vendored binaries" currently
re-creates vendor-bins by running ./vendor_download.sh in each matrix job;
instead remove that step and fetch the previously uploaded vendor-bins
artifact/cache produced by the build job (i.e., stop re-downloading in the
matrix leg), so the matrix jobs use the same vendored snapshot as the build;
replace the mkdir + ./vendor_download.sh usage with the artifact/cache download
action referencing the upload from the build job (ensure names match the upload
in the build job and that subsequent steps use the downloaded vendor-bins
directory).
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e392e47-0a5a-481e-aa76-f8d61f0e5f10
📒 Files selected for processing (3)
.github/workflows/bench.yml.github/workflows/build-all.yml.github/workflows/integrations-test.yml
zlav
force-pushed
the
zach/parallel-integration-tests
branch
2 times, most recently
from
170e59a to
b408dc1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/bench.yml:
- Around line 21-23: The "Get latest release tag" step (id: latest-tag) sets an
empty tag when there are no git tags because the current pipeline uses a pipe
where head's exit status masks git tag's failure and the `|| echo none` never
runs; replace the one-liner with a robust sequence that captures git tag output
into a shell variable, test if that variable is empty, and set it to "none" when
empty, then write "tag=<value>" to $GITHUB_OUTPUT; apply the same fix to the
identical steps in the other workflows (the steps in build-all.yml and
integrations-test.yml).
In @.github/workflows/build-all.yml:
- Around line 73-83: The ghcup cache key hardcodes the Cabal/GHC version in the
actions/cache step (uses: actions/cache@v5) causing duplication with the
cabal-version elsewhere; move the version into a single source (either matrix or
env like a new matrix entry cabal_version or env.CABAL_VERSION) and replace both
the hardcoded literal and any other occurrences (e.g., the cache key expression
using ${{ matrix.ghc }}-3.10.3.0 and the cabal-version reference) to reference
that single variable so cache invalidation stays consistent.
In @.github/workflows/integrations-test.yml:
- Around line 103-113: The upload steps using actions/upload-artifact@v4 for the
artifacts named "vendor-bins" and "rust-release-bins" should fail the job when
paths are missing; change those steps to include the input if-no-files-found:
"error" so the workflow errors instead of warning when artifacts are absent
(update the two steps with uses: actions/upload-artifact@v4 that set name:
vendor-bins and name: rust-release-bins to include if-no-files-found: "error").
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e9b3f4d-481b-4d18-82de-8867edeeedd6
📒 Files selected for processing (3)
.github/workflows/bench.yml.github/workflows/build-all.yml.github/workflows/integrations-test.yml
zlav
force-pushed
the
zach/parallel-integration-tests
branch
from
b408dc1 to
b069069
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/integrations-test.yml:
- Around line 114-137: The grep -c invocation can exit with code 1 when there
are zero matches, aborting the step before the TOTAL vs MATCHED comparison;
update the two places that compute TOTAL and MATCHED (the commands assigning
TOTAL=$(...) and MATCHED=$(...)) to append "|| true" after the pipeline into
grep (e.g., ... | grep -c "^ " || true) so the command substitution returns "0"
instead of causing a non-zero exit, preserving the subsequent comparison logic.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43fb2cd4-6ff1-44fb-9043-158d6dc299c2
📒 Files selected for processing (3)
.github/workflows/bench.yml.github/workflows/build-all.yml.github/workflows/integrations-test.yml
zlav
force-pushed
the
zach/parallel-integration-tests
branch
from
b069069 to
441e9cf
Compare
1. Split integration tests into build + 3 parallel matrix jobs (jvm, compiled, scripting-and-other). Build job populates caches, test jobs restore them. Adds concurrency group to cancel stale runs. 2. Cache ~/.ghcup on macOS/Windows in build-all.yml to skip the ~500 MB GHC download on cache hit. 3. Fix release tag resolution in dist-newstyle cache keys. git describe needs reachable history (fails with fetch-depth: 2). Switch to git tag --sort=-v:refname which lists fetched tag refs regardless of shallow clone depth. Also upgrades actions/cache v4 to v5 (node20 -> node22 runtime). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zlav
force-pushed
the
zach/parallel-integration-tests
branch
from
441e9cf to
e9ff2d2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/integrations-test.yml (2)
116-129:⚠️ Potential issue | 🟠 Major
grep -c ... || echo 0makes the count non-numeric on zero matches.Lines 116-129 use
|| echo 0, butgrep -calready prints0before returning exit code 1 when there are no matches. In that caseTOTAL/MATCHEDbecome a two-line string, and the numeric comparison at Line 133 fails before your custom error message runs.Suggested fix
- TOTAL=$(cabal test --project-file=cabal.project.ci.linux \ - --test-show-details=direct \ - integration-tests \ - --test-option=--dry-run 2>&1 | grep -c "^ " || echo 0) + TOTAL=$(cabal test --project-file=cabal.project.ci.linux \ + --test-show-details=direct \ + integration-tests \ + --test-option=--dry-run 2>&1 | grep -c "^ " || true) @@ - MATCHED=$(cabal test --project-file=cabal.project.ci.linux \ - --test-show-details=direct \ - $MATCH_ARGS \ - integration-tests \ - --test-option=--dry-run 2>&1 | grep -c "^ " || echo 0) + MATCHED=$(cabal test --project-file=cabal.project.ci.linux \ + --test-show-details=direct \ + $MATCH_ARGS \ + integration-tests \ + --test-option=--dry-run 2>&1 | grep -c "^ " || true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integrations-test.yml around lines 116 - 129, The issue is that using "|| echo 0" after "grep -c" can produce a two-line result when there are no matches, breaking numeric comparisons for TOTAL and MATCHED; update the assignments that set TOTAL and MATCHED (the blocks that run "cabal test ... | grep -c "^ "" and the MATCH_ARGS loop) to remove the "|| echo 0" fallback (or alternatively append "|| true" to preserve a single-line numeric output), ensuring TOTAL and MATCHED remain single numeric strings usable in subsequent comparisons.
141-151:⚠️ Potential issue | 🟡 MinorFail the build immediately if required artifacts are missing.
These uploads are mandatory inputs for every matrix leg, but
upload-artifactonly warns on missing paths by default. Letting thebuildjob fail here keeps the error at the source instead of turning it into downstream misses or rebuilds.Suggested fix
- uses: actions/upload-artifact@v4 with: name: vendor-bins path: vendor-bins/ + if-no-files-found: error - uses: actions/upload-artifact@v4 with: name: rust-release-bins path: | target/release/berkeleydb target/release/millhone + if-no-files-found: error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integrations-test.yml around lines 141 - 151, The workflow currently uploads two required artifacts with actions/upload-artifact@v4 (the steps named by with.name: "vendor-bins" and "rust-release-bins") but doesn't fail on missing paths; update both upload steps to set the if-no-files-found option to "error" so the job fails immediately when the specified paths (vendor-bins/ and the two target/release binaries) are not present—modify the upload steps for the "vendor-bins" and "rust-release-bins" uses of actions/upload-artifact@v4 to include if-no-files-found: "error"..github/workflows/bench.yml (1)
21-23:⚠️ Potential issue | 🟡 MinorFix the empty-tag fallback.
Line 23 can still write
tag=when no tags are available, becausehead -1succeeds on empty input and prevents the|| echo nonefallback from running.Suggested fix
- run: echo "tag=$(git tag --sort=-v:refname | head -1 2>/dev/null || echo none)" >> $GITHUB_OUTPUT + run: | + tag="$(git tag --sort=-v:refname 2>/dev/null | head -1)" + echo "tag=${tag:-none}" >> "$GITHUB_OUTPUT"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bench.yml around lines 21 - 23, The "Get latest release tag" step (id: latest-tag) can still produce an empty tag because the current pipeline lets head succeed on empty input; change the run so the fallback applies to the whole tag-producing pipeline (or add a post-check) — either ensure the command piping git tag into head uses a trailing "|| echo none" applied to the pipeline as a unit, or assign the output to tag and then test if it's empty and set it to "none" before writing to GITHUB_OUTPUT; update the run line for latest-tag accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-all.yml:
- Around line 66-68: The "Get latest release tag" step (id: latest-tag)
currently exports an empty tag when no git tags exist because the one-liner's
fallback never runs; modify the run block so you first capture the output of git
tag --sort=-v:refname | head -1 into a variable, then test if that variable is
empty and set it to "none" when empty, and finally echo "tag=$VAR" to
$GITHUB_OUTPUT; update the step that currently runs echo "tag=$(git tag
--sort=-v:refname | head -1 2>/dev/null || echo none)" >> $GITHUB_OUTPUT to use
this two-step capture-and-check approach.
In @.github/workflows/integrations-test.yml:
- Around line 14-22: The lists for test groups are duplicated; centralize them
into a single env variable (TEST_GROUPS) and derive both MATCH_ARGS and the
GitHub Actions matrix.matches from that single source. Update the workflow to
keep TEST_GROUPS as the canonical space-separated string and add a step or use
workflow expressions to parse TEST_GROUPS into an array (e.g., split or fromJson
after converting to JSON), then set MATCH_ARGS from TEST_GROUPS and set
strategy.matrix.matches from the parsed array so both MATCH_ARGS and
matrix.matches are generated programmatically from TEST_GROUPS (references:
TEST_GROUPS, MATCH_ARGS, matrix.matches).
---
Duplicate comments:
In @.github/workflows/bench.yml:
- Around line 21-23: The "Get latest release tag" step (id: latest-tag) can
still produce an empty tag because the current pipeline lets head succeed on
empty input; change the run so the fallback applies to the whole tag-producing
pipeline (or add a post-check) — either ensure the command piping git tag into
head uses a trailing "|| echo none" applied to the pipeline as a unit, or assign
the output to tag and then test if it's empty and set it to "none" before
writing to GITHUB_OUTPUT; update the run line for latest-tag accordingly.
In @.github/workflows/integrations-test.yml:
- Around line 116-129: The issue is that using "|| echo 0" after "grep -c" can
produce a two-line result when there are no matches, breaking numeric
comparisons for TOTAL and MATCHED; update the assignments that set TOTAL and
MATCHED (the blocks that run "cabal test ... | grep -c "^ "" and the MATCH_ARGS
loop) to remove the "|| echo 0" fallback (or alternatively append "|| true" to
preserve a single-line numeric output), ensuring TOTAL and MATCHED remain single
numeric strings usable in subsequent comparisons.
- Around line 141-151: The workflow currently uploads two required artifacts
with actions/upload-artifact@v4 (the steps named by with.name: "vendor-bins" and
"rust-release-bins") but doesn't fail on missing paths; update both upload steps
to set the if-no-files-found option to "error" so the job fails immediately when
the specified paths (vendor-bins/ and the two target/release binaries) are not
present—modify the upload steps for the "vendor-bins" and "rust-release-bins"
uses of actions/upload-artifact@v4 to include if-no-files-found: "error".
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba46e597-15c8-4e0f-b6eb-5f83531be872
📒 Files selected for processing (3)
.github/workflows/bench.yml.github/workflows/build-all.yml.github/workflows/integrations-test.yml
…g artifacts - Upgrade actions/checkout, actions/upload-artifact, actions/download-artifact from v4 to v5 across all workflows (Node.js 20 deprecation). - Fix broken tag fallback in build-all.yml (head -1 always exits 0, so || echo none never ran). - Add if-no-files-found: error to upload-artifact steps in integration tests so missing vendor-bins or Rust binaries fail the build job. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GitHub Actions runs bash with -e -o pipefail. When git tag exits 128
(e.g. no tags fetched), pipefail propagates the error even though
head -1 succeeds. Add || true to the pipeline so ${tag:-none} handles
the empty case.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/integrations-test.yml:
- Around line 46-48: The "Get latest release tag" step (id: latest-tag) uses a
one-liner where `head -1` masks the failure so the `|| echo none` fallback never
runs; change it to a two-step capture-and-default pattern: first assign the git
output to a shell variable (e.g., tag="$(git tag --sort=-v:refname | head -1
2>/dev/null)") and then echo "tag=${tag:-none}" >> $GITHUB_OUTPUT so the
`${tag:-none}` default is applied when the git command yields empty output.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28dbe13c-c528-4efa-ac9f-d5e4531ab297
📒 Files selected for processing (8)
.github/workflows/badges.yml.github/workflows/bench.yml.github/workflows/build-all.yml.github/workflows/dependency-scan.yml.github/workflows/install-script-test.yml.github/workflows/integrations-test.yml.github/workflows/lint.yml.github/workflows/report.yml
zlav
changed the title
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/integrations-test.yml (1)
14-22: 🧹 Nitpick | 🔵 TrivialConsider generating matrix from TEST_GROUPS to eliminate duplication.
The test patterns are duplicated between
env.TEST_GROUPS(lines 17-22) andmatrix.matches(lines 170, 173, 176). While the validation step catches missing patterns, it won't catch if a pattern exists inTEST_GROUPSbut isn't assigned to any matrix group. A future edit could add a pattern toTEST_GROUPSfor validation while forgetting to add it to a matrix group, causing tests to never run.One approach: define groups in a JSON structure and use
fromJson()to generate the matrix dynamically.Also applies to: 165-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integrations-test.yml around lines 14 - 22, The TEST_GROUPS environment value is duplicated with the matrix.matches entries; change the workflow to generate the matrix from a single canonical JSON structure instead of repeating patterns: move the group definitions into a JSON string (e.g., set as an env or workflow variable named TEST_GROUPS_JSON), then replace the hard-coded matrix.matches with a dynamic expression that uses fromJson(env.TEST_GROUPS_JSON) (or fromJson(vars.TEST_GROUPS_JSON)) to build the matrix entries; update any validation step to read the same JSON source and remove the duplicated literal TEST_GROUPS list so matrix.matches and TEST_GROUPS are derived from the same JSON data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/integrations-test.yml:
- Around line 14-22: The TEST_GROUPS environment value is duplicated with the
matrix.matches entries; change the workflow to generate the matrix from a single
canonical JSON structure instead of repeating patterns: move the group
definitions into a JSON string (e.g., set as an env or workflow variable named
TEST_GROUPS_JSON), then replace the hard-coded matrix.matches with a dynamic
expression that uses fromJson(env.TEST_GROUPS_JSON) (or
fromJson(vars.TEST_GROUPS_JSON)) to build the matrix entries; update any
validation step to read the same JSON source and remove the duplicated literal
TEST_GROUPS list so matrix.matches and TEST_GROUPS are derived from the same
JSON data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6364931b-52a4-413e-8c3c-46898bcd669f
📒 Files selected for processing (3)
.github/workflows/bench.yml.github/workflows/build-all.yml.github/workflows/integrations-test.yml
The cabal version is intentionally the same across all matrix entries. Keep it hardcoded in the setup and cache key rather than adding a matrix variable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Define test groups as JSON in env.TEST_MATRIX. The matrix strategy uses fromJson() and the validation step extracts patterns with jq, both from the same variable. Prevents adding a pattern to the validation list without assigning it to a matrix group. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/integrations-test.yml:
- Around line 194-210: Remove the dead reference to
steps.setup-haskell.outputs.cabal-store and use the literal cabal store path
instead: replace any occurrence of "${{ steps.setup-haskell.outputs.cabal-store
|| '~/.local/state/cabal' }}" with the concrete "~/.local/state/cabal" in the
cache step that names "Cache cabal store" (and mirror the same change in the
build job where the same expression appears around line 82); this keeps the
actions/cache path explicit and avoids referring to a non-existent setup-haskell
step.
- Around line 166-169: The strategy.matrix include currently uses
fromJson(env.TEST_MATRIX), but the env context is unavailable inside strategy
blocks; change the expression to use an allowed context (for example replace
env.TEST_MATRIX with vars.TEST_MATRIX if you set TEST_MATRIX in
repository/organization vars, or with needs.build.outputs.TEST_MATRIX if the
matrix is produced by a prior job, or with github.event.inputs.TEST_MATRIX for
workflow_dispatch). Update the matrix include line that references
fromJson(env.TEST_MATRIX) to one of these supported contexts and ensure the
producing place (vars, needs.build outputs, or event inputs) actually defines
TEST_MATRIX.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 64cc980c-4ed5-401f-8a87-47b66ee006b5
📒 Files selected for processing (1)
.github/workflows/integrations-test.yml
env is not available in strategy.matrix evaluation. Pass TEST_MATRIX through build job outputs so the integration-test matrix can consume it via needs.build.outputs.test-matrix. Also replace dead steps.setup-haskell.outputs.cabal-store references with the literal ~/.local/state/cabal path (no setup-haskell step exists in these jobs). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
On Windows, ghcup installs to C:\ghcup\, not ~/.ghcup/. Use runner.os to select the correct path per platform so the Windows ghcup cache actually hits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/build-all.yml (1)
84-92: 🧹 Nitpick | 🔵 TrivialUse a single source of truth for Cabal version.
3.10.3.0is duplicated in both the ghcup cache key andsetup-haskell. If one changes independently, cache invalidation can drift.Proposed refactor
build-all: name: ${{ matrix.os-name }}-build runs-on: ${{ matrix.os }} container: ${{ matrix.container }} + env: + CABAL_VERSION: '3.10.3.0' defaults: run: shell: bash @@ - key: ${{ matrix.os-name }}-ghcup-${{ matrix.ghc }}-3.10.3.0 + key: ${{ matrix.os-name }}-ghcup-${{ matrix.ghc }}-${{ env.CABAL_VERSION }} @@ - cabal-version: '3.10.3.0' + cabal-version: ${{ env.CABAL_VERSION }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-all.yml around lines 84 - 92, The Cabal version literal is duplicated; introduce a single source (e.g., add a matrix or env entry like matrix.cabal or env.CABAL_VERSION) and reference that variable in both the cache key (key: ${{ matrix.os-name }}-ghcup-${{ matrix.ghc }}-${{ matrix.cabal }}) and the haskell-actions/setup@v2 step (id: setup-haskell, with: cabal-version: ${{ matrix.cabal }}), so updates only need to be made in one place; update all occurrences of the literal "3.10.3.0" to use that variable (ensure matrix.cabal is defined alongside matrix.ghc).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build-all.yml:
- Around line 84-92: The Cabal version literal is duplicated; introduce a single
source (e.g., add a matrix or env entry like matrix.cabal or env.CABAL_VERSION)
and reference that variable in both the cache key (key: ${{ matrix.os-name
}}-ghcup-${{ matrix.ghc }}-${{ matrix.cabal }}) and the haskell-actions/setup@v2
step (id: setup-haskell, with: cabal-version: ${{ matrix.cabal }}), so updates
only need to be made in one place; update all occurrences of the literal
"3.10.3.0" to use that variable (ensure matrix.cabal is defined alongside
matrix.ghc).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 450f4714-8dcc-4d23-9d46-53cb2afc3d8e
📒 Files selected for processing (1)
.github/workflows/build-all.yml
csasarak
requested changes
Mar 13, 2026
Contributor
csasarak
left a comment
csasarak
left a comment
There was a problem hiding this comment.
My only blocking comment is the one asking about upload-artifact behavior.
| id: latest-tag | ||
| run: echo "tag=$(git describe --tags --abbrev=0 2>/dev/null || echo none)" >> $GITHUB_OUTPUT | ||
| run: | | ||
| tag="$(git tag --sort=-v:refname 2>/dev/null | head -1 || true)" |
Contributor
There was a problem hiding this comment.
Why not git describe? It seems tailor made for this: https://git-scm.com/docs/git-describe
| ${{ runner.os == 'Windows' && 'C:\ghcup\bin' || '~/.ghcup/bin' }} | ||
| ${{ runner.os == 'Windows' && format('C:\ghcup\ghc\{0}', matrix.ghc) || format('~/.ghcup/ghc/{0}', matrix.ghc) }} | ||
| ${{ runner.os == 'Windows' && 'C:\ghcup\cache' || '~/.ghcup/cache' }} | ||
| key: ${{ matrix.os-name }}-ghcup-${{ matrix.ghc }}-3.10.3.0 |
Contributor
There was a problem hiding this comment.
What is this 3.10.3.0 at the end? The version of cabal used? I think it should be in a variable which names it.
Comment on lines
+19
to
+21
| {"group": "jvm", "matches": "Analysis.Maven Analysis.Gradle Analysis.Scala Analysis.Clojure"}, | ||
| {"group": "compiled", "matches": "Analysis.Go Analysis.Rust Analysis.Erlang Analysis.Elixir"}, | ||
| {"group": "scripting-and-other", "matches": "Analysis.Python Analysis.Ruby Analysis.Swift Analysis.Cocoapods Analysis.Pnpm Analysis.Nuget Analysis.Carthage Analysis.LicenseScanner Container.Analysis Reachability.Upload"} |
Comment on lines
+25
to
+27
| # Build once, then run test groups in parallel. | ||
| # The build job populates the cabal and dist-newstyle caches. | ||
| # Each test job restores those caches so it doesn't need to rebuild. |
Contributor
There was a problem hiding this comment.
This comment just describes what the code does.
Comment on lines
+143
to
+157
| # Upload artifacts that test jobs need so they don't have to rebuild | ||
| # Rust or re-download vendor binaries. | ||
| - uses: actions/upload-artifact@v5 | ||
| with: | ||
| name: vendor-bins | ||
| path: vendor-bins/ | ||
| if-no-files-found: error | ||
|
|
||
| - uses: actions/upload-artifact@v5 | ||
| with: | ||
| name: rust-release-bins | ||
| path: | | ||
| target/release/berkeleydb | ||
| target/release/millhone | ||
| if-no-files-found: error |
Contributor
There was a problem hiding this comment.
I'm not super familiar with this action, but this is how we upload artifacts to display in our release lists. I don't think we want these exposed in our list of release artifacts. Can you verify for me that they won't be?
This is blocking for me.
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
2 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.
Overview
Four CI improvements across integration tests, build-all, and benchmarks workflows.
Impact
1. Parallelize integration tests
Split the single integration test job into a build job + 3 parallel test matrix jobs:
Build job compiles once and populates caches. Test jobs restore caches and run their subset via
--match. Adds a concurrency group to cancel stale in-progress runs on the same branch.A
TEST_GROUPSenv var at the workflow level lists all match patterns. After building, a validation step runs--dry-runto count total tests vs matched tests and fails if any test is uncovered by a group. This prevents new tests from not being added to the matrix.Vendor binaries and Rust release binaries are uploaded as artifacts from the build job and downloaded in test jobs, avoiding redundant
cargo build --release(~1m) andvendor_download.sh(~20s) per test job.The build job exports the release tag via job outputs so test jobs use the same tag for cache keys (avoids a race if a release happens mid-run).
Note This update also stops previous integration tests from running if a new commit is pushed. We have limited runner space, and I've been seeing a lot of tests in the "Waiting to run this spec" state. This should fix that.
2. Cache ghcup in build-all.yml
Cache
~/.ghcup/{bin,ghc,cache}on macOS and Windows. Skips the ~500 MB GHC download on cache hit, reducing theSetup ghc/cabalstep from ~4m to ~30s. Cache key includes both GHC and cabal versions.3. Fix release tag resolution
git describe --tags --abbrev=0requires the tagged commit to be reachable in local history. Withfetch-depth: 2, the tag is never reachable and always resolves tonone. Switched togit tag --sort=-v:refname | head -1which lists fetched tag refs regardless of shallow clone depth.4. Upgrade actions/cache to v5
Upgrade
actions/cachefrom v4 to v5 across all workflows (integration tests, build-all, benchmarks). v5 uses the node22 runtime. v4 uses node20 which GitHub is deprecating (forced migration to node24 starts June 2, 2026). Self-hosted runner is on v2.332.0, above the v2.327.1 minimum.Testing plan
App.Version.THchain).Setup ghc/cabaldropped from ~4m to ~30s.Risks
--dry-rungrep pattern (grep -c "^ ") depends on hspec's output format. If hspec changes its dry-run output, the validation step may need updating.Checklist
docs/.docs/README.msand gave consideration to how discoverable or not my documentation is.Changelog.md. If this PR did not mark a release, I added my changes into an## Unreleasedsection at the top..fossa.ymlorfossa-deps.{json.yml}, I updateddocs/references/files/*.schema.jsonAND I have updated example files used byfossa initcommand. You may also need to update these if you have added/removed new dependency type (e.g.pip) or analysis target type (e.g.poetry).docs/references/subcommands/<subcommand>.md.