fix: resolve CLA action bugs related to exempt personnel and status checks#5
Conversation
…hecks - Fixed syntax errors in GitHub Actions expressions (e.g., removed spaces after dots). - Removed the `github.event_name != 'issue_comment'` condition that prevented the `cla-assistant` job from running on issue comments, causing PRs to be initially green but subsequently red. - Updated the workflow to dynamically collect exempt users (org members, bots, etc.) and append them to the `cla-assistant` allowlist, fixing the issue where mixed-committer PRs would inappropriately require exempt personnel to sign the CLA. - Documented troubleshooting steps for authentication issues related to `CLA_ACCESS_TOKEN` and SAML Single Sign-On in the README. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR updates the reusable CLA-check workflow to better handle exempt contributors and to ensure the CLA status is correctly evaluated on signature comments, plus adds troubleshooting documentation for common auth/SSO issues.
Changes:
- Adds an
exempt_users_csvoutput from the membership-check step and injects it into the CLA action allowlist. - Ensures the CLA assistant job runs for
issue_comment-driven signature flows (avoids “skipped == success” behavior). - Documents troubleshooting steps for PAT scope and SAML SSO authorization issues.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/cla-check.yml | Exports CSV allowlist entries for exempt users; adjusts job conditions; fixes expressions and github-script syntax. |
| .github/workflows/README.md | Adds troubleshooting guidance for membership/auth failures (scopes + SSO authorization). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -245,7 +252,7 @@ jobs: | |||
| state: 'success', | |||
| context: 'CLA Assistant', | |||
| description: 'All contributors are exempt (org members/bots)', | |||
| target_url: `https://github. com/${context.repo.owner}/${context.repo.repo}/pull/${prNumber || ''}` | |||
| target_url: `https://github.com/${context.repo.owner}/${context.repo.repo}/pull/${prNumber || ''}` | |||
There was a problem hiding this comment.
In issue_comment events, context.payload.pull_request and context.sha are typically not set, so sha will be empty and this step will silently skip creating the status check (and prNumber will also be undefined, producing a bad target_url). Consider deriving the PR number from context.payload.issue.number and fetching the PR’s head.sha via github.rest.pulls.get before calling createCommitStatus so the status check works reliably for signature comments.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
This is a command for GitHub Copilot, not for me. I am ignoring it.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
|
@bashandbone I've opened a new pull request, #6, to work on those changes. Once the pull request is ready, I'll request review from you. |
…hecks - Fixed syntax errors in GitHub Actions expressions (e.g., removed spaces after dots). - Removed the `github.event_name != 'issue_comment'` condition that prevented the `cla-assistant` job from running on issue comments, causing PRs to be initially green but subsequently red. - Updated the workflow to dynamically collect exempt users (org members, bots, etc.) and append them to the `cla-assistant` allowlist, fixing the issue where mixed-committer PRs would inappropriately require exempt personnel to sign the CLA. - Documented troubleshooting steps for authentication issues related to `CLA_ACCESS_TOKEN` and SAML Single Sign-On in the README. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
… check Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
fix: derive PR number and head SHA for issue_comment events in CLA status check
|
@jules please merge in new changes from main -- I want you to consider the approach from main against this implementation and decide on the best strategy, or if the two approaches can work together, then ensure that's handled smartly. |
I've merged |
Resolve conflicts by combining the dynamic allowlist approach from main with the syntax error fixes and condition removal from this branch. Co-authored-by: bashandbone <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 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const prNumber = context.payload.pull_request?.number; | ||
| const sha = context.payload.pull_request?.head?. sha || context.sha; | ||
| const sha = context.payload.pull_request?.head?.sha || context.sha; | ||
|
|
There was a problem hiding this comment.
The Create Success Status Check script derives prNumber/sha from context.payload.pull_request, which is not present for issue_comment events. In that case sha will be empty and the step will skip creating the status check (and the target_url may end up pointing to /pull/). Consider deriving the PR number from context.payload.issue.number and fetching the PR via github.rest.pulls.get to obtain head.sha before calling createCommitStatus so signature comments reliably update the PR status.
| if: | | ||
| needs.check-cla.outputs.is_knitli_repo == 'true' && | ||
| needs.check-cla.outputs.should_run_cla == 'true' && | ||
| github.event_name != 'issue_comment' | ||
| needs.check-cla.outputs.should_run_cla == 'true' |
There was a problem hiding this comment.
Now that the cla-assistant job runs for issue_comment events, it will also run when someone comments on a regular issue (not a PR) if the caller workflow triggers on all issue_comment events (as shown in the README example). Add a guard so this job only runs for PRs (e.g., require github.event.issue.pull_request when github.event_name == 'issue_comment') to avoid posting CLA instructions / writing signatures for non-PR issues.
| # Output exempt usernames as comma-separated string for allowlist | ||
| if [ ${#EXEMPT_USERNAMES[@]} -eq 0 ]; then | ||
| EXEMPT_USERNAMES_CSV="" | ||
| else | ||
| EXEMPT_USERNAMES_CSV=$(IFS=','; echo "${EXEMPT_USERNAMES[*]}") | ||
| EXEMPT_USERNAMES_CSV="$(IFS=','; echo "${EXEMPT_USERNAMES[*]}")" |
There was a problem hiding this comment.
The PR description refers to a new exempt_users_csv output, but the workflow output is named exempt_usernames (and that’s what downstream steps reference). To avoid confusion for callers and future maintainers, either align the output name in the workflow with the description or update the PR/README wording to match the implemented output name.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@bashandbone I've opened a new pull request, #9, to work on those changes. Once the pull request is ready, I'll request review from you. |
This is a command for GitHub Copilot. I will ignore it. |
Resolve conflicts by combining the dynamic allowlist approach from main with the syntax error fixes and condition removal from this branch. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
…users_csv output Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
fix: handle issue_comment events in CLA status check and guard against non-PR issues
This PR addresses the persistent issues with the CLA action.
cla-assistantaction was sometimes failing to recognize organization members, especially in PRs with mixed committers (some exempt, some not). To solve this, the workflow now outputs the list of exempt users identified by ourcheck-membershipstep as a CSV string (exempt_users_csv). This string is directly interpolated into theallowlistinput for thecontributor-assistantaction, forcing it to correctly ignore those users.cla-assistantjob had a conditiongithub.event_name != 'issue_comment'. When this job was skipped, GitHub marked it as successful. The next push would trigger the job, and it would fail. Removing this condition ensures the job processes the signature comment correctly.README.md. As suspected, the root cause for thebashandbonemembership checks failing (returning 302/403/404) is highly likely related to theCLA_ACCESS_TOKEN. Even if it's an admin's token, if the Knitli organization enforces SAML Single Sign-On (SSO), the Personal Access Token must be explicitly authorized for SSO. The README now explains this and provides steps to fix it.${{ steps.check-membership. outputs.should_run_cla }}) and malformed JavaScript in theactions/github-scriptstep.PR created automatically by Jules for task 8608742043899548302 started by @bashandbone