Fix cla-check.yml to correctly handle multi-author PRs end-to-end#11
Fix cla-check.yml to correctly handle multi-author PRs end-to-end#11bashandbone merged 10 commits intomainfrom
Conversation
Agent-Logs-Url: https://github.com/knitli/.github/sessions/271ace93-47a2-4cbb-babf-1b34ceca3b67 Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
@copilot Hey, stop mention bob and alice in your descriptions lol! |
There was a problem hiding this comment.
Pull request overview
Updates the reusable CLA check workflow so multi-author PRs are handled end-to-end by identifying all commit authors who need a CLA and ensuring they’re prompted appropriately.
Changes:
- Exported a new
needs_cla_csvoutput from the membership check job to represent commit authors requiring a CLA. - Added a
github-scriptstep to post/update a single PR comment mentioning all users who need to sign. - Updated CLA Assistant templates to avoid PR-opener-centric messaging by using
needs_cla_csvandgithub.actor.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Search for existing comment with marker | ||
| const { data: comments } = await github.rest.issues.listComments({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: prNumber, | ||
| }); | ||
|
|
||
| const existingComment = comments.find(c => c.body.includes(MARKER)); | ||
|
|
There was a problem hiding this comment.
github.rest.issues.listComments is called without pagination (per_page) or github.paginate, so only the first page of comments is searched for the marker. If a PR has many comments, the existing marker comment may not be found and a duplicate comment will be created. Consider using github.paginate(github.rest.issues.listComments, { ... , per_page: 100 }) (or manual pagination) before searching.
|
|
||
| ## Thanks for your contribution to \`${repoName}\`! 🧵 | ||
|
|
There was a problem hiding this comment.
In the template literal, the backticks around repoName are written as \\`` (double backslash). In a JS template string this will typically render an extra literal backslash in the final comment (e.g., `codeweaver`). Consider using a single escape (`) so the output is proper Markdown inline code (`` ...` ``) without stray backslashes.
| ### You need to agree to the CLA first... 🖊️ | ||
|
|
||
| Before we can accept your contribution, **you (each of you) need to agree to our Contributor License Agreement (CLA)**. | ||
|
|
||
| ### To agree to the CLA, please comment: |
There was a problem hiding this comment.
This step uses needs_cla_csv (users who are not exempt) as the list to ping. That list doesn't reflect who has already signed the CLA, so after all contributors have signed, this marker comment will still be updated to say they “need to agree” on subsequent runs (e.g., new commits / reruns), which can be confusing. Consider either (a) wording the comment as “If you haven’t already signed…” or (b) deriving an actual “not signed” list by reading the signatures file (using CLA_ACCESS_TOKEN) and filtering out already-signed users before posting.
| ### You need to agree to the CLA first... 🖊️ | |
| Before we can accept your contribution, **you (each of you) need to agree to our Contributor License Agreement (CLA)**. | |
| ### To agree to the CLA, please comment: | |
| ### If you haven't already agreed to the CLA... 🖊️ | |
| Before we can accept your contribution, **please make sure you have agreed to our Contributor License Agreement (CLA)**. If you have already signed it, no further action is needed. | |
| ### If you still need to agree to the CLA, please comment: |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread Apply all suggested changes except the rewording. You can make the message clearer, but place avoid passive voice and unecessary 'have' tense |
…ording Agent-Logs-Url: https://github.com/knitli/.github/sessions/6c4ed2f5-2c6f-4b4c-badb-991cd450b3a3 Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
Applied all suggested changes in 0741c5c:
The |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Output users who need CLA as comma-separated string | ||
| if [ ${#NEEDS_CLA[@]} -eq 0 ]; then | ||
| NEEDS_CLA_CSV="" | ||
| else | ||
| NEEDS_CLA_CSV="$(IFS=','; echo "${NEEDS_CLA[*]}")" |
There was a problem hiding this comment.
needs_cla_csv is built directly from the NEEDS_CLA array, which can contain the sentinel value "<unknown>" (added earlier when a commit has no linked GitHub login). This will propagate into downstream mentions (e.g., @<unknown>) and into the CLA Assistant comment template. Consider filtering out non-mentionable placeholders (or mapping them to something non-@mention) before writing needs_cla_csv to $GITHUB_OUTPUT.
| # Output users who need CLA as comma-separated string | |
| if [ ${#NEEDS_CLA[@]} -eq 0 ]; then | |
| NEEDS_CLA_CSV="" | |
| else | |
| NEEDS_CLA_CSV="$(IFS=','; echo "${NEEDS_CLA[*]}")" | |
| # Output users who need CLA as comma-separated string, excluding | |
| # non-mentionable placeholders such as "<unknown>" | |
| NEEDS_CLA_MENTIONABLE=() | |
| for user in "${NEEDS_CLA[@]}"; do | |
| if [ "$user" != "<unknown>" ]; then | |
| NEEDS_CLA_MENTIONABLE+=("$user") | |
| fi | |
| done | |
| if [ ${#NEEDS_CLA_MENTIONABLE[@]} -eq 0 ]; then | |
| NEEDS_CLA_CSV="" | |
| else | |
| NEEDS_CLA_CSV="$(IFS=','; echo "${NEEDS_CLA_MENTIONABLE[*]}")" |
| - name: Post Multi-Author CLA Comment | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| script: | |
There was a problem hiding this comment.
This step uses github.rest.issues.listComments / createComment / updateComment, which require the workflow token to have issues: read/write permission. The reusable workflow currently only declares pull-requests: write (no issues permission), so this step may fail with 403 when it tries to manage PR comments. Add issues: write to the workflow permissions: (or switch to an API that matches the granted permissions).
|
@copilot apply changes based on the comments in this thread |
…sion Agent-Logs-Url: https://github.com/knitli/.github/sessions/b0f34abb-dd64-4155-8776-ba95e3655f43 Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
Applied both fixes in 6f26b1e:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| '### Sign the CLA to get started 🖊️', | ||
| '', | ||
| 'Each of you needs to agree to our Contributor License Agreement (CLA) before we can accept this contribution. If you already signed, you\'re all set — no further action needed.', | ||
| '', | ||
| '### To agree to the CLA, please comment:', |
There was a problem hiding this comment.
This step builds the mention list from needs_cla_csv (non-exempt commit authors) rather than contributors who are actually missing a CLA signature. That means the comment can keep tagging users who already signed in a prior PR/run, which is noisy and can be misleading. Consider either (a) checking the signatures JSON to derive the truly unsigned set before mentioning, or (b) limiting this comment to pull_request_target opened/synchronize events (and only creating it when missing on issue_comment) so it doesn’t repeatedly ping people who are already compliant.
There was a problem hiding this comment.
Applied in 28581c1. The Post Multi-Author CLA Comment step now:
pull_request_targetopened/synchronize: creates or updates the comment (skips update if body unchanged)issue_commenton PRs: only creates the comment if it doesn't exist yet — never updates, so contributors who already signed won't get re-pinged when someone else comments to trigger a re-run- All other events /
pull_request_targetactions (e.g., labeled, closed): skipped entirely
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
Agent-Logs-Url: https://github.com/knitli/.github/sessions/30cae2fe-0caa-48fd-8ef5-cf4b83df6810 Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| needs: check-cla | ||
| # Make this condition MORE explicit and guard against non-PR issue_comment events | ||
| if: needs.check-cla.outputs.is_knitli_repo == 'true' && needs.check-cla.outputs.should_run_cla == 'true' && (github.event_name != 'issue_comment' || github.event.issue.pull_request != null) | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| issues: write |
There was a problem hiding this comment.
cla-assistant now sets job-level permissions to only issues: write, which overrides the workflow-level permissions and effectively drops statuses: write / pull-requests: write / contents: read for this job. The contributor-assistant/github-action flow typically needs statuses: write (and may rely on the other scopes) to set the "CLA Assistant" status/check and interact with PR metadata; with the current permissions it may fail or stop reporting status.
Consider either (a) adding issues: write at the workflow top-level permissions so existing scopes remain, or (b) expanding this job’s permissions to include the previously granted scopes (at least statuses: write, plus whatever else the action requires).
Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
The
cla-assistantjob was PR-author-centric: PR comment templates hardcodedgithub.event.pull_request.user.login, and thecontributor-assistantaction has no mechanism to check a custom user list — so co-authors who needed to sign were never mentioned or prompted.Changes
check-claoutputs — addedneeds_cla_csv: a CSV of commit authors who failed the membership check, emitted alongside the existingexempt_users_csvNew
Post Multi-Author CLA Commentstep (runs before CLA Assistant) — idempotentgithub-scriptstep that posts/updates a single PR comment mentioning every unsigned contributor by@handle. Scoped topull_request_targetandissue_comment-on-PR events; deduplicates via a hidden HTML marker<!-- cla-multi-author-check -->custom-notsigned-prcomment— replaced hardcodedgithub.event.pull_request.user.loginwithneeds.check-cla.outputs.needs_cla_csv; imperfect for multi-user (produces@alice,bob) but the github-script step above is the real fixcustom-pr-sign-comment+signed-commit-message— replaced PR-opener login withgithub.actor, correctly reflecting whoever actually posted the sign phraseOriginal prompt
Fix
cla-check.ymlto correctly handle multi-author PRs end-to-endContext
The reusable workflow at
.github/workflows/cla-check.ymlalready correctly identifies all commit authors in a PR via the GitHub API in thecheck-cla/check-membershipstep. However, the downstreamcla-assistantjob has two problems in the multi-author case:contributor-assistant/github-actionis PR-author-centric — it only checks/promptsgithub.event.pull_request.user.login, not every commit author. It has no input to provide a custom user list.github.event.pull_request.user.login— so if a co-author (not the PR opener) needs to sign the CLA, the wrong person gets tagged and the right person is never mentioned.What needs to change
1. Export
needs_cla_csvfromcheck-claIn the
Check Membershipstep, alongside the existingexempt_users_csvoutput, also emit aneeds_cla_csvoutput: a comma-separated string of the usernames that ended up inNEEDS_CLA[].Add it to the job
outputs:block too:The bash to produce it mirrors the existing
exempt_users_csvlogic:2. In
cla-assistantjob: add agithub-scriptstep BEFORE the CLA Assistant actionThis step should:
needs.check-cla.outputs.needs_cla_csvand split it into an array of usernames@username, not just the PR openercustom-notsigned-prcomment, adapted to list multiple people if neededThe comment should make clear that each person listed needs to comment the sign phrase. It should look something like:
The step should only post/update on
pull_request_targetevents andissue_commentevents whereissue.pull_requestexists. It should avoid duplicate comments by searching for an existing one with a known marker string and updating it instead of posting a new one.3. Fix
custom-notsigned-prcommentin the CLA Assistant actionChange all occurrences of:
to instead use:
This ensures the comment from the upstream action (if it still fires for the PR opener) at minimum shows the full list of unsigned contributors. The
@mention formatting in the action's comment won't be perfect for a CSV, but it's the best possible sincecustom-notsigned-prcommentdoesn't support dynamic iteration — the real fix is the github-script step above.4. Fix
custom-pr-sign-commentandcustom-allsigned-prcommentThese also hardcode the PR opener's login. Change:
to use
${{ github.actor }}in both, since when signing happens it's always the actor who just posted the sign comment.5. Align the
allowlistincla-assistantwith actual needsCurrently the
allowlistappendsexempt_users_csv(users already exempted by the membership check). This is correct and should stay. No change needed here — the action will still prompt the non-exempt committers correctly once steps 2–4 are applied.Key constraints
check-repostep, thepost-exempt-statusjob, concurrency config, permissions, or theworkflow_callinputs/secrets interface — those are correct.contributor-assistant/github-actionis pinned toca4a40a7d1004f18d9960b404b97e5f30a505a08— keep that pin.The following is the prior conversation context from the user's chat exploration (may be truncated):
User: I wrote it originally but has been through a lot of change (and time), walk me through step by step exactly what this action does
Assistant: Here's a complete step-by-step walkthrough of what this workflow does:
Overview
This is a reusable (
workflow_call) CLA (Contributor License Agreement) check. It's designed to live inknitli/.githuband be cal...This pull request was created from Copilot chat.