ci: scoped Rust test fast path for wallet-only PRs#4003
Merged
Conversation
Same-repo PRs that touch only the platform-wallet crates (rs-platform-wallet, rs-platform-wallet-ffi, rs-platform-wallet-storage; Swift sources ignored) now skip the full Rust workspace job and run a scoped tests-rs-wallet.yml workflow instead: fmt, machete, immutable-structure check, clippy over the wallet crates plus their sole dependent rs-unified-sdk-ffi, nextest over the wallet crates, and wallet-scoped doctests when doc examples changed. Safety rails baked in after review: - fast path restricted to same-repo pull requests (fork PRs and push/schedule runs always take the full path, keeping the Ubuntu backup machinery and nightly full runs intact) - scope diff uses --no-renames so a file moved out of another crate still counts against that crate; any diff failure falls back to full - a cargo-metadata closure guard fails the job if any new workspace crate starts depending on the wallet crates - timeout matches the full job (cold builds still compile ~77% of the workspace dependency graph) Also adds the missing platform-wallet-storage entries to the rs-packages filter files (previously a storage-only PR matched no filter and ran zero Rust tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR introduces a wallet-only Rust CI fast path. It adds package filters for a new ChangesWallet-scoped CI fast path
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant TestsYml as tests.yml
participant ScopeStep as rs-scope step
participant WalletWF as tests-rs-wallet.yml
participant WorkspaceWF as tests-rs-workspace.yml
participant ClosureScript as check-wallet-closure.py
PR->>TestsYml: trigger CI
TestsYml->>ScopeStep: compute rs-scope
ScopeStep->>ScopeStep: inspect diff (fork/non-PR -> full)
alt scope == wallet
ScopeStep-->>TestsYml: scope=wallet
TestsYml->>WalletWF: run rs-wallet-tests
WalletWF->>ClosureScript: verify closure invariant
ClosureScript-->>WalletWF: OK / fail
WalletWF-->>TestsYml: job result
else scope == full
ScopeStep-->>TestsYml: scope=full
TestsYml->>WorkspaceWF: run rs-workspace-tests
WorkspaceWF->>ClosureScript: verify closure invariant
ClosureScript-->>WorkspaceWF: OK / fail
WorkspaceWF-->>TestsYml: job result
end
TestsYml->>TestsYml: gate swift-sdk-build on Rust job results
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 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 complete (commit 5910d47) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4003 +/- ##
============================================
+ Coverage 87.18% 87.19% +0.01%
============================================
Files 2632 2636 +4
Lines 327563 327822 +259
============================================
+ Hits 285593 285852 +259
Misses 41970 41970
🚀 New features to boost your workflow:
|
thepastaclaw
reviewed
Jul 4, 2026
Collaborator
There was a problem hiding this comment.
Code Review
The CI fast path is well-designed and mostly correct. Two in-scope suggestions worth noting: the reverse-dependency closure guard cannot catch the PR that introduces a violation (it only runs on the wallet fast path, but any new non-wallet dependent forces scope=full and skips the guard), and the swift-sdk-changed filter was not extended to include the newly promoted platform-wallet-storage crate.
🟡 2 suggestion(s)
Source: reviewers claude/opus and codex/gpt-5.5; verifier claude/opus.
🤖 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.yml`:
- [SUGGESTION] .github/workflows/tests.yml:316-324: Closure guard cannot catch the PR that adds a new wallet dependent
The reverse-dependency closure guard lives in `tests-rs-wallet.yml`, which only runs when `rs-scope == 'wallet'`. The scope classifier in tests.yml (lines 155-168) marks any non-wallet path as `non_wallet=true` and forces `scope=full`. A PR that introduces a new dependency from a non-wallet crate on `platform-wallet`, `platform-wallet-ffi`, or `platform-wallet-storage` necessarily edits a non-wallet `Cargo.toml`, so `rs-wallet-tests` is skipped and the guard never runs on the offending PR. The invariant is only enforced later, on an unrelated wallet-only PR that happens to trigger the fast path — by which point the merge that violated the invariant is already in. Run the closure check from the full Rust path as well (or as a separate lightweight job gated on Rust package metadata changes) so the invariant fails at the point it is broken, not on the next wallet-only contributor.
- [SUGGESTION] .github/workflows/tests.yml:209-210: swift-sdk-changed filter missing platform-wallet-storage
This PR promotes `platform-wallet-storage` to a first-class wallet crate in `.github/package-filters/*.yml`, in the scoped `--package` lists in `tests-rs-wallet.yml`, and in the closure guard's `wallet = {...}` set. The inline `swift-sdk-changed` filter here still lists only `packages/rs-platform-wallet/**` and `packages/rs-platform-wallet-ffi/**`. Today no crate on the Swift SDK compile path depends on `platform-wallet-storage`, so skipping `swift-sdk-build` for a storage-only PR is currently correct. But as soon as `platform-wallet` or `platform-wallet-ffi` starts depending on `platform-wallet-storage` (its implied purpose), a storage-only PR will be classified `scope=wallet`, run `rs-wallet-tests`, and silently skip `swift-sdk-build` even though the Swift SDK compile could break. Add `packages/rs-platform-wallet-storage/**` to this filter now to keep the invariant aligned with the storage-crate promotion; the closure guard checks workspace dependents but does not cross-check this filter.
Review follow-ups from thepastaclaw on #4003: - The reverse-dependency closure guard only ran on the wallet fast path, but a PR introducing a new wallet dependent always edits a non-wallet Cargo.toml and takes the full path — so the guard could never fire on the offending PR. Extract it to .github/scripts/check-wallet-closure.py and run it from both tests-rs-workspace.yml (catches the violating PR) and tests-rs-wallet.yml (protects against a stale scoped list). - Add packages/rs-platform-wallet-storage/** to the swift-sdk-changed filter, keeping it aligned with the storage crate's promotion in the rs-packages filter files. - Add the closure script to the rs-workflows trigger list so edits to it are exercised by CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
approved these changes
Jul 4, 2026
Collaborator
There was a problem hiding this comment.
Code Review
The latest push resolves both prior suggestions. The wallet reverse-dependency closure check was extracted to .github/scripts/check-wallet-closure.py and is now invoked from tests-rs-workspace.yml (where new non-wallet dependents actually go, fixing the guard's blind spot) as well as tests-rs-wallet.yml (as a stale-scoped-list guard). The swift-sdk-changed filter in tests.yml now includes packages/rs-platform-wallet-storage/**, and the rs-workflows filter covers the new script. No carried-forward prior findings and no new latest-delta findings.
Source: reviewers: claude-general (opus), codex-general (gpt-5.5); verifier: claude (opus).
bezibalazs
pushed a commit
that referenced
this pull request
Jul 5, 2026
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
bezibalazs
added a commit
that referenced
this pull request
Jul 5, 2026
… clippy lints The scoped Rust test fast path (#4003) enforces that the wallet crates' reverse-dependency closure matches an allowlist; rs-unified-sdk-jni is a new dependent. Add it to EXPECTED_DEPENDENTS and the scoped clippy package list, and clean up the lints the -D warnings gate surfaces in the crate (ptr::slice_from_raw_parts_mut casts, repeat_n, boxed_local on the sign-completion helper, two doc-comment rewraps). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
A PR touching only the platform-wallet crates (e.g. #3985) triggers the full Rust CI:
clippy --workspace, coverage-instrumented tests over ~22 packages, and the whole-workspace lint battery — even though the wallet crates are a near-leaf of the workspace graph. Nothing outsiders-unified-sdk-ffidepends on them, so a wallet-only change provably cannot break any other crate, yet every wallet PR pays the full-workspace wall-clock.Separately, the
rs-packagesfilter files had no entry forpackages/rs-platform-wallet-storage/**at all — a storage-only PR matched no filter, leftrs-packages == '[]', and ran zero Rust tests.What was done?
tests-rs-wallet.yml(reusable, called fromtests.yml): on the same self-hosted macOS runner it runscargo fmt --check --all,cargo machete, the immutable-structure check, scoped clippy overplatform-wallet+platform-wallet-storage+platform-wallet-ffi+rs-unified-sdk-ffi(the last preserves the downstream-compile checkclippy --workspaceprovided),cargo nextest runover the three wallet crates with the same-E 'not test(~shield)'filter the full job uses for them, and wallet-scoped doctests when doc examples changed. Coverage upload is intentionally omitted (codecov never gates the merge).tests.yml'schangesjob setsrs-scope=walletonly when all of the following hold, otherwisefull:UBUNTU_BACKUP_ENABLEDis set), and push/schedule runs are never downgraded (they have no reliable base SHA; the nightly full run stays full);packages/swift-sdk/**is ignored — not part of the Rust workspace); anything else, includingCargo.lock, forces the full run;--no-renamesso a file moved out of another crate still counts as a change to that crate; any diff failure falls back tofull.cargo metadatastep in the wallet workflow fails if any workspace crate other thanrs-unified-sdk-ffistarts depending (transitively) on the wallet crates, so the scoped--packagelists can't silently go stale. Verified locally: passes today, and correctly fails (listing the new dependents) when a dependency edge is injected.rs-workspace-testsandrs-wallet-testsare mutually exclusive viars-scope;swift-sdk-buildnow gates on whichever one actually ran (skipped ≠ failure).platform-wallet-storageentries tors-packages-no-workflows.yml(and its sibling filter files), fixing the pre-existing zero-tests gap for storage-only PRs.An adversarial multi-angle review of the first draft surfaced (and this version fixes): fork-PR/zero-CI green runs, nightly/push downgrade via last-commit-only scoping, the git rename-detection hole, missing doctest coverage, an unenforced closure invariant, and a too-tight timeout for cold builds (~77% of the workspace dependency graph, including the orchard/halo2 stack, still compiles in the scoped job).
Known trade-offs (documented in the workflow header): no Ubuntu backup jobs in the wallet workflow (if
UBUNTU_BACKUP_ENABLED— currentlyfalse— is turned on, wallet PRs still depend on the mac runner being online), and scoped-pbuilds feature-unify shared deps differently than--workspacebuilds, so the sharedtarget/carries an extra artifact flavor.How Has This Been Tested?
cargo metadata: positive case passes; a doctored metadata (fakedash-sdk → platform-walletedge) correctly fails with the expected message.schedule→ full, fork PR → full, same-repo PR with unfetchable base SHA → full (fail-closed), same-repo PR with a real non-wallet diff → full.Cargo.lock→ full; Swift-only → full.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit