Skip to content

Conversation

@iinuwa
Copy link
Contributor

@iinuwa iinuwa commented Oct 28, 2025

🎟️ Tracking

Cf. recent GitHub Actions checks failing, e.g. https://github.com/bitwarden/sdk-internal/actions/runs/18876687408/job/53868218878?pr=494#step:5:68

📔 Objective

A transitive dependency of llvm-cov was published 2025-10-23 that is incompatible with our current Rust version, 1.87. This adds the --locked flag so that the tool can successfully install.

Also updates other CLI tools in this repo with locked versions.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@iinuwa iinuwa requested a review from a team as a code owner October 28, 2025 15:23
@iinuwa iinuwa requested a review from coroiu October 28, 2025 15:23
@claude
Copy link

claude bot commented Oct 28, 2025

Claude finished @iinuwa's task —— View job


Code Review Summary

This PR adds the --locked flag to cargo install commands in CI workflows to prevent transitive dependency issues. The changes are focused, minimal, and address a real CI failure caused by incompatible dependency updates.


Changes Since Last Review

New commits:

  • 3578e9cf - Lock versions of cargo CLI tools
  • f054fe73 - fix(ci): Use locked version of llvm-cov

Modified files (4 total):

  • .github/workflows/build-rust-crates.yml - Added --locked to cargo-release
  • .github/workflows/build-wasm-internal.yml - Added --locked to wasm-bindgen-cli
  • .github/workflows/publish-rust-crates.yml - Added --locked to cargo-release
  • .github/workflows/rust-test.yml - Added --locked to wasm-bindgen-cli and cargo-llvm-cov

Critical Issues

None found.


Suggested Improvements

1. ℹ️ Inconsistent application of --locked flag

The PR title mentions "Lock versions of Cargo CLI tools" and the objective states "Also updates other CLI tools in this repo with locked versions", but several cargo install commands in other workflows already had the --locked flag before this PR:

Already had --locked on main branch:

  • .github/workflows/check-powerset.yml:39 - cargo-hack
  • .github/workflows/lint.yml:60 - clippy-sarif sarif-fmt
  • .github/workflows/lint.yml:84 - cargo-sort
  • .github/workflows/lint.yml:90 - cargo-udeps
  • .github/workflows/lint.yml:96 - cargo-dylint dylint-link
  • .github/workflows/build-android.yml:49 - cross
  • .github/workflows/version-bump.yml:38 - cargo-edit

This PR only updates the 4 commands that were missing --locked. The PR description could be clearer that it's completing the existing pattern rather than introducing a new one across the repository.

2. 📝 Version pinning added to cargo-release

In addition to adding --locked, this PR also adds --version 0.25.20 to the cargo-release installations in two files:

  • build-rust-crates.yml:73
  • publish-rust-crates.yml:101

This is good for reproducibility, but:

  • The PR description doesn't explicitly mention version pinning
  • Other tools (wasm-bindgen-cli and cargo-llvm-cov) already had versions pinned
  • This is actually an improvement beyond just adding --locked
Diff showing version pinning
-        run: cargo install cargo-release
+        run: cargo install cargo-release --version 0.25.20 --locked

3. ❓ Review comment on unrelated file

There's a review comment from @addisonbeck on crates/bitwarden-crypto/src/keys/prf.rs asking "Are these tests unrelated?" However:

  • This file is not included in the PR's changed files
  • The git diff shows no changes to this file
  • This appears to be a misplaced comment, possibly from another PR

Good Practices Observed

  • Consistent application of --locked across all previously unlocked cargo install commands
  • Version pinning added to cargo-release for better reproducibility
  • Focused scope addressing a specific CI failure
  • All changes are in CI configuration, not production code

Action Items

  1. No required changes - The implementation correctly addresses the CI failure
  2. 📝 Optional: Update PR description to mention that version pinning was also added to cargo-release
  3. Clarify: Address or resolve the misplaced review comment on prf.rs

Technical Assessment

Problem diagnosis: Correct. The --locked flag ensures cargo install uses the exact dependency versions specified in the tool's Cargo.lock file, preventing failures from incompatible transitive dependencies.

Solution quality: ✅ Excellent

  • Standard Rust practice for CI environments
  • Minimal, surgical changes
  • Consistent with existing patterns in the codebase
  • Addresses root cause rather than symptoms

Security implications: ✅ Positive

  • Using --locked improves build reproducibility
  • Reduces supply chain risk by preventing unexpected dependency updates
  • Version pinning provides additional protection

Performance considerations: Neutral

  • No impact on runtime performance
  • CI installation times may be slightly affected but negligible

@github-actions
Copy link
Contributor

github-actions bot commented Oct 28, 2025

Logo
Checkmarx One – Scan Summary & Details3142d26a-1a14-4359-90fd-16af6728d61e

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Oct 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.35%. Comparing base (ac71502) to head (3578e9c).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #537      +/-   ##
==========================================
- Coverage   78.36%   78.35%   -0.01%     
==========================================
  Files         291      290       -1     
  Lines       29343    29332      -11     
==========================================
- Hits        22994    22983      -11     
  Misses       6349     6349              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@iinuwa iinuwa force-pushed the iinuwa/fix-test-coverage-ci branch from 376be2f to 8cd5202 Compare October 28, 2025 15:59
@iinuwa iinuwa requested review from a team as code owners October 28, 2025 15:59
@iinuwa iinuwa requested review from fntyler and quexten October 28, 2025 15:59
@iinuwa iinuwa changed the title fix(ci): Use locked version of llvm-cov fix(ci): Use locked version of Cargo CLI tools Oct 28, 2025
A transitive dependency of llvm-cov was published 2025-10-23 that is
incompatible with our current Rust version, 1.87. This adds the `--locked` flag
so that the tool can successfully install.
@iinuwa iinuwa force-pushed the iinuwa/fix-test-coverage-ci branch from 8cd5202 to 3578e9c Compare October 28, 2025 16:24
@quexten quexten removed request for a team and quexten October 28, 2025 17:45
@quexten
Copy link
Contributor

quexten commented Oct 28, 2025

Removing myself as reviewer as it seems to have been on accident. LMK if it was actually on purpose and I'll review.

@iinuwa iinuwa merged commit 11df05b into main Oct 30, 2025
53 checks passed
@iinuwa iinuwa deleted the iinuwa/fix-test-coverage-ci branch October 30, 2025 18:04
bw-ghapp bot pushed a commit to bitwarden/sdk-swift that referenced this pull request Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants