Skip to content

chore: add PR gate workflows and contribution policy - #282

Open
melekes wants to merge 2 commits into
circlefin:mainfrom
melekes:chore/pr-gate-workflows
Open

chore: add PR gate workflows and contribution policy#282
melekes wants to merge 2 commits into
circlefin:mainfrom
melekes:chore/pr-gate-workflows

Conversation

@melekes

@melekes melekes commented Aug 26, 2026

Copy link
Copy Markdown

Summary

  • Add Malachite-style pr-gate.yml, pr.yml, and need-triage-label.yml so unsolicited external PRs are closed unless the author is an org member/collaborator or assigned to a referenced issue.
  • Update CONTRIBUTING.md and the README Contributing section with the assignment-before-PR policy.
  • Add a PR template requiring Closes: #XXX.

Test plan

  • Open a test PR from a non-member without an assigned issue and confirm PR Gate closes it with a helpful comment and need-triage label
  • Confirm Circle org members / collaborators are not blocked
  • Confirm Copybara bot (circle-github-action-bot) PRs are allowed
  • Confirm conventional title check and unsigned-commit check run on a PR
  • Confirm new issues from non-members get need-triage

Made with Cursor

Port Malachite-style eligibility, title/signature, and issue triage
workflows, and document the no-unsolicited-PR policy for external contributors.
@osr21

osr21 commented Aug 26, 2026

Copy link
Copy Markdown

Looked through both workflows and the policy docs. The security posture here is right for what pull_request_target demands: no checkout anywhere, explicit SECURITY headers stating why, actions pinned to SHAs consistent with the rest of the repo, github.repository == 'circlefin/arc-node' guards so forks don't run the gate, and scoped per-job permissions. The need-triage label already exists in this repo, and allowlisting circle-github-action-bot matches how the Copybara mirror lands changes. The opened/reopened trigger pair also gives closed authors a clean retry loop: fix the body, reopen, get re-checked.

A few findings from checking the workflow's assumptions against the actual repo state:

1. This repo has no CODEOWNERS file — two of the six eligibility paths are dead on arrival. There is no .github/CODEOWNERS (nor at the root or docs/), so check-codeowner always takes the catch path with an empty list, and check-tagged-by-codeowner — the /assign @user escape hatch — can never fire, since it requires the /assign comment to come from a codeowner. Net effect as merged: the only way an external contributor passes the gate is being a literal GitHub assignee on the referenced issue. That's a coherent policy, but worth stating deliberately, because (a) maintainers may believe the /assign comment convention works when it silently doesn't, and (b) CONTRIBUTING.md never mentions /assign either, so nothing breaks visibly. Either commit a CODEOWNERS file alongside this PR, or drop the two dead steps to keep the gate honest. One related note if CODEOWNERS is added later: the @[\w-]+ extraction turns a team entry like @circlefin/some-team into the username circlefin, so team-syntax files would still leave the individual-matching logic empty.

2. check-org-member is likely inert with the default GITHUB_TOKEN. orgs.checkMembershipForUser needs org-membership read that the installation token doesn't get by default, so it will 404 for private-membership members — and public members never reach this step because author_association == MEMBER already passed them. The failure mode this leaves open: a Circle engineer with private org membership and no direct collaborator grant on this repo gets their PR auto-closed by their own gate. The test plan's "confirm org members aren't blocked" item is the right instinct — I'd suggest running that test specifically with a private-membership member, since a public member passing proves nothing about this step.

3. Defense-in-depth on the ${{ }} interpolations. check-tagged-by-codeowner and the close step inline step outputs directly into JavaScript source (parseInt('${{ ... }}'), JSON.parse('${{ ... }}')). Today this is safe only by regex accident — issue_number is a \d+ capture, usernames are [\w-]+, reason is from a fixed set. But this is the exact pattern that becomes script injection in a pull_request_target workflow with pull-requests: write the moment someone loosens one of those regexes. Passing the outputs through env: and reading process.env costs three lines and removes the trap.

Small nits, take or leave: the signature-check cleanup deletes any comment containing the marker string, so a user quoting "⚠️ Unsigned Commits Detected" in a reply gets their comment deleted (matching on the bot's own login would be tighter); and README's new "We invite all contributors" sits a little oddly one click away from CONTRIBUTING's "We do not accept unsolicited contributions" — the README line may be worth aligning with the actual policy to avoid setting up the exact first-contact disappointment the gate then delivers. For what it's worth, recent main history is fully verified-signed, so the signature check should be quiet in practice.

None of this blocks the direction — the gate logic itself sequences its checks correctly (skipped-step outputs can't accidentally satisfy the close condition; I traced the conditions). Point 1 is the one I'd resolve before merge, since it decides whether the documented contribution path actually exists.

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