test: added scripts for testing examples #47
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughDependency bumps to ephemeral-rollups-sdk/kit (0.6.5) across many packages; added Cargo features (custom-heap, custom-panic, anchor-debug) to multiple crates; anchor-counter program ID, keypair and IDL regenerated; tests made environment-driven and many RPC/transaction calls now skip preflight; added repo scripts and tightened test timeouts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (20)
anchor-counter/Cargo.lockis excluded by!**/*.lockanchor-counter/app/app/yarn.lockis excluded by!**/yarn.lock,!**/*.lockanchor-counter/yarn.lockis excluded by!**/yarn.lock,!**/*.lockanchor-minter/Cargo.lockis excluded by!**/*.lockanchor-minter/target/deploy/token_minter.sois excluded by!**/*.soanchor-minter/yarn.lockis excluded by!**/yarn.lock,!**/*.lockanchor-rock-paper-scissor/Cargo.lockis excluded by!**/*.lockanchor-rock-paper-scissor/yarn.lockis excluded by!**/yarn.lock,!**/*.lockbolt-counter/Cargo.lockis excluded by!**/*.lockdummy-token-transfer/Cargo.lockis excluded by!**/*.lockdummy-token-transfer/yarn.lockis excluded by!**/yarn.lock,!**/*.lockephemeral-validator.logis excluded by!**/*.logmagic-actions/Cargo.lockis excluded by!**/*.lockmagic-actions/yarn.lockis excluded by!**/yarn.lock,!**/*.lockoncurve-delegation/yarn.lockis excluded by!**/yarn.lock,!**/*.lockroll-dice/Cargo.lockis excluded by!**/*.lockrust-counter/Cargo.lockis excluded by!**/*.lockrust-counter/yarn.lockis excluded by!**/yarn.lock,!**/*.locksession-keys/Cargo.lockis excluded by!**/*.locksession-keys/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
anchor-counter/app/app/package.json(1 hunks)anchor-counter/package.json(1 hunks)anchor-counter/programs/anchor-counter/Cargo.toml(1 hunks)anchor-counter/programs/anchor-counter/src/lib.rs(1 hunks)anchor-counter/target/deploy/anchor_counter-keypair.json(1 hunks)anchor-counter/target/idl/anchor_counter.json(3 hunks)anchor-counter/tests/advanced-magic.ts(1 hunks)anchor-minter/package.json(1 hunks)anchor-minter/programs/token-minter/Cargo.toml(1 hunks)anchor-rock-paper-scissor/package.json(1 hunks)anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/Cargo.toml(1 hunks)bolt-counter/Anchor.toml(0 hunks)dummy-token-transfer/package.json(1 hunks)dummy-token-transfer/programs/dummy-transfer/Cargo.toml(1 hunks)dummy-token-transfer/tests/dummy-transfer.ts(11 hunks)magic-actions/package.json(1 hunks)magic-actions/programs/magic-actions/Cargo.toml(1 hunks)oncurve-delegation/package.json(1 hunks)roll-dice/programs/roll-dice-delegated/Cargo.toml(1 hunks)roll-dice/programs/roll-dice-delegated/Xargo.toml(0 hunks)roll-dice/programs/roll-dice/Cargo.toml(1 hunks)roll-dice/programs/roll-dice/Xargo.toml(0 hunks)rust-counter/Cargo.toml(1 hunks)rust-counter/package.json(1 hunks)rust-counter/tests/kit/advanced-magic.test.ts(1 hunks)rust-counter/tests/web3js/advanced-magic.test.ts(1 hunks)session-keys/package.json(1 hunks)session-keys/programs/anchor-counter-session/Cargo.toml(1 hunks)session-keys/tests/advanced-magic.ts(1 hunks)test-locally.sh(1 hunks)update-sdk-versions.sh(1 hunks)
💤 Files with no reviewable changes (3)
- roll-dice/programs/roll-dice/Xargo.toml
- bolt-counter/Anchor.toml
- roll-dice/programs/roll-dice-delegated/Xargo.toml
🧰 Additional context used
🪛 Shellcheck (0.11.0)
test-locally.sh
[warning] 19-19: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 19-19: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 108-108: i appears unused. Verify use (or export if used externally).
(SC2034)
update-sdk-versions.sh
[warning] 8-8: FAILED_UPDATES appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 9-9: WARNINGS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 15-15: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[error] 21-21: Argument mixes string and array. Use * or separate argument.
(SC2145)
[info] 30-30: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Modification of UPDATED_FILES is local (to subshell caused by pipeline).
(SC2030)
[info] 47-47: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 53-53: Modification of UPDATED_FILES is local (to subshell caused by pipeline).
(SC2030)
[info] 53-53: UPDATED_FILES was modified in a subshell. That change might be lost.
(SC2031)
[info] 67-67: Modification of YARN_ERRORS is local (to subshell caused by pipeline).
(SC2030)
[info] 81-81: Modification of CARGO_ERRORS is local (to subshell caused by pipeline).
(SC2030)
[info] 93-93: UPDATED_FILES was modified in a subshell. That change might be lost.
(SC2031)
[info] 94-94: UPDATED_FILES was modified in a subshell. That change might be lost.
(SC2031)
[info] 95-95: UPDATED_FILES was modified in a subshell. That change might be lost.
(SC2031)
[info] 101-101: YARN_ERRORS was modified in a subshell. That change might be lost.
(SC2031)
[info] 102-102: YARN_ERRORS was modified in a subshell. That change might be lost.
(SC2031)
[info] 103-103: YARN_ERRORS was modified in a subshell. That change might be lost.
(SC2031)
[info] 111-111: CARGO_ERRORS was modified in a subshell. That change might be lost.
(SC2031)
[info] 112-112: CARGO_ERRORS was modified in a subshell. That change might be lost.
(SC2031)
[info] 113-113: CARGO_ERRORS was modified in a subshell. That change might be lost.
(SC2031)
[info] 121-121: YARN_ERRORS was modified in a subshell. That change might be lost.
(SC2031)
[info] 121-121: CARGO_ERRORS was modified in a subshell. That change might be lost.
(SC2031)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-examples
🔇 Additional comments (31)
session-keys/programs/anchor-counter-session/Cargo.toml (2)
18-20: Remove unused feature declarations from Cargo.toml.The three features
custom-heap,custom-panic, andanchor-debug(lines 18-20) are declared but never referenced anywhere in the codebase. They are not used in any conditional compilation or conditional dependencies, making them dead code. Either remove them or add conditional compilation logic that actually uses them.
24-24: ephemeral-rollups-sdk 0.6.5 is compatible and properly configured.Version 0.6.5 exists on crates.io and contains only a bug fix ("remove warnings of unexpected cfg values") with no breaking changes from 0.6.4. The dependency is widely used across the codebase with consistent checksums, and the feature flags ("anchor", "disable-realloc") are appropriate for this Anchor program context.
anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/Cargo.toml (2)
1-24: Scope mismatch: Review the complete PR context.The PR title indicates "added scripts for testing examples," but the provided file only shows Cargo.toml dependency and feature changes. The AI summary also mentions "two new shell automation scripts added for testing and SDK version management."
Please provide visibility into:
- The actual test/automation scripts added in this PR.
- How these Cargo.toml changes relate to the testing infrastructure mentioned in the PR title.
- Whether other files in this PR contain the script additions and related changes.
This will allow for a comprehensive review of how the dependency bumps and feature additions integrate with the testing scripts.
24-24: No action needed—ephemeral-rollups-sdk 0.6.5 is a safe, non-breaking patch update.Version 0.6.5 (released Nov 30, 2025) contains only a small fix to remove warnings about unexpected cfg values with no breaking changes. The preserved features (
["anchor", "access-control"]) remain valid in this version.anchor-counter/tests/advanced-magic.ts (1)
15-20: Env-based router endpoints with sensible defaultsUsing
process.env.ROUTER_ENDPOINT/WS_ROUTER_ENDPOINTwith fallbacks preserves existing behavior while enabling configuration per environment. No issues from the change itself.rust-counter/tests/web3js/advanced-magic.test.ts (1)
24-29: Configurable HTTP/WS router endpoints in web3js testReading
ROUTER_ENDPOINT/WS_ROUTER_ENDPOINT(with the previous devnet defaults as fallbacks) plusdotenv.config()makes this test easily target different clusters without breaking existing usage.rust-counter/tests/kit/advanced-magic.test.ts (1)
55-58: Kit connection endpoints now environment-drivenBoth
connectionandephemeralConnectionuseROUTER_ENDPOINT/WS_ROUTER_ENDPOINTwith devnet fallbacks, which keeps behavior consistent and makes the tests configurable across environments.session-keys/tests/advanced-magic.ts (1)
17-21: Session-keys tests now use configurable router endpointsSwitching to
process.env.ROUTER_ENDPOINT/WS_ROUTER_ENDPOINTwith the original devnet URLs as fallbacks keeps current behavior while allowing per-environment configuration.roll-dice/programs/roll-dice-delegated/Cargo.toml (1)
25-25: Verify ephemeral-rollups-sdk 0.6.5 compatibility and breaking changes.The version bump from 0.2.4 to 0.6.5 is substantial. Ensure that this crate's code (particularly in delegated rollups logic) is compatible with the new SDK version and that there are no breaking API changes requiring code modifications.
[scratchpad_end] -->
session-keys/package.json (1)
9-9: Verify SDK compatibility with session-keys implementation.The dependency bump to 0.6.5 is a minor version change. Ensure session management logic remains compatible with any SDK changes, particularly around ephemeral session handling or signature validation if used.
anchor-counter/package.json (1)
9-9: Verify version pinning is intentional for reproducibility.The dependency is now pinned to exactly 0.6.5 (no caret). Confirm this stricter constraint is intentional for reproducible builds. Be aware that this locks out future patch/minor updates, including potential security fixes released in 0.6.6+. Consider whether this aligns with your dependency management strategy.
anchor-rock-paper-scissor/package.json (1)
8-8: Note version pinning change from flexible to strict.The caret (^) has been removed, changing the constraint from
^0.6.4(allowing 0.6.x updates) to0.6.5(exact pinning). This pattern appears intentional across the PR. Ensure this aligns with your project's dependency pinning strategy and security patch policy.dummy-token-transfer/package.json (1)
8-8: Verify SDK 0.6.5 compatibility with token transfer logic.The patch-level bump from 0.6.4 to 0.6.5 is low-risk. Confirm that any token initialization, transfer, or routing logic remains compatible with the updated SDK, particularly if you rely on any SDK internals for token handling.
rust-counter/Cargo.toml (1)
11-11: Verify ephemeral-rollups-sdk feature flags consistency.Unlike other Cargo.toml files in this PR (magic-actions, dummy-transfer, roll-dice-delegated), rust-counter omits the
["anchor"]features for ephemeral-rollups-sdk. Confirm this is intentional—whether this crate is a pure Rust counter without Anchor dependencies, or if the feature flag was accidentally omitted during the coordinated SDK version bump.anchor-counter/app/app/package.json (1)
9-9: LGTM! Major version jump from 0.0.4 to 0.6.5.This is a significant version upgrade with the caret removed for exact version locking. Ensure the application has been tested with this new version, as breaking changes are likely between 0.0.x and 0.6.x.
Please verify that:
- The application works correctly with SDK version 0.6.5
- All breaking changes have been addressed
- The version exists in the npm registry (same verification script as for magic-actions/package.json)
anchor-minter/package.json (1)
8-8: LGTM! Version locked to 0.6.5.The caret has been removed to lock to exact version 0.6.5, ensuring deterministic builds across the repository.
anchor-counter/programs/anchor-counter/Cargo.toml (2)
18-20: LGTM! Added optional features for Solana program compilation.The added features (custom-heap, custom-panic, anchor-debug) are standard feature flags for Solana programs, allowing conditional compilation for different deployment scenarios.
24-24: LGTM! Version bumped to 0.6.5 with features preserved.The ephemeral-rollups-sdk dependency has been updated from 0.6.4 to 0.6.5 while maintaining the existing features (anchor, disable-realloc).
rust-counter/package.json (1)
7-8: LGTM! Both ephemeral-rollups packages locked to 0.6.5.The version bump from 0.6.4 to 0.6.5 has been applied to both ephemeral-rollups-kit and ephemeral-rollups-sdk with exact version locking for deterministic builds.
oncurve-delegation/package.json (1)
7-8: LGTM! Both ephemeral-rollups packages locked to 0.6.5.Consistent with other packages in the repository, both dependencies are updated to exact version 0.6.5.
dummy-token-transfer/tests/dummy-transfer.ts (4)
5-7: LGTM! Updated imports for new SDK API.The imports have been correctly updated to use ConnectionMagicRouter from the ephemeral-rollups-sdk and to add necessary types (PublicKey, sendAndConfirmTransaction, Transaction) from @solana/web3.js.
40-45: LGTM! Replaced Connection with ConnectionMagicRouter.The router connection is now properly instantiated using ConnectionMagicRouter with environment-driven endpoint configuration, which improves flexibility for testing across different environments.
99-103: LGTM! Replaced sendMagicTransaction with sendAndConfirmTransaction.All transaction submissions have been correctly migrated from the legacy sendMagicTransaction API to the standard Solana sendAndConfirmTransaction pattern, using the routerConnection and appropriate signers.
Also applies to: 127-131, 159-163, 203-207, 235-239, 251-255, 278-282, 291-295
181-181: LGTM! Updated getClosestValidator usage.The getClosestValidator function has been correctly migrated to a method call on routerConnection, with the result properly wrapped in a PublicKey constructor.
roll-dice/programs/roll-dice/Cargo.toml (1)
24-24: LGTM! Ensure the program has been built and tested with ephemeral-rollups-sdk 0.6.5.Verify the program compiles with the updated SDK version and that the "anchor" feature integration works as expected.
magic-actions/package.json (1)
9-9: LGTM! Version 0.6.5 exists and has no known security advisories.The version exists in the npm registry and no public security vulnerabilities are recorded for this package version. The patch version bump from 0.6.4 to 0.6.5 is consistent with other packages in this PR.
test-locally.sh (1)
77-84: LGTM: Validator setup looks correct.The script properly initializes Solana configuration and creates a keypair only if it doesn't exist, avoiding unnecessary overwrites.
anchor-counter/target/idl/anchor_counter.json (2)
108-139: Buffer PDA seeds updated correctly.The buffer PDA program seed has been updated to reflect the new program ID bytes. This is automatically generated by Anchor and maintains internal consistency.
2-2: Generated IDL file is consistent with program ID change.The IDL correctly reflects the new program ID
9RPwaXayVZHna1BYuRS4cLPJZuNGU1uS5V3heXB7v6Qiin both the top-level address and theowner_programfield. This file is intentionally tracked in the repository per the.gitignoreexceptions (lines 16-18).anchor-counter/target/deploy/anchor_counter-keypair.json (1)
1-1: This is an intentional design choice for the examples repository.The keypair files are explicitly whitelisted in
.gitignoreusing negation patterns (!/anchor-counter/target/deploy/anchor_counter-keypair.json) alongside the blanket**/target/*exclusion. Multiple example projects follow this same pattern. For demo/example repositories, committing known keypairs is standard practice to enable reproducible local deployments and consistent testing without regeneration.anchor-counter/programs/anchor-counter/src/lib.rs (1)
6-6: Program ID change is properly implemented with no stale references.The old program ID has been completely removed from the codebase, and the new ID is set in the correct location. No remaining references to the old ID exist, and the migration appears intentional and complete.
| custom-heap = [] | ||
| custom-panic = [] | ||
| anchor-debug = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of the newly added features
echo "=== Checking for feature gates in source code ==="
rg -n 'cfg\(feature\s*=\s*"(custom-heap|custom-panic|anchor-debug)"' --type rust
echo ""
echo "=== Checking for features array in Cargo.toml files ==="
rg -n 'features\s*=\s*\[' --type toml -A 5 | grep -E '(custom-heap|custom-panic|anchor-debug|features)'
echo ""
echo "=== Checking for conditional compilation with these features ==="
rg -n '#\[cfg.*feature' --type rust | grep -E '(custom-heap|custom-panic|anchor-debug)'
echo ""
echo "=== Searching for any direct references to these feature names ==="
rg -n '(custom-heap|custom-panic|anchor-debug)' --type rustRepository: magicblock-labs/magicblock-engine-examples
Length of output: 2957
🏁 Script executed:
#!/bin/bash
# Examine the Cargo.toml file where features are defined
echo "=== Full Cargo.toml content ==="
cat -n anchor-minter/programs/token-minter/Cargo.toml
echo ""
echo "=== Check if these features are used in dev dependencies or elsewhere ==="
rg -n 'custom-heap|custom-panic|anchor-debug' anchor-minter/programs/token-minter/
echo ""
echo "=== Check for similar empty feature patterns in other programs ==="
find . -name "Cargo.toml" -type f | head -20 | while read f; do
echo "--- $f ---"
rg -c 'features\s*=' "$f" 2>/dev/null && cat -n "$f" | grep -A 5 'features\s*='
doneRepository: magicblock-labs/magicblock-engine-examples
Length of output: 3815
Remove unused features or document their purpose.
The three empty features (custom-heap, custom-panic, anchor-debug) defined on lines 18–20 are not referenced anywhere in the codebase—no conditional compilation gates, no feature dependencies, and no source code conditionals use them. If these are placeholders for future use, add a comment documenting their intent. Otherwise, remove them.
🤖 Prompt for AI Agents
In anchor-minter/programs/token-minter/Cargo.toml around lines 18 to 20, the
three empty features (custom-heap, custom-panic, anchor-debug) are not used
anywhere; either delete these unused feature entries from the Cargo.toml or, if
they are intended as placeholders, add a short inline comment next to each
feature explaining the future intent and where they will be referenced, and
ensure any placeholder features are commented with expected gating/usage to
avoid confusion.
| custom-heap = [] | ||
| custom-panic = [] | ||
| anchor-debug = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of the new feature flags in the codebase
echo "=== Searching for custom-heap usage ==="
rg -n 'cfg.*custom-heap|custom-heap' anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/src/ 2>/dev/null || echo "No matches found for custom-heap"
echo -e "\n=== Searching for custom-panic usage ==="
rg -n 'cfg.*custom-panic|custom-panic' anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/src/ 2>/dev/null || echo "No matches found for custom-panic"
echo -e "\n=== Searching for anchor-debug usage ==="
rg -n 'cfg.*anchor-debug|anchor-debug' anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/src/ 2>/dev/null || echo "No matches found for anchor-debug"
echo -e "\n=== Checking Cargo.toml for feature definitions and dependency info ==="
cat -n anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/Cargo.toml | head -30Repository: magicblock-labs/magicblock-engine-examples
Length of output: 1102
🏁 Script executed:
# Check if other Anchor projects use this pattern - search for similar unused feature patterns
rg -n 'anchor-debug|custom-heap|custom-panic' anchor-rock-paper-scissor/programs/ 2>/dev/null | head -20Repository: magicblock-labs/magicblock-engine-examples
Length of output: 363
Remove unused feature flags or implement their intended functionality.
The three new feature flags (custom-heap, custom-panic, anchor-debug) are defined in Cargo.toml but are not used anywhere in the codebase—no #[cfg(feature = "...")] guards reference them. Either implement these features or remove the empty definitions to avoid cluttering the manifest.
🤖 Prompt for AI Agents
In anchor-rock-paper-scissor/programs/anchor-rock-paper-scissor/Cargo.toml
around lines 18 to 20, the feature flags `custom-heap`, `custom-panic`, and
`anchor-debug` are declared but not referenced anywhere; remove these unused
empty feature entries from Cargo.toml or, if they are intended, add
corresponding #[cfg(feature = "…")] guards and implementation code (e.g.,
conditional modules or feature-gated code paths) in the crate so the features
are actually exercised; update Cargo.toml to either delete the three lines or
provide non-empty feature definitions that enable the implemented code paths,
and run cargo check to verify no unused-feature warnings remain.
| custom-heap = [] | ||
| custom-panic = [] | ||
| anchor-debug = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'cfg.*custom-heap|cfg.*custom-panic|cfg.*anchor-debug|custom_heap|custom_panic' dummy-token-transfer/programs/dummy-transfer/src/ -iRepository: magicblock-labs/magicblock-engine-examples
Length of output: 68
🏁 Script executed:
# Also check for any feature-gating in general
rg -n '#\[cfg\(feature' dummy-token-transfer/programs/dummy-transfer/src/Repository: magicblock-labs/magicblock-engine-examples
Length of output: 68
🏁 Script executed:
# Let's verify the src directory exists and check its contents
find dummy-token-transfer/programs/dummy-transfer/src -type f -name "*.rs" | head -20Repository: magicblock-labs/magicblock-engine-examples
Length of output: 141
🏁 Script executed:
# Check the Cargo.toml to confirm the features are actually declared
cat dummy-token-transfer/programs/dummy-transfer/Cargo.toml | grep -A 5 '\[features\]'Repository: magicblock-labs/magicblock-engine-examples
Length of output: 184
🏁 Script executed:
# Get the full Cargo.toml with line numbers to see what's on lines 18-20
cat -n dummy-token-transfer/programs/dummy-transfer/Cargo.tomlRepository: magicblock-labs/magicblock-engine-examples
Length of output: 764
🏁 Script executed:
# Check the lib.rs content to confirm there are no feature implementations
cat dummy-token-transfer/programs/dummy-transfer/src/lib.rsRepository: magicblock-labs/magicblock-engine-examples
Length of output: 3444
Remove unused feature flags or implement their corresponding functionality.
The three features declared on lines 18-20 (custom-heap, custom-panic, anchor-debug) have no corresponding implementations in the source code. Either remove these unused feature declarations from Cargo.toml, or add the conditional code guarded by #[cfg(feature = "...")] directives in the source files.
🤖 Prompt for AI Agents
In dummy-token-transfer/programs/dummy-transfer/Cargo.toml around lines 18-20,
the feature flags custom-heap, custom-panic, and anchor-debug are declared but
not implemented; either remove these unused entries from the Cargo.toml features
list or implement the corresponding conditional code paths. If removing: delete
the three feature names from the [features] section and run cargo build to
ensure nothing depends on them. If implementing: add source code guarded by
#[cfg(feature = "custom-heap")], #[cfg(feature = "custom-panic")] and
#[cfg(feature = "anchor-debug")] in the appropriate Rust modules (with real
implementations or stubs), update documentation/tests as needed, and run cargo
build/test to verify correctness.
| anchor-debug = [] | ||
| custom-heap = [] | ||
| custom-panic = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'cfg.*custom-heap|cfg.*custom-panic|custom_heap|custom_panic' magic-actions/programs/magic-actions/src/Repository: magicblock-labs/magicblock-engine-examples
Length of output: 68
🏁 Script executed:
find magic-actions/programs/magic-actions -name "*.rs" -type f | head -20Repository: magicblock-labs/magicblock-engine-examples
Length of output: 133
🏁 Script executed:
cat -n magic-actions/programs/magic-actions/src/lib.rsRepository: magicblock-labs/magicblock-engine-examples
Length of output: 7219
Remove unused feature flags or implement their corresponding functionality.
The custom-heap and custom-panic features declared in Cargo.toml are not utilized anywhere in the crate—no #[cfg(feature = "...")] guards or implementations exist. Either remove these features from the manifest or implement the corresponding custom allocator and panic handler code with proper feature gates.
test-locally.sh
Outdated
| echo "Testing $test_name..." | ||
|
|
||
| # Run test with color forced for various tools | ||
| FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR= eval "$test_command" 2>&1 > >(tee "$test_log") |
There was a problem hiding this comment.
Fix incorrect redirection order.
The redirection 2>&1 > >(tee "$test_log") is incorrect. It redirects stderr to the current stdout first, then redirects stdout to tee. This means stderr will go to the terminal instead of the log file.
Apply this diff:
- FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR= eval "$test_command" 2>&1 > >(tee "$test_log")
+ FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR= eval "$test_command" > >(tee "$test_log") 2>&1Also, for the empty NO_COLOR variable, use a clearer syntax:
- FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR= eval "$test_command" > >(tee "$test_log") 2>&1
+ FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR='' eval "$test_command" > >(tee "$test_log") 2>&1📝 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.
| FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR= eval "$test_command" 2>&1 > >(tee "$test_log") | |
| FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR='' eval "$test_command" > >(tee "$test_log") 2>&1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 19-19: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 19-19: To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).
(SC2069)
🤖 Prompt for AI Agents
In test-locally.sh around line 19, the stdout/stderr redirection order is wrong
and the NO_COLOR assignment is ambiguous; change the command to first pipe
stdout through tee and then redirect stderr to that stream by using > >(tee
"$test_log") 2>&1 instead of 2>&1 > >(tee "$test_log"), and make the NO_COLOR
intent explicit (e.g., NO_COLOR=1) so the environment variable clearly disables
color.
| echo "SDK Version Update Script" | ||
| echo "========================================" | ||
| echo "Target Version: $VERSION" | ||
| echo "Target Packages: ${PACKAGES[@]}" |
There was a problem hiding this comment.
Fix array expansion in echo.
The array expansion syntax is incorrect and will produce unexpected output.
Apply this diff:
-echo "Target Packages: ${PACKAGES[@]}"
+echo "Target Packages: ${PACKAGES[*]}"📝 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.
| echo "Target Packages: ${PACKAGES[@]}" | |
| echo "Target Packages: ${PACKAGES[*]}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 21-21: Argument mixes string and array. Use * or separate argument.
(SC2145)
🤖 Prompt for AI Agents
In update-sdk-versions.sh around line 21, the echo uses incorrect array
expansion which can yield unexpected output; replace the current expansion with
a safe one such as using ${PACKAGES[*]} inside the quoted string to produce a
single space-separated list (e.g. echo "Target Packages: ${PACKAGES[*]}"), or
alternatively use printf to join elements reliably (e.g. printf 'Target
Packages: %s\n' "${PACKAGES[@]}").
| find . -name "package.json" -type f -not -path "*/node_modules/*" -not -path "*/private-payments/*" | while read -r file; do | ||
| if grep -q "$package" "$file"; then | ||
| # Create backup for diff | ||
| cp "$file" "$TEMP_DIR/$(basename $file).bak" | ||
|
|
||
| sed -i '' "s/\"@magicblock-labs\/$package\": \"[^\"]*\"/\"@magicblock-labs\/$package\": \"$VERSION\"/g" "$file" | ||
| sed -i '' "s/\"$package\": \"[^\"]*\"/\"$package\": \"$VERSION\"/g" "$file" | ||
|
|
||
| echo " ✓ $file" | ||
| UPDATED_FILES+=("$file") | ||
| fi | ||
| done | ||
| done |
There was a problem hiding this comment.
Critical: Array modifications in subshell are lost.
The while read loop runs in a subshell due to the pipe from find. Modifications to UPDATED_FILES inside the loop won't persist outside it, causing the summary report to always show 0 files updated.
Use process substitution instead of a pipe:
-find . -name "package.json" -type f -not -path "*/node_modules/*" -not -path "*/private-payments/*" | while read -r file; do
+while read -r file; do
if grep -q "$package" "$file"; then
# Create backup for diff
- cp "$file" "$TEMP_DIR/$(basename $file).bak"
+ cp "$file" "$TEMP_DIR/$(basename "$file").bak"
sed -i '' "s/\"@magicblock-labs\/$package\": \"[^\"]*\"/\"@magicblock-labs\/$package\": \"$VERSION\"/g" "$file"
sed -i '' "s/\"$package\": \"[^\"]*\"/\"$package\": \"$VERSION\"/g" "$file"
echo " ✓ $file"
UPDATED_FILES+=("$file")
fi
-done
+done < <(find . -name "package.json" -type f -not -path "*/node_modules/*" -not -path "*/private-payments/*")📝 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.
| find . -name "package.json" -type f -not -path "*/node_modules/*" -not -path "*/private-payments/*" | while read -r file; do | |
| if grep -q "$package" "$file"; then | |
| # Create backup for diff | |
| cp "$file" "$TEMP_DIR/$(basename $file).bak" | |
| sed -i '' "s/\"@magicblock-labs\/$package\": \"[^\"]*\"/\"@magicblock-labs\/$package\": \"$VERSION\"/g" "$file" | |
| sed -i '' "s/\"$package\": \"[^\"]*\"/\"$package\": \"$VERSION\"/g" "$file" | |
| echo " ✓ $file" | |
| UPDATED_FILES+=("$file") | |
| fi | |
| done | |
| done | |
| while read -r file; do | |
| if grep -q "$package" "$file"; then | |
| # Create backup for diff | |
| cp "$file" "$TEMP_DIR/$(basename "$file").bak" | |
| sed -i '' "s/\"@magicblock-labs\/$package\": \"[^\"]*\"/\"@magicblock-labs\/$package\": \"$VERSION\"/g" "$file" | |
| sed -i '' "s/\"$package\": \"[^\"]*\"/\"$package\": \"$VERSION\"/g" "$file" | |
| echo " ✓ $file" | |
| UPDATED_FILES+=("$file") | |
| fi | |
| done < <(find . -name "package.json" -type f -not -path "*/node_modules/*" -not -path "*/private-payments/*") | |
| done |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 30-30: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Modification of UPDATED_FILES is local (to subshell caused by pipeline).
(SC2030)
🤖 Prompt for AI Agents
In update-sdk-versions.sh around lines 27 to 39, the while-read loop is executed
in a subshell because of the pipe from find, so updates to the UPDATED_FILES
array are lost; replace the pipeline with process substitution (i.e. feed find's
output into the loop using done < <(find ...)) so the loop runs in the current
shell, keep read -r file and proper quoting of variables inside the loop, and
preserve the existing file-modification and backup steps so UPDATED_FILES is
appended in the parent shell.
| find . -name "Cargo.toml" -type f -not -path "*/private-payments/*" | while read -r file; do | ||
| if grep -q "$package" "$file"; then | ||
| # Create backup for diff | ||
| cp "$file" "$TEMP_DIR/$(basename $file).bak" | ||
|
|
||
| sed -i '' "s/$package = { version = \"[^\"]*\"/$package = { version = \"$VERSION\"/g" "$file" | ||
| sed -i '' "s/$package = \"[^\"]*\"/$package = \"$VERSION\"/g" "$file" | ||
|
|
||
| echo " ✓ $file" | ||
| UPDATED_FILES+=("$file") | ||
| fi | ||
| done | ||
| done |
There was a problem hiding this comment.
Critical: Array modifications in subshell are lost.
Same issue as with package.json updates - modifications to UPDATED_FILES won't persist.
Apply this diff:
-find . -name "Cargo.toml" -type f -not -path "*/private-payments/*" | while read -r file; do
+while read -r file; do
if grep -q "$package" "$file"; then
# Create backup for diff
- cp "$file" "$TEMP_DIR/$(basename $file).bak"
+ cp "$file" "$TEMP_DIR/$(basename "$file").bak"
sed -i '' "s/$package = { version = \"[^\"]*\"/$package = { version = \"$VERSION\"/g" "$file"
sed -i '' "s/$package = \"[^\"]*\"/$package = \"$VERSION\"/g" "$file"
echo " ✓ $file"
UPDATED_FILES+=("$file")
fi
-done
+done < <(find . -name "Cargo.toml" -type f -not -path "*/private-payments/*")📝 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.
| find . -name "Cargo.toml" -type f -not -path "*/private-payments/*" | while read -r file; do | |
| if grep -q "$package" "$file"; then | |
| # Create backup for diff | |
| cp "$file" "$TEMP_DIR/$(basename $file).bak" | |
| sed -i '' "s/$package = { version = \"[^\"]*\"/$package = { version = \"$VERSION\"/g" "$file" | |
| sed -i '' "s/$package = \"[^\"]*\"/$package = \"$VERSION\"/g" "$file" | |
| echo " ✓ $file" | |
| UPDATED_FILES+=("$file") | |
| fi | |
| done | |
| done | |
| while read -r file; do | |
| if grep -q "$package" "$file"; then | |
| # Create backup for diff | |
| cp "$file" "$TEMP_DIR/$(basename "$file").bak" | |
| sed -i '' "s/$package = { version = \"[^\"]*\"/$package = { version = \"$VERSION\"/g" "$file" | |
| sed -i '' "s/$package = \"[^\"]*\"/$package = \"$VERSION\"/g" "$file" | |
| echo " ✓ $file" | |
| UPDATED_FILES+=("$file") | |
| fi | |
| done < <(find . -name "Cargo.toml" -type f -not -path "*/private-payments/*") | |
| done |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 47-47: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 53-53: Modification of UPDATED_FILES is local (to subshell caused by pipeline).
(SC2030)
[info] 53-53: UPDATED_FILES was modified in a subshell. That change might be lost.
(SC2031)
🤖 Prompt for AI Agents
In update-sdk-versions.sh around lines 44 to 56, the while loop is reading from
a piped find which runs the loop in a subshell so changes to UPDATED_FILES are
lost; change the loop to avoid the subshell (for example, feed find into the
loop via process substitution or redirect input: use "while read -r file; do ...
done < <(find ...)" or equivalent) so UPDATED_FILES is modified in the main
shell, and keep the existing file update logic intact.
| find . -name "package.json" -type f -not -path "*/node_modules/*" | while read -r file; do | ||
| dir=$(dirname "$file") | ||
| if [ -f "$dir/yarn.lock" ]; then | ||
| if (cd "$dir" && yarn install 2>/dev/null); then | ||
| echo " ✓ $dir/yarn.lock" | ||
| else | ||
| echo " ✗ $dir/yarn.lock (yarn install failed)" | ||
| YARN_ERRORS+=("$dir") | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
Critical: Array modifications in subshell are lost.
Modifications to YARN_ERRORS won't persist outside the loop.
Apply this diff:
-find . -name "package.json" -type f -not -path "*/node_modules/*" | while read -r file; do
+while read -r file; do
dir=$(dirname "$file")
if [ -f "$dir/yarn.lock" ]; then
if (cd "$dir" && yarn install 2>/dev/null); then
echo " ✓ $dir/yarn.lock"
else
echo " ✗ $dir/yarn.lock (yarn install failed)"
YARN_ERRORS+=("$dir")
fi
fi
-done
+done < <(find . -name "package.json" -type f -not -path "*/node_modules/*")📝 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.
| find . -name "package.json" -type f -not -path "*/node_modules/*" | while read -r file; do | |
| dir=$(dirname "$file") | |
| if [ -f "$dir/yarn.lock" ]; then | |
| if (cd "$dir" && yarn install 2>/dev/null); then | |
| echo " ✓ $dir/yarn.lock" | |
| else | |
| echo " ✗ $dir/yarn.lock (yarn install failed)" | |
| YARN_ERRORS+=("$dir") | |
| fi | |
| fi | |
| done | |
| while read -r file; do | |
| dir=$(dirname "$file") | |
| if [ -f "$dir/yarn.lock" ]; then | |
| if (cd "$dir" && yarn install 2>/dev/null); then | |
| echo " ✓ $dir/yarn.lock" | |
| else | |
| echo " ✗ $dir/yarn.lock (yarn install failed)" | |
| YARN_ERRORS+=("$dir") | |
| fi | |
| fi | |
| done < <(find . -name "package.json" -type f -not -path "*/node_modules/*") |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 67-67: Modification of YARN_ERRORS is local (to subshell caused by pipeline).
(SC2030)
🤖 Prompt for AI Agents
In update-sdk-versions.sh around lines 60 to 70, the loop uses a pipeline which
runs the while-body in a subshell so appends to YARN_ERRORS are lost; change the
loop to avoid the subshell (e.g., use process substitution or redirect find
output into the while loop) so modifications to the YARN_ERRORS array persist in
the parent shell, ensure you preserve the existing logic and quoting/IFS
handling when making the change.
| find . -name "Cargo.toml" -type f | while read -r file; do | ||
| dir=$(dirname "$file") | ||
| if [ -f "$dir/Cargo.lock" ]; then | ||
| if (cd "$dir" && cargo build 2>/dev/null); then | ||
| echo " ✓ $dir/Cargo.lock" | ||
| else | ||
| echo " ✗ $dir/Cargo.lock (cargo build failed)" | ||
| CARGO_ERRORS+=("$dir") | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
Critical: Array modifications in subshell are lost.
Modifications to CARGO_ERRORS won't persist outside the loop.
Apply this diff:
-find . -name "Cargo.toml" -type f | while read -r file; do
+while read -r file; do
dir=$(dirname "$file")
if [ -f "$dir/Cargo.lock" ]; then
if (cd "$dir" && cargo build 2>/dev/null); then
echo " ✓ $dir/Cargo.lock"
else
echo " ✗ $dir/Cargo.lock (cargo build failed)"
CARGO_ERRORS+=("$dir")
fi
fi
-done
+done < <(find . -name "Cargo.toml" -type f)📝 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.
| find . -name "Cargo.toml" -type f | while read -r file; do | |
| dir=$(dirname "$file") | |
| if [ -f "$dir/Cargo.lock" ]; then | |
| if (cd "$dir" && cargo build 2>/dev/null); then | |
| echo " ✓ $dir/Cargo.lock" | |
| else | |
| echo " ✗ $dir/Cargo.lock (cargo build failed)" | |
| CARGO_ERRORS+=("$dir") | |
| fi | |
| fi | |
| done | |
| while read -r file; do | |
| dir=$(dirname "$file") | |
| if [ -f "$dir/Cargo.lock" ]; then | |
| if (cd "$dir" && cargo build 2>/dev/null); then | |
| echo " ✓ $dir/Cargo.lock" | |
| else | |
| echo " ✗ $dir/Cargo.lock (cargo build failed)" | |
| CARGO_ERRORS+=("$dir") | |
| fi | |
| fi | |
| done < <(find . -name "Cargo.toml" -type f) |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 81-81: Modification of CARGO_ERRORS is local (to subshell caused by pipeline).
(SC2030)
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dummy-token-transfer/tests/dummy-transfer.ts (1)
184-205: Validator key derivation via getClosestValidator seems fine; could be made clearer/safer
const validatorKey = new PublicKey((await routerConnection.getClosestValidator()).identity);is a reasonable way to derive the validator public key, assuminggetClosestValidator()always resolves to an object with anidentityfield. For readability and easier error handling, you might split this into steps and decide what to do if the router can’t return a validator (e.g., skip the test with a clear message instead of throwing):const closest = await routerConnection.getClosestValidator(); if (!closest) { throw new Error("No validator returned from routerConnection.getClosestValidator()"); } const validatorKey = new PublicKey(closest.identity);Not blocking, but it makes failures easier to interpret.
♻️ Duplicate comments (4)
test-locally.sh (4)
29-32: ClarifyNO_COLORassignment; redirection order is now correct.The current invocation works, but
NO_COLOR=is ambiguous and triggers Shellcheck (SC1007). It’s clearer either to use an explicit value or the empty-string pattern.- ( FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR= eval "$test_command" ) > "$test_log" 2>&1 & + ( FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR='' eval "$test_command" ) > "$test_log" 2>&1 &This keeps the fixed redirection order and makes the intent of disabling color explicit.
206-226: Spliterror_detailsdeclaration and assignment to avoid masking exit status.Both failure branches use
local error_details=$(...), which Shellcheck flags (SC2155) and which can mask the exit status of the pipeline.Refactor like this in both places:
- local error_details=$(tac "$test_log" | sed -n '/^ [0-9]\+)/,/^[[:space:]]*$/p' | tac) + local error_details + error_details=$(tac "$test_log" | sed -n '/^ [0-9]\+)/,/^[[:space:]]*$/p' | tac) @@ - local error_details=$(grep -B 2 -A 8 "error\[" "$test_log" | head -60) + local error_details + error_details=$(grep -B 2 -A 8 "error\[" "$test_log" | head -60)This keeps behavior the same while making the intent clearer and avoiding SC2155.
303-320: Drop unused--clone/--clone-upgradeable-programentries to speed validator startup.The validator currently clones many accounts/programs; prior analysis showed several of these addresses are not referenced anywhere in the repo. Keeping only the ones actually used in tests will reduce startup time and network load.
Concretely, you can remove the unused
--clone*flags and keep just the actively used ones (mAGicPQY...,noopb9...,DELeGGv...) unless new tests were added that depend on the others.
347-363: Standardize endpoint environment variables acrossrun_testcalls.Only some tests set
EPHEMERAL_PROVIDER_ENDPOINT/EPHEMERAL_WS_ENDPOINT/PROVIDER_ENDPOINT/WS_ENDPOINTinline, others rely on defaults. For reproducibility and easier debugging, it’s better to either:
- Export these endpoints once near the top of the script so every test inherits them, or
- Pass the same set of env vars into every
run_testinvocation that needs them.This will avoid subtle discrepancies between projects and between local vs. scripted runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
.gitignore(1 hunks)anchor-minter/Anchor.toml(1 hunks)anchor-rock-paper-scissor/Anchor.toml(1 hunks)bolt-counter/Anchor.toml(1 hunks)bolt-counter/programs-ecs/components/counter/Cargo.toml(1 hunks)bolt-counter/programs-ecs/systems/increase/Cargo.toml(1 hunks)dummy-token-transfer/Anchor.toml(1 hunks)dummy-token-transfer/tests/dummy-transfer.ts(11 hunks)magic-actions/Anchor.toml(1 hunks)magic-actions/tests/magic-actions.ts(8 hunks)roll-dice/Anchor.toml(1 hunks)roll-dice/programs/roll-dice/Cargo.toml(1 hunks)roll-dice/tests/roll-dice-delegated.ts(1 hunks)roll-dice/tests/roll-dice.ts(1 hunks)test-locally.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
test-locally.sh
[warning] 30-30: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 34-34: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 51-51: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 54-54: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 57-57: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 70-70: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 208-208: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 222-222: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 235-235: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 235-235: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
🔇 Additional comments (13)
bolt-counter/programs-ecs/components/counter/Cargo.toml (1)
18-20: Clarify purpose of unused feature flags or remove them.The three empty feature flags (
anchor-debug,custom-heap,custom-panic) are not referenced in the crate's source code via conditional compilation directives, nor are they propagated to dependencies. These same unused features appear systematically across multiple programs in the repository (roll-dice, session-keys, anchor-minter, dummy-token-transfer, magic-actions, anchor-counter, anchor-rock-paper-scissor, and bolt-counter), suggesting either:
- Intentional scaffolding for planned future functionality—in which case they should be documented
- Incomplete feature implementation—features should be properly integrated into source code or dependency declarations
- Unnecessary boilerplate—features should be removed to keep manifests clean
Either implement these features properly or remove them from the Cargo.toml.
magic-actions/tests/magic-actions.ts (1)
54-55: The Magic Router expectsskipPreflight: trueto optimize latency—this is the correct pattern, not a workaround.The
skipPreflight: trueoption is the recommended approach when usingConnectionMagicRouterfrom@magicblock-labs/ephemeral-rollups-sdkto avoid extra latency during transaction submission. This is intentional router design, not a limitation being worked around.However, when skipping preflight checks, the implementation should include compensating controls to prevent transaction failures from consuming fees undetected:
- Simulate transactions before sending using
simulateTransaction()to validate logic client-side.- Use correct blockhash retrieval for delegated accounts via the SDK's
getBlockhashForAccounts()(or equivalent), not justgetLatestBlockhash().- Implement robust error handling and retries with proper exponential backoff and idempotency.
- Verify account delegation state before submission to ensure transactions route to the correct endpoint.
Confirm these safeguards are in place to prevent failed transactions on the ephemeral rollup from silently consuming SOL.
Also applies to: 71-72, 90-91, 141-142, 161-162, 180-181, 199-200, 216-217
dummy-token-transfer/tests/dummy-transfer.ts (1)
40-45: Router connection construction is reasonable; ensure env/cluster alignmentInstantiating
ConnectionMagicRouterwithROUTER_ENDPOINT/ROUTER_WS_ENDPOINTand loggingrouterConnection.rpcEndpointlooks fine and should keep tests configurable. Just make sure CI/test envs set these to a router that fronts the same cluster asAnchorProvider.env()to avoid subtle state mismatches betweenprovider.connectionandrouterConnection.Also applies to: 47-47
anchor-rock-paper-scissor/Anchor.toml (1)
19-20: Timeout reduction looks reasonable; confirm suite still fits within 120s.The script change only tightens the mocha timeout and adds
NODE_OPTIONS='--no-deprecation', which should be fine as long as the longest test run comfortably finishes within 120s.bolt-counter/Anchor.toml (1)
19-20: Shorter mocha timeout is fine if tests are fast enough.Reducing
-tto 120000ms should be safe provided the slowest bolt-counter test runs complete within 2 minutes; otherwise, they’ll start failing with timeouts instead of real errors.dummy-token-transfer/Anchor.toml (1)
18-19: Consistent timeout setting; verify dummy-token-transfer tests stay under 120s.This aligns the test timeout with other packages. Just ensure the dummy-token-transfer suite doesn’t occasionally exceed 120s under load.
magic-actions/Anchor.toml (1)
21-22: Shorter test timeout mirrors other projects; confirm no flakes.The 120s mocha timeout should be fine as long as the magic-actions tests don’t occasionally spike above that duration.
anchor-minter/Anchor.toml (1)
24-26: Timeout reduction aligns with the rest of the repo.Anchor-minter’s mocha timeout is now 120s; make sure the full test flow (including any on-chain interactions) fits within that budget on typical dev/CI hardware.
roll-dice/tests/roll-dice-delegated.ts (1)
33-50: UsingskipPreflighton test RPC calls is reasonable but bypasses some safety checks.Passing
{ skipPreflight: true }to.rpc()matches the Anchor client’s SendOptions shape and will speed tests up, but any issues normally caught by preflight will instead surface as full transaction failures (or not at all if not propagated). That’s acceptable for deterministic tests as long as you’re aware of the tradeoff.roll-dice/Anchor.toml (1)
23-24: Roll-dice mocha timeout matches the new standard; ensure tests are within 120s.This keeps roll-dice aligned with other projects’ test timeouts. Just confirm the combined
roll-dice+ delegated tests don’t occasionally exceed 2 minutes, especially when run throughtest-locally.sh.roll-dice/tests/roll-dice.ts (2)
12-12: Confirm intent to bypass preflight on initialize()Adding
{ skipPreflight: true }will skip simulation and can hide sim-only failures; it’s fine for faster tests if this is deliberate, but please confirm this is what you want for initialization as well.
17-17: Consistent skipPreflight usage on rollDice(0)Same note here: skipping preflight keeps these tests fast and aligned with the delegated tests, but double‑check that you don’t rely on simulation catching errors for this instruction.
roll-dice/programs/roll-dice/Cargo.toml (1)
24-25: Verify ephemeral SDK versions are consistent and compatibleThe bump to
ephemeral-rollups-sdk = "0.6.5"(andephemeral-vrf-sdk = "0.2.1") looks fine here; just ensure:
- All related programs/crates in the workspace use compatible versions of these SDKs.
- The new SDK versions are confirmed to work with
anchor-lang = "0.32.1"and your current Solana toolchain.
| **/tests-kit | ||
|
|
||
| # Logs | ||
| **/**/**.log |
There was a problem hiding this comment.
Simplify the .log ignore pattern.
The pattern **/**/**.log is redundant and uses incorrect syntax. The ** token is for matching directory paths, not filenames. To ignore all .log files in any subdirectory, use **/*.log instead.
-**/**/**.log
+**/*.log📝 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.
| **/**/**.log | |
| **/*.log |
🤖 Prompt for AI Agents
In .gitignore around line 25, the pattern '**/**/**.log' is redundant and
incorrect for matching log files; replace it with a single recursive pattern
'**/*.log' to ignore all .log files in any directory (including root), remove
the invalid extra slashes and stars, and save the updated .gitignore.
| anchor-debug = [] | ||
| custom-heap = [] | ||
| custom-panic = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the directory structure of the increase crate
find bolt-counter/programs-ecs/systems/increase -type f -name "*.rs" | head -20Repository: magicblock-labs/magicblock-engine-examples
Length of output: 139
🏁 Script executed:
# Check the Cargo.toml to see the current state
cat bolt-counter/programs-ecs/systems/increase/Cargo.tomlRepository: magicblock-labs/magicblock-engine-examples
Length of output: 640
🏁 Script executed:
# Search for feature gating in source files
rg '#\[cfg\(feature' bolt-counter/programs-ecs/systems/increase/src/ -nRepository: magicblock-labs/magicblock-engine-examples
Length of output: 68
🏁 Script executed:
# Search for mentions of the specific features in comments or code
rg '(anchor-debug|custom-heap|custom-panic)' bolt-counter/programs-ecs/systems/increase/ -nRepository: magicblock-labs/magicblock-engine-examples
Length of output: 309
🏁 Script executed:
# Check if these features exist in other crates mentioned in the PR
fd 'Cargo.toml' bolt-counter/programs-ecs/systems/ --exec grep -l 'anchor-debug\|custom-heap\|custom-panic' {} +Repository: magicblock-labs/magicblock-engine-examples
Length of output: 191
🏁 Script executed:
# Read the lib.rs file to check for feature gating
cat -n bolt-counter/programs-ecs/systems/increase/src/lib.rsRepository: magicblock-labs/magicblock-engine-examples
Length of output: 641
🏁 Script executed:
# Search for any feature gates more broadly in the increase crate
rg 'feature' bolt-counter/programs-ecs/systems/increase/src/ -nRepository: magicblock-labs/magicblock-engine-examples
Length of output: 68
🏁 Script executed:
# Check if these features might be used by dependent crates
rg 'anchor-debug|custom-heap|custom-panic' bolt-counter/ -n --type tomlRepository: magicblock-labs/magicblock-engine-examples
Length of output: 539
🏁 Script executed:
# Find all Cargo.toml files in the bolt-counter project and check for these features
fd 'Cargo.toml' bolt-counter/programs-ecs/ --exec grep -l 'anchor-debug' {} +Repository: magicblock-labs/magicblock-engine-examples
Length of output: 282
🏁 Script executed:
# Count how many crates have these features
fd 'Cargo.toml' bolt-counter/programs-ecs/ | xargs grep -l 'anchor-debug\|custom-heap\|custom-panic' | wc -lRepository: magicblock-labs/magicblock-engine-examples
Length of output: 87
🏁 Script executed:
# Check for any documentation or README files that explain these features
find bolt-counter/ -type f \( -name "README*" -o -name "*.md" \) | xargs grep -l 'anchor-debug\|custom-heap\|custom-panic' 2>/dev/null || echo "No documentation found"Repository: magicblock-labs/magicblock-engine-examples
Length of output: 108
🏁 Script executed:
# Verify if any crate actually uses these features as dependencies
rg 'anchor-debug|custom-heap|custom-panic' bolt-counter/programs-ecs/ --type toml -B 2 -A 2Repository: magicblock-labs/magicblock-engine-examples
Length of output: 1098
Remove undocumented and unused Cargo feature flags.
The three new features (anchor-debug, custom-heap, custom-panic) are declared in Cargo.toml but are not gated with #[cfg(feature)] attributes in the source code, nor are they documented. They exist only as empty feature declarations with no functional purpose. Remove them unless they are intended for external consumers—in which case they require explicit documentation explaining their purpose and usage.
🤖 Prompt for AI Agents
In bolt-counter/programs-ecs/systems/increase/Cargo.toml around lines 18 to 20,
three feature flags (anchor-debug, custom-heap, custom-panic) are declared but
unused and undocumented; remove these empty feature declarations from Cargo.toml
unless you intend them for external consumers, in which case instead add
documentation comments to Cargo.toml and wire them into the source with
#[cfg(feature = "...")] guards where appropriate and explain their purpose and
usage in the crate README or Cargo.toml package metadata.
| DELEGATION_PROGRAM_ID, ConnectionMagicRouter | ||
| } from "@magicblock-labs/ephemeral-rollups-sdk"; | ||
| import { getClosestValidator, sendMagicTransaction } from "magic-router-sdk"; | ||
| import { Transaction} from "@solana/web3.js"; | ||
| import { PublicKey, sendAndConfirmTransaction, Transaction} from "@solana/web3.js"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Imports updated for router/Solana helpers look correct; minor consistency nit
The switch to ConnectionMagicRouter, PublicKey, and sendAndConfirmTransaction matches the new router-based flow and Solana helpers. One small optional clean-up: you’re mixing Transaction from @solana/web3.js (for typing) and web3.Transaction (for construction); consider standardizing on one for consistency, but it’s not blocking.
🤖 Prompt for AI Agents
In dummy-token-transfer/tests/dummy-transfer.ts around lines 5 to 7, you’re
mixing Transaction typing/import from "@solana/web3.js" with uses of
web3.Transaction elsewhere; standardize to a single source: import Transaction
from "@solana/web3.js" and replace all occurrences of web3.Transaction with
Transaction (or alternatively import the whole web3 namespace and use
web3.Transaction consistently), update construction and type annotations
accordingly, and remove any unused web3 namespace import to keep imports
consistent.
| const signature = await sendAndConfirmTransaction( | ||
| routerConnection, | ||
| tx, | ||
| [provider.wallet.payer] | ||
| [provider.wallet.payer], | ||
| { skipPreflight: true } | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
sendAndConfirmTransaction migration is consistent; consider a small helper to DRY it up
All call sites now use sendAndConfirmTransaction(routerConnection, tx, signers, { skipPreflight: true }) with the expected signers for each transaction, which aligns with the new router integration. Given the identical pattern across tests, you could introduce a tiny helper like sendViaRouter(tx, signers) to centralize the confirm options and make future tweaks (e.g., commitment level) easier:
+async function sendViaRouter(tx: Transaction, signers: web3.Signer[]) {
+ return sendAndConfirmTransaction(routerConnection, tx, signers, {
+ skipPreflight: true,
+ });
+}
...
-const signature = await sendAndConfirmTransaction(
- routerConnection,
- tx,
- [provider.wallet.payer],
- { skipPreflight: true }
-);
+const signature = await sendViaRouter(tx, [provider.wallet.payer]);Same pattern can be applied to the other send sites.
Also applies to: 128-133, 161-166, 206-212, 241-246, 258-263, 286-291, 300-305
🤖 Prompt for AI Agents
In dummy-token-transfer/tests/dummy-transfer.ts around lines 99-104 (and also
apply the same change at 128-133, 161-166, 206-212, 241-246, 258-263, 286-291,
300-305), multiple test sites call sendAndConfirmTransaction(routerConnection,
tx, signers, { skipPreflight: true }) with identical options; extract a small
helper function (e.g., sendViaRouter(tx, signers)) near the top of the test file
that calls sendAndConfirmTransaction with routerConnection and the fixed options
{ skipPreflight: true } and then replace each call site with sendViaRouter(tx,
signers) to DRY up the code and centralize future option changes.
| # Wait for completion | ||
| wait $test_pid | ||
|
|
||
| # Check if test failed | ||
| local test_failed=false | ||
| if grep -q "[0-9] failing" "$test_log" || grep -q "error\[" "$test_log" || grep -q "could not compile" "$test_log"; then | ||
| test_failed=true | ||
| fi | ||
|
|
There was a problem hiding this comment.
Use the test process exit code and the test_failed flag instead of log heuristics/array regex.
Right now PASS/FAIL is inferred only from log patterns and the FAILED_TESTS array, ignoring the actual exit code. A test that exits non‑zero without those strings would be reported as PASSED, and the array/regex membership check on Line 235 is fragile (Shellcheck SC2199/SC2076).
Consider:
- # Wait for completion
- wait $test_pid
+ # Wait for completion and capture exit code
+ wait $test_pid
+ local exit_code=$?
@@
- local test_failed=false
- if grep -q "[0-9] failing" "$test_log" || grep -q "error\[" "$test_log" || grep -q "could not compile" "$test_log"; then
- test_failed=true
- fi
+ local test_failed=false
+ if [ $exit_code -ne 0 ] || \
+ grep -q "[0-9] failing" "$test_log" || \
+ grep -q "error\[" "$test_log" || \
+ grep -q "could not compile" "$test_log"; then
+ test_failed=true
+ fi
@@
- if grep -q "[0-9] failing" "$test_log"; then
+ if grep -q "[0-9] failing" "$test_log"; then
@@
- elif grep -q "error\[" "$test_log" || grep -q "could not compile" "$test_log"; then
+ elif grep -q "error\[" "$test_log" || grep -q "could not compile" "$test_log"; then
@@
- else
- PASSED_TESTS+=("$test_name")
- fi
+ else
+ PASSED_TESTS+=("$test_name")
+ fi
@@
- if [[ " ${FAILED_TESTS[@]} " =~ " ${test_name} " ]]; then
- echo "Result: ✗ FAILED"
- else
- echo "Result: ✓ PASSED"
- fi
+ if [ "$test_failed" = true ]; then
+ echo "Result: ✗ FAILED"
+ else
+ echo "Result: ✓ PASSED"
+ fiThis makes the runner respect the underlying test process status and avoids brittle array/regex checks.
Also applies to: 201-239
🤖 Prompt for AI Agents
In test-locally.sh around lines 113-121 (and similarly 201-239), stop relying
solely on log-greps/regex to determine PASS/FAIL: after wait $test_pid capture
the test process exit code (rc=$?) and set test_failed=true if rc is non-zero or
if any of the existing log-heuristics already found failures; then use that
test_failed flag (and the saved rc if needed) for final reporting. Replace
fragile array/regex membership checks with a safe loop over the FAILED_TESTS
array (or use printf '%s\n' "${FAILED_TESTS[@]}" | grep -Fx) to test exact
equality rather than pattern matching. Ensure all downstream logic uses the
exit-code-aware test_failed variable to decide pass/fail.
| # Cleanup function | ||
| cleanup() { | ||
| # Disable trap to prevent recursion | ||
| trap - EXIT INT TERM | ||
|
|
||
| # Clear current line | ||
| echo -ne "\r \r" | ||
| echo "" | ||
| echo -n "Stopping validators... " | ||
|
|
||
| # Kill by PID if available | ||
| if [ -n "$SOLANA_PID" ]; then | ||
| kill -TERM $SOLANA_PID 2>/dev/null || true | ||
| fi | ||
| if [ -n "$EPHEMERAL_PID" ]; then | ||
| kill -TERM $EPHEMERAL_PID 2>/dev/null || true | ||
| fi | ||
|
|
||
| # Give them a moment to gracefully shutdown | ||
| sleep 1 | ||
|
|
||
| # Force kill any remaining processes | ||
| if [ -n "$SOLANA_PID" ]; then | ||
| kill -9 $SOLANA_PID 2>/dev/null || true | ||
| fi | ||
| if [ -n "$EPHEMERAL_PID" ]; then | ||
| kill -9 $EPHEMERAL_PID 2>/dev/null || true | ||
| fi | ||
|
|
||
| # Also kill by process name as fallback | ||
| pkill -f "solana-test-validator" 2>/dev/null || true | ||
| pkill -f "ephemeral-validator" 2>/dev/null || true | ||
|
|
||
| # Wait for background jobs silently | ||
| { wait 2>/dev/null || true; } 2>/dev/null | ||
|
|
||
| # Check if validators are actually stopped | ||
| if ! pgrep -f "solana-test-validator" >/dev/null 2>&1 && ! pgrep -f "ephemeral-validator" >/dev/null 2>&1; then | ||
| echo "✓ Stopped" | ||
| else | ||
| echo "✗ Failed to stop" | ||
| fi | ||
|
|
||
| exit 0 | ||
| } | ||
|
|
||
| # Set up trap to catch INT (Ctrl+C), TERM, and EXIT | ||
| trap cleanup EXIT INT TERM |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test-locally.sh | sed -n '244,291p'Repository: magicblock-labs/magicblock-engine-examples
Length of output: 1715
🏁 Script executed:
cat -n test-locally.sh | sed -n '365,400p'Repository: magicblock-labs/magicblock-engine-examples
Length of output: 1349
🏁 Script executed:
# Check all exit statements in the script
rg -n "exit" test-locally.shRepository: magicblock-labs/magicblock-engine-examples
Length of output: 160
🏁 Script executed:
# Check if there's any exit code logic after the error details section
wc -l test-locally.shRepository: magicblock-labs/magicblock-engine-examples
Length of output: 105
🏁 Script executed:
# Get the end of the script to see final exit code handling
tail -20 test-locally.shRepository: magicblock-labs/magicblock-engine-examples
Length of output: 659
cleanup forces exit code 0; script cannot fail when tests do.
cleanup() ends with exit 0 (line 287) and is registered for EXIT (line 291), so the script always returns success regardless of whether tests failed. The summary section (lines 365–390) computes pass/fail counts but never checks them to set an exit code. This makes the script unsuitable for automation and hides failures.
A safer pattern:
cleanup() {
@@
- if ! pgrep -f "solana-test-validator" >/dev/null 2>&1 && ! pgrep -f "ephemeral-validator" >/dev/null 2>&1; then
- echo "✓ Stopped"
- else
- echo "✗ Failed to stop"
- fi
-
- exit 0
+ if ! pgrep -f "solana-test-validator" >/dev/null 2>&1 && ! pgrep -f "ephemeral-validator" >/dev/null 2>&1; then
+ echo "✓ Stopped"
+ else
+ echo "✗ Failed to stop"
+ fi
}
@@
-trap cleanup EXIT INT TERM
+trap cleanup INT TERMThen, after printing the summary, explicitly choose the exit code:
echo "Total: ${#PASSED_TESTS[@]} passed, ${#FAILED_TESTS[@]} failed"
echo "========================================"
echo ""
# Print detailed error report for failed tests
if [ ${#FAILED_TESTS[@]} -gt 0 ]; then
@@
done
fi
+
+if [ ${#FAILED_TESTS[@]} -gt 0 ]; then
+ exit 1
+fi
+
+exit 0This keeps cleanup behavior on interrupts/TERM while accurately reflecting test failures in the script's exit status.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test-locally.sh around lines 244–291, cleanup() currently ends with exit 0
and is registered on EXIT, so the script always returns success; change cleanup
to accept an optional exit-status argument and exit with that status (do not
hardcode 0), and update the trap to pass the current exit status (e.g., trap
'cleanup $?' EXIT INT TERM). Also ensure the summary logic later (lines
~365–390) sets an appropriate non-zero exit code when failures are present (call
exit with the failure-derived status instead of relying on cleanup to force
success).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
test-locally.sh (5)
113-121: Use the test process exit code, not just log heuristics, to determine pass/fail.The script only checks logs for failure patterns and ignores the actual exit code. A test that exits non-zero without the magic strings would be reported as PASSED. Capture and check the exit code instead.
Apply this diff:
# Wait for completion wait $test_pid + local exit_code=$? # Check if test failed local test_failed=false - if grep -q "[0-9] failing" "$test_log" || grep -q "error\[" "$test_log" || grep -q "could not compile" "$test_log"; then + if [ $exit_code -ne 0 ] || \ + grep -q "[0-9] failing" "$test_log" || \ + grep -q "error\[" "$test_log" || \ + grep -q "could not compile" "$test_log"; then test_failed=true fi
30-30: Fix redirection order and use explicit NO_COLOR assignment.The current command redirects stderr to the original stdout first (before stdout is piped to tee), so stderr still goes to the terminal. Also,
NO_COLOR=is ambiguous; useNO_COLOR=''to make the intent explicit.Apply this diff:
- ( FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR= eval "$test_command" ) > "$test_log" 2>&1 & + ( FORCE_COLOR=1 CARGO_TERM_COLOR=always NO_COLOR='' eval "$test_command" ) > "$test_log" 2>&1 &
305-320: Remove unused cloned accounts and programs to reduce test startup overhead.A prior analysis found that 8 of the 11 cloned addresses are never referenced in the codebase. Cloning unused accounts increases test initialization time unnecessarily.
Remove these unused clones:
EpJnX7ueXk7fKojBymqmVuCuwyhDQsYcLVL1XMsBbvDX(--clone)7JrkjmZPprHwtuvtuGTXp9hwfGYFAQLnLeFM52kqAgXg(--clone)Cuj97ggrhhidhbu39TijNVqE74xvKJ69gDervRUXAxGh(--clone-upgradeable-program)5hBR571xnXppuCPveTrctfTU7tJLSN94nq7kv7FRK5Tc(--clone)F72HqCR8nwYsVyeVd38pgKkjXmXFzVAM8rjZZsXWbdE(--clone)vrfkfM4uoisXZQPrFiS2brY4oMkU9EWjyvmvqaFd5AS(--clone)Vrf1RNUjXmQGjmQrQLvJHs9SNkvDJEsRVFPkfSQUwGz(--clone-upgradeable-program)BTWAqWNBmF2TboMh3fxMJfgR16xGHYD7Kgr2dPwbRPBi(--clone-upgradeable-program)Only keep the actively used clones:
mAGicPQYBMvcYveUZA5F5UNNwyHvfYh5xkLS2Fr1mevnoopb9bkMVfRPU8AsbpTUg8AQkHtKwMYZiFUjNRtMmVDELeGGvXpWV2fqJUhqcF5ZSYMS4JTLjteaAMARRSaeShApply this diff:
solana-test-validator \ --ledger ./test-ledger \ --reset \ --clone mAGicPQYBMvcYveUZA5F5UNNwyHvfYh5xkLS2Fr1mev \ - --clone EpJnX7ueXk7fKojBymqmVuCuwyhDQsYcLVL1XMsBbvDX \ - --clone 7JrkjmZPprHwtuvtuGTXp9hwfGYFAQLnLeFM52kqAgXg \ --clone noopb9bkMVfRPU8AsbpTUg8AQkHtKwMYZiFUjNRtMmV \ --clone-upgradeable-program DELeGGvXpWV2fqJUhqcF5ZSYMS4JTLjteaAMARRSaeSh \ - --clone Cuj97ggrhhidhbu39TijNVqE74xvKJ69gDervRUXAxGh \ - --clone 5hBR571xnXppuCPveTrctfTU7tJLSN94nq7kv7FRK5Tc \ - --clone F72HqCR8nwYsVyeVd38pgKkjXmXFzVAM8rjZZsXWbdE \ - --clone vrfkfM4uoisXZQPrFiS2brY4oMkU9EWjyvmvqaFd5AS \ - --clone-upgradeable-program Vrf1RNUjXmQGjmQrQLvJHs9SNkvDJEsRVFPkfSQUwGz \ - --clone-upgradeable-program BTWAqWNBmF2TboMh3fxMJfgR16xGHYD7Kgr2dPwbRPBi \ --url https://api.devnet.solana.com > ./test-ledger.log 2>&1 &
235-235: Use a safe loop to check array membership; avoid implicit concatenation.The condition
[[ " ${FAILED_TESTS[@]} " =~ " ${test_name} " ]]implicitly concatenates array elements and uses regex matching on the right-hand side. This is error-prone and flagged by Shellcheck. Use a safe loop to check exact membership instead.Apply this diff:
- if [[ " ${FAILED_TESTS[@]} " =~ " ${test_name} " ]]; then - echo "Result: ✗ FAILED" - else - echo "Result: ✓ PASSED" - fi + if [ "$test_failed" = true ]; then + echo "Result: ✗ FAILED" + else + echo "Result: ✓ PASSED" + fiThis uses the already-computed
test_failedvariable from line 117, which is more reliable than array membership checks.
287-287: Remove hardcodedexit 0from cleanup so the script can fail when tests do.The cleanup function always exits with status 0, making the script succeed even when tests fail. This breaks automation and CI/CD pipelines that rely on non-zero exit codes for failure detection. Move the exit logic to the end of the script so it reflects the actual test results.
Apply these diffs:
First, remove
exit 0from cleanup:cleanup() { # Disable trap to prevent recursion trap - EXIT INT TERM @@ if ! pgrep -f "solana-test-validator" >/dev/null 2>&1 && ! pgrep -f "ephemeral-validator" >/dev/null 2>&1; then echo "✓ Stopped" else echo "✗ Failed to stop" fi - - exit 0 }Then, update the trap and add exit logic at the end:
-trap cleanup EXIT INT TERM +trap cleanup INT TERM +trap 'cleanup; exit $?' EXITFinally, add the following after line 403 (after the error details section):
# Print detailed error report for failed tests if [ ${#FAILED_TESTS[@]} -gt 0 ]; then echo "========================================" echo "FAILED TESTS - ERROR DETAILS" echo "========================================" echo "" for i in "${!FAILED_TESTS_NAMES[@]}"; do echo "--- ${FAILED_TESTS_NAMES[$i]} ---" echo "${FAILED_TESTS_ERRORS[$i]}" echo "" done + exit 1 fi + +exit 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-locally.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
test-locally.sh
[warning] 30-30: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 34-34: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 51-51: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 54-54: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 57-57: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 70-70: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 208-208: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 222-222: Declare and assign separately to avoid masking return values.
(SC2155)
[error] 235-235: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 235-235: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
| local test_pid=$! | ||
|
|
||
| # Handle interrupt signal to kill test | ||
| trap "kill -TERM $test_pid 2>/dev/null; exit 1" INT TERM |
There was a problem hiding this comment.
Use single quotes in trap to prevent early expansion.
When using a variable in a trap, single quotes prevent it from being expanded at trap registration time, ensuring it uses the current value when the signal fires.
Apply this diff:
- trap "kill -TERM $test_pid 2>/dev/null; exit 1" INT TERM
+ trap 'kill -TERM $test_pid 2>/dev/null; exit 1' INT TERM📝 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.
| trap "kill -TERM $test_pid 2>/dev/null; exit 1" INT TERM | |
| trap 'kill -TERM $test_pid 2>/dev/null; exit 1' INT TERM |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 34-34: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🤖 Prompt for AI Agents
In test-locally.sh around line 34, the trap uses double quotes which causes
$test_pid to be expanded when the trap is registered; change the trap to use
single quotes so the variable is expanded at signal time (i.e., replace the
surrounding double quotes with single quotes while keeping the same command
string and redirection).
| local current_status="" | ||
|
|
||
| # Get the most recent meaningful line from log | ||
| local last_line=$(tail -1 "$test_log" 2>/dev/null) |
There was a problem hiding this comment.
Declare and assign shell variables separately to avoid masking return values.
Combined declaration and assignment (e.g., local var=$(cmd)) masks the exit status of the command, making it impossible to detect failures.
Apply these diffs to separate declaration from assignment:
Lines 51–52:
- local last_line=$(tail -1 "$test_log" 2>/dev/null)
+ local last_line
+ last_line=$(tail -1 "$test_log" 2>/dev/null)Lines 54–55:
- local prog_name=$(grep "Deploying program" "$test_log" | tail -1 | sed 's/.*Deploying program "\([^"]*\)".*/\1/')
+ local prog_name
+ prog_name=$(grep "Deploying program" "$test_log" | tail -1 | sed 's/.*Deploying program "\([^"]*\)".*/\1/')Lines 57–58:
- local prog_id=$(grep "Waiting for program" "$test_log" | tail -1 | sed 's/.*for program \([^ ]*\).*/\1/')
+ local prog_id
+ prog_id=$(grep "Waiting for program" "$test_log" | tail -1 | sed 's/.*for program \([^ ]*\).*/\1/')Lines 70–71:
- local crate=$(echo "$last_line" | sed 's/.*Compiling \([^ ]*\).*/\1/' | sed 's/.*Checking \([^ ]*\).*/\1/')
+ local crate
+ crate=$(echo "$last_line" | sed 's/.*Compiling \([^ ]*\).*/\1/' | sed 's/.*Checking \([^ ]*\).*/\1/')Also applies to: 54-54, 57-57, 70-70
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 51-51: Declare and assign separately to avoid masking return values.
(SC2155)
|
|
||
| # Extract test failures section - find last occurrence of test suite and get failures from there | ||
| # Get the last test suite block (after the last blank line followed by test name pattern) | ||
| local error_details=$(tac "$test_log" | sed -n '/^ [0-9]\+)/,/^[[:space:]]*$/p' | tac) |
There was a problem hiding this comment.
Declare and assign shell variables separately to avoid masking return values.
Combined declaration and assignment masks the exit status of command substitutions, preventing detection of failures in piped commands.
Apply these diffs:
Lines 208–209:
- local error_details=$(tac "$test_log" | sed -n '/^ [0-9]\+)/,/^[[:space:]]*$/p' | tac)
+ local error_details
+ error_details=$(tac "$test_log" | sed -n '/^ [0-9]\+)/,/^[[:space:]]*$/p' | tac)Lines 222–223:
- local error_details=$(grep -B 2 -A 8 "error\[" "$test_log" | head -60)
+ local error_details
+ error_details=$(grep -B 2 -A 8 "error\[" "$test_log" | head -60)Also applies to: 222-222
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 208-208: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In test-locally.sh around lines 208 and 222, the script uses combined
declaration+assignment for variables (e.g., local var=$(...)) which masks the
exit status of the command substitution; change to declare the variable first
(local error_details) and then assign it on the next line (error_details=$(...))
so the script can detect failures from the piped commands, repeat the same
pattern for the other variable at line 222, and ensure you check the command
exit status after the assignment.
| # run_test "anchor-counter" "cd anchor-counter && anchor build && anchor deploy --provider.cluster localnet && yarn install && EPHEMERAL_PROVIDER_ENDPOINT='http://localhost:7799' EPHEMERAL_WS_ENDPOINT='ws://localhost:7800' PROVIDER_ENDPOINT=http://localhost:8899 WS_ENDPOINT=http://localhost:8900 anchor test --provider.cluster localnet --skip-local-validator --skip-deploy; cd .." | ||
|
|
||
| run_test "anchor-minter" "cd anchor-minter && anchor build && anchor deploy --provider.cluster localnet && yarn install && anchor test --skip-build --skip-deploy --skip-local-validator; cd .." | ||
|
|
||
| run_test "anchor-rock-paper-scissor" "cd anchor-rock-paper-scissor && anchor build && yarn install && anchor test --skip-deploy; cd .." | ||
|
|
||
| run_test "dummy-token-transfer" "cd dummy-token-transfer && anchor build && yarn install && anchor test --skip-build --skip-deploy --skip-local-validator; cd .." | ||
|
|
||
| run_test "magic-actions" "cd magic-actions && yarn install && anchor build && yarn install && anchor test --skip-build --skip-deploy --skip-local-validator; cd .." | ||
|
|
||
| run_test "oncurve-delegation" "cd oncurve-delegation && yarn install && EPHEMERAL_PROVIDER_ENDPOINT=http://localhost:7799 EPHEMERAL_WS_ENDPOINT=ws://localhost:7800 PROVIDER_ENDPOINT=http://localhost:8899 WS_ENDPOINT=http://localhost:8900 yarn test && EPHEMERAL_PROVIDER_ENDPOINT=http://localhost:7799 EPHEMERAL_WS_ENDPOINT=ws://localhost:7800 PROVIDER_ENDPOINT=http://localhost:8899 WS_ENDPOINT=http://localhost:8900 yarn test-web3js; cd .." | ||
|
|
||
| run_test "roll-dice + roll-dice-delegated" "cd roll-dice && anchor build && anchor deploy --provider.cluster localnet && yarn install && EPHEMERAL_PROVIDER_ENDPOINT=http://localhost:7799 EPHEMERAL_WS_ENDPOINT=ws://localhost:7800 PROVIDER_ENDPOINT=http://localhost:8899 WS_ENDPOINT=http://localhost:8900 anchor test --skip-build --skip-deploy --skip-local-validator; cd .." | ||
|
|
||
| # run_test "rust-counter" "cd rust-counter && yarn install && EPHEMERAL_PROVIDER_ENDPOINT=http://localhost:7799 EPHEMERAL_WS_ENDPOINT=ws://localhost:7800 PROVIDER_ENDPOINT=http://localhost:8899 WS_ENDPOINT=http://localhost:8900 yarn test; cd .." |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Standardize environment variable usage across test invocations.
Some tests explicitly set EPHEMERAL_PROVIDER_ENDPOINT, EPHEMERAL_WS_ENDPOINT, PROVIDER_ENDPOINT, and WS_ENDPOINT, while others rely on defaults. This inconsistency makes it unclear which tests are configured correctly and complicates maintenance.
Consider exporting these variables once at the top of the script (after validators are ready, around line 344) so all tests inherit them:
# Set environment variables for all tests
export EPHEMERAL_PROVIDER_ENDPOINT='http://localhost:7799'
export EPHEMERAL_WS_ENDPOINT='ws://localhost:7800'
export PROVIDER_ENDPOINT='http://localhost:8899'
export WS_ENDPOINT='ws://localhost:8900'Then simplify the test commands by removing the redundant inline assignments:
-run_test "oncurve-delegation" "cd oncurve-delegation && yarn install && EPHEMERAL_PROVIDER_ENDPOINT=http://localhost:7799 EPHEMERAL_WS_ENDPOINT=ws://localhost:7800 PROVIDER_ENDPOINT=http://localhost:8899 WS_ENDPOINT=http://localhost:8900 yarn test && EPHEMERAL_PROVIDER_ENDPOINT=http://localhost:7799 EPHEMERAL_WS_ENDPOINT=ws://localhost:7800 PROVIDER_ENDPOINT=http://localhost:8899 WS_ENDPOINT=http://localhost:8900 yarn test-web3js; cd .."
+run_test "oncurve-delegation" "cd oncurve-delegation && yarn install && yarn test && yarn test-web3js; cd .."Apply the same simplification to other tests that redundantly inline these variables (lines 357, 359, 361).
🤖 Prompt for AI Agents
In test-locally.sh around lines 347 to 361, the test commands inconsistently set
EPHEMERAL_PROVIDER_ENDPOINT, EPHEMERAL_WS_ENDPOINT, PROVIDER_ENDPOINT and
WS_ENDPOINT inline; export these four environment variables once just after
validators are ready (around line 344) so every test inherits them, then remove
the redundant inline variable assignments from the test invocations on the
affected lines (including the commands currently setting those vars at ~lines
357, 359, 361) and simplify the test command strings accordingly; ensure no
behavior changes other than using the exported environment variables.
Summary by CodeRabbit
Chores
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.