fix: fail CLA allowlist step on API errors - #251
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe CLA workflow now checks the GitHub API result when it retrieves pull-request commits. It logs an error and exits on failure. Successful requests retain the existing author extraction, deduplication, and pull-request author exclusion. ChangesCLA workflow
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR makes the CLA allowlist workflow fail explicitly on GitHub API errors while preserving valid author-only behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🟢 Low | 2 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
| echo "Failed to fetch pull request commits" >&2 | ||
| exit 1 | ||
| fi | ||
| others=$(printf '%s\n' "$commit_authors" | sort -u | grep -vix "${PR_AUTHOR}" | paste -sd, -) |
There was a problem hiding this comment.
🟠 High — The .commit.author.name fallback is fully contributor-controlled (any commit can set git config user.name, and an email with no matching GitHub account forces the fallback), and nothing strips commas or wildcards before the value is joined with paste -sd, into allowlist=${BASE_ALLOWLIST},${others}. A commit authored as Hacker,admin injects an extra allowlist entry, and since contributor-assistant treats the list as globs (the base list relies on *[bot]), an author name of * exempts every contributor from signing the CLA. Filter out or reject entries containing ,/* before joining. Raised by 3 of 8 reviewers (claude-opus-5-thinking-max adversarial, gemini-3.1-pro adversarial, claude-opus-5-thinking-max edge-case).
| echo "Failed to fetch pull request commits" >&2 | ||
| exit 1 | ||
| fi | ||
| others=$(printf '%s\n' "$commit_authors" | sort -u | grep -vix "${PR_AUTHOR}" | paste -sd, -) |
There was a problem hiding this comment.
🟡 Medium — The "only the PR author must sign" invariant rests on grep -vix "${PR_AUTHOR}", but the jq fallback can emit an arbitrary raw git name for that same person, and the login is interpreted as a regex rather than a fixed string. A PR author can push a commit whose author name is a near-miss of their login (e.g. alic*, or the login with a trailing space) so it survives the filter, lands in the allowlist, and then matches them in the CLA action — self-exempting. Use grep -vixF and normalize/trim entries. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, kimi-k2.7-code adversarial).
| echo "Failed to fetch pull request commits" >&2 | ||
| exit 1 | ||
| fi | ||
| others=$(printf '%s\n' "$commit_authors" | sort -u | grep -vix "${PR_AUTHOR}" | paste -sd, -) |
There was a problem hiding this comment.
🟡 Medium — grep -vix exits 1 when the PR author is the only committer — the common case — and the new if ! guard covers only the gh call, not this pipeline. Today the step stays green solely because the default step shell is bash -e {0} without pipefail and the assignment takes only paste's status; adding an explicit shell: bash (which is -eo pipefail) or set -o pipefail would break every single-author PR. Append || true to the grep so the intent is explicit and the step is safe to harden later. Raised by 4 of 8 reviewers (gemini-3.1-pro edge-case, gemini-3.1-pro adversarial, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| echo "Failed to fetch pull request commits" >&2 | ||
| exit 1 | ||
| fi | ||
| others=$(printf '%s\n' "$commit_authors" | sort -u | grep -vix "${PR_AUTHOR}" | paste -sd, -) |
There was a problem hiding this comment.
🟢 Low — Since pipefail is off, a failure in sort or grep (e.g. grep's exit 2 on an error) leaves others silently empty and emits a base-only allowlist — the same silent-wrong-allowlist outcome this diff sets out to prevent, now only closed for the gh call. Setting set -o pipefail at the top would close the gap, but only in combination with the || true fix above. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
| if ! commit_authors=$(gh api "repos/${{ github.repository }}/pulls/${PR_NUMBER}/commits" --paginate \ | ||
| --jq '.[] | (.author.login // .commit.author.name // empty), (.committer.login // .commit.committer.name // empty)'); then | ||
| echo "Failed to fetch pull request commits" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
🟢 Low — The new exit 1 fails the whole step, and the following "CLA Assistant" step has no always()/if: override, so it is skipped — a transient gh api blip now means no CLA status is ever posted for the PR (and a recheck comment silently does nothing), where previously the run degraded to the base allowlist. Consider falling back to BASE_ALLOWLIST with a warning annotation instead of hard-failing. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
| others=$(gh api "repos/${{ github.repository }}/pulls/${PR_NUMBER}/commits" --paginate \ | ||
| --jq '.[] | (.author.login // .commit.author.name // empty), (.committer.login // .commit.committer.name // empty)' \ | ||
| | sort -u | grep -vix "${PR_AUTHOR}" | paste -sd, -) | ||
| if ! commit_authors=$(gh api "repos/${{ github.repository }}/pulls/${PR_NUMBER}/commits" --paginate \ |
There was a problem hiding this comment.
⚪ Nit — ${{ github.repository }} is interpolated directly into the shell script of a pull_request_target job holding a write-scoped token, while the neighboring values (PR_NUMBER, PR_AUTHOR) correctly come through env:. A repository slug cannot contain shell metacharacters so this is not exploitable, but it is the one spot in the step that deviates from the safe pattern. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
|
@mattmillerai not sure if you're the right person for this, but any idea what's happening with the CI? |
2 participants
Summary
Make the CLA author allowlist step fail when its GitHub API request fails instead of silently continuing with only the base allowlist.
Changes
Verification
The shared implementation was validated for author-only, co-author, and simulated API-failure scenarios in
Comfy-Org/comfy-cla.Propagates Comfy-Org/comfy-cla#1 and addresses Comfy-Org/ComfyUI_frontend#15555.