ci(repo): Improve CI/CD pipeline performance#7896
ci(repo): Improve CI/CD pipeline performance#7896nikosdouvlis wants to merge 13 commits intomainfrom
Conversation
Re-enable turbo remote cache that was accidentally left disabled since Dec 2025 during the Verdaccio migration debugging. Compute affected integration suites once in a pre-job instead of spawning 20+ runners that each discover "no work" after setup. Unblock static-analysis, unit-tests, and pkg-pr-new from the build-packages gate so they start immediately with cache hits. Use turbo build in pkg-pr-new instead of plain pnpm build.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request refactors the CI workflow in 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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/ci.yml:
- Around line 341-349: The loop that computes TASK_COUNT for each name (using
TEST_NAMES, AFFECTED_NAMES and TASK_COUNT) currently swallows turbo/jq errors
via "2>/dev/null" and "|| echo '0'", which can silently skip suites; change the
command to preserve failures and fail-fast (or mark as affected) by removing the
stderr suppression and the "|| echo '0'" fallback, capture the command exit
status (e.g., run pnpm turbo ... --dry=json | jq ... into TASK_COUNT and if the
command or jq returns a non-zero status set TASK_COUNT to a sentinel (e.g., -1)
or treat any non-zero exit as affected), then modify the conditional to treat
TASK_COUNT <= 0 as skipped only on success and consider TASK_COUNT < 0 (or
non-zero exit) as affected so CI fails or runs the tests instead of silently
skipping them.
- Line 279: The workflow uses a custom runner label
"blacksmith-8vcpu-ubuntu-2204" in the runs-on key which actionlint doesn't
recognize; fix by either adding an .github/actionlint.yaml that declares this
label under runner-labels (e.g., mapping "blacksmith-8vcpu-ubuntu-2204" as
valid) or modify the runs-on value to include a recognized token such as
"self-hosted" (e.g., ["self-hosted","blacksmith-8vcpu-ubuntu-2204"]) so
actionlint accepts the runner label; update all occurrences of the runs-on entry
(the "runs-on" keys using "blacksmith-8vcpu-ubuntu-2204") accordingly.
| contents: read | ||
| actions: write # needed for actions/upload-artifact | ||
| name: Compute Integration Matrix | ||
| runs-on: "blacksmith-8vcpu-ubuntu-2204" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for actionlint config and whether custom labels are declared
config_files=$(fd -a 'actionlint\.ya?ml' .github || true)
if [ -n "$config_files" ]; then
echo "=== Actionlint Config Files ==="
echo "$config_files"
fd -a 'actionlint\.ya?ml' .github -x cat {}
else
echo "No actionlint config found."
fi
echo ""
echo "=== Runner Label Usage in Workflows ==="
# Show runner label usage in workflows
rg -n 'blacksmith-8vcpu-ubuntu-2204' .github/workflowsRepository: clerk/javascript
Length of output: 709
🏁 Script executed:
# Check if there's any reference to blacksmith runner configuration
rg -i 'blacksmith' .github/ --type yamlRepository: clerk/javascript
Length of output: 1467
🏁 Script executed:
# Check the init-blacksmith action for more context
cat -n .github/actions/init-blacksmith/action.ymlRepository: clerk/javascript
Length of output: 5321
Configure actionlint to recognize the Blacksmith runner label.
The blacksmith-8vcpu-ubuntu-2204 label is used in this workflow (lines 279, 404, and others) but lacks actionlint configuration to recognize it. Add an .github/actionlint.yaml to declare custom runner labels, or include self-hosted in the runs-on value to satisfy linting without configuration.
🧰 Tools
🪛 actionlint (1.7.11)
[error] 279-279: label "blacksmith-8vcpu-ubuntu-2204" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2025-vs2026", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26-large", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 279, The workflow uses a custom runner
label "blacksmith-8vcpu-ubuntu-2204" in the runs-on key which actionlint doesn't
recognize; fix by either adding an .github/actionlint.yaml that declares this
label under runner-labels (e.g., mapping "blacksmith-8vcpu-ubuntu-2204" as
valid) or modify the runs-on value to include a recognized token such as
"self-hosted" (e.g., ["self-hosted","blacksmith-8vcpu-ubuntu-2204"]) so
actionlint accepts the runner label; update all occurrences of the runs-on entry
(the "runs-on" keys using "blacksmith-8vcpu-ubuntu-2204") accordingly.
.github/workflows/ci.yml
Outdated
| AFFECTED_NAMES=() | ||
| for name in "${TEST_NAMES[@]}"; do | ||
| TASK_COUNT=$(pnpm turbo run "test:integration:${name}" --affected --dry=json 2>/dev/null | jq '.tasks | length' 2>/dev/null || echo "0") | ||
| if [ "$TASK_COUNT" -gt 0 ]; then | ||
| AFFECTED_NAMES+=("$name") | ||
| echo " affected: $name" | ||
| else | ||
| echo " skipped: $name" | ||
| fi |
There was a problem hiding this comment.
Don’t swallow turbo/jq failures when computing affected suites.
The current command masks errors and defaults to 0, which can silently mark suites as unaffected and skip all integration tests if turbo/jq fails. Fail fast (or treat failures as affected) to avoid false negatives.
🔧 Proposed fix (fail fast on errors)
- for name in "${TEST_NAMES[@]}"; do
- TASK_COUNT=$(pnpm turbo run "test:integration:${name}" --affected --dry=json 2>/dev/null | jq '.tasks | length' 2>/dev/null || echo "0")
+ for name in "${TEST_NAMES[@]}"; do
+ if ! TASK_COUNT=$(pnpm turbo run "test:integration:${name}" --affected --dry=json | jq '.tasks | length'); then
+ echo "Failed to compute affected tasks for ${name}" >&2
+ exit 1
+ fi
if [ "$TASK_COUNT" -gt 0 ]; then
AFFECTED_NAMES+=("$name")
echo " affected: $name"📝 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.
| AFFECTED_NAMES=() | |
| for name in "${TEST_NAMES[@]}"; do | |
| TASK_COUNT=$(pnpm turbo run "test:integration:${name}" --affected --dry=json 2>/dev/null | jq '.tasks | length' 2>/dev/null || echo "0") | |
| if [ "$TASK_COUNT" -gt 0 ]; then | |
| AFFECTED_NAMES+=("$name") | |
| echo " affected: $name" | |
| else | |
| echo " skipped: $name" | |
| fi | |
| AFFECTED_NAMES=() | |
| for name in "${TEST_NAMES[@]}"; do | |
| if ! TASK_COUNT=$(pnpm turbo run "test:integration:${name}" --affected --dry=json | jq '.tasks | length'); then | |
| echo "Failed to compute affected tasks for ${name}" >&2 | |
| exit 1 | |
| fi | |
| if [ "$TASK_COUNT" -gt 0 ]; then | |
| AFFECTED_NAMES+=("$name") | |
| echo " affected: $name" | |
| else | |
| echo " skipped: $name" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 341 - 349, The loop that computes
TASK_COUNT for each name (using TEST_NAMES, AFFECTED_NAMES and TASK_COUNT)
currently swallows turbo/jq errors via "2>/dev/null" and "|| echo '0'", which
can silently skip suites; change the command to preserve failures and fail-fast
(or mark as affected) by removing the stderr suppression and the "|| echo '0'"
fallback, capture the command exit status (e.g., run pnpm turbo ... --dry=json |
jq ... into TASK_COUNT and if the command or jq returns a non-zero status set
TASK_COUNT to a sentinel (e.g., -1) or treat any non-zero exit as affected),
then modify the conditional to treat TASK_COUNT <= 0 as skipped only on success
and consider TASK_COUNT < 0 (or non-zero exit) as affected so CI fails or runs
the tests instead of silently skipping them.
…n tests Dynamic matrix breaks required status checks because GitHub expects specific job names to be reported. Switch to: compute affected list in a pre-job, keep the static matrix so all job names are reported, but skip expensive steps (checkout, setup, build, test) for non-affected entries using the shared affected output.
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/ci.yml:
- Around line 515-517: The conditional for the Upload test-results step uses
"!steps.affected.outputs.skip" which is always truthy because outputs are
strings; change the predicate to explicitly compare the string value (e.g.
"steps.affected.outputs.skip != 'true'" or use toBoolean conversion) so the
guard works as intended, e.g. replace "!steps.affected.outputs.skip &&
(cancelled() || failure())" with "steps.affected.outputs.skip != 'true' &&
(cancelled() || failure())" (refer to steps.affected.outputs.skip and the Upload
test-results step).
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 279-280: Actionlint flags the custom runner label
"blacksmith-8vcpu-ubuntu-2204" in the runs-on key; update the workflow to make
the runner label actionlint-safe by either adding "self-hosted" alongside the
custom label in the runs-on value (e.g., include self-hosted so actionlint
recognizes it) or add an actionlint config file to whitelist this custom label;
locate the runs-on entry (the string "blacksmith-8vcpu-ubuntu-2204") and change
it to include "self-hosted" or add the actionlint configuration that permits
this custom runner.
- Around line 325-333: The current loop masks turbo/jq failures by piping to jq
and falling back to "0" on any error; change the TASK_COUNT computation to
capture the raw output and exit status instead of silencing errors: run pnpm
turbo ... --dry=json and store its stdout/stderr in a variable, check the
command exit code and only run jq on success (or detect jq errors), and if
either turbo or jq fails, print the error output and exit non‑zero rather than
defaulting to 0; update references to TASK_COUNT, AFFECTED_JSON, and the loop
over TEST_NAMES to use this checked value so failures are surfaced instead of
swallowed.
| - name: Upload test-results | ||
| if: ${{ cancelled() || failure() }} | ||
| if: ${{ !steps.affected.outputs.skip && (cancelled() || failure()) }} | ||
| uses: actions/upload-artifact@v4 |
There was a problem hiding this comment.
Artifact upload guard is always false.
steps.affected.outputs.skip is a non‑empty string, so !steps.affected.outputs.skip always evaluates to false. This prevents failure artifacts from uploading.
🔧 Suggested fix
- if: ${{ !steps.affected.outputs.skip && (cancelled() || failure()) }}
+ if: ${{ steps.affected.outputs.skip != 'true' && (cancelled() || failure()) }}📝 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: Upload test-results | |
| if: ${{ cancelled() || failure() }} | |
| if: ${{ !steps.affected.outputs.skip && (cancelled() || failure()) }} | |
| uses: actions/upload-artifact@v4 | |
| - name: Upload test-results | |
| if: ${{ steps.affected.outputs.skip != 'true' && (cancelled() || failure()) }} | |
| uses: actions/upload-artifact@v4 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 515 - 517, The conditional for the
Upload test-results step uses "!steps.affected.outputs.skip" which is always
truthy because outputs are strings; change the predicate to explicitly compare
the string value (e.g. "steps.affected.outputs.skip != 'true'" or use toBoolean
conversion) so the guard works as intended, e.g. replace
"!steps.affected.outputs.skip && (cancelled() || failure())" with
"steps.affected.outputs.skip != 'true' && (cancelled() || failure())" (refer to
steps.affected.outputs.skip and the Upload test-results step).
Add trailing newline to force turbo affected detection for integration suites. This verifies turbo remote cache, affected computation, and the full E2E pipeline work end-to-end. Revert after verification.
…sking GitHub Actions was suppressing the affected JSON output because INTEGRATION_INSTANCE_KEYS was in the env, causing the secret masker to redact the output. The dry-run check doesn't need the secret.
There was a problem hiding this comment.
🤖 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/ci.yml:
- Around line 514-516: The conditional for the "Upload test-results" step uses a
boolean negation on the string output `steps.affected.outputs.skip`, which is
always truthy; change the condition to explicitly compare the string value
(e.g., `steps.affected.outputs.skip != 'true'` or `steps.affected.outputs.skip
== 'false'`) so the step runs when skip is actually "false", and keep the rest
of the expression (the `cancelled() || failure()`) intact.
- Around line 326-327: The current TASK_COUNT computation for each TEST_NAMES
entry swallows errors by redirecting stderr to /dev/null and falling back to
echo "0", so failures in the pnpm turbo run or jq are ignored; update the pnpm
turbo run "test:integration:${name}" --affected --dry=json | jq '.tasks |
length' invocation to surface failures instead of hiding them: remove the
2>/dev/null and the || echo "0" fallback, capture and check the command's exit
status (or pipe through a small error-checking step) and fail the job or emit a
clear error/log if turbo or jq fails, ensuring TASK_COUNT is only set when the
command succeeds and tests are not silently skipped.
… check Turbo integration tasks only track integration/** as inputs, so changes in packages/clerk-js, packages/ui, packages/react etc. don't trigger them via turbo --affected. Add a git diff check against the base branch to detect source changes in SDK packages and force all integration suites to run when they're found.
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/ci.yml:
- Around line 329-335: The git commands are silenced with "|| true" which hides
failures for BASE_REF/fetch and CHANGED_FILES/diff; update the script so git
fetch and git diff failures are detected and handled: replace the "|| true"
patterns around git fetch and git diff with explicit checks (e.g., run git fetch
... || { echo "git fetch origin ${BASE_REF} failed" >&2; exit 1; } and
CHANGED_FILES=$(git diff --name-only "origin/${BASE_REF}...HEAD") || { echo "git
diff failed" >&2; FORCE_ALL=true; }) so failures either fail the job or safely
force tests by setting FORCE_ALL=true; ensure you still populate CHANGED_FILES
when successful and reference BASE_REF, CHANGED_FILES and FORCE_ALL in the
updated logic.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Line 532: The if condition uses a boolean negation on a string output, so
change the guard to compare the output string explicitly: replace the expression
using !steps.affected.outputs.skip with an explicit string comparison against
'true' (i.e., use steps.affected.outputs.skip != 'true') while keeping the
existing cancelled()/failure() checks; locate the conditional line that
references steps.affected.outputs.skip in the workflow and update it
accordingly.
- Line 279: The workflow uses a custom runner label
"blacksmith-8vcpu-ubuntu-2204" which triggers an actionlint warning; update
runs-on in .github/workflows/ci.yml by replacing that label with a supported
runner (for example "ubuntu-22.04") or, if using a self-hosted runner, switch to
the documented self-hosted label set (e.g., "self-hosted", "linux", "x86_64") so
actionlint no longer flags the runs-on value.
- Around line 343-344: The current pipeline line that sets TASK_COUNT by running
pnpm turbo run "test:integration:${name}" --affected --dry=json | jq '.tasks |
length' uses 2>/dev/null || echo "0", which silently hides failures; change it
to run the turbo/jq pipeline without discarding stderr, capture its exit status,
and only fall back to TASK_COUNT=0 on a detected error. Concretely, run the
pipeline into a variable, check the command's exit code (or jq's exit code) and
if non-zero set TASK_COUNT=0 (and optionally emit a warning), otherwise parse
and assign the length; update the logic that sets IS_AFFECTED to rely on
TASK_COUNT as before. Ensure you modify the code that defines TASK_COUNT and the
subsequent IS_AFFECTED assignment so errors are not silently swallowed.
.github/workflows/ci.yml
Outdated
| BASE_REF="${{ github.event.pull_request.base.ref || 'main' }}" | ||
| git fetch origin "${BASE_REF}" --depth=1 2>/dev/null || true | ||
| CHANGED_FILES=$(git diff --name-only "origin/${BASE_REF}...HEAD" 2>/dev/null || true) | ||
| if echo "$CHANGED_FILES" | grep -qE '^packages/(clerk-js|ui|react|shared|nextjs|remix|tanstack-react-start|vue|nuxt|astro|express|react-router|backend|types)/src/'; then | ||
| echo "Frontend/SDK package source changes detected, forcing all suites" | ||
| FORCE_ALL=true | ||
| fi |
There was a problem hiding this comment.
Silent git failures may cause unexpected behavior.
If git fetch or git diff fails silently (via || true), CHANGED_FILES will be empty and FORCE_ALL stays false, potentially skipping integration tests that should run. Consider logging or failing explicitly when these commands fail unexpectedly.
🔧 Proposed fix to handle git failures more explicitly
FORCE_ALL=false
BASE_REF="${{ github.event.pull_request.base.ref || 'main' }}"
- git fetch origin "${BASE_REF}" --depth=1 2>/dev/null || true
- CHANGED_FILES=$(git diff --name-only "origin/${BASE_REF}...HEAD" 2>/dev/null || true)
+ if ! git fetch origin "${BASE_REF}" --depth=1 2>/dev/null; then
+ echo "Warning: Could not fetch base ref ${BASE_REF}, forcing all suites"
+ FORCE_ALL=true
+ fi
+ if [ "$FORCE_ALL" != "true" ]; then
+ CHANGED_FILES=$(git diff --name-only "origin/${BASE_REF}...HEAD" 2>/dev/null || echo "")
+ if [ -z "$CHANGED_FILES" ] && [ "$(git rev-list --count origin/${BASE_REF}...HEAD 2>/dev/null || echo 0)" -gt 0 ]; then
+ echo "Warning: git diff returned empty unexpectedly, forcing all suites"
+ FORCE_ALL=true
+ fi
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 329 - 335, The git commands are
silenced with "|| true" which hides failures for BASE_REF/fetch and
CHANGED_FILES/diff; update the script so git fetch and git diff failures are
detected and handled: replace the "|| true" patterns around git fetch and git
diff with explicit checks (e.g., run git fetch ... || { echo "git fetch origin
${BASE_REF} failed" >&2; exit 1; } and CHANGED_FILES=$(git diff --name-only
"origin/${BASE_REF}...HEAD") || { echo "git diff failed" >&2; FORCE_ALL=true; })
so failures either fail the job or safely force tests by setting FORCE_ALL=true;
ensure you still populate CHANGED_FILES when successful and reference BASE_REF,
CHANGED_FILES and FORCE_ALL in the updated logic.
Replace the git diff hack with proper turbo.json dependsOn declarations. Each integration task now declares its framework SDK + clerk-js as dependencies, so turbo --affected naturally detects package source changes through the build task graph (^build cascading). For example, changing packages/shared/src/ triggers: @clerk/shared#build affected -> @clerk/nextjs#build affected (via ^build) -> //#test:integration:nextjs affected (via dependsOn) This gives precise per-suite detection instead of the previous approach which forced ALL suites on ANY package change.
… config Two safeguards against drift between integration presets and turbo.json: 1. Safe default in compute step: if a suite has no dependsOn in turbo.json, it's forced to run (with a warning). Failure mode is "runs too much" not "silently never runs." 2. Validation in pre-checks: cross-references package.json test:integration:* scripts against turbo.json entries. Fails CI if any suite is missing dependsOn, catching the problem at PR time.
…o.json Address Codex review findings: - Add @clerk/testing#build to all integration task dependsOn (the integration harness imports @clerk/testing directly) - Derive TEST_NAMES from turbo.json instead of hardcoding, eliminating one source of drift between the matrix, compute step, and turbo config
The CI validation correctly caught that test:integration:cache-components had no turbo.json task entry. Add it with the right dependsOn for its Next.js template.
Summary
The turbo remote cache was accidentally left disabled in CI since Dec 2025. It was turned off during a Verdaccio migration debugging session and never re-enabled, meaning every PR has been rebuilding all packages from scratch across every job.
This PR fixes that and makes several additional improvements:
compute-affected-integrationpre-job that checks which integration suites are affected, then passes the result to each matrix entry. Unaffected entries exit in ~4 seconds instead of spending 2-3 minutes on setuppnpm turbo build $TURBO_ARGSin pkg-pr-new instead of plainpnpm run buildso it benefits from cache hitsdependsOnto turbo.json integration tasks soturbo --affectedproperly detects SDK package changesMeasured results (before vs after)
Compared run 22210297547 (before, no cache) vs 22220173363 (after, with cache):
How integration affected detection works
Each integration task in turbo.json now declares
dependsOnwith@clerk/clerk-js#build(used by all suites) plus the framework-specific SDK:Since build tasks use
"dependsOn": ["^build"], changes cascade through the package graph:This gives precise per-suite detection. Changing
@clerk/vueonly triggers the vue and billing suites, not all 19.Drift prevention
Two safeguards ensure new suites don't silently skip:
test:integration:*scripts against turbo.json. Fails if any suite is missing dependsOn.See
integration/CLAUDE.mdfor guidance on adding new suites.Test plan