Skip to content

ci(repo): Improve CI/CD pipeline performance#7896

Open
nikosdouvlis wants to merge 13 commits intomainfrom
nk/ci-perf-improvements
Open

ci(repo): Improve CI/CD pipeline performance#7896
nikosdouvlis wants to merge 13 commits intomainfrom
nk/ci-perf-improvements

Conversation

@nikosdouvlis
Copy link
Member

@nikosdouvlis nikosdouvlis commented Feb 20, 2026

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:

  • Re-enable turbo remote cache in all CI jobs (pre-checks, build-packages, static-analysis, unit-tests, integration-tests, pkg-pr-new)
  • Add a compute-affected-integration pre-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 setup
  • Unblock static-analysis, unit-tests, and pkg-pr-new from the build-packages gate so they can start immediately and pull build outputs from remote cache
  • Use pnpm turbo build $TURBO_ARGS in pkg-pr-new instead of plain pnpm run build so it benefits from cache hits
  • Add dependsOn to turbo.json integration tasks so turbo --affected properly detects SDK package changes
  • Add drift safeguards: safe default (suites without dependsOn always run) + CI validation (pre-checks fails if any suite is missing dependsOn)

Measured results (before vs after)

Compared run 22210297547 (before, no cache) vs 22220173363 (after, with cache):

  • Build Packages: 59s -> 25s (2.4x)
  • Unit Tests: 176s -> 32s (5.5x)
  • Static analysis: 208s -> 40s (5.2x)
  • pkg-pr-new: 85s -> 52s (1.6x)
  • Total non-integration CI: 591s -> 196s (3x faster)

How integration affected detection works

Each integration task in turbo.json now declares dependsOn with @clerk/clerk-js#build (used by all suites) plus the framework-specific SDK:

"//#test:integration:nextjs": {
  "dependsOn": ["@clerk/clerk-js#build", "@clerk/nextjs#build"],
  "inputs": ["integration/**"]
}

Since build tasks use "dependsOn": ["^build"], changes cascade through the package graph:

packages/shared/src/ change
  -> @clerk/shared#build affected
  -> @clerk/nextjs#build affected (via ^build)
  -> //#test:integration:nextjs affected (via dependsOn)

This gives precise per-suite detection. Changing @clerk/vue only triggers the vue and billing suites, not all 19.

Drift prevention

Two safeguards ensure new suites don't silently skip:

  1. Safe default: the compute step checks turbo.json for dependsOn. Suites without it are forced to run with a warning.
  2. CI validation: pre-checks cross-references package.json test:integration:* scripts against turbo.json. Fails if any suite is missing dependsOn.

See integration/CLAUDE.md for guidance on adding new suites.

Test plan

  • Verify turbo remote cache produces cache hits (Build Packages: 25s)
  • Verify static-analysis and unit-tests start without waiting for build-packages
  • Verify affected detection correctly identifies affected suites (18/19 affected when integration/ file changed)
  • Verify unaffected suites skip in ~4s (cache-components: 4s)
  • Verify all integration tests pass when affected (full run: all green)
  • Verify validation passes locally (all suites have dependsOn)
  • Verify SDK package changes trigger correct subset of suites via dependsOn

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.
@vercel
Copy link

vercel bot commented Feb 20, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clerk-js-sandbox Ready Ready Preview, Comment Feb 20, 2026 2:16pm

Request Review

@changeset-bot
Copy link

changeset-bot bot commented Feb 20, 2026

⚠️ No Changeset found

Latest commit: d132529

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@pkg-pr-new
Copy link

pkg-pr-new bot commented Feb 20, 2026

Open in StackBlitz

@clerk/agent-toolkit

npm i https://pkg.pr.new/@clerk/agent-toolkit@7896

@clerk/astro

npm i https://pkg.pr.new/@clerk/astro@7896

@clerk/backend

npm i https://pkg.pr.new/@clerk/backend@7896

@clerk/chrome-extension

npm i https://pkg.pr.new/@clerk/chrome-extension@7896

@clerk/clerk-js

npm i https://pkg.pr.new/@clerk/clerk-js@7896

@clerk/dev-cli

npm i https://pkg.pr.new/@clerk/dev-cli@7896

@clerk/expo

npm i https://pkg.pr.new/@clerk/expo@7896

@clerk/expo-passkeys

npm i https://pkg.pr.new/@clerk/expo-passkeys@7896

@clerk/express

npm i https://pkg.pr.new/@clerk/express@7896

@clerk/fastify

npm i https://pkg.pr.new/@clerk/fastify@7896

@clerk/hono

npm i https://pkg.pr.new/@clerk/hono@7896

@clerk/localizations

npm i https://pkg.pr.new/@clerk/localizations@7896

@clerk/nextjs

npm i https://pkg.pr.new/@clerk/nextjs@7896

@clerk/nuxt

npm i https://pkg.pr.new/@clerk/nuxt@7896

@clerk/react

npm i https://pkg.pr.new/@clerk/react@7896

@clerk/react-router

npm i https://pkg.pr.new/@clerk/react-router@7896

@clerk/shared

npm i https://pkg.pr.new/@clerk/shared@7896

@clerk/tanstack-react-start

npm i https://pkg.pr.new/@clerk/tanstack-react-start@7896

@clerk/testing

npm i https://pkg.pr.new/@clerk/testing@7896

@clerk/ui

npm i https://pkg.pr.new/@clerk/ui@7896

@clerk/upgrade

npm i https://pkg.pr.new/@clerk/upgrade@7896

@clerk/vue

npm i https://pkg.pr.new/@clerk/vue@7896

commit: d132529

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 20, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This pull request refactors the CI workflow in .github/workflows/ci.yml by introducing a new compute-affected-integration job that determines which integration test suites are affected by changes. Integration tests now depend on this new job and use its computed results to conditionally skip non-affected tests. Several jobs (static-analysis, unit-tests, pkg-pr-new) have updated dependencies. Turbo input configurations previously commented out are now explicitly defined with blocks across multiple jobs. Per-step gating logic is added to skip execution when specific integration tests are not affected. The workflow shifts from task-status-based conditionals to a compute-driven approach for determining test execution.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'ci(repo): Improve CI/CD pipeline performance' accurately reflects the main objective of the PR, which is to optimize CI/CD performance through turbo remote cache re-enablement and a new compute-affected-integration job.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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/workflows

Repository: clerk/javascript

Length of output: 709


🏁 Script executed:

# Check if there's any reference to blacksmith runner configuration
rg -i 'blacksmith' .github/ --type yaml

Repository: clerk/javascript

Length of output: 1467


🏁 Script executed:

# Check the init-blacksmith action for more context
cat -n .github/actions/init-blacksmith/action.yml

Repository: 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.

Comment on lines 341 to 349
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 515 to 517
- name: Upload test-results
if: ${{ cancelled() || failure() }}
if: ${{ !steps.affected.outputs.skip && (cancelled() || failure()) }}
uses: actions/upload-artifact@v4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
- 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.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 329 to 335
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments