feat(coprocessor): add cargo-llvm-cov code coverage to Rust CI workflows by PanGan21 · Pull Request #2086 · zama-ai/fhevm

PanGan21 · 2026-03-10T16:08:41Z

Summary

Adds cargo-llvm-cov (LLVM source-based instrumentation) to coprocessor Rust CI test workflow
Coverage replaces cargo test (not in addition to it) so overhead is minimal (~10% compile time)
Coverage results are printed to the GitHub Actions job summary tab (non-blocking, no artifacts uploaded)
Posts comment on pr with test coverage of added/modified lines of code

Related: https://github.com/zama-ai/fhevm-internal/issues/1146

Eikix · 2026-03-10T16:12:12Z

@claude /pr-review

claude · 2026-03-10T16:12:40Z

Claude finished @Eikix's task in 8m 8s —— View job

[AUTOMATED]

PR Review: feat(ci): add cargo-llvm-cov code coverage to Rust CI workflows

Gather context and read changed files
Run parallel review agents (bug hunting, error handling)
Validate and deduplicate findings
Post final review

Summary

Good initiative adding LLVM source-based coverage instrumentation to the Rust CI workflows. The approach of replacing cargo test with cargo llvm-cov (rather than running both) is sensible to keep overhead low. The changes are well-structured and consistent across the three workflows. I found a few issues worth addressing:

🔴 Issue 1: Coverage thresholds are dead code — never enforced

Files: ci/coverage-thresholds.json + all 3 workflow files
Confidence: 95/100

The ci/coverage-thresholds.json file defines thresholds (60 for all components), and all three workflows reference it in their paths-filter sections. However, no workflow step ever reads, parses, or compares actual coverage against these thresholds. Coverage can silently drop to 0% and CI will still pass green.

The only effect of this file today is triggering CI re-runs when the JSON changes — which does nothing useful since the thresholds aren't enforced.

The PR description mentions this is "for future enforcement," which is fine — but I'd recommend either:

Adding a TODO comment in the workflows referencing the thresholds file, or
Implementing basic enforcement now with cargo llvm-cov --fail-under-lines <threshold>

Also, the PR description states thresholds of "40%/50%/60%" but the actual file has all three set to 60.

🟡 Issue 2: Coverage profile inherits `opt-level = 3` — may produce inaccurate coverage

File: coprocessor/fhevm-engine/Cargo.toml, lines 115-118
Confidence: 85/100

[profile.coverage]
inherits = "release"
lto = false
codegen-units = 16

This inherits from [profile.release] which has opt-level = 3. At high optimization levels, LLVM aggressively inlines functions, eliminates dead code, and merges branches — all of which distort coverage data. The cargo-llvm-cov documentation and LLVM coverage best practices recommend lower optimization levels for accurate coverage.

Consider adding opt-level = 1 (matching the existing [profile.local] pattern) or opt-level = 0:

[profile.coverage]
inherits = "release"
opt-level = 1
lto = false
codegen-units = 16

Fix this →

🟡 Issue 3: `2>/dev/null` in coverage summary silently swallows errors

Files: All 3 workflow files (coverage summary steps)
Confidence: 95/100

cargo llvm-cov report --workspace --profile coverage >> "$GITHUB_STEP_SUMMARY" 2>/dev/null || echo 'No coverage data available' >> "$GITHUB_STEP_SUMMARY"

When cargo llvm-cov report fails, the stderr is discarded entirely. The actual error could be corrupted .profraw files, LLVM version mismatches, filesystem issues, etc. — all hidden behind a generic "No coverage data available" message.

Consider redirecting stderr to a separate line in the summary or to the step log instead:

cargo llvm-cov report --workspace --profile coverage >> "$GITHUB_STEP_SUMMARY" 2>&1 || echo 'No coverage data available' >> "$GITHUB_STEP_SUMMARY"

Or capture stderr separately:

if ! cargo llvm-cov report --workspace --profile coverage >> "$GITHUB_STEP_SUMMARY" 2>/tmp/cov-err.log; then
  echo 'No coverage data available' >> "$GITHUB_STEP_SUMMARY"
  echo "<details><summary>Error details</summary>" >> "$GITHUB_STEP_SUMMARY"
  echo "" >> "$GITHUB_STEP_SUMMARY"
  cat /tmp/cov-err.log >> "$GITHUB_STEP_SUMMARY"
  echo "</details>" >> "$GITHUB_STEP_SUMMARY"
fi

🟡 Issue 4: Coprocessor coverage summary may hang due to `DATABASE_URL` with stopped DB

File: .github/workflows/coprocessor-cargo-tests.yml, lines 118-126
Confidence: 75/100

The coverage summary step runs with if: always() and sets DATABASE_URL=postgresql://.... If the PostgreSQL container or the test step itself has already failed/been torn down, the cargo llvm-cov report command might attempt to connect to the database (if workspace compilation triggers sqlx macros). Combined with 2>/dev/null, any connection timeout would be silently hidden. This is likely not an issue for a pure report command, but worth verifying.

Nit: Missing `rust-toolchain.toml` for `sdk/rust-sdk`

The PR adds components = ["llvm-tools-preview"] to coprocessor/fhevm-engine/rust-toolchain.toml and kms-connector/rust-toolchain.toml, but sdk/rust-sdk doesn't have a rust-toolchain.toml at all. This isn't a CI bug (the workflow installs the component), but local cargo llvm-cov usage in sdk/rust-sdk/ would fail without the component.

✅ What looks good

Pinned action SHAs — taiki-e/install-action@a37010ded... is properly pinned with a comment tag
Cache key change — The cargo-coverage- prefix prevents cache pollution between coverage and non-coverage builds
Non-blocking summary — Coverage is printed to job summary without failing the build, which is appropriate for initial rollout
Consistent pattern — All 3 workflows follow the same structure, making them maintainable
if: always() on summary — Good practice to show coverage data even when tests fail

…age only on coprocessor

Eikix · 2026-03-10T17:04:04Z

@claude /pr-review

claude · 2026-03-10T17:05:16Z

Claude finished @Eikix's task in 7m 25s —— View job

[AUTOMATED]

PR Review: feat(coprocessor): add cargo-llvm-cov code coverage to Rust CI workflows

Gather context and read changed files
Run parallel review agents (bug hunting, error handling)
Validate and deduplicate findings
Post final review

Summary

Clean, well-structured CI change that replaces cargo test --release with cargo llvm-cov --workspace --profile coverage in the coprocessor workflow. The author has already addressed multiple items from the earlier review (removed dead thresholds file, set opt-level = 1, scoped to coprocessor only). No bugs found. One minor suggestion below.

I ran three parallel review agents (2× bug hunter, 1× error handling auditor) and validated all findings:

Bug hunters: No real bugs found. The cargo llvm-cov invocation correctly passes workspace, profile, and test harness arguments. Environment variables are properly set. The coverage profile correctly inherits from release without changing debug_assertions behavior.
Error handling auditor: Flagged if: always() as potentially masking test failures — validated as false positive. GitHub Actions job results are only ever escalated to worse states (runner source); a succeeding cleanup step cannot undo a prior failure.

🟢 No Blocking Issues

Nit: `2>/dev/null` hides diagnostic context on report failure

File: .github/workflows/coprocessor-cargo-tests.yml, line 121

REPORT=$(DATABASE_URL=... cargo llvm-cov report --workspace --profile coverage 2>/dev/null) || true

When cargo llvm-cov report fails (e.g., corrupted .profraw files, LLVM version mismatch), the error details are discarded entirely. The user sees only the generic "No coverage data available" fallback. Since this is a non-blocking reporting step, the impact is low — but surfacing the error would help debug coverage tooling issues. Consider:

REPORT=$(DATABASE_URL=... cargo llvm-cov report --workspace --profile coverage 2>&1) || true

This way stderr content would be captured into $REPORT alongside stdout, and displayed in the summary block if present. Not blocking — the current approach is fine for initial rollout.

✅ What looks good

Aspect	Assessment
Pinned action SHAs	`taiki-e/install-action@a37010ded...` properly pinned with version comment
Cache key isolation	`cargo-coverage-` prefix prevents pollution between coverage and release builds
Coverage profile	`opt-level = 1`, `debug = 1`, `lto = false` — good balance of accuracy vs compile time
Non-blocking summary	Coverage printed to job summary without failing the build — appropriate for rollout
`if: always()` on summary	Correct pattern — shows coverage data even when tests fail, without masking failures
`rust-toolchain.toml`	`llvm-tools-preview` component enables local `cargo llvm-cov` usage too
Toolchain pinning	Changed from `stable` to `"1.91.1"` — matches `rust-toolchain.toml`, good for reproducibility

Previous Review Items — Status

The earlier review raised several issues. Here's where they stand after the author's subsequent commits:

#	Previous Issue	Status
1	Coverage thresholds are dead code	✅ Fixed — `ci/coverage-thresholds.json` removed in `af7a1fdc`
2	Coverage profile inherits `opt-level = 3`	✅ Fixed — now uses `opt-level = 1` in `af7a1fdc`
3	`2>/dev/null` swallows errors	⚪ Nit — kept as-is, acceptable for initial rollout (see above)
4	DB connection hang risk	⚪ Not a real issue — `cargo llvm-cov report` reads profiling data from disk, doesn't connect to DB

LGTM — this is ready to merge. 🚀

.github/workflows/coprocessor-cargo-tests.yml

mergify · 2026-03-10T22:01:14Z

🧪 CI Insights

Here's what we observed from your CI run for fb2e99d.

🟢 All jobs passed!

But CI Insights is watching 👀

PanGan21 · 2026-03-11T12:43:13Z

@claude /pr-review

claude · 2026-03-11T12:43:56Z

Claude finished @PanGan21's request. — View run

claude

[AUTOMATED]

Code Review — PR #2086

Summary

This PR adds cargo-llvm-cov code coverage instrumentation to the coprocessor Rust CI workflow. It replaces cargo test --release with cargo llvm-cov using a new [profile.coverage] Cargo profile, and writes coverage results to the GitHub Actions job summary.

Files reviewed:

.github/workflows/coprocessor-cargo-tests.yml — CI workflow changes
coprocessor/fhevm-engine/Cargo.toml — New [profile.coverage] section
coprocessor/fhevm-engine/rust-toolchain.toml — Added llvm-tools-preview component

What looks good

Actions are properly pinned to SHA hashes (taiki-e/install-action, dtolnay/rust-toolchain)
Cache key updated to avoid conflicts with old release-profile artifacts
The [profile.coverage] sensibly inherits from release with debug = 1 for source mapping and reduced opt-level for better instrumentation accuracy
Test step correctly does not have || true, so test failures will still fail the workflow
The if: always() on the report step is appropriate for informational output
rust-toolchain.toml change is consistent with the workflow toolchain pin

Issues found: 2 (non-blocking)

See inline comments below.

.github/workflows/coprocessor-cargo-tests.yml

claude · 2026-03-11T12:53:44Z

The review has been posted successfully. Here's a summary of what was done:

Review Complete ✅

I posted a code review on PR #2086 with:

Review Summary (COMMENT — non-blocking)

The PR is a solid CI enhancement that adds code coverage instrumentation. All actions are properly SHA-pinned, the coverage profile is well-configured, and test failures correctly propagate to fail the workflow.

2 Non-blocking Inline Comments (Nits)

Missing --workspace on cargo llvm-cov report (line 120, confidence 82/100) — The test-run step instruments all 9 workspace crates with --workspace, but the report step omits it. This could produce an incomplete coverage report. Suggested adding --workspace to the report command.
Error messages rendered as coverage data (line 117, confidence 80/100) — The 2>&1 + || true pattern means if the report command fails, the error message gets displayed inside the markdown code block as if it were valid coverage data. The fallback "No coverage data available" message will never trigger in practice. Suggested using the exit code to distinguish success from failure.

Validated Non-Issues

cargo llvm-cov clean on first run — validated as likely safe (clean commands are typically idempotent)
Profile behavioral differences (--release → --profile coverage) — no semantic changes; debug = 1 only controls debuginfo, not debug_assertions
Cache key change — intentional and correct to avoid mixing release/coverage artifacts

PanGan21 · 2026-03-12T13:15:49Z

@claude /pr-review

claude · 2026-03-12T13:16:38Z

Claude finished @PanGan21's request. — View run

claude

[AUTOMATED]

Code Review

✅ No issues found.

Reviewed for bugs, security issues, guideline compliance, and error handling across all 3 changed files:

.github/workflows/coprocessor-cargo-tests.yml — CI workflow additions for cargo-llvm-cov coverage
coprocessor/fhevm-engine/Cargo.toml — new [profile.coverage] section
coprocessor/fhevm-engine/rust-toolchain.toml — llvm-tools-preview component added

Summary

This PR cleanly integrates LLVM-based code coverage into the coprocessor CI workflow. Key observations:

Workflow structure is sound: Steps are correctly ordered — clean → test → report → export → diff-cover → comment
Error handling is appropriate for CI: Non-blocking steps use || true and if: !cancelled() consistently, with proper fallback messages when coverage data is unavailable
Actions are pinned by SHA: Both taiki-e/install-action and marocchino/sticky-pull-request-comment follow the repository's established pattern of SHA-pinned action references with version comments
Cargo profile is well-configured: The [profile.coverage] inherits from release with debug = 1 added for source mapping — appropriate for instrumented coverage builds
Permissions are minimal: Only pull-requests: 'write' was added, which is the minimum required for the sticky PR comment action
Cache key updated: The cache key change (cargo → cargo-coverage) correctly avoids poisoning the existing cache with coverage-instrumented artifacts

Nit: The workflow specifies toolchain: "1.91.1" explicitly while rust-toolchain.toml already declares the same version — the action parameter is redundant since rustup respects the local toolchain file, but this is consistent with how coprocessor-cargo-clippy.yml and coprocessor-cargo-fmt.yml handle it, so no change needed.

LGTM 👍

claude · 2026-03-12T13:22:10Z

The review has been posted. Here's a summary of what was done:

PR #2086 Review Complete — No issues found.

Four specialized review agents (2× bug hunting with opus, guideline compliance, error handling audit) analyzed the changes across 3 files. All agents confirmed the PR is clean:

No bugs or security issues in the workflow YAML, shell scripts, or Cargo configuration
Error handling is appropriate for CI context — proper use of || true, !cancelled(), and fallback messages
Actions follow SHA-pinning conventions established in the repo
One minor observation (below reporting threshold): the explicit toolchain: "1.91.1" in the workflow is technically redundant with rust-toolchain.toml, but it's consistent with other workflows in the repo

The review was posted as an approving comment on the PR.

github-actions · 2026-03-12T15:07:35Z

Changed Lines Coverage

Coverage of added/modified lines: N/A

Per-file breakdown

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

.github/workflows/coprocessor-cargo-tests.yml

…olchain.toml

.github/workflows/coprocessor-cargo-tests.yml

Eikix

gg!

PanGan21 · 2026-03-13T10:32:53Z

@Mergifyio queue

mergify · 2026-03-13T10:32:58Z

feat(ci): add llvm-cov in ci

82905b0

PanGan21 requested review from a team and dartdart26 as code owners March 10, 2026 16:08

cla-bot bot added the cla-signed label Mar 10, 2026

feat(ci): set coverage threshold to 60

10ae4e8

PanGan21 changed the title ~~feat(ci): add cargo-llvm-cov code coverage to Rust CI workflows~~ feat(common): add cargo-llvm-cov code coverage to Rust CI workflows Mar 10, 2026

PanGan21 removed request for a team March 10, 2026 16:36

PanGan21 changed the title ~~feat(common): add cargo-llvm-cov code coverage to Rust CI workflows~~ feat(coprocessor): add cargo-llvm-cov code coverage to Rust CI workflows Mar 10, 2026

fix(common): remove coverage thresholds, use opt level 1, apply cover…

af7a1fd

…age only on coprocessor

PanGan21 force-pushed the panos/add-coverage-tool branch from 1803cc8 to af7a1fd Compare March 10, 2026 16:38

fix(coprocessor): lint workflow

2fdb594

PanGan21 commented Mar 10, 2026

View reviewed changes

.github/workflows/coprocessor-cargo-tests.yml Show resolved Hide resolved

fix(ci): capture cargo-llvm-cov report output by redirecting stderr

4ea96e1

PanGan21 force-pushed the panos/add-coverage-tool branch from 1422c9c to 4ea96e1 Compare March 10, 2026 22:06

fix(ci): fix warning

38dc077

claude bot reviewed Mar 11, 2026

View reviewed changes

.github/workflows/coprocessor-cargo-tests.yml Show resolved Hide resolved

claude bot reviewed Mar 11, 2026

View reviewed changes

.github/workflows/coprocessor-cargo-tests.yml Show resolved Hide resolved

Merge branch 'main' into panos/add-coverage-tool

ced9648

claude bot reviewed Mar 12, 2026

View reviewed changes

rudy-6-4 reviewed Mar 12, 2026

View reviewed changes

.github/workflows/coprocessor-cargo-tests.yml Outdated Show resolved Hide resolved

rudy-6-4 reviewed Mar 12, 2026

View reviewed changes

.github/workflows/coprocessor-cargo-tests.yml Show resolved Hide resolved

rudy-6-4 previously approved these changes Mar 12, 2026

View reviewed changes

fix(coprocessor): use rust-toolchain-file action to read from rust-to…

0b6066b

…olchain.toml

PanGan21 dismissed rudy-6-4’s stale review via 0b6066b March 12, 2026 19:00

PanGan21 and others added 2 commits March 12, 2026 20:00

Merge branch 'main' into panos/add-coverage-tool

474c100

fix(coprocessor): copy rust-toolchain.toml to root

074f211

Eikix reviewed Mar 13, 2026

View reviewed changes

.github/workflows/coprocessor-cargo-tests.yml Outdated Show resolved Hide resolved

Eikix reviewed Mar 13, 2026

View reviewed changes

.github/workflows/coprocessor-cargo-tests.yml Show resolved Hide resolved

PanGan21 and others added 2 commits March 13, 2026 10:40

ci(coprocessor): handle empty-diff coverage and fork PR comment posting

ba38329

Merge branch 'main' into panos/add-coverage-tool

fb2e99d

Eikix approved these changes Mar 13, 2026

View reviewed changes

mergify bot added merge-queued queued and removed merge-queued labels Mar 13, 2026

mergify bot added a commit that referenced this pull request Mar 13, 2026

Merge of #2086

cb4063c

This was referenced Mar 13, 2026

merge queue: embarking main (fd3ef74) and [#2086 + #2107] together #2110

Closed

fix(host-contracts): bump ACL reinitializer version #2107

Merged

mergify bot merged commit 6a670a1 into main Mar 13, 2026
63 checks passed

mergify bot deleted the panos/add-coverage-tool branch March 13, 2026 14:34

mergify bot removed the queued label Mar 13, 2026

PanGan21 mentioned this pull request Mar 17, 2026

refactor(coprocessor): enable multi threaded test execution in CI #2121

Merged

Conversation

PanGan21 commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Eikix commented Mar 10, 2026

Uh oh!

claude bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: feat(ci): add cargo-llvm-cov code coverage to Rust CI workflows

Summary

🔴 Issue 1: Coverage thresholds are dead code — never enforced

🟡 Issue 2: Coverage profile inherits opt-level = 3 — may produce inaccurate coverage

🟡 Issue 3: 2>/dev/null in coverage summary silently swallows errors

🟡 Issue 4: Coprocessor coverage summary may hang due to DATABASE_URL with stopped DB

Nit: Missing rust-toolchain.toml for sdk/rust-sdk

✅ What looks good

Uh oh!

Eikix commented Mar 10, 2026

Uh oh!

claude bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: feat(coprocessor): add cargo-llvm-cov code coverage to Rust CI workflows

Summary

🟢 No Blocking Issues

Nit: 2>/dev/null hides diagnostic context on report failure

✅ What looks good

Previous Review Items — Status

Uh oh!

Uh oh!

mergify bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧪 CI Insights

🟢 All jobs passed!

Uh oh!

PanGan21 commented Mar 11, 2026

Uh oh!

claude bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Code Review — PR #2086

Summary

What looks good

Issues found: 2 (non-blocking)

Uh oh!

Uh oh!

Uh oh!

claude bot commented Mar 11, 2026

Review Complete ✅

Review Summary (COMMENT — non-blocking)

2 Non-blocking Inline Comments (Nits)

Validated Non-Issues

Uh oh!

PanGan21 commented Mar 12, 2026

Uh oh!

claude bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Code Review

Summary

Uh oh!

claude bot commented Mar 12, 2026

Uh oh!

github-actions bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changed Lines Coverage

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Eikix left a comment

Choose a reason for hiding this comment

Uh oh!

PanGan21 commented Mar 10, 2026 •

edited

Loading

claude bot commented Mar 10, 2026 •

edited

Loading

🟡 Issue 2: Coverage profile inherits `opt-level = 3` — may produce inaccurate coverage

🟡 Issue 3: `2>/dev/null` in coverage summary silently swallows errors

🟡 Issue 4: Coprocessor coverage summary may hang due to `DATABASE_URL` with stopped DB

Nit: Missing `rust-toolchain.toml` for `sdk/rust-sdk`

claude bot commented Mar 10, 2026 •

edited

Loading

Nit: `2>/dev/null` hides diagnostic context on report failure

mergify bot commented Mar 10, 2026 •

edited

Loading

claude bot commented Mar 11, 2026 •

edited

Loading

claude bot commented Mar 12, 2026 •

edited

Loading

github-actions bot commented Mar 12, 2026 •

edited

Loading

mergify bot commented Mar 13, 2026 •

edited

Loading