Skip to content

Fix mutation diff source mismatch#75

Merged
msureshkumar88 merged 1 commit intomainfrom
fix/mutation-diff-head
May 1, 2026
Merged

Fix mutation diff source mismatch#75
msureshkumar88 merged 1 commit intomainfrom
fix/mutation-diff-head

Conversation

@lucarlig
Copy link
Copy Markdown
Collaborator

@lucarlig lucarlig commented May 1, 2026

Summary

  • Generate the cargo-mutants incremental diff from the PR base SHA to the checked-out tree instead of the PR head SHA.
  • Remove the unused HEAD_SHA environment value from the mutation diff step.
  • Update the workflow catalog test to lock this behavior.

Root cause

cargo mutants --in-diff validates that the new side of the diff matches the source tree under test. In pull request CI, actions/checkout may test the merge ref, while the generated diff used base.sha..head.sha. That can make the diff's new-side context differ from the checked-out files and fail with exit code 5 before mutation testing starts.

Validation

  • python3 -m unittest tests.test_plugin_catalog.PluginCatalogTests.test_ci_workflow_includes_parity_jobs_for_rust_plugin_checks
  • python3 -m unittest tests.test_plugin_catalog
  • Real Rust-diff smoke using the updated command shape: temporarily changed secrets_detection/src/object_model.rs, generated git diff "${BASE_SHA}" -- '*.rs', then ran cargo mutants -p secrets_detection --in-diff /tmp/cargo-mutants-real-diff.diff; result: Found 5 mutants to test, baseline passed, 5 mutants tested in 27s: 1 caught, 4 unviable.

Note: standard detailed code review workflow has not been run yet.

@lucarlig lucarlig marked this pull request as ready for review May 1, 2026 11:52
@gandhipratik203
Copy link
Copy Markdown
Collaborator

What's good

  • Root cause is well diagnosed: actions/checkout checks out the PR merge ref by default, so working-tree files don't match base..head context, which trips cargo mutants --in-diff with exit 5.
  • Diff is tiny and self-locking — the test_plugin_catalog assertion is updated to pin the new command and forbid HEAD_SHA: from reappearing.
  • Removing the now-unused HEAD_SHA env keeps the workflow honest.

Findings

F1 (low) — Validation only covers the empty-diff path.
The PR notes "cargo mutants ... --in-diff ". An empty diff can't reproduce the original failure, since the bug was about new-side context lines diverging from the working tree. Worth running this once on a PR with actual *.rs changes (or a draft test PR) to confirm cargo-mutants now passes the source-match check end-to-end. The PR even calls out "standard detailed code review workflow has not been run yet" — that's the validation that actually proves the fix.

F2 (suggestion) — git diff "${BASE_SHA}" -- '*.rs' reads more clearly than ${BASE_SHA}...
With an empty right side, BASE_SHA.. resolves to BASE_SHA..HEAD, i.e. base-vs-working-tree. Same result, but git diff <commit> is the more idiomatic form for "diff a commit against the working tree" and avoids a reader wondering whether the trailing .. is a typo. Non-blocking.

Sanity check (looks fine) — fetch-depth: 0 is already asserted in mutants_section, so BASE_SHA is reachable. Good.

Verdict

Correct fix, minimal blast radius, well-locked by the catalog test. I'd land it after a real-PR smoke run on the workflow itself; if that's already implicit in your merge process, this is good to approve as-is.

Signed-off-by: lucarlig <luca.carlig@ibm.com>
@lucarlig lucarlig force-pushed the fix/mutation-diff-head branch from 427af75 to 4d26983 Compare May 1, 2026 11:59
@lucarlig
Copy link
Copy Markdown
Collaborator Author

lucarlig commented May 1, 2026

Addressed both points from Pratik's comment.

  • Switched the mutation diff command to the clearer form: git diff "${BASE_SHA}" -- '*.rs' > cargo-mutants.diff.
  • Updated the catalog assertion to pin that exact command and still forbid HEAD_SHA:.
  • Ran a real Rust-diff smoke: temporarily changed secrets_detection/src/object_model.rs, generated the diff with the updated command shape, and ran cargo mutants -p secrets_detection --in-diff /tmp/cargo-mutants-real-diff.diff. It selected 5 mutants, baseline passed, and exited 0: 5 mutants tested in 27s: 1 caught, 4 unviable.

Also updated the PR body validation section with that evidence.

@lucarlig lucarlig marked this pull request as draft May 1, 2026 12:00
Copy link
Copy Markdown
Collaborator

@msureshkumar88 msureshkumar88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Clean fix for the mutation diff source mismatch.

What this does: Changes the git diff command from a range diff (BASE_SHA..HEAD_SHA) to a single-commit diff (BASE_SHA against the checked-out tree). This is the correct approach for cargo mutants --in-diff — on GitHub Actions, the checked-out tree already reflects the PR changes (via the merge commit), so HEAD_SHA is redundant and the range diff was comparing wrong refs.

Changes verified:

  • .github/workflows/: HEAD_SHA env var removed, diff command simplified to git diff $BASE_SHA
  • Tests updated to assert new command shape and absence of HEAD_SHA
  • DCO sign-off present on commit
  • No version bumps needed (infra-only change)

Logic is sound. Test coverage accurate. No issues found.

Approved.

@lucarlig lucarlig marked this pull request as ready for review May 1, 2026 12:09
@msureshkumar88 msureshkumar88 merged commit 696e9db into main May 1, 2026
35 checks passed
@msureshkumar88 msureshkumar88 deleted the fix/mutation-diff-head branch May 1, 2026 13:23
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.

3 participants