fix: simplify CI and integration test timeout#1416
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ctrlc03
force-pushed
the
fix/ci-simplification
branch
from
9a3a8ab to
fcfc8d9
Compare
Contributor
📝 WalkthroughWalkthroughAdds a GitHub Actions detect_changes job that computes per-path boolean outputs and gates ~16 downstream CI jobs; updates integration test helpers to add timeout handling (JS waitForEvent and shell waiton) and increases test durations. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
actor Developer
end
rect rgba(200,255,200,0.5)
participant GH as "GitHub Actions"
participant Detect as "detect_changes job"
participant Compute as "Compute step"
participant Jobs as "Downstream CI jobs"
end
Developer->>GH: push / open PR
GH->>Detect: start detect_changes
Detect->>Compute: evaluate path filters
Compute->>Detect: emit boolean outputs
Detect->>Jobs: provide outputs via needs.outputs
Jobs->>Jobs: conditional execution if outputs == "true"
Jobs-->>GH: report results / artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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 |
ctrlc03
force-pushed
the
fix/ci-simplification
branch
from
fcfc8d9 to
91f40ec
Compare
ctrlc03
force-pushed
the
fix/ci-simplification
branch
from
91f40ec to
642d8f5
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
7-12:⚠️ Potential issue | 🔴 CriticalRestore a
pushtrigger for post-merge automation.With only
workflow_dispatchandpull_request, this workflow never runs on branch pushes. That makes thegithub.ref == 'refs/heads/main'paths unreachable after merge, including the image publish steps andcontrib-readme-job.🔧 Suggested fix
on: workflow_dispatch: + push: + branches: + - main + - dev pull_request: branches: - main - dev🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 7 - 12, The workflow's triggers only include workflow_dispatch and pull_request so post-merge branch pushes never run; restore a push trigger by adding "push" with the same branch list (main and dev) under the existing "on:" block so jobs that check github.ref == 'refs/heads/main' (e.g., image publish and contrib-readme-job) will execute after merges; update the on: section to include pull_request, workflow_dispatch and push (branches: - main - dev).
🤖 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 100-114: The ciphernode_e2e output flag is enabled for INTEGRATION
and CIRCUITS but the dependent producer flags build_sdk and integration_prebuild
do not include the same path conditions, allowing ciphernode_integration_test to
run while its producers are skipped; update the echo expressions for build_sdk
and integration_prebuild so their any(...) calls include the same variables as
ciphernode_e2e (specifically include INTEGRATION and CIRCUITS where appropriate)
to ensure build_sdk and integration_prebuild are selected whenever
ciphernode_e2e/ciphernode_integration_test are selected.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 7-12: The workflow's triggers only include workflow_dispatch and
pull_request so post-merge branch pushes never run; restore a push trigger by
adding "push" with the same branch list (main and dev) under the existing "on:"
block so jobs that check github.ref == 'refs/heads/main' (e.g., image publish
and contrib-readme-job) will execute after merges; update the on: section to
include pull_request, workflow_dispatch and push (branches: - main - dev).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2a33b65-bbe8-4d96-b084-2581ee2ef8bd
📒 Files selected for processing (3)
.github/workflows/ci.ymltemplates/default/tests/integration.spec.tstests/integration/fns.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
455-456:build_enclave_clinow bypasses the new pruning logic.All of its current consumers are already gated by
detect_changes, so keeping this job unconditional still pays for a full CLI build on PRs where none of those consumers can run. Consider deriving abuild_enclave_cliflag from the union of those consumer conditions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 455 - 456, The build_enclave_cli job is unconditional and bypasses pruning; change it to run only when any of its consumers would run by deriving a boolean flag (e.g., build_enclave_cli_needed) from the union of the existing detect_changes conditions used by its consumer jobs, then add an if: condition on the build_enclave_cli job to reference that flag; update any outputs or matrix entries the consumers expect so they still find the artifact when the job runs. Ensure you reference and reuse the existing detect_changes outputs/expressions so the new flag accurately reflects the consumers' gating logic.
🤖 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 98-114: The job filter lines call the helper any() with unquoted
variable expansions (e.g., any $FORCE $RUST $CONTRACTS ...) causing SC2086
word-splitting and also append to GITHUB_OUTPUT with many separate >> redirects
(SC2129); fix by quoting each argument when calling any (e.g., any "$FORCE"
"$RUST" "$CONTRACTS" ...) and collapse the multiple echo >> "$GITHUB_OUTPUT"
statements into a single grouped redirect (for example wrap all echo lines in {
...; } >> "$GITHUB_OUTPUT" or use a here-doc) so "$GITHUB_OUTPUT" is quoted and
writes occur with a single redirect.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 455-456: The build_enclave_cli job is unconditional and bypasses
pruning; change it to run only when any of its consumers would run by deriving a
boolean flag (e.g., build_enclave_cli_needed) from the union of the existing
detect_changes conditions used by its consumer jobs, then add an if: condition
on the build_enclave_cli job to reference that flag; update any outputs or
matrix entries the consumers expect so they still find the artifact when the job
runs. Ensure you reference and reuse the existing detect_changes
outputs/expressions so the new flag accurately reflects the consumers' gating
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ff50ba9-71ba-4690-a095-6aaa7617f9ff
📒 Files selected for processing (3)
.github/workflows/ci.ymltemplates/default/tests/integration.spec.tstests/integration/fns.sh
ctrlc03
force-pushed
the
fix/ci-simplification
branch
from
642d8f5 to
caa1eee
Compare
ctrlc03
force-pushed
the
fix/ci-simplification
branch
from
caa1eee to
420de5a
Compare
cedoor
previously approved these changes
Mar 12, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
98-114:⚠️ Potential issue | 🟠 Major
Compute job filtersstill has lint errors, andbuild_sdkis being overwritten.actionlint/shellcheck will keep complaining about the unquoted
anyarguments and repeated redirects here, and Lines 101 and 110 both writebuild_sdk, so the narrower expression at Line 110 overrides the broader one from Line 101.🔧 Suggested fix
any() { for v in "$@"; do [ "$v" = "true" ] && echo "true" && return; done; echo "false"; } - - echo "rust_tests=$(any $FORCE $RUST $CONTRACTS $CIRCUITS $CI)" >> $GITHUB_OUTPUT - echo "build_sdk=$(any "$FORCE" "$RUST" "$CONTRACTS" "$SDK" "$INTEGRATION" "$CIRCUITS" "$CI" "$TEMPLATES")" >> $GITHUB_OUTPUT - echo "crisp=$(any $FORCE $CRISP $CONTRACTS $CIRCUITS $RUST $CI)" >> $GITHUB_OUTPUT - echo "templates=$(any $FORCE $TEMPLATES $RUST $CONTRACTS $SDK $CI)" >> $GITHUB_OUTPUT - echo "zk=$(any $FORCE $RUST $CIRCUITS $CI)" >> $GITHUB_OUTPUT - echo "contracts=$(any $FORCE $CONTRACTS $CI)" >> $GITHUB_OUTPUT - echo "docker_support=$(any $FORCE $DOCKER $CI)" >> $GITHUB_OUTPUT - echo "docker_ciphernode=$(any $FORCE $RUST $CONTRACTS $DOCKER $CI)" >> $GITHUB_OUTPUT - echo "net=$(any $FORCE $INTEGRATION $CI)" >> $GITHUB_OUTPUT - echo "init=$(any $FORCE $TEMPLATES $RUST $CONTRACTS $CI)" >> $GITHUB_OUTPUT - echo "build_sdk=$(any $FORCE $RUST $CONTRACTS $SDK $CI $TEMPLATES)" >> $GITHUB_OUTPUT - echo "build_e3_support_dev=$(any $FORCE $TEMPLATES $RUST $CONTRACTS $SDK $CI)" >> $GITHUB_OUTPUT - echo "build_circuits=$(any $FORCE $RUST $CIRCUITS $CI)" >> $GITHUB_OUTPUT - echo "integration_prebuild=$(any "$FORCE" "$RUST" "$CONTRACTS" "$CIRCUITS" "$INTEGRATION" "$CI")" >> $GITHUB_OUTPUT - echo "zk_prover_integration=$(any $FORCE $RUST $CIRCUITS $CI)" >> $GITHUB_OUTPUT + { + echo "rust_tests=$(any "$FORCE" "$RUST" "$CONTRACTS" "$CIRCUITS" "$CI")" + echo "build_sdk=$(any "$FORCE" "$RUST" "$CONTRACTS" "$SDK" "$INTEGRATION" "$CIRCUITS" "$CI" "$TEMPLATES")" + echo "crisp=$(any "$FORCE" "$CRISP" "$CONTRACTS" "$CIRCUITS" "$RUST" "$CI")" + echo "templates=$(any "$FORCE" "$TEMPLATES" "$RUST" "$CONTRACTS" "$SDK" "$CI")" + echo "zk=$(any "$FORCE" "$RUST" "$CIRCUITS" "$CI")" + echo "contracts=$(any "$FORCE" "$CONTRACTS" "$CI")" + echo "docker_support=$(any "$FORCE" "$DOCKER" "$CI")" + echo "docker_ciphernode=$(any "$FORCE" "$RUST" "$CONTRACTS" "$DOCKER" "$CI")" + echo "net=$(any "$FORCE" "$INTEGRATION" "$CI")" + echo "init=$(any "$FORCE" "$TEMPLATES" "$RUST" "$CONTRACTS" "$CI")" + echo "build_e3_support_dev=$(any "$FORCE" "$TEMPLATES" "$RUST" "$CONTRACTS" "$SDK" "$CI")" + echo "build_circuits=$(any "$FORCE" "$RUST" "$CIRCUITS" "$CI")" + echo "integration_prebuild=$(any "$FORCE" "$RUST" "$CONTRACTS" "$CIRCUITS" "$INTEGRATION" "$CI")" + echo "zk_prover_integration=$(any "$FORCE" "$RUST" "$CIRCUITS" "$CI")" + } >> "$GITHUB_OUTPUT"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 98 - 114, The job filter block calls any() with unquoted variables and also defines build_sdk twice causing the second echo of "build_sdk" to overwrite the first; fix by passing all arguments to any as quoted variables (e.g., change any $FORCE $RUST ... to any "$FORCE" "$RUST" ... for every echo) and remove or rename the duplicate echo "build_sdk=..." (keep the intended broader definition from the first build_sdk echo and delete the narrower/duplicated one), referencing the any() function and the two "build_sdk" echo lines to locate the edits.
🤖 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 98-114: The job filter block calls any() with unquoted variables
and also defines build_sdk twice causing the second echo of "build_sdk" to
overwrite the first; fix by passing all arguments to any as quoted variables
(e.g., change any $FORCE $RUST ... to any "$FORCE" "$RUST" ... for every echo)
and remove or rename the duplicate echo "build_sdk=..." (keep the intended
broader definition from the first build_sdk echo and delete the
narrower/duplicated one), referencing the any() function and the two "build_sdk"
echo lines to locate the edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cea739eb-cb4a-4918-9dce-187536b7ed62
📒 Files selected for processing (3)
.github/workflows/ci.ymltemplates/default/tests/integration.spec.tstests/integration/fns.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/default/tests/integration.spec.ts
ctrlc03
force-pushed
the
fix/ci-simplification
branch
from
c9ef829 to
fb30e3e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/ci.yml (2)
453-454: Consider gatingbuild_enclave_clito avoid unnecessary builds.This job always runs regardless of path changes. While safe, it wastes CI resources when no downstream jobs (ciphernode_integration_test, crisp_e2e, template_integration, test_enclave_init) are triggered.
You could add a combined condition that runs
build_enclave_clionly when at least one of its dependent jobs would run:needs: [detect_changes] if: >- needs.detect_changes.outputs.ciphernode_e2e == 'true' || needs.detect_changes.outputs.crisp == 'true' || needs.detect_changes.outputs.templates == 'true' || needs.detect_changes.outputs.init == 'true'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 453 - 454, The build_enclave_cli job currently always runs; modify its job definition (job name: build_enclave_cli) to depend on the detect_changes job and add a combined if condition so it only runs when at least one downstream job would run — specifically add needs: [detect_changes] and an if that checks needs.detect_changes.outputs.ciphernode_e2e == 'true' || needs.detect_changes.outputs.crisp == 'true' || needs.detect_changes.outputs.templates == 'true' || needs.detect_changes.outputs.init == 'true', ensuring the job is gated by those detect_changes outputs.
394-396: Track the removedbasetest suite.The TODO indicates the
basetest was intentionally removed from the matrix. Consider creating an issue to track re-enabling it once the underlying issue is resolved.Would you like me to help create an issue to track re-enabling the
basetest suite?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 394 - 396, Create a new issue to track re-enabling the removed "base" test suite: reference the TODO and the CI matrix change where "test-suite: [persist]" was left (and "base" removed), describe the current state and failure that prompted removal, include steps to reproduce the CI failure, any logs or PRs that led to the change, and propose a remediation plan; tag the issue with labels like "ci" and "tests", assign to the team or owner responsible for CI, and add a follow-up reminder to re-check and re-enable the "base" suite once the underlying issue is fixed.tests/integration/fns.sh (1)
68-77: Good timeout implementation with one shellcheck fix needed.The timeout logic is well-designed with a sensible 600-second default. However, line 69 combines declaration and assignment, which masks the return value of
date +%s(SC2155).🔧 Proposed fix to separate declaration and assignment
waiton() { local file_path="$1" local timeout="${2:-600}" # default 10 minutes - local start_time=$(date +%s) + local start_time + start_time=$(date +%s) until [ -f "$file_path" ]; do if [ $(($(date +%s) - start_time)) -ge $timeout ]; then echo "Timeout after ${timeout}s waiting for: $file_path" >&2 return 1 fi sleep 1 done }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/fns.sh` around lines 68 - 77, Split the combined declaration+assignment for start_time to satisfy shellcheck SC2155: replace the single-line "local start_time=$(date +%s)" with a separate declaration and assignment so you first declare local start_time and then set start_time=$(date +%s); keep the rest of the timeout loop (the until [ -f "$file_path" ] loop and timeout logic) unchanged and ensure you reference the same start_time variable in the time comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 453-454: The build_enclave_cli job currently always runs; modify
its job definition (job name: build_enclave_cli) to depend on the detect_changes
job and add a combined if condition so it only runs when at least one downstream
job would run — specifically add needs: [detect_changes] and an if that checks
needs.detect_changes.outputs.ciphernode_e2e == 'true' ||
needs.detect_changes.outputs.crisp == 'true' ||
needs.detect_changes.outputs.templates == 'true' ||
needs.detect_changes.outputs.init == 'true', ensuring the job is gated by those
detect_changes outputs.
- Around line 394-396: Create a new issue to track re-enabling the removed
"base" test suite: reference the TODO and the CI matrix change where
"test-suite: [persist]" was left (and "base" removed), describe the current
state and failure that prompted removal, include steps to reproduce the CI
failure, any logs or PRs that led to the change, and propose a remediation plan;
tag the issue with labels like "ci" and "tests", assign to the team or owner
responsible for CI, and add a follow-up reminder to re-check and re-enable the
"base" suite once the underlying issue is fixed.
In `@tests/integration/fns.sh`:
- Around line 68-77: Split the combined declaration+assignment for start_time to
satisfy shellcheck SC2155: replace the single-line "local start_time=$(date
+%s)" with a separate declaration and assignment so you first declare local
start_time and then set start_time=$(date +%s); keep the rest of the timeout
loop (the until [ -f "$file_path" ] loop and timeout logic) unchanged and ensure
you reference the same start_time variable in the time comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec737ba2-2f1d-41a3-86cf-f4acebe2039c
📒 Files selected for processing (3)
.github/workflows/ci.ymltemplates/default/tests/integration.spec.tstests/integration/fns.sh
hmzakhalid
approved these changes
Mar 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit