Open
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
Contributor
|
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:
📝 WalkthroughWalkthroughRemoved four GitHub Actions workflows (.github/workflows/ci-fmt.yml, ci-lint.yml, ci-test-integration.yml, ci-test-unit.yml) and added a consolidated CI workflow (.github/workflows/ci.yml) defining prepare-env, build, integration-tests, and ci-status jobs with concurrency control, self-hosted runners, per-branch target directories, and artifact caching/restoration. Made Chainlink lifecycle mode configurable from ValidatorParams. Marked a flaky ledger test with #[ignore]. Updated test-integration Makefile and test-runner to prefer prebuilt binaries via CARGO_TARGET_DIR/TEST_RUNNER_BIN, added offline and dynamic test-threads support, improved toml_to_args canonicalization diagnostics, and changed validator startup to prefer a prebuilt binary when present. Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
lucacillario
force-pushed
the
chore/improve-ci
branch
from
1f45eb3 to
24f36f5
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: 7
🤖 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 14-24: The prepare-env job currently creates per-branch
TARGET_DIRs under /var/lib/gh-runners/global-targets (calculated in the calc
step using SAFE_BRANCH) but never cleans them up; add a cleanup policy by either
(A) appending a post-step to the prepare-env job that removes target dirs older
than a threshold (e.g., find /var/lib/gh-runners/global-targets -type d -mtime
+7 -exec rm -rf {} \;) or (B) create a separate scheduled workflow that runs
daily/weekly to prune stale directories matching the repository pattern,
ensuring you target only /var/lib/gh-runners/global-targets/${{
github.event.repository.name }}-* to avoid collateral deletion and reference the
TARGET_DIR/SAFE_BRANCH naming scheme used in the calc step.
- Around line 49-50: The CI step named "Build binaries and tests" uses `cargo
build --bins --tests` which can mutate Cargo.lock; update that command to
include the `--locked` flag so it becomes `cargo build --bins --tests --locked`
to ensure the workflow fails if the lockfile is out of date; modify the run line
in the workflow step (the one running the cargo build command) to add
`--locked`.
- Line 9: The push trigger currently includes a feature branch name
("chore/improve-ci") in the branches list; remove that specific entry from the
branches array (leave only the intended persistent branches such as "master" and
"dev" or replace with a stable pattern) so the CI workflow does not run for
transient feature branches; update the branches declaration that contains
branches: [master, dev, chore/improve-ci] to exclude "chore/improve-ci".
- Around line 19-24: The run step for job id "calc" interpolates ${{
github.ref_name }} directly into the shell, which allows expression injection;
instead pass the ref into the job as an environment variable (e.g., REF_NAME set
to ${{ github.ref_name }}) and then reference "$REF_NAME" inside the run to
avoid GitHub Actions parsing into the shell; update the SAFE_BRANCH assignment
to operate on the quoted variable (SAFE_BRANCH=$(echo "$REF_NAME" | tr '/' '-'))
and ensure all variable expansions (TARGET_DIR, $GITHUB_OUTPUT) are quoted to
prevent word-splitting or globbing.
- Around line 140-156: Pass the matrix value through an environment variable and
enable -e: export BATCH="${{ matrix.batch }}" before invoking bash, change the
inner script to use set -euo pipefail (so failures like ip link set lo up
abort), iterate over "$BATCH" (quoted) instead of injecting ${{ matrix.batch }},
and keep the failure-aggregation logic inside the loop by using the existing if
! RUN_TESTS=$test make ci-test-integration ...; then FAILED=... pattern (no
change to RUN_TESTS or make ci-test-integration); ensure all variable expansions
(e.g., "$BATCH", "$test", "$FAILED") are quoted.
- Around line 103-104: The workflow step using the external tool "git
restore-mtime" will fail if the self-hosted runner doesn't have the package; add
an installation or verification step before the "Restore source file timestamps"
step: either install the tool (e.g., via apt-get install git-restore-mtime or
pip install git-restore-mtime) or run a command to detect availability and
skip/fail with a clear message; ensure the step names ("Restore source file
timestamps") and the command "git restore-mtime" are updated accordingly so the
workflow reliably installs or verifies the dependency before invoking it.
In `@test-integration/test-runner/bin/run_tests.rs`:
- Around line 786-787: The hard-coded .arg("--offline") on the cargo invocation
(the call chain that includes .arg("test").arg("--offline")) forces offline mode
for all runs; change it to only add "--offline" when the environment indicates
offline mode (e.g., check std::env::var("CARGO_NET_OFFLINE") or an equivalent
env flag) and conditionally push the .arg("--offline") into the Command only
when that env var is set/true so local dev runs without pre-fetched deps
continue to work.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 26-28: Add explicit timeouts to all GitHub Actions jobs to prevent
self-hosted runners from hanging indefinitely: update the job definitions such
as the build job (symbol "build" which has needs: prepare-env and runs-on:
self-hosted) and the other jobs referenced around the diff (jobs at the other
locations) to include a timeout-minutes entry (choose appropriate values for
each job) so every job block has a top-level timeout-minutes field to cap
execution time for self-hosted runners.
- Around line 124-126: The cp -r "$SBF_CACHE_DIR/test-integration"/*
"$LOCAL_DEPLOY_TEST"/ will fail when the source directory exists but is empty;
replace that globbed copy with a find-based copy like using find
"$SBF_CACHE_DIR/test-integration" -mindepth 1 -exec cp -r {}
"$LOCAL_DEPLOY_TEST"/ \; (or similar) so it only attempts to copy when entries
exist, then keep the subsequent find "$LOCAL_DEPLOY_TEST" -type f -exec touch {}
+ to update timestamps; reference the SBF_CACHE_DIR/test-integration and
LOCAL_DEPLOY_TEST variables and the cp -r .../* invocation when making the
change.
- Around line 69-73: The current cp invocation using the glob "cp -r
test-integration/target/deploy/* \"$SBF_CACHE_DIR/test-integration/\"" will fail
when the source directory exists but is empty; change the copy to a safe form
that handles empty dirs (for example use "cp -r test-integration/target/deploy/.
\"$SBF_CACHE_DIR/test-integration/\"" or use "rsync -a
test-integration/target/deploy/ \"$SBF_CACHE_DIR/test-integration/\"") and
ensure the destination directory is created first (mkdir -p
"$SBF_CACHE_DIR/test-integration/") so the step won't abort under set -e due to
an unmatched glob.
In `@magicblock-ledger/tests/test_ledger_truncator.rs`:
- Around line 80-82: Remove the #[ignore] suppression from
test_truncator_non_empty_ledger and replace the fixed sleep
(TEST_TRUNCATION_TIME_INTERVAL * 2) with a retry-based polling loop that
repeatedly checks the truncation invariant on LedgerTruncator until a deadline
(e.g., start + configurable timeout) and fails if the condition isn't met;
locate the wait and assertion logic in test_truncator_non_empty_ledger and poll
the same predicate used to verify that slots before the cleanup boundary are
deleted and later slots remain, using short sleeps between retries to avoid
busy-waiting. If you cannot make the test robust now, restore the #[ignore] but
add a tracking issue reference in the attribute/comment and link that issue ID
in the test to ensure it isn't forgotten.
In `@test-integration/Makefile`:
- Around line 6-11: The Makefile currently sets CARGO_TARGET_DIR with a
conditional assignment (CARGO_TARGET_DIR ?= ...) which isn't exported to
subprocesses; ensure the variable is exported or passed to child commands so
TEST_RUNNER_BIN and any invoked binaries see the same target dir. Update the
Makefile to either add an explicit export for CARGO_TARGET_DIR (so
TEST_RUNNER_BIN := $(CARGO_TARGET_DIR)/debug/run-tests and downstream
invocations inherit it) or modify the invocation that runs TEST_RUNNER_BIN to
pass CARGO_TARGET_DIR=<value> on the command line to make the wiring explicit.
In `@test-integration/test-tools/src/toml_to_args.rs`:
- Around line 89-96: The eprintln! message incorrectly labels the directory
variable as "Config File"; update the log to accurately reflect that
abs_config_dir (derived from config_dir via fs::canonicalize) is a directory by
changing the label to "Config Dir:" in the eprintln! invocation where
abs_config_dir is printed so the fields match their values (reference
abs_config_dir, config_dir, and the eprintln! call).
In `@test-integration/test-tools/src/validator.rs`:
- Around line 39-41: The fallback target dir uses
workspace_dir.join("../target") which leaves a literal .. segment and can make
validator_bin relative to the process CWD when CARGO_TARGET_DIR is a relative
path; update the logic around target_dir to resolve and canonicalize paths: when
CARGO_TARGET_DIR is present, convert to a PathBuf and if it's relative join it
to workspace_dir before calling std::fs::canonicalize (or at least
.canonicalize() on the final PathBuf), and for the fallback use
workspace_dir.join("target") (or canonicalize workspace_dir.join("../target"))
so the resulting target_dir and any downstream values like validator_bin are
absolute and canonicalized.
- Around line 53-57: The fallback invocation of cargo in validator.rs builds a
Command named cmd that runs "cargo run" without a package specifier, which fails
in a workspace; change the fallback args to include the package and binary like
"--package test-runner" and "--bin run-tests" before the "--" and config_path
(i.e., update
cmd.arg("run").arg("--package").arg("test-runner").arg("--bin").arg("run-tests").arg("--").arg(config_path))
so the correct package/bin are used when the prebuilt binary is missing.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 140-156: The workflow injects `${{ matrix.batch }}` directly into
the inline shell and omits `-e` from set, so change the step to pass the matrix
value through an environment variable (e.g., export BATCH or use env: BATCH: ${{
matrix.batch }}) and reference that variable inside the unshare -r -n bash -c
block (use a quoted "$BATCH" when iterating) to prevent expression injection,
and add `-e` to the shell flags (use set -euo pipefail) so failures like `ip
link set lo up` cause immediate exit; update references in this step (the inline
bash block invoked by unshare and the RUN_TESTS assignment) accordingly.
- Around line 49-50: The CI step "Build binaries and tests" runs `cargo build
--bins --tests` without `--locked`, which can allow Cargo.lock drift; update the
run command in that step to `cargo build --bins --tests --locked` so the build
uses the lockfile and produces reproducible builds.
- Around line 19-24: The workflow is interpolating github.ref_name and
github.event.repository.name directly into shell which risks expression
injection; instead define safe environment variables via the step's env: (e.g.,
SAFE_REF and REPO_NAME) and reference those env vars inside the run block when
constructing SAFE_BRANCH and TARGET_DIR, and ensure you sanitize/normalize the
values (replace '/' with '-' for SAFE_BRANCH) before using them; update uses of
SAFE_BRANCH, TARGET_DIR and the echo to GITHUB_OUTPUT to use the env vars rather
than direct github.* interpolation.
- Around line 103-104: The workflow uses the non-built-in command "git
restore-mtime" in the "Restore source file timestamps" step but doesn't ensure
the git-restore-mtime tool is installed; add a preceding job step to install the
git-restore-mtime package (e.g., install Python and run pip install
git-restore-mtime or use actions/setup-python then pip) or explicitly document
the runner prerequisite — create a step named like "Install git-restore-mtime"
that runs the installation before the existing restore-mtime step so the command
will exist on self-hosted runners.
- Around line 14-24: The prepare-env step (step id "calc") creates TARGET_DIR
under /var/lib/gh-runners/global-targets/${{ github.event.repository.name
}}-<branch> and never deletes it; add a cleanup to prevent unbounded growth by
either (A) adding a post-step to the prepare-env job that prunes stale
directories under /var/lib/gh-runners/global-targets (e.g., find directories
matching the repo prefix and remove those with mtime older than a retention
threshold like 30 days) or (B) create a separate scheduled workflow/job that
runs regularly to remove directories older than the retention window; implement
the cleanup logic referencing TARGET_DIR and the path prefix
/var/lib/gh-runners/global-targets so it targets only these global-targets
directories and avoid deleting current runs.
In `@test-integration/test-runner/bin/run_tests.rs`:
- Around line 786-787: The .arg("--offline") invocation is hard-coded and should
be conditional; modify the test command construction (the place that currently
chains .arg("test").arg("--offline")) to only append .arg("--offline") when an
environment flag is set (e.g. check std::env::var("TESTS_OFFLINE").is_ok() or,
as a sensible default, detect CI with std::env::var("CI").is_ok()), so replace
the unconditional .arg("--offline") with an if guard that calls
.arg("--offline") only when the env var indicates offline mode.
…e --no-run --locked
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 109-117: The run script directly interpolates
needs.prepare-env.outputs.target_dir into the shell which allows
expression/command injection; change the job to pass the output via an env var
(e.g. set environment GLOBAL_TARGET: ${{ needs.prepare-env.outputs.target_dir }}
under the step) and reference that env var inside the run block (use
"$GLOBAL_TARGET" everywhere), ensuring all uses (the cp -rl/cp -r lines and any
other references in the step and the similar block at lines 121-141) are
replaced with the safe env expansion instead of the ${ { ... } } substitution.
In `@test-integration/test-runner/bin/run_tests.rs`:
- Around line 796-799: The code reads the env var "CARGO_TEST_THREADS" into
test_threads and forwards it to the test runner; change that env var name to
"RUST_TEST_THREADS" wherever it is used (the std::env::var call that sets
test_threads and any references in the Makefile) so the standard test harness
variable is used, but keep the existing forwarding of test_threads into
cmd.arg(format!("--test-threads={}", test_threads)) and the "--nocapture" flag
unchanged.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 29-31: Add a timeout-minutes setting to the three GitHub Actions
jobs so self-hosted runners can't hang indefinitely: update the jobs named
build, integration-tests, and ci-status in .github/workflows/ci.yml to include
an appropriate timeout-minutes value (for example, timeout-minutes: 60 for
build, timeout-minutes: 180 for integration-tests, and timeout-minutes: 30 for
ci-status) placed at the job level alongside runs-on; choose values appropriate
for your workload and ensure all three jobs are updated consistently.
- Around line 106-107: The workflow step named "Restore source file timestamps"
uses the external command git restore-mtime which may not exist on the runner;
update the CI job to ensure availability by adding an explicit install or check
before that step (for example, a preceding step that checks for
git-restore-mtime and installs it via apt/pip if missing) and keep the existing
"git restore-mtime" invocation in the "Restore source file timestamps" step;
this ensures the command will run reliably on self-hosted or minimal runner
images.
- Line 9: The CI workflow still lists the literal branch name "chore/improve-ci"
in the branches array (branches: [master, dev, chore/improve-ci]), so remove
that entry from the branches list in the workflow configuration (ci.yml) so CI
won't trigger on that specific feature branch; if you need a persistent include,
replace the literal with the intended branch pattern (e.g., add/remove entries
from the branches array) and commit the change.
- Around line 14-27: The prepare-env job currently has no timeout and creates
per-branch TARGET_DIRs that never get reclaimed; add a job-level timeout-minutes
(e.g., 30) to prepare-env and ensure all other jobs also include a reasonable
timeout-minutes to avoid self-hosted stalls, and add a cleanup step in the
prepare-env flow (e.g., in the step with id "calc" or immediately after) that
prunes old directories under /var/lib/gh-runners/global-targets/ (use
REF_NAME/REPO_NAME to build TARGET_DIR as shown and run a safe find/delete for
directories older than a retention threshold like 30 days) so branch-specific
TARGET_DIRs are garbage-collected.
In `@test-integration/test-runner/bin/run_tests.rs`:
- Around line 787-789: The code should gate adding the "--offline" flag to the
test command on the CARGO_NET_OFFLINE environment variable; update or keep the
check using std::env::var("CARGO_NET_OFFLINE").as_deref() == Ok("true") and only
call cmd.arg("--offline") when that condition is true, ensuring the flag is not
hardcoded into the command invocation.
In `@test-integration/test-tools/src/validator.rs`:
- Around line 39-41: The default target_dir may contain a raw ".." from
workspace_dir.join("../target") which reintroduces a non-canonical path; change
the fallback branch used when CARGO_TARGET_DIR is missing to canonicalize the
joined path (call canonicalize on workspace_dir.join("../target")) and use that
canonical path (falling back to the joined path only if canonicalize fails) so
that subsequent checks (e.g. existence checks and resolution in
resolve_workspace_dir / validator_bin creation) operate on an absolute,
canonical target_dir.
9650677 to
c283b9f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 38-40: Replace both occurrences of the actions/checkout step that
currently reference "actions/checkout@v5" with "actions/checkout@v6" in the
workflow; locate the checkout steps (the uses: actions/checkout entries) around
the two blocks noted in the diff and update their version tag to "v6" (e.g.,
"actions/checkout@v6" or the specific "v6.0.2" if you prefer a pinned patch
version) so both checkout steps use the latest stable release.
In `@test-integration/test-tools/src/validator.rs`:
- Line 43: Replace the hardcoded "debug" when building the validator path:
instead of target_dir.join("debug/magicblock-validator"), read the intended
build profile from an environment variable (e.g. CARGO_PROFILE or another
test-specific var), default to "debug" if unset, and use
target_dir.join(format!("{}/magicblock-validator", profile)) so release builds
point to "release/magicblock-validator"; also align the Makefile's
TEST_RUNNER_BIN decision with the same env var if necessary.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 14-27: The prepare-env job currently creates per-branch dirs under
/var/lib/gh-runners/global-targets/${REPO_NAME}-${SAFE_BRANCH} (see job
prepare-env, step id calc using env vars REF_NAME and REPO_NAME and TARGET_DIR)
but never prunes them; add a cleanup action either as a post-step in the calc
step or as a scheduled workflow that removes directories not accessed for a
retention window (e.g., 7 days) using a safe find+rm invocation targeting
/var/lib/gh-runners/global-targets/${REPO_NAME}-* and ensure errors are
suppressed so the CI step never fails on cleanup.
- Around line 106-107: The workflow step calling the external command `git
restore-mtime` is invalid on runners without the `git-restore-mtime` tool; add a
preceding install step (e.g., `apt-get install git-restore-mtime` or `pip
install git-restore-mtime`) or conditionally skip/remove the `git restore-mtime`
step so the job won't fail — update the workflow to install the
`git-restore-mtime` package before the step that runs `git restore-mtime`.
- Around line 29-31: Add a top-level timeout-minutes entry to each self-hosted
job to prevent indefinite runs: update the build, integration-tests, and
ci-status job definitions to include e.g. "timeout-minutes: 30" (or your org
standard) as a sibling to runs-on and needs; apply the same change to the other
occurrences of those job blocks mentioned in the comment so every self-hosted
job has a timeout-minutes setting.
- Around line 109-117: Summary: avoid expression injection by not interpolating
needs.prepare-env.outputs.target_dir directly into run: script; instead pass it
via env and use safe quoting and -- to prevent shell/filename interpretation.
Fix: move the two direct interpolations into the job-level or step-level env
(e.g. set env: GLOBAL_TARGET: ${{ needs.prepare-env.outputs.target_dir }} and
env: SBF_CACHE_DIR: ${{ needs.prepare-env.outputs.target_dir }}/sbf-artifacts
}), then in the step use the env vars (GLOBAL_TARGET and SBF_CACHE_DIR) and call
commands with robust quoting and protections like cp -rl --
"$GLOBAL_TARGET/debug" target/ and fallback cp -r -- "$GLOBAL_TARGET/debug"
target/ so special chars (`, $, \, etc.) are not interpreted by the shell and
filenames beginning with - are handled safely.
In `@magicblock-ledger/tests/test_ledger_truncator.rs`:
- Around line 80-82: The #[ignore = "Flaky test"] on the
test_truncator_non_empty_ledger function is missing a tracking issue and the
previous request to either make the test robust or link an issue wasn’t applied;
either implement a deterministic retry/polling-based wait inside
test_truncator_non_empty_ledger to remove flakiness (e.g., poll the condition
under test with a timeout) or replace the attribute with #[ignore = "flaky -
tracking issue #<ISSUE_NUMBER>"] (substitute a real issue number) so future
readers can find context; update the test annotation or the test body
accordingly and ensure the ignored message includes the issue id and short
reason.
In `@test-integration/Makefile`:
- Around line 6-11: The Makefile sets CARGO_TARGET_DIR with a make-only
assignment `CARGO_TARGET_DIR ?= $(DIR)../target` but does not export it, so
child processes (including the prebuilt test-runner referenced by
TEST_RUNNER_BIN) won't receive it; fix by exporting CARGO_TARGET_DIR after its
assignment (ensure `CARGO_TARGET_DIR` is exported so the environment variable is
available to subprocesses and the prebuilt binary resolution uses the intended
path).
In `@test-integration/test-runner/bin/run_tests.rs`:
- Around line 796-799: Replace the reserved CARGO_ env var usage with the
conventional RUST_TEST_THREADS: update the env lookup in run_tests.rs (the let
test_threads = std::env::var("CARGO_TEST_THREADS")... and subsequent
format!("--test-threads={}", test_threads)) to read "RUST_TEST_THREADS" and keep
the same default ("1") and argument formatting; also update the matching
variable names in the Makefile where CARGO_TEST_THREADS is referenced so they
use RUST_TEST_THREADS (the two occurrences noted in the review) to keep behavior
consistent.
In `@test-integration/test-tools/src/validator.rs`:
- Around line 39-41: The fallback path uses workspace_dir.join("../target")
which leaves a literal ".." and can make validator_bin relative to CWD; instead
compute the parent directory explicitly: replace the fallback expression with
logic that takes workspace_dir.parent().map(|p|
p.join("target")).unwrap_or_else(|| workspace_dir.join("target")) so target_dir
is anchored to the workspace root's parent without any ".." segments (update the
variable referenced as target_dir and keep workspace_dir usage intact).
Contributor
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 29-36: The three GitHub Actions jobs ("build",
"integration-tests", "ci-status") lack a timeout setting and on self-hosted
runners can hang indefinitely; add a timeout-minutes property to each job
definition (e.g., timeout-minutes: 60 or project-appropriate value) so the
workflow will automatically cancel long-running/hung jobs—update the job blocks
for build, integration-tests, and ci-status to include this timeout-minutes key.
- Around line 38-40: The workflow is using actions/checkout@v5 in both checkout
steps; update every occurrence of the action reference (the lines where "uses:
actions/checkout@v5" appears) to the latest stable release "actions/checkout@v6"
so both checkout steps use v6 consistently; verify both instances (the initial
checkout and the later checkout step) are changed and run a quick lint/CI to
confirm no syntax changes are needed.
- Around line 106-107: The CI step labeled "Restore source file timestamps" uses
the non-built-in command git restore-mtime which may not exist on self-hosted
runners; update that step to first check for the command and install the
git-restore-mtime package if missing (for example run a shell check like `if !
command -v git-restore-mtime; then pip install --user git-restore-mtime || pipx
install git-restore-mtime; fi`), or alternatively replace the step with an
explicit installation line (`pip install git-restore-mtime`) before invoking
`git restore-mtime`, so the workflow will not fail silently when the tool is
absent.
- Around line 109-123: The workflow in the "Link pre-built artifacts" and
"Restore SBF artifacts" steps directly inlines the untrusted expression `${{
needs.prepare-env.outputs.target_dir }}` into `run:` scripts (used as
`GLOBAL_TARGET="...` and `SBF_CACHE_DIR="...`), which risks shell injection;
instead expose that output via the step-level `env:` and reference the
environment variable inside `run:` (e.g., add env: GLOBAL_TARGET: ${{
needs.prepare-env.outputs.target_dir }} for the "Link pre-built artifacts" step
and env: SBF_CACHE_DIR: ${{ needs.prepare-env.outputs.target_dir
}}/sbf-artifacts for the "Restore SBF artifacts" step, then use $GLOBAL_TARGET
and $SBF_CACHE_DIR inside the respective `run:` bodies), so the value is passed
safely without being interpreted by the shell during workflow parsing.
- Around line 14-27: The workflow currently creates per-branch TARGET_DIRs in
the prepare-env job (step id calc) under
/var/lib/gh-runners/global-targets/${REPO_NAME}-${SAFE_BRANCH} but never prunes
stale directories; update the calc step to run a cleanup before mkdir -p by
discovering directories matching
/var/lib/gh-runners/global-targets/${REPO_NAME}-* and removing either files
older than a retention threshold (e.g., 30 days) or all but the N most-recent
entries for that REPO_NAME; implement this retention logic in the same calc run
block (using REF_NAME/REPO_NAME/SAFE_BRANCH to preserve the current TARGET_DIR)
so stale per-branch directories are removed automatically.
| } | ||
|
|
||
| // Tests that ledger got truncated but not after finality slot | ||
| #[ignore = "Flaky test"] |
Collaborator
There was a problem hiding this comment.
This should now be fixed
GabrielePicco
approved these changes
Feb 19, 2026
Collaborator
GabrielePicco
left a comment
GabrielePicco
left a comment
There was a problem hiding this comment.
LGTM!
We can reenable the ledger integration test, it shouldn't be flaky anymore
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
Compatibility
Testing
Checklist
Summary by CodeRabbit
Chores
Features
Bug Fixes