ci: unblock v4.1-dev pipelines (Swift build, Docker images, contract tests, coverage cleanup)#4205
Conversation
The Swift SDK CI build compiles SwiftTests with warnings escalated to errors, and the integration-test support wallet still called the broadcastTransaction compatibility wrapper that #4198 deprecated, so any PR triggering the Swift workflow now fails to build. Switch TestWallet to broadcastTransactionWithOutcome and surface rejected/unknown outcomes as the same errors the wrapper raised. The base branch never notices because the Swift job is path-filtered and rs-only pushes skip it. Also make the coverage-artifact cleanup retry loop break only when the removal succeeded and target/llvm-cov-target is actually absent, so a concurrently recreated directory is retried instead of silently kept. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe macOS workflow now verifies LLVM coverage artifacts were removed. Swift integration tests now inspect transaction broadcast outcomes and return the transaction ID or throw outcome-specific errors. ChangesmacOS workflow cleanup
Swift transaction broadcasting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/tests-rs-workspace.yml (1)
239-249: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the same existence check after exhausting retries.
The retry condition checks whether the path is absent with
! -e, but the final warning checks only-d. If the target is recreated as a file or symlink, cleanup can finish without warning even thoughtarget/llvm-cov-targetstill exists. Use-econsistently for the final verification (and the outer guard if non-directory artifacts are possible).Proposed fix
- if [ -d target/llvm-cov-target ]; then + if [ -e target/llvm-cov-target ]; then echo "::warning::Coverage artifacts could not be fully removed" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/tests-rs-workspace.yml around lines 239 - 249, Update the cleanup verification around the target/llvm-cov-target retry loop to use the same existence semantics throughout: replace the final directory-only check with an existence check, and update the outer guard to -e as well if files or symlinks may be present. Preserve the retry behavior and warning messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/tests-rs-workspace.yml:
- Around line 239-249: Update the cleanup verification around the
target/llvm-cov-target retry loop to use the same existence semantics
throughout: replace the final directory-only check with an existence check, and
update the outer guard to -e as well if files or symlinks may be present.
Preserve the retry behavior and warning messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68af796a-dd7a-4e41-bbfb-40e9b33452cd
📒 Files selected for processing (2)
.github/workflows/tests-rs-workspace.ymlpackages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Support/TestWallet.swift
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4205 +/- ##
=========================================
Coverage 87.51% 87.51%
=========================================
Files 2667 2667
Lines 336972 336994 +22
=========================================
+ Hits 294903 294924 +21
- Misses 42069 42070 +1
🚀 New features to boost your workflow:
|
Three more v4.1-dev breakages that only surface on full (dispatch or version-change) CI runs: - Docker image builds fail at cargo-chef prepare: #4171 made document-history-contract a dependency of data-contracts but the Dockerfile never copies the new crate. Add it everywhere token-history-contract appears. This also broke Test Suite and Dashmate E2E, which need the images. - keyword-search-contract unit tests were copy-pasted from wallet-utils-contract and still tested its txMetadata document type, which this contract does not define; every case threw. Rewrite the suite against the real schema (contractKeywords, shortDescription, fullDescription). Two behaviors surfaced while doing so are asserted as such: wrong-length identifier fields throw at document creation (identifier conversion precedes schema validation), and fullDescription's schema maxLength (10000) sits above the system per-field cap (5120), so overlength strings yield a field-size error, not a maxLength error. - document-history-contract asserted a minimum on purchase.price, but the schema deliberately leaves the integer unbounded so the property stays sum-aggregatable; consensus validates actual amounts. Assert the type check and the intentional absence of a schema minimum instead. These suites were never exercised in CI before #4203 added the missing package filters, and the contract-test jobs only run on full pipelines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
changed the title
…f ts-node The ts-node ESM loader is broken under the pinned Node 24 + Yarn PnP combination: its vendored module resolver walks node_modules directories that PnP never creates, so mocha dies with "Cannot find package 'ts-node'" before running a single test. The previous loader-invocation fix (8cb06a9) shipped without its Tests job running (path filters), so the breakage was never caught; the job now fails on every PR that triggers it. Node 24 strips type annotations natively, so mocha can consume the .ts specs directly — no loader, no ts-node env vars. 212 mocha tests and the karma browser suite pass locally under Node 24 + PnP. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Issue being fixed or feature implemented
Several
v4.1-devbreakages that CI's path filters and PR-gating hide from ordinary pushes, but which fail any PR or full-pipeline run that exercises them:SwiftTestswith warnings escalated to errors, andTestWallet.swiftstill called thebroadcastTransactionwrapper deprecated by fix(wallet): trustless SPV broadcast acceptance with authoritative outcomes #4198. Base pushes skip the job (path filter), so only PRs see it — currently blocking fix(drive): strengthen execution proof result validation #4155 and fix(sdk): fail closed without proof verification #4168's dispatched run.cargo chef prepare:failed to load manifest for dependency document-history-contract): feat(platform)!: document history system contract with per-doctype opt-in #4171 made the new crate a dependency ofdata-contracts, but the Dockerfile never copies it. This also takes down Test Suite and Dashmate E2E on full runs, which need the images. Both of the latestv4.1-devbranch runs fail on exactly this.txMetadatadocument type this contract doesn't define — all nine cases threw raw WASM errors. The suite never ran in CI until ci: cover rs-unified-sdk-jni and missing contract crates in package filters #4203 added the missing package filters.minimumonpurchase.pricethat the schema deliberately omits (the property stays an unbounded, sum-aggregatable i64; consensus validates actual amounts — per the schema's own description).rm -rfexit status alone (CodeRabbit finding on fix(drive): strengthen execution proof result validation #4155; the file isn't part of that PR's diff, so the fix lands here).What was done?
TestWallet.send()usesbroadcastTransactionWithOutcome(_:), mapping rejected/unknown outcomes to the same errors the deprecated wrapper raised.document-history-contractadded to every Dockerfile package-copy block, mirroringtoken-history-contract.contractKeywords,shortDescription,fullDescription). Two platform behaviors surfaced and are asserted as such: wrong-length identifier fields throw at document creation (identifier conversion precedes schema validation), andfullDescription's schemamaxLength(10000) sits above the system per-field cap (5120), so overlength strings yield a field-size error.How Has This Been Tested?
keyword-search-contract: 20 passing locally against freshly built wasm-dpp; document-history-contract: 19 passing; both lints clean. TestWallet change mirrors the deprecated wrapper's own switch verbatim; the Swift CI job on this PR is the compile validation. Dockerfile/workflow changes are validated by this PR's image builds.
Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code