ci: speed up PR test workflow#473
Merged
Merged
Conversation
Contributor
📝 WalkthroughGitHub Actions Test Workflow OptimizationWorkflow restructuring (fast vs. surface-gated full tests)
Path-based gating (surface detection)
Coverage gating (reduced CI load)
Soroban CI optimization
Rust cache improvements (composite action)Updated
Workflow permissions hardening
Performance results
Critical aspects for human review
WalkthroughUpdates the Rust cache action with per-job shared keys and conditional saves. Restructures the test workflow with ChangesCI Gating and Test Restructuring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
carrion256
force-pushed
the
codex/ci-speed-pr472
branch
from
June 16, 2026 16:17
21e0403 to
420a684
Compare
carrion256
force-pushed
the
codex/ci-speed-pr472
branch
from
June 16, 2026 16:31
420a684 to
fa634e4
Compare
carrion256
marked this pull request as ready for review
June 17, 2026 08:22
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b883233eb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+541
to
+543
| require_success "Code Formatting" "$FORMAT_RESULT" | ||
| require_success "Code Linter" "$LINTER_RESULT" | ||
| require_success "Fast Tests" "$FAST_TEST_RESULT" |
There was a problem hiding this comment.
In this required gate, the job runs after failed dependencies via if: always(), but the mandatory checks never validate needs.changes.result. If checkout or dorny/paths-filter fails on a PR, the path-gated jobs are skipped because their outputs are unavailable, fail_if_failed accepts those skipped results, and the required gate can still pass without running the integration jobs that the changed paths would have required; add a require_success for Change Detection before trusting its outputs.
Useful? React with 👍 / 👎.
Comment on lines
+90
to
+95
| soroban: | ||
| - 'contract/proxy-oracle/soroban/**' | ||
| - 'contract/vault/curator-primitives/**' | ||
| - 'contract/vault/soroban/**' | ||
| - 'script/soroban/**' | ||
| - 'tools/soroban-vault-cli/**' |
There was a problem hiding this comment.
For PRs touching only shared Soroban kernel crates, this filter remains false because it watches contract/proxy-oracle/soroban/** and contract/vault/soroban/** but not the kernels those contracts compile. I checked contract/vault/soroban/Cargo.toml:19 and contract/proxy-oracle/soroban/contract/Cargo.toml:22 plus governance at contract/proxy-oracle/soroban/governance-contract/Cargo.toml:20; those packages depend on contract/vault/kernel, contract/proxy-oracle/kernel, and contract/proxy-oracle/governance-kernel, so kernel-only PRs skip the Soroban build/size gates that should catch ABI and wasm-size regressions.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 40-43: Remove the id-token: write permission from the
workflow-level permissions block, keeping only contents: read and pull-requests:
read at that level. Then add a permissions section to the coverage job
definition that includes id-token: write to scope this sensitive permission only
to the job that requires it for Codecov OIDC authentication.
- Around line 59-61: Replace the version tag `@v3` in the dorny/paths-filter
action with a pinned commit hash to improve supply chain security. Change the
action reference from `dorny/paths-filter@v3` to
`dorny/paths-filter@6852f92c20ea7fd3b0c25de3b5112db3a98da050` to ensure the
workflow always uses the specific verified commit rather than allowing the
maintainer to modify code under the tag.
- Around line 486-569: The gate script does not verify that the changes job
succeeded before proceeding with other validations. To fix this, add
CHANGES_RESULT variable to the env section to capture the result of
needs.changes.result, then call require_success with "Changes Detection" and the
CHANGES_RESULT variable at the beginning of the validation checks (after the
full_ci and coverage_required variables are set but before any other
require_success calls). Additionally, add explicit handling for the pull_request
event in the case statement that evaluates EVENT_NAME to ensure consistent
behavior across all event types.
In `@script/soroban/install-stellar-cli.sh`:
- Line 25: The INSTALL_MODE variable assignment currently silently defaults to
binary mode if an invalid STELLAR_CLI_INSTALL_MODE value is provided, which
masks configuration errors. After assigning INSTALL_MODE on line 25, add
explicit validation to check if the STELLAR_CLI_INSTALL_MODE environment
variable (when set) contains a valid value. If the value is invalid, immediately
exit the script with a clear error message. Apply the same validation logic to
the other location mentioned at lines 134-138 to ensure consistent validation
throughout the script.
- Around line 126-128: The issue is that the stellar-cli installation via cargo
install at line 126 doesn't ensure the stellar binary is accessible in the PATH
during verification at line 142. In shells where the Cargo bin directory isn't
automatically exported, the verification fails despite successful installation.
Fix this by explicitly adding the Cargo bin directory to PATH after the cargo
install command completes, ensuring that the subsequent verification check (the
stellar command) can locate the installed binary and prevent false failures from
occurring.
- Around line 121-123: Remove the error suppression mechanism `|| true` and
stderr redirection `2>/dev/null` from the rustup toolchain install command so
that failures are properly propagated. The success message using the `ok`
function should only be printed when the rustup toolchain install command
actually succeeds, not unconditionally. Modify the rustup toolchain install
invocation to allow the script to detect and handle actual installation failures
appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4394d3d-e35e-4da6-9fdf-b5c8f6a628ea
📒 Files selected for processing (3)
.github/actions/rust/action.yml.github/workflows/test.ymlscript/soroban/install-stellar-cli.sh
Comment on lines
+121
to
+123
| info "Installing Rust ${RUST_TOOLCHAIN} toolchain (for CLI build only)..." | ||
| rustup toolchain install "${RUST_TOOLCHAIN}" --profile minimal 2>/dev/null || true | ||
| ok "Rust ${RUST_TOOLCHAIN} ready" |
Contributor
There was a problem hiding this comment.
Do not mask Rust toolchain installation failures.
Line 122 uses || true, then Line 123 prints success unconditionally. This can hide the real failure cause and mislead troubleshooting.
Suggested fix
- rustup toolchain install "${RUST_TOOLCHAIN}" --profile minimal 2>/dev/null || true
+ rustup toolchain install "${RUST_TOOLCHAIN}" --profile minimal📝 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
| info "Installing Rust ${RUST_TOOLCHAIN} toolchain (for CLI build only)..." | |
| rustup toolchain install "${RUST_TOOLCHAIN}" --profile minimal 2>/dev/null || true | |
| ok "Rust ${RUST_TOOLCHAIN} ready" | |
| info "Installing Rust ${RUST_TOOLCHAIN} toolchain (for CLI build only)..." | |
| rustup toolchain install "${RUST_TOOLCHAIN}" --profile minimal | |
| ok "Rust ${RUST_TOOLCHAIN} ready" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/soroban/install-stellar-cli.sh` around lines 121 - 123, Remove the
error suppression mechanism `|| true` and stderr redirection `2>/dev/null` from
the rustup toolchain install command so that failures are properly propagated.
The success message using the `ok` function should only be printed when the
rustup toolchain install command actually succeeds, not unconditionally. Modify
the rustup toolchain install invocation to allow the script to detect and handle
actual installation failures appropriately.
Comment on lines
+126
to
+128
| RUSTUP_TOOLCHAIN="${RUST_TOOLCHAIN}" cargo install --locked "stellar-cli@${STELLAR_CLI_VERSION}" | ||
| ok "stellar-cli v${STELLAR_CLI_VERSION} installed" | ||
| } |
Contributor
There was a problem hiding this comment.
Source mode can fail verification even after a successful install.
Line 126 installs via Cargo, but verification at Line 142 depends on stellar being on PATH. In shells where Cargo bin isn’t pre-exported, this exits with failure despite install success.
Suggested fix
install_source() {
@@
info "Building stellar-cli v${STELLAR_CLI_VERSION} from source..."
RUSTUP_TOOLCHAIN="${RUST_TOOLCHAIN}" cargo install --locked "stellar-cli@${STELLAR_CLI_VERSION}"
+ export PATH="$(install_dir):${PATH}"
ok "stellar-cli v${STELLAR_CLI_VERSION} installed"
}Also applies to: 142-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/soroban/install-stellar-cli.sh` around lines 126 - 128, The issue is
that the stellar-cli installation via cargo install at line 126 doesn't ensure
the stellar binary is accessible in the PATH during verification at line 142. In
shells where the Cargo bin directory isn't automatically exported, the
verification fails despite successful installation. Fix this by explicitly
adding the Cargo bin directory to PATH after the cargo install command
completes, ensuring that the subsequent verification check (the stellar command)
can locate the installed binary and prevent false failures from occurring.
peer2f00l
reviewed
Jun 17, 2026
Comment on lines
+488
to
+501
| EVENT_NAME: ${{ github.event_name }} | ||
| RUN_FULL_TESTS: ${{ inputs.run_full_tests || 'false' }} | ||
| RUN_COVERAGE: ${{ inputs.run_coverage || 'false' }} | ||
| SKIP_COVERAGE: ${{ inputs.skip_coverage || 'false' }} | ||
| NEAR_CHANGED: ${{ needs.changes.outputs.near_integration }} | ||
| SOROBAN_CHANGED: ${{ needs.changes.outputs.soroban }} | ||
| FEATURE_MATRIX_CHANGED: ${{ needs.changes.outputs.feature_matrix }} | ||
| FORMAT_RESULT: ${{ needs['code-formatting'].result }} | ||
| LINTER_RESULT: ${{ needs['code-linter'].result }} | ||
| FAST_TEST_RESULT: ${{ needs['fast-tests'].result }} | ||
| NEAR_TEST_RESULT: ${{ needs['near-integration-tests'].result }} | ||
| SOROBAN_RESULT: ${{ needs.soroban.result }} | ||
| FEATURE_MATRIX_RESULT: ${{ needs['feature-matrix'].result }} | ||
| COVERAGE_RESULT: ${{ needs.coverage.result }} |
Collaborator
There was a problem hiding this comment.
this part seems especially brittle, but idk, it might be fine if we just "throw AI at it" when we want to update it...?
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/test.yml (2)
513-577:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUndefined variable
EVENT_NAMEin final echo statement.Line 577 references
${EVENT_NAME}but this variable is not defined in the step'senvblock (lines 514-526). The echo will output an incomplete message.🐛 Proposed fix
Add
EVENT_NAMEto the env block:- name: Evaluate required CI surface env: + EVENT_NAME: ${{ github.event_name }} NEAR_REQUIRED: ${{ needs.changes.outputs.run_near_integration }} SOROBAN_REQUIRED: ${{ needs.changes.outputs.run_soroban }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 513 - 577, The final echo statement in the "Evaluate required CI surface" step references the undefined variable EVENT_NAME, which will result in an incomplete output message. Add EVENT_NAME to the env block at the beginning of the step by defining it using the GitHub Actions context variable that provides the event name (typically github.event_name), so it can be properly referenced in the final echo statement that currently outputs "Full required gate passed for ${EVENT_NAME}".
489-497:⚠️ Potential issue | 🟡 MinorPin
codecov/codecov-actionto a commit hash for supply chain consistency.All other third-party actions in this workflow are pinned to commit hashes (e.g.,
actions/checkout@34e...,dorny/paths-filter@d1c...,taiki-e/install-action@cde...), butcodecov/[email protected]uses a version tag. For consistent supply chain security posture, pin to the commit hash:e53489f4d376d79066609109e7a95a29eb3740b1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 489 - 497, The codecov/codecov-action step uses a version tag reference (v7.0.0) instead of a commit hash for supply chain security consistency, while all other third-party actions in this workflow are pinned to commit hashes. Replace the version tag in the codecov/codecov-action uses statement with the commit hash e53489f4d376d79066609109e7a95a29eb3740b1 to match the security posture of other actions in the workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/test.yml:
- Around line 513-577: The final echo statement in the "Evaluate required CI
surface" step references the undefined variable EVENT_NAME, which will result in
an incomplete output message. Add EVENT_NAME to the env block at the beginning
of the step by defining it using the GitHub Actions context variable that
provides the event name (typically github.event_name), so it can be properly
referenced in the final echo statement that currently outputs "Full required
gate passed for ${EVENT_NAME}".
- Around line 489-497: The codecov/codecov-action step uses a version tag
reference (v7.0.0) instead of a commit hash for supply chain security
consistency, while all other third-party actions in this workflow are pinned to
commit hashes. Replace the version tag in the codecov/codecov-action uses
statement with the commit hash e53489f4d376d79066609109e7a95a29eb3740b1 to match
the security posture of other actions in the workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c3f443b-6908-48e7-a139-0db75720f717
📒 Files selected for processing (1)
.github/workflows/test.yml
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
66-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a global CI/toolchain surface to the path filters.
Right now, changes to
.github/workflows/test.yml,.github/actions/rust/**, rootCargo.toml/Cargo.lock,.cargo/**, or Rust toolchain files do not match any gated surface. On PRs, that lets NEAR/Soroban/feature-matrix jobs skip and the required gate pass even though the CI/toolchain contract changed.Suggested fix
- name: Detect changed surfaces uses: dorny/paths-filter@d1c1ffe0248fe513906c8e24db8ea791d46f8590 id: filter with: filters: | + global_ci: + - '.github/workflows/test.yml' + - '.github/actions/rust/**' + - '.cargo/**' + - 'Cargo.toml' + - 'Cargo.lock' + - 'rust-toolchain' + - 'rust-toolchain.toml' near_integration:Then expose
global_ciand force all gated surfaces on when it is true:outputs: + global_ci: ${{ steps.filter.outputs.global_ci }} near_integration: ${{ steps.filter.outputs.near_integration }}NEAR_CHANGED: ${{ steps.filter.outputs.near_integration }} SOROBAN_CHANGED: ${{ steps.filter.outputs.soroban }} FEATURE_MATRIX_CHANGED: ${{ steps.filter.outputs.feature_matrix }} + GLOBAL_CI_CHANGED: ${{ steps.filter.outputs.global_ci }}[ "$NEAR_CHANGED" = "true" ] && run_near_integration=true [ "$SOROBAN_CHANGED" = "true" ] && run_soroban=true [ "$FEATURE_MATRIX_CHANGED" = "true" ] && run_feature_matrix=true + if [ "$GLOBAL_CI_CHANGED" = "true" ]; then + run_near_integration=true + run_soroban=true + run_feature_matrix=true + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 66 - 105, Add a new filter entry called global_ci within the filters section that includes all CI and toolchain-related file paths such as .github/workflows/test.yml, .github/actions/rust/**, root Cargo.toml and Cargo.lock, .cargo/**, and Rust toolchain configuration files. Then modify the conditional logic for the near_integration, soroban, and feature_matrix jobs so that they run not only when their respective filter conditions are met, but also when the global_ci filter is triggered, ensuring that changes to CI/toolchain infrastructure force all gated test surfaces to execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/test.yml:
- Around line 66-105: Add a new filter entry called global_ci within the filters
section that includes all CI and toolchain-related file paths such as
.github/workflows/test.yml, .github/actions/rust/**, root Cargo.toml and
Cargo.lock, .cargo/**, and Rust toolchain configuration files. Then modify the
conditional logic for the near_integration, soroban, and feature_matrix jobs so
that they run not only when their respective filter conditions are met, but also
when the global_ci filter is triggered, ensuring that changes to CI/toolchain
infrastructure force all gated test surfaces to execute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 999be786-2fbf-4e2f-99c3-5fbdab897242
📒 Files selected for processing (1)
.github/workflows/test.yml
This was referenced Jul 22, 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
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.
Summary
Timing evidence
Baseline PR #472 Test workflow, run
27627445976:Tests: 37m21s, withRun testsat 35m35sCoverage: 20m56sSoroban: 16m33s, withInstall stellar CLIsource compile at about 12m39sThis PR, cold cache, run
27632640884before rerun:Fast Tests: 9m34sCode Linter: 6m08sSoroban: 4m06sCoverage,Feature Matrix, andNEAR / Workspace Integration Tests: skipped for this PR-shaped diffFull Required Gate: successSame commit rerun after caches were saved, run
27632640884rerun:Fast Tests: 3m53sCode Linter: 1m57sSoroban: 1m47sVerification
/data/tmp/actionlint-bin/actionlint -color=false .github/workflows/test.yml.github/workflows/test.ymland.github/actions/rust/action.ymlbash -n script/soroban/install-stellar-cli.sh script/test.sh script/prebuild-test-contracts.shgit diff --check128902 bytesThis change is