Skip to content

Added automation for code coverage in PRs and pushes#659

Open
maifeeulasad wants to merge 3 commits intointel:mainfrom
maifeeulasad:per-commit-coverage-comment-bot-in-mr-maifee
Open

Added automation for code coverage in PRs and pushes#659
maifeeulasad wants to merge 3 commits intointel:mainfrom
maifeeulasad:per-commit-coverage-comment-bot-in-mr-maifee

Conversation

@maifeeulasad
Copy link
Copy Markdown
Contributor

Description & Motivation

For every pull request targeting main branch we will be generating comment with coverage difference in each commit.

Related Issue(s)

closes #270

Testing

I have tested this on my own fork, related PR: maifeeulasad#1

quick peek:
maifeeulasad implementing devops automation for intel rohd an expert coverage bot

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

n/a

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

n/a

As covered in previous PRs

Copy link
Copy Markdown
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

I think this looks great to me, thank you!

@mkorbel1
Copy link
Copy Markdown
Contributor

mkorbel1 commented May 1, 2026

@desmonddak any comments on this?

@maifeeulasad
Copy link
Copy Markdown
Contributor Author

Regarding the failing workflow:

  • either it is due to settings, as you are using self hosted runner, definetly you guys have your own setup and configurations, and i am not so sure about this. in the workflow section, we may need to enable the write permission from workflow as well. it is accessible at: https://github.com/intel/rohd/settings/actions
  • other can be it is not merged yet and it is being blocked by default, cause otherwise it can a security vulnerability, as some random user just raise and mr triggering workflow and start doing stuff.

Just thinking out loud. Open for discussion as well.

And definetly open for change requests as well.

@mkorbel1
Copy link
Copy Markdown
Contributor

mkorbel1 commented May 3, 2026

Here's a suggestion the Copilot gave me as I was chatting with it about potential fixes and security issues. Thoughts?

===================

Implement it as a two-workflow “compute on PR, comment on workflow_run” setup so fork PRs can run coverage on untrusted code, but the comment is posted by a trusted workflow with write permissions.

High-level plan

1) Workflow A: compute coverage on pull_request (untrusted)

Create/modify a workflow that runs on:

  • on: pull_request

It should:

  1. actions/checkout@v4 (this checks out the PR’s head by default)
  2. Setup Dart + deps + iverilog
  3. Run tool/generate_coverage.sh to produce coverage/lcov.info
  4. Extract the numeric coverage percent (same logic you already have)
  5. Upload artifacts:
    • coverage/lcov.info (and/or the whole coverage/ directory)
    • a small file containing the computed percent (e.g. coverage_percent.txt)
    • optionally: pr_number.txt, target_branch.txt so the next workflow doesn’t have to query much

Permissions for Workflow A should be minimal, typically:

  • permissions: contents: read (or even default)
  • no PR write permissions required

Key point: Workflow A does not try to create/update PR comments (that’s what currently fails with Resource not accessible by integration).


2) Workflow B: comment on workflow_run (trusted)

Add a second workflow that runs on:

  • on: workflow_run
    • workflows: ["<name of Workflow A>"]
    • types: [completed]

This workflow should:

  1. Check the triggering run concluded successfully (or handle failures gracefully)
  2. Download the artifacts from Workflow A using the Actions API
    • download lcov.info / coverage_percent.txt
  3. Determine PR number and target branch
    • In workflow_run, GitHub includes PR context: github.event.workflow_run.pull_requests[0].number
    • Or read pr_number.txt if you saved it as an artifact
  4. Fetch “target coverage” (your existing badge parse logic) or compute target coverage separately (optional)
  5. Compute delta
  6. Post/update the PR comment using actions/github-script (this now works because it’s running in base-repo context)

Permissions for Workflow B:

  • permissions: pull-requests: write, contents: read

Security property: Workflow B must not checkout and execute PR code. It should only download artifacts and call GitHub APIs.


Implementation notes / gotchas to tell the implementer

  • Do not use pull_request_target for the coverage computation job; it’s risky because it can combine elevated token + executing fork-provided code.
  • The comment workflow (Workflow B) should treat artifact contents as untrusted input:
    • don’t execute anything from artifacts
    • only parse numeric values / text
  • If you want “update existing comment” behavior instead of spamming new comments:
    • in Workflow B, list existing PR comments and find one containing a marker like <!-- coverage-diff-comment -->, then update it; otherwise create it.
  • Keep Workflow A artifact retention short (e.g. default or 1–3 days) to reduce storage.

What they’re fixing relative to the failing run you linked

That run fails at the end with Resource not accessible by integration when actions/github-script tries to issues.createComment under a pull_request run from a fork. The two-workflow approach moves only the commenting to a trusted context where that API call is allowed.

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.

Add automation for code coverage in PRs and pushes

2 participants