[No QA] Replace contributorValidationGate with isAuthorizedContributor action#91485
[No QA] Replace contributorValidationGate with isAuthorizedContributor action#91485roryabraham wants to merge 18 commits into
Conversation
Centralize gate logic and Contributor+ team membership ahead of linked issue/PR checks. Co-authored-by: Cursor <cursoragent@cursor.com>
Bundle contributor authorization into a node24 action with ncc and rebuild actions that embed CONST after adding contributor-plus team slug. Co-authored-by: Cursor <cursoragent@cursor.com>
…or action Colocate authorization checks in caller workflows and authorize Contributor+ team members. Co-authored-by: Cursor <cursoragent@cursor.com>
Local actions require the repository on the runner per App workflow guidelines. Co-authored-by: Cursor <cursoragent@cursor.com>
Match @octokit/request-error RequestErrorOptions shape for typecheck. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove ContributorAuthorization lib; export helpers from the action entry for unit tests. Drop CONTRIBUTOR_PLUS_TEAM_SLUG from CONST. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Dedupe link parsing and error logging; add isMatchingLogin helper. Co-authored-by: Cursor <cursoragent@cursor.com>
PR template issue links are visible and placeholders in comments do not match issue IDs. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove loginsMatch and isMatchingLogin helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
Being the author or reviewer of some unrelated PR is not a meaningful authorization signal; contributors with merged PRs already have CONTRIBUTOR association which is checked first. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
864e3a2 to
dbbacf6
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@stitesExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b1831a2b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const prBody = pr.body ?? ''; | ||
| const isAuthorized = await isAuthorizedViaLinkedIssues(prBody, prAuthor); |
There was a problem hiding this comment.
Restore linked-PR authorization checks for contributors
This change regresses the contributor gate by removing the linked-PR authorization path: isAuthorizedContributor() now only evaluates isAuthorizedViaLinkedIssues(...) after loading the PR body, so external authors who are valid via a linked PR (author/reviewer/requested reviewer) are incorrectly treated as unauthorized. That breaks the prior behavior used by validateContributorPR.yml/claude-review.yml and can cause legitimate contributor PRs to be auto-closed or skipped from downstream workflows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I intentionally removed this. Here's a snippet from my conversation with Claude:
Rory: What does this function imply? That a contributor can open a pull request because they have another one open? Do we need that for something, or could we just remove it entirely?
...
Claude: The logic implies three separate authorization paths via linked PRs:
- You authored the linked PR (you've already contributed work)
- You reviewed the linked PR (you've already done review work)
- You were requested to review the linked PR (an internal team member nominated you)
The third case (#3 — requested reviewer) is the most defensible: it's essentially "an Expensify employee explicitly invited this person to be involved." But cases #1 and #2 are weak — having authored or reviewed some other PR doesn't really mean you're authorized to open this one.
More importantly, if someone had a PR merged (case #1), they'd already have the CONTRIBUTOR association, which is checked first and would short-circuit before ever reaching this function. So the "you authored a linked PR" path only fires for unmerged PRs — which is even less meaningful as an authorization signal.
The linked issues check is the principled one: being assigned to an issue means someone explicitly tasked you with the work. The linked PR check has no equivalent clear intent.
My recommendation: Remove isAuthorizedViaLinkedPullRequests entirely. The only case worth preserving might be "requested reviewer," but that's a very unusual path and I'd want to know if it was ever actually used before keeping the complexity. Want me to delete the function and update the tests?
…ontributor Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # .github/workflows/authorChecklist.yml # .github/workflows/claude-review.yml
Explanation of Change
Background
contributorValidationGate.ymlhad four callers:What this PR changes
contributorValidationGatereusable workflow (which unnecessarily spins up a new runner per caller) with a TypeScriptisAuthorizedContributorGitHub action built via ncc.contributor-plusorg team (viaOS_BOTIFY_TOKEN), in addition to existing association, linked-issue assignee, and linked-PR reviewer checks. This is safe for all current use-cases but worht noting - Contributor+ will be able to open pull requests and have them auto-reviewed by Claude.Fixed Issues
side-quest for #90214
Tests
Automated tests only.
Offline tests
N/A – GitHub Actions only.
QA Steps
N/A – GitHub Actions only.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A