Uh oh!
There was an error while loading. Please reload this page.
ci: skip shielded Rust tests on PRs without shielded changes - #4293
Merged
Conversation
The shielded test phase (cargo llvm-cov test -p dpp -p drive -p drive-abci -p dash-sdk -- shield) costs ~4.5 minutes of the ~13-minute warm-runner Rust workspace job and runs on every PR that touches any Rust crate, shielded-related or not. The changes job now classifies a PR as shielded-relevant if a changed path or changed .rs diff line mentions shield/orchard/halo2, or if build configuration changed (any Cargo.toml, Cargo.lock, rust-toolchain.toml, the rust setup action, or the Rust test workflows). The workspace job splits its coverage report into lcov-nonshielded.info and lcov-shielded.info (via cargo llvm-cov clean --profraw-only between phases), caches the shielded half with actions/cache, and on shielded-irrelevant PRs restores it and skips the shielded run entirely; codecov merges the two uploaded files so combined coverage is unchanged. A cache miss falls back to running the suite, so the mechanism self-heals. The detection is a heuristic: shielded tests exercise shared consensus code, so a shared-code change could in principle break them while classified unrelated. Push, nightly, and workflow_dispatch runs always execute the shielded suite (and refresh the cache), so an escape is caught minutes after merge and bisects to one PR. Replayed over the last 30 merges to v4.2-dev, about half would take the fast path and all shielded-adjacent PRs were correctly flagged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe workflows detect shielded-related changes, pass the result to Rust workspace tests, reuse unchanged shielded coverage, generate separate LCOV reports, cache successful shielded coverage, and upload both reports. ChangesShielded coverage control
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ChangesJob
participant RustWorkspaceTests
participant CoverageCache
participant Codecov
ChangesJob->>RustWorkspaceTests: Pass shielded-changed
RustWorkspaceTests->>CoverageCache: Restore shielded LCOV when inputs are unchanged
RustWorkspaceTests->>RustWorkspaceTests: Run required non-shielded and shielded coverage tests
RustWorkspaceTests->>CoverageCache: Cache shielded LCOV after successful shielded tests
RustWorkspaceTests->>Codecov: Upload non-shielded and shielded LCOV
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Collaborator
🔍 Review in progress — actively reviewing now (commit 8b77178) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tests-rs-workspace.yml:
- Around line 238-246: Update the shielded coverage flow around the “Restore
shielded coverage from cache” step so prefix-matched cached LCOV is not restored
for an unverified source tree. Require validation against a shielded
dependency-closure or source-content hash before reuse; when validation is
unavailable or fails, prevent the cache path from supplying coverage and ensure
the shielded test suite runs instead. Preserve cache reuse only for verified
matching trees.
In @.github/workflows/tests.yml:
- Around line 277-278: Update the Rust content-diff condition in the workflow so
any failure in the pipeline, including the final grep non-match or command
error, is treated as shielded-relevant rather than setting
shielded-changed=false. Preserve the existing matching behavior for Rust diffs
while ensuring the conditional handles pipeline failures with the documented
fail-safe outcome.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8714535a-c068-4683-8151-81325679f083
📒 Files selected for processing (2)
.github/workflows/tests-rs-workspace.yml.github/workflows/tests.yml
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## v4.2-dev #4293 +/- ##
============================================
+ Coverage 87.55% 87.60% +0.05%
============================================
Files 2700 2703 +3 Lines 343157 344990 +1833 ============================================
+ Hits 300464 302242 +1778 - Misses 42693 42748 +55
🚀 New features to boost your workflow:
|
Address review feedback on the shielded skip path: - Key the shielded coverage cache by a content hash over every tracked source file mentioning the shielded keywords (plus Cargo.lock and rust-toolchain.toml) instead of commit sha with prefix restore. An exact-key hit now proves the cached lcov was produced from byte-identical shielded sources, so a PR can never reuse coverage from a tree whose shielded code differs from its own. - Capture the content diff to a file in the detector so a git failure is distinguishable from "no keyword match" — previously the final grep's non-match status would have masked it and failed open to skipping the suite. - Extend content detection and the hash set to .proto files: shielded gRPC message changes regenerate into dapi-grpc and must run the suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Aug 5, 2026
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: none. New latest-delta findings: two blocking cache-reuse defects remain—the fast path can suppress the post-merge shielded test run, and the content key omits sources and build inputs that the detector classifies as shielded-relevant.
Source: reviewers codex/general=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/tests-rs-workspace.yml`:
- [BLOCKING] .github/workflows/tests-rs-workspace.yml:357-359: Do not mark cache-restored coverage as a fully tested tree
This writes `HEAD^{tree}` after both fresh shielded coverage and cache-restored shielded coverage. A PR that takes the fast path therefore marks its merge tree as fully reusable even though the shielded suite did not run. If the subsequent post-merge push has the same tree—which is common because the PR merge ref and merged commit can differ only in history—and is assigned to the same persistent runner, the initial coverage check finds the marker and both LCOV files and skips the entire test phase. That defeats the documented non-PR safety net for shared-code changes that the heuristic misses. Record the full-tree marker only after the shielded suite actually succeeds.
- [BLOCKING] .github/workflows/tests-rs-workspace.yml:245-249: Hash every input that the detector treats as shielded-relevant
The detector forces shielded tests for keyword-bearing paths and changes to any Cargo manifest, the Rust setup action, or the Rust test workflows, but this cache key hashes only `.rs` and `.proto` files whose contents contain a keyword, plus the root `Cargo.lock` and `rust-toolchain.toml`. Several Rust files under shielded paths are consequently omitted; for example, `packages/rs-dpp/src/errors/consensus/state/shielded/invalid_anchor_error.rs` contains none of the three keywords. Editing such a file without introducing a keyword, or changing a `Cargo.toml` feature without changing `Cargo.lock`, leaves the key unchanged. Although that change's run executes the suite, the immutable Actions cache entry cannot be replaced, so later unrelated PRs restore and upload shielded coverage generated from the pre-change source or build configuration. Build the key from the union of path matches, content matches, and every build input recognized by the detector.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Two blocking review findings on the shielded fast path: - Write the coverage tree-hash marker only when the shielded suite actually ran and succeeded. A fast-path run (cache-restored shielded coverage) previously wrote it too, and since the post-merge push commonly has the same tree as the PR merge ref, the marker could let the push run on the same runner skip the entire test phase — defeating the safety net for shared-code changes the detector heuristic misses. - Build the cache key from the union of everything the detector treats as shielded-relevant: keyword-matching paths (shielded module files such as error types under shielded/ directories don't all mention a keyword in their content), keyword-matching .rs/.proto contents, every Cargo manifest and lockfile, rust-toolchain.toml, the rust setup action, and the Rust test workflows. Previously a shielded-relevant edit that left the key unchanged could leave a stale immutable cache entry reachable under a current key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
commented
Aug 5, 2026
Collaborator
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Uh oh!
There was an error while loading. Please reload this page.
6 tasks
QuantumExplorer added a commit
that referenced
this pull request
Aug 5, 2026
Excluding the chain-simulation suite on PRs made every PR report a spurious ~0.57% project-coverage drop (drive-abci -1.33%): base coverage comes from full-suite push runs while PRs upload gated coverage. Split coverage into three codecov flags — rust (always uploaded), rust-strategy (chain simulations, push/nightly only), and rust-shielded (shielded phase, when it runs) — with carryforward enabled, so codecov reuses the base commit's coverage for any flag a PR run doesn't upload and PR reports stay comparable. The chain simulations now run as their own phase (push/nightly only) with their own lcov, and the nextest phase uses the same gated filter on every event so the rust flag measures the same population everywhere. Carryforward also replaces the shielded coverage cache from #4293 entirely — instead of restoring a content-verified cached lcov on skip runs, the flag is simply not uploaded and codecov carries the base's forward, which deletes the content-hash, cache-restore, and cache-save steps. The complete-suite tree-hash marker now additionally requires the chain-simulation phase to have succeeded, so a PR fast-path run cannot mark its tree as fully tested for a same-tree post-merge push run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6 tasks
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
The shielded test phase of the Rust workspace job (
cargo llvm-cov test -p dpp -p drive -p drive-abci -p dash-sdk -- shield) costs ~4.5 minutes of the ~13-minute warm-runner job — about as long as the entire non-shielded nextest phase — and runs on every PR that touches any Rust crate, even ones completely unrelated to shielded functionality. The Swift SDK build queues behind this job, so the cost compounds downstream.What was done?
Detection — a new
Check for shielded-relevant changesstep in thechangesjob oftests.ymlclassifies a PR as shielded-relevant if:shield/orchard/halo2(all shielded modules live in*shield*directories), or.rsfile mentions those keywords (catches edits to shielded match arms, error variants, and test names in shared files outside the shielded directories), orCargo.toml,Cargo.lock,rust-toolchain.toml, the rust setup action, or the Rust test workflows.Coverage split and cache —
tests-rs-workspace.ymlnow reports the non-shielded phase tolcov-nonshielded.info, drops its profraw files withcargo llvm-cov clean --profraw-only, and reports the shielded phase separately tolcov-shielded.info, which is saved toactions/cache(keyrs-shielded-lcov-v1-<sha>, prefix restore). A PR with no shielded-relevant changes restores the most recent shielded lcov — in practice the one saved by the last post-merge push run on the base branch — and skips the shielded phase entirely. Both files are uploaded together and codecov merges them, so combined PR coverage is unchanged. A cache miss simply falls back to running the suite, so the mechanism self-heals with no bootstrap step.Safety net — the detection is a heuristic, not a dependency proof: shielded tests exercise shared consensus code, so a shared-code-only change could in principle break them while being classified unrelated. Push-to-dev, nightly, and
workflow_dispatchruns always execute the shielded suite (and refresh the cache), so an escape is caught minutes after merge and bisects to exactly one PR. Codecov never gates merges here (fail_ci_if_error: false), so coverage staleness on the skip path is cosmetic only.One subtlety worth noting for reviewers: the detector deliberately uses
set -euwithoutpipefail— the detection pipelines end ingrep -q, which exits on first match and SIGPIPEs the upstreamgit diff; underpipefaila match would read as pipeline failure and silently flip the answer to "unchanged".How Has This Been Tested?
v4.2-dev: about half would take the fast path; every shielded-adjacent PR (the shielded denominations protocol change, CheckTx verification bounds touching shielded state-transition dirs) and every build-config change was correctly flagged to run.cargo llvm-cov clean --profraw-onlyexists in the pinned cargo-llvm-cov (0.8.4).Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit