Skip to content

Skip coverage CI for draft PRs#1280

Merged
sbryngelson merged 3 commits intoMFlowCode:masterfrom
sbryngelson:pause-coverage
Mar 1, 2026
Merged

Skip coverage CI for draft PRs#1280
sbryngelson merged 3 commits intoMFlowCode:masterfrom
sbryngelson:pause-coverage

Conversation

@sbryngelson
Copy link
Copy Markdown
Member

@sbryngelson sbryngelson commented Mar 1, 2026

Summary

  • Skip the ~3hr coverage job on draft PRs, matching the existing pattern used for self-hosted runners in test.yml
  • When a draft PR is marked ready for review, coverage runs automatically via the ready_for_review event trigger

Test plan

  • Open a draft PR and verify coverage job is skipped
  • Mark the draft as ready and verify coverage job runs

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 1, 2026

Claude Code Review

Head SHA: fa6c79c

Files changed: 1

  • .github/workflows/coverage.yml

Summary:

  • Adds ready_for_review to the pull_request event trigger types so coverage runs automatically when a draft PR is published
  • Adds a github.event.pull_request.draft != true guard to the run job's if condition to skip coverage on draft PRs
  • Pattern mirrors the existing draft-skip logic used in test.yml

Findings:

No functional issues found. The change is minimal, correct, and consistent with the existing pattern.

Improvement opportunities:

  1. Implicit vs. explicit event types (.github/workflows/coverage.yml, line 6): Previously, pull_request: without a types: key defaulted to [opened, synchronize, reopened]. The new explicit list adds ready_for_review to that set. This is intentional and correct — just worth noting that the behavior for the other three event types is preserved exactly.

  2. Consistency check (optional): You may want to verify that push: to master (the other trigger) is not affected by the draft guard. It isn't — github.event.pull_request.draft evaluates to false/empty for push events, so the condition != true remains satisfied. Coverage will still run on every push to master. ✓

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 1, 2026

Claude Code Review

Head SHA: 8a3458d6294e5343d046602160e107b33448dfc4

Files changed: 2

  • .github/workflows/coverage.yml
  • src/simulation/m_viscous.fpp

Summary

  • Adds ready_for_review (and explicit opened, synchronize, reopened) event types to coverage.yml so coverage runs when a draft is promoted to ready
  • Guards the run job with github.event.pull_request.draft != true to skip coverage on draft PRs
  • Minor doc-comment tweak in m_viscous.fpp (removes "the" before "Navier--Stokes")

Findings

.github/workflows/coverage.yml — logic is correct but has a subtle edge case

The pull_request trigger without an explicit types list previously defaulted to [opened, synchronize, reopened]. Adding the explicit list now includes ready_for_review, which is the key addition. This is correct.

However, the if condition on the run job:

if: needs.file-changes.outputs.checkall == 'true' && github.event.pull_request.draft != true

uses != true rather than the more idiomatic == false. For push events and workflow_dispatch events, github.event.pull_request is null, so github.event.pull_request.draft evaluates to null, and null != true is true — meaning the job will run for push/dispatch events as intended. This is correct behavior, just worth noting the reasoning.

No issues found. The implementation correctly mirrors the existing draft-skip pattern already used in test.yml and handles all event types properly.

Improvement Opportunities (optional)

  1. Consider adding a comment in the YAML explaining why != true is used instead of == false — it handles push/workflow_dispatch events where pull_request.draft is null.
  2. The m_viscous.fpp change (removing "the") is unrelated to the PR's stated goal; it could be a separate trivial commit or omitted to keep the diff focused.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 1, 2026

Claude Code Review

Head SHA: 1ba50d718b454305258d04537a5ea140d055b322

Files changed: 1

  • .github/workflows/coverage.yml

Summary:

  • Adds types: [opened, synchronize, reopened, ready_for_review] to the pull_request trigger so coverage runs when a draft is promoted to ready
  • Adds github.event.pull_request.draft != true guard to the run job's if condition to skip the ~3 hr coverage job on draft PRs
  • Matches the existing pattern already used for self-hosted runners in test.yml

Findings: No issues found.

Improvement opportunities (optional):

  1. Minor: explicit false comparisongithub.event.pull_request.draft != true works correctly, but the idiomatic GitHub Actions style is !github.event.pull_request.draft (or github.event.pull_request.draft == false). Either form is functionally equivalent; this is purely cosmetic.

  2. Note on push to master — The draft guard only applies to pull_request events; pushes to master (e.g. merge commits) are unaffected, which is the correct and intended behaviour.

Overall this is a clean, minimal, and well-scoped change. ✅

@sbryngelson sbryngelson marked this pull request as ready for review March 1, 2026 18:40
Copilot AI review requested due to automatic review settings March 1, 2026 18:40
@sbryngelson sbryngelson merged commit 4ee892c into MFlowCode:master Mar 1, 2026
30 of 31 checks passed
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Skip coverage CI for draft pull requests

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Skip coverage CI job for draft pull requests
• Add ready_for_review event trigger to run coverage when draft marked ready
• Reduces unnecessary ~3hr coverage job runs on draft PRs
Diagram
flowchart LR
  A["Draft PR opened"] -->|"skip coverage"| B["Coverage job skipped"]
  C["Draft marked ready"] -->|"ready_for_review event"| D["Coverage job runs"]
Loading

Grey Divider

File Changes

1. .github/workflows/coverage.yml ⚙️ Configuration changes +2/-1

Add draft PR detection to coverage workflow

• Added types filter to pull_request trigger with opened, synchronize, reopened, and
 ready_for_review events
• Updated coverage job condition to skip when github.event.pull_request.draft is true
• Prevents coverage job execution on draft PRs while allowing it to run when marked ready for review

.github/workflows/coverage.yml


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Mar 1, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. Draft PR still "passes" check 🐞 Bug ⛯ Reliability
Description
On draft PRs, the expensive coverage job is skipped, but the workflow still runs and can conclude
successfully because the file-changes job always runs. This can mislead reviewers/automation because
the workflow named “Coverage Check” appears green even though coverage was not executed for that
commit while in draft.
Code

.github/workflows/coverage.yml[32]

+    if: needs.file-changes.outputs.checkall == 'true' && github.event.pull_request.draft != true
Evidence
The workflow triggers on pull_request (including opened) and has an unconditional file-changes job.
Only the coverage run job is gated on draft status, so draft PRs will still produce a completed
workflow run (via file-changes) with coverage skipped.

.github/workflows/coverage.yml[3-8]
.github/workflows/coverage.yml[14-33]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
On draft PRs, `run` is skipped but `file-changes` still runs, allowing the workflow to complete successfully and appear green even though coverage didn’t execute.

### Issue Context
The workflow is triggered for `pull_request` events including `opened`, so draft PRs will still create workflow runs.

### Fix Focus Areas
- .github/workflows/coverage.yml[14-33]

### Suggested change
- Add a draft gate to `file-changes` (and optionally make the `run` condition more explicit):
 - `file-changes.if: github.event_name != &#x27;pull_request&#x27; || github.event.pull_request.draft != true`
 - `run.if: needs.file-changes.outputs.checkall == &#x27;true&#x27; &amp;&amp; (github.event_name != &#x27;pull_request&#x27; || github.event.pull_request.draft != true)`
This makes the entire workflow skip on draft PRs (rather than “succeed without coverage”), while still running on push/workflow_dispatch and on ready_for_review.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

2. Draft check not event-guarded 🐞 Bug ⛯ Reliability
Description
The condition references github.event.pull_request.draft even though the workflow also runs on push
and workflow_dispatch events. While this often evaluates as null and works, it’s clearer and more
robust to explicitly guard on github.event_name before dereferencing pull_request fields.
Code

.github/workflows/coverage.yml[32]

+    if: needs.file-changes.outputs.checkall == 'true' && github.event.pull_request.draft != true
Evidence
The workflow declares push and workflow_dispatch triggers, but the job-level if unconditionally
reads from the pull_request payload. Making the condition explicit avoids relying on
null-propagation semantics and improves maintainability if triggers expand later.

.github/workflows/coverage.yml[3-8]
.github/workflows/coverage.yml[30-33]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`github.event.pull_request.draft` is referenced even when the workflow is triggered by `push`/`workflow_dispatch`.

### Issue Context
The workflow intentionally runs on pushes to master, so the condition should explicitly allow non-PR events.

### Fix Focus Areas
- .github/workflows/coverage.yml[30-33]

### Suggested change
Update the condition to:
- `if: needs.file-changes.outputs.checkall == &#x27;true&#x27; &amp;&amp; (github.event_name != &#x27;pull_request&#x27; || github.event.pull_request.draft != true)`
This preserves current behavior but makes the intent explicit and avoids relying on implicit null behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 1, 2026

Claude Code Review

Head SHA: 1ba50d718b454305258d04537a5ea140d055b322

Files changed: 1

  • .github/workflows/coverage.yml

Summary

  • Adds ready_for_review event trigger so coverage runs automatically when a draft PR is promoted to ready
  • Adds draft != true guard on the run job so coverage is skipped on draft PRs
  • Change is small (2 additions, 1 deletion) and self-contained to the CI workflow
  • Mirrors the existing pattern used in test.yml for self-hosted runners

Findings

No issues found.


Improvement Opportunities

  1. Explicit type list side-effect (.github/workflows/coverage.yml, line 7): Adding types: [opened, synchronize, reopened, ready_for_review] removes the implicit default GitHub Actions behavior of [opened, synchronize, reopened]. The three explicitly listed types match the original implicit defaults, so behavior is preserved — but it is worth knowing that any future addition of a new default type (e.g., if GitHub ever adds one) will no longer be picked up automatically. This is acceptable and mirrors the project's existing pattern.

  2. draft != true vs !github.event.pull_request.draft: Both expressions are equivalent and correct. The chosen form github.event.pull_request.draft != true is explicit and readable, consistent with GitHub Actions idioms. No change needed.

Overall this is a clean, minimal CI improvement. Approved.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the CI coverage workflow to avoid running the long coverage job on draft pull requests, while ensuring coverage runs automatically once a PR is marked ready for review (matching the existing draft-handling pattern used elsewhere in the repo’s workflows).

Changes:

  • Add ready_for_review to the pull_request workflow trigger types for coverage.
  • Gate the coverage job to skip when the PR is a draft (github.event.pull_request.draft != true).

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 1, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 021322c and 1ba50d7.

📒 Files selected for processing (1)
  • .github/workflows/coverage.yml

📝 Walkthrough

Walkthrough

The pull request modifies the GitHub Actions coverage workflow configuration. The workflow trigger for pull requests is updated to specify trigger events: opened, synchronize, reopened, and ready_for_review. Additionally, the Coverage job condition is modified to include a check ensuring the pull request is not in draft status, alongside the existing checkall condition requirement. These changes result in a net addition of 2 lines and removal of 1 line.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants