docs: explain org-member review flow for external contributor PRs#18
Conversation
Document the UX for org members handling external and fork contributor PRs: approve the workflow run, then request a review from docker-agent via GitHub's native review-request UI. No special commands or workflow inputs are needed. Additive only (new sections), kept out of the regions PRs #16 and #13 edit, so it merges cleanly against both. Aligns with PR #16's requester-authorized model, which is what enables review of external contributor PRs; that behavior requires PR #16 to land first.
derekmisler
left a comment
derekmisler
left a comment
There was a problem hiding this comment.
good effort structuring this, and the merge-order note in the PR description shows good instincts. two accuracy issues though: the CONTRIBUTING.md auto-review claim doesn't match this repo's actual trigger config, and the review-pr/README.md section documents a flow that silently fails without PR #16 landing first. details inline.
|
|
||
| This repo uses the `docker-agent` AI reviewer on pull requests. How a review is triggered depends on who opened the PR: | ||
|
|
||
| - **Org members:** a review runs automatically when the PR is opened or marked ready for review. Re-request a review from `docker-agent` in the sidebar to re-run it. |
There was a problem hiding this comment.
issue: this isn't accurate for contributors to this repo. self-review-pr-trigger.yml only fires on pull_request: types: [review_requested], there's no opened or ready_for_review trigger here. self-review-pr.yml listens only for issue_comment and workflow_run. so org members also need to explicitly request a review; the only difference vs external contributors is skipping the workflow-approval step.
(the Mode B/A examples in review-pr/README.md do include opened/ready_for_review, but those are setups for consumers of the reusable workflow, this repo's own self-review config doesn't use them.)
suggested fix:
- **Org members:** request a review from `docker-agent` in the PR sidebar (Reviewers → add `docker-agent`). The review starts automatically once requested.There was a problem hiding this comment.
Fixed in d4b7ac3. The org-member bullet now reads "request a review from docker-agent in the PR sidebar (Reviewers, add docker-agent); the review starts automatically once requested." The external/fork bullet is reframed so the workflow-run approval is the only extra step, matching self-review-pr-trigger.yml (fires on review_requested only, no opened/ready_for_review).
| 1. **Approve the workflow run.** For PRs from first-time and external contributors, GitHub holds all Actions runs until a maintainer approves them (governed by the repository's `Settings` → `Actions` → `General` fork-PR approval policy). Click **Approve and run workflows** on the PR; until then nothing runs, including the PR review trigger. | ||
| 2. **Request a review from `docker-agent`.** In the PR sidebar, under **Reviewers**, add `docker-agent`. This fires a `review_requested` event and starts the review, shown as a check run. | ||
|
|
||
| That is the entire flow. **No special commands or workflow inputs are needed**: not the deprecated `/review` comment, not `workflow_dispatch`, and no caller-side configuration. The review is authorized by the requesting org member rather than the PR author, which is what lets an external contributor's PR be reviewed on demand. The request is safe by construction: GitHub only lets users with triage or write access request a reviewer, and the reusable workflow verifies org membership before any review work runs. An external contributor cannot trigger a review of their own PR. |
There was a problem hiding this comment.
issue: this documents behavior that doesn't exist yet on main without PR #16. in the current check-org-membership/index.ts, for PR_SOURCE=event the username is COMMENT_AUTHOR, which is "" on a pull_request event (github.event.comment is null). passing an empty string to the membership check returns 404 → is_member=false → review silently skipped. the REQUESTER env var and evaluateMembership/resolveReviewRequester logic are all added by PR #16.
your PR description says "merge #16 first", which is the right call. but that's engineering metadata in the PR body, not user-facing. if merge order slips, anyone following these docs hits a two-step flow that quietly does nothing.
recommend adding a visible caveat in the doc itself, e.g.:
> [!NOTE]
> This flow requires the `check-org-membership` update shipping with PR #16. Merge that PR before this one.There was a problem hiding this comment.
Added in d4b7ac3 as a > [!NOTE] at the top of the section: the requester-authorized path requires the check-org-membership update from PR #16; until it ships, membership is checked against the PR author rather than the requester, so the review is silently skipped. This makes the merge-order dependency visible in the doc itself rather than only in the PR description.
| Auto-review only runs on PRs authored by org members. A PR opened by an external or fork contributor is **not** reviewed automatically. To get one reviewed, an org member drives it through GitHub's native UI in two steps: | ||
|
|
||
| 1. **Approve the workflow run.** For PRs from first-time and external contributors, GitHub holds all Actions runs until a maintainer approves them (governed by the repository's `Settings` → `Actions` → `General` fork-PR approval policy). Click **Approve and run workflows** on the PR; until then nothing runs, including the PR review trigger. | ||
| 2. **Request a review from `docker-agent`.** In the PR sidebar, under **Reviewers**, add `docker-agent`. This fires a `review_requested` event and starts the review, shown as a check run. |
There was a problem hiding this comment.
suggestion (non-blocking): the existing "What you get" table on this same page says "Shows as a check run if checks: write is granted." the Create check run step in review-pr.yml has continue-on-error: true, so the review still runs without that permission, just without a visible check run. this line is inconsistent with that caveat.
quick fix: "shown as a check run (if \checks: write` is granted)"` to match the rest of the doc.
There was a problem hiding this comment.
Fixed in d4b7ac3: step 2 now reads "shown as a check run (if checks: write is granted)", matching the "What you get" table and the continue-on-error: true on the Create check run step in review-pr.yml.
- CONTRIBUTING.md: org-member PRs are not auto-reviewed on open or ready-for-review. The self-review trigger fires only on review_requested, so org members also request a review explicitly; the only difference from external contributors is approving the workflow run. - review-pr/README.md: add a visible note that the requester-authorized path depends on the check-org-membership update from PR #16; without it, membership is checked against the PR author and the review is silently skipped. - review-pr/README.md: caveat the check run with "if checks: write is granted" to match the rest of the doc (the Create check run step uses continue-on-error).
derekmisler
left a comment
derekmisler
left a comment
There was a problem hiding this comment.
all issues addressed and ci is green, approving!
Reconcile the abuse safeguards with the requester-based authorization rework that landed on main (PRs #16, #17, #18). check-org-membership: take main's evaluateMembership model. It subsumes this branch's resolveUsername helper (the directly wired pull_request auto-review path now verifies the PR author via a live API lookup) and adds the review_requested requester path so an org member can pull an external contributor's PR into review. review-pr.yml: provide EVENT_NAME, EVENT_ACTION and REQUESTER to the membership step; drop the PR_AUTHOR env that main's code never reads. tsup.config.ts: keep both the rate-limit (this branch) and score-confidence (main) entry points. review-pr/README.md: keep main's requester-model wording and preserve this branch's audit, throttle and force-push safeguards. SECURITY.md: update section 1 to the requester-based model and remove the stale resolveUsername and PR_AUTHOR references.
2 participants
Important
Merge #16 first. This PR documents the requester-authorized review flow for external and fork contributor PRs, which only works once #16 (
feat(review-pr): authorize review_requested via the requester for external contributors) lands. Merging this ahead of #16 would describe a flow that does not yet function and would briefly contradict the on-main defense-in-depth text that #16 updates.Summary
Documents the UX for org members handling external and fork contributor PRs across three docs. The flow, with no special commands or workflow inputs:
docker-agentvia the native review-request UI (PR sidebar, Reviewers).review-pr/README.mdExternal and fork contributor PRssection with the full two-step flowCONTRIBUTING.mdAutomated PR Reviewsection (org members vs external and fork contributors)README.mdAlignment with open PRs
Conflict avoidance
Changes are additive. The new section in
review-pr/README.mdis inserted beforeCustomizing, outside the regions #16 and #13 edit (the "What you get" trigger table and the defense-in-depth blockquote), so it merges cleanly against both.Validation
#external-and-fork-contributor-prs)