Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a Contributor License Agreement (CLA) management system using GitHub Actions workflows to automate the process of checking and approving CLA signatures for pull request authors.
- Automated CLA signature verification on pull requests
- Maintainer-triggered workflow to approve new contributors via
/approve-clacommand - Batch processing of CLA approvals to comply with repository rules
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
.github/workflows/cla-file-check.yml |
Verifies PR authors have signed the CLA by checking the .cla-signed-users file and comments on PRs when signature is missing |
.github/workflows/add-cla-user.yml |
Allows authorized maintainers to approve CLA signatures via /approve-cla command, managing additions through a batch branch and PR |
.cla-signed-users |
Initializes the list of CLA signers with header comment and initial maintainer |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/cla-file-check.yml
Outdated
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
There was a problem hiding this comment.
This workflow uses actions/checkout@v4.2.2, while the existing ci.yml workflow uses v5.0.0. For consistency and to use the latest features and security fixes, consider using the same version across all workflows in the repository.
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| uses: actions/checkout@v5.0.0 |
.github/workflows/add-cla-user.yml
Outdated
|
|
||
| - name: Checkout main branch | ||
| if: steps.command_check.outputs.should_proceed == 'true' | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
There was a problem hiding this comment.
This workflow uses actions/checkout@v4.2.2, while the existing ci.yml workflow uses v5.0.0. For consistency and to use the latest features and security fixes, consider using the same version across all workflows in the repository.
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| uses: actions/checkout@v5.0.0 |
| echo "Adding ${AUTHOR_USERNAME} to .cla-signed-users" | ||
|
|
||
| # Check if user is already in the list on main branch | ||
| if grep -v '^#' .cla-signed-users | grep -q -w "$AUTHOR_USERNAME"; then |
There was a problem hiding this comment.
Missing file existence check before grep operation. If the .cla-signed-users file doesn't exist on main branch (e.g., in a new repository), this will fail. Add a check: if [ ! -f .cla-signed-users ]; then echo '# Contributor License Agreement Signers' > .cla-signed-users; fi before attempting to grep.
| fi | ||
|
|
||
| # Check if user is already in the batch branch | ||
| if grep -v '^#' .cla-signed-users | grep -q -w "$AUTHOR_USERNAME"; then |
There was a problem hiding this comment.
Missing file existence check before grep operation. After switching to the batch branch, if the .cla-signed-users file doesn't exist yet on that branch, this grep will fail. Add a file existence check before attempting to grep.
| if grep -v '^#' .cla-signed-users | grep -q -w "$AUTHOR_USERNAME"; then | |
| if [ -f .cla-signed-users ] && grep -v '^#' .cla-signed-users | grep -q -w "$AUTHOR_USERNAME"; then |
| run: | | ||
| PR_NUMBER="${{ github.event.issue.number }}" | ||
| echo "Getting PR details for #${PR_NUMBER}" | ||
| PR_DATA=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER) |
There was a problem hiding this comment.
The gh api command can fail if the API is unavailable or rate-limited, but there's no error handling. Consider adding error handling: PR_DATA=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER) || { echo 'Failed to fetch PR details'; exit 1; }
| PR_DATA=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER) | |
| PR_DATA=$(gh api repos/${{ github.repository }}/pulls/$PR_NUMBER) || { echo 'Failed to fetch PR details'; exit 1; } |
| if git ls-remote --exit-code --heads origin "$BATCH_BRANCH" > /dev/null 2>&1; then | ||
| echo "Batch branch exists, checking it out" | ||
| git fetch origin "$BATCH_BRANCH" | ||
| git checkout "$BATCH_BRANCH" | ||
| else | ||
| echo "Creating new batch branch" | ||
| git checkout -b "$BATCH_BRANCH" |
There was a problem hiding this comment.
There's a potential race condition if multiple maintainers approve different users simultaneously. Between checking if the batch branch exists (line 104) and pushing changes (line 130), another workflow instance could create or modify the same branch, causing conflicts. Consider using git pull --rebase before pushing or implementing a retry mechanism on push failures.
- Update actions/checkout to v5.0.0 for consistency - Add file existence checks before grep operations - Document maintainer authorization list updates
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Aha! this was the bug I encountered! Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.cla-signed-users
Outdated
| @@ -0,0 +1,5 @@ | |||
| # Contributor License Agreement Signers | |||
| #Anchorage maintainers | |||
There was a problem hiding this comment.
Missing space after # in the comment. For consistency with the comment on line 1, this should be # Anchorage maintainers instead of #Anchorage maintainers.
| #Anchorage maintainers | |
| # Anchorage maintainers |
.github/workflows/add-cla-user.yml
Outdated
| if [ $? -ne 0 ]; then | ||
| echo "❌ Failed to list PRs for batch branch '$BATCH_BRANCH'. Aborting to avoid duplicate PR creation." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The error handling logic is flawed. gh pr list returns exit code 0 even when no PRs are found (it just returns an empty array). The $? check on this line will never be non-zero in normal operation, making this safety check ineffective. Consider checking if the command succeeded by validating the output format instead, or remove this check entirely since gh pr list failures are rare and would be caught by the script's error handling.
| if [ $? -ne 0 ]; then | |
| echo "❌ Failed to list PRs for batch branch '$BATCH_BRANCH'. Aborting to avoid duplicate PR creation." | |
| exit 1 | |
| fi |
.github/workflows/add-cla-user.yml
Outdated
| Recent additions: | ||
| - @${AUTHOR_USERNAME} (approved by @${COMMENTER} in PR #${{ github.event.issue.number }}) | ||
|
|
||
| This PR will be updated automatically as more users are approved." \ |
There was a problem hiding this comment.
The indentation is inconsistent. Line 169 uses 12 spaces while lines 170-172 appear to use different indentation. This should use consistent indentation (likely 2 spaces per level) for better readability.
| Recent additions: | |
| - @${AUTHOR_USERNAME} (approved by @${COMMENTER} in PR #${{ github.event.issue.number }}) | |
| This PR will be updated automatically as more users are approved." \ | |
| Recent additions: | |
| - @${AUTHOR_USERNAME} (approved by @${COMMENTER} in PR #${{ github.event.issue.number }}) | |
| This PR will be updated automatically as more users are approved." \ |
| # Check if batch branch exists, if not create it from main | ||
| if git ls-remote --exit-code --heads origin "$BATCH_BRANCH" > /dev/null 2>&1; then | ||
| echo "Batch branch exists, checking it out" | ||
| git fetch origin "$BATCH_BRANCH" | ||
| git checkout "$BATCH_BRANCH" | ||
| else | ||
| echo "Creating new batch branch" | ||
| git checkout -b "$BATCH_BRANCH" | ||
| fi | ||
|
|
||
| # Check if file exists on batch branch before attempting to grep | ||
| if [ ! -f .cla-signed-users ]; then | ||
| echo "⚠️ CLA file not found on batch branch. This should not happen." | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Check if user is already in the batch branch | ||
| if grep -v '^#' .cla-signed-users | grep -q -w "$AUTHOR_USERNAME"; then | ||
| echo "User is already in the batch branch." | ||
| gh pr comment ${{ github.event.issue.number }} --body "ℹ️ @${AUTHOR_USERNAME} is already pending approval in the CLA signers batch." | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Add user to file and sort | ||
| # Preserve header, sort and deduplicate only usernames | ||
| grep '^#' .cla-signed-users > .cla-signed-users.tmp | ||
| (grep -v '^#' .cla-signed-users; echo "${AUTHOR_USERNAME}") | sort -u >> .cla-signed-users.tmp | ||
| mv .cla-signed-users.tmp .cla-signed-users | ||
|
|
||
| # Commit the change to batch branch | ||
| git add .cla-signed-users | ||
| git commit -m "chore: Add @${AUTHOR_USERNAME} to CLA signers list | ||
|
|
||
| Approved by: @${COMMENTER} | ||
| Related PR: #${{ github.event.issue.number }}" | ||
| git push origin "$BATCH_BRANCH" |
There was a problem hiding this comment.
This workflow lacks concurrency controls. If multiple maintainers run /approve-cla simultaneously for different users, race conditions could occur when checking out, modifying, and pushing to the same batch branch. Consider adding a concurrency group to the workflow to serialize executions, for example:
concurrency:
group: cla-batch-update
cancel-in-progress: falseThere was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Add user to file and sort | ||
| # Preserve header, sort and deduplicate only usernames | ||
| grep '^#' .cla-signed-users > .cla-signed-users.tmp | ||
| (grep -v '^#' .cla-signed-users; echo "${AUTHOR_USERNAME}") | sort -u >> .cla-signed-users.tmp |
There was a problem hiding this comment.
If .cla-signed-users contains only comment lines (all lines start with '#'), grep -v '^#' will have no output and exit with code 1, which could cause the command to fail in strict shell modes. Consider using grep -v '^#' .cla-signed-users || true to handle this case.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
Add Contributor License Agreement (CLA) workflows and initial signers list to establish CLA requirements for the project.
What's included
cla-file-check.yml: GitHub Actions workflow that validates PR authors against the.cla-signed-userslistadd-cla-user.yml: GitHub Actions workflow for maintainers to approve and add CLA signers/approve-clacomment on PRs (maintainers only).cla-signed-users.cla-signed-users: Initial file with current maintainer(s)Why
This establishes a formal CLA process for external contributors, providing legal protection for the project while
maintaining a transparent, automated approval process.
Test plan
add-cla-user.ymlRelated
Mirrors the CLA setup from https://github.com/anchorageoss/visualsign-parser