Skip to content

Accept reviewed external contributors via base-branch allowlist#343

Merged
sayalinvidia merged 2 commits into
mainfrom
external-contributor-allowlist
Jul 15, 2026
Merged

Accept reviewed external contributors via base-branch allowlist#343
sayalinvidia merged 2 commits into
mainfrom
external-contributor-allowlist

Conversation

@mosheabr

Copy link
Copy Markdown
Collaborator

Onboarding type

  • New product onboarding (new components.d/<slug>.yml file)
  • Other (catalog change, README fix, infrastructure, etc.)

What

Adds .github/external-contributors.yml and teaches the verify-authors check to consult it.

External contributions were mechanically blocked: verify-authors only accepts @nvidia.com or github-noreply emails, so even a reviewed and approved external PR could not pass CI unless the contributor rewrote their commits to a noreply address — which hides real provenance instead of recording it.

New flow: after a maintainer runs the contribution review, they record the contributor's email in the allowlist, scoped to specific PR numbers, with reviewer and date. The authors check then accepts that email for those PRs only.

Security properties

  • A PR cannot allowlist itself. The check reads the allowlist from the BASE branch (git show $BASE_SHA:...), never from the PR head.
  • No blanket entries. prs is required; an email is accepted only for its listed PR numbers.
  • Unparsable allowlist grants nothing — YAML errors fail closed.
  • Everything else (nvidia.com / noreply patterns, bot-branch exemption) is unchanged.

Tested locally

  • Both YAML files parse; extracted run script passes bash -n.
  • NVIDIA-email commits: pass (unchanged behavior).
  • Synthetic commit authored+committed by the seeded external email: accepted on PR 76, rejected on PR 99 with the updated guidance text.

Seed entry

wenyuchiou12@gmail.com scoped to PR #76 (one-line CONTRIBUTING.md anchor fix), reviewed 2026-07-13. Merging this unblocks #76 without the contributor rewriting history.

Also replaces the failure-message wording with the maintainer-review flow.

External contributions were mechanically blocked: verify-authors only
accepts @nvidia.com or github-noreply emails, so a reviewed and
approved external PR could not pass CI without the contributor
rewriting their commits to a noreply address, which hides the real
provenance instead of recording it.

This adds .github/external-contributors.yml. After a maintainer runs
the contribution review, they record the contributor's email scoped to
specific PR numbers. verify-authors reads the allowlist from the BASE
branch only, so a PR cannot allowlist itself, and accepts listed
emails for their listed PRs.

Seeds the list with the reviewed one-line docs fix in PR #76.

Signed-off-by: Moshe Abramovitch <moshea@nvidia.com>
@mosheabr mosheabr requested a review from sayalinvidia as a code owner July 13, 2026 18:40
@mosheabr mosheabr requested a review from sayalinvidia July 13, 2026 18:40
@mosheabr mosheabr requested a review from jasonnvidia as a code owner July 13, 2026 18:40
@sayalinvidia

Copy link
Copy Markdown
Collaborator

This is great, happy to see we are accepting external contribs now @mosheabr

One question, not a blocker: are we ok adding a YAML entry for every external contributor going forward? For how rarely we get them today it's totally fine. I'm just thinking about the toil if it picks up — each one needs a new entry, a merge, and a re-trigger on the PR. Might be worth a note that this is a strict v1, and down the line a maintainer-applied external-approved label could do the same thing with less overhead.

Curious what you think, happy to keep it as-is if you expect these to stay infrequent.

A few things before this goes in:

Does DCO still block #76? This teaches the authors check about external contributors, but if the sign-off check is separate, #76 might still be red until the commit has a Signed-off-by. Worth double-checking so we know this actually unblocks it.
PyYAML — if import yaml ever isn't on the runner, the || true swallows the error and the whole allowlist silently stops working (reviewed PRs get rejected with no clue why). Could we install it explicitly or at least fail loudly?
Heads up on re-runs: after we merge an allowlist entry, the target PR won't see it on a plain re-run — base.sha only refreshes on a new push/reopen. Worth a line in the guidance so it doesn't look broken.

…e tip

Per review feedback on #343:
- Preflight the YAML parser and make an unparsable allowlist fail the
  job with a visible error, instead of silently granting nothing.
- Read the allowlist from the tip of the base branch rather than the
  event's base SHA, so a re-run picks up entries merged after the PR
  was opened. Same security property: still base-branch content only.
- Note in the allowlist header that per-entry flow is a deliberate v1.

Signed-off-by: Moshe Abramovitch <moshea@nvidia.com>
@mosheabr

Copy link
Copy Markdown
Collaborator Author

Thanks Sayali, all three were good catches.

On DCO and 76, I verified the commit already carries a Signed-off-by trailer, and the DCO check validates the trailer not the email domain. So only the authors check was blocking, and this PR fully unblocks it.

On PyYAML, you were right that the fallback swallowed it. Pushed a fix that preflights the parser and makes an unparsable allowlist fail the job loudly with a visible error, so it can't silently reject reviewed externals.

On re-runs, fixed properly rather than documented. The check now reads the allowlist from the tip of the base branch instead of the event's base SHA, so a plain re-run picks up entries merged after the PR was opened. A PR still can't allowlist itself.

On the toil question, at current volume per-entry is fine and the friction doubles as the review record. I added a note in the file that this is a deliberate v1 and your maintainer-label idea is the likely v2 if volume grows.

@sayalinvidia sayalinvidia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the issues @mosheabr
LGTM!! Approving!

@sayalinvidia sayalinvidia merged commit 1700ebd into main Jul 15, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants