ci: implement ruleset-first safe PR Gate by mattbodle · Pull Request #880 · mParticle/mparticle-apple-sdk

mattbodle · 2026-08-28T20:13:43Z

Summary

Implements a ruleset-first status gate for a narrow documentation-only safe surface. It keeps public CODEOWNERS unchanged and removes the dedicated reviewer user and reviewer token entirely.

Design

The Gate posts only a source-pinned check run. It cannot approve, merge, push, change repository settings, or bypass rules.
The main ruleset moves from global Code Owner enforcement to path-aware SDK-team required reviewers: all paths except the five explicit Markdown paths remain manual-review protected.
A verified Rokt employee with a safe diff and passing Pull request workflow receives Gate success without a review.
A non-employee safe PR remains blocked by the Gate until a fresh SDK-team approval is observed on the current SHA.
A source or mixed PR receives Gate success immediately, but the ruleset still requires SDK-team approval; this deliberately preserves the repository current advisory-CI behavior for non-safe changes.
The Gate reads only the trusted default branch. It has scheduled and manual rechecks for fork-safe review propagation, re-evaluates base retargets, and fails closed for a head SHA shared by multiple open PRs.

Credential model

Two narrowly scoped GitHub Apps are required:

Rokt Safe PR Gate, installed only on mParticle/mparticle-apple-sdk, for checks and read-only PR, Actions, contents, and team membership access.
Rokt Employee Lookup, installed in ROKT, for Members read only.

There is no machine user, PAT, automated PR review, or ruleset bypass actor.

Required administrator setup

The deployment steps, variables, secrets, audit mode, fixture PRs, atomic ruleset edit, and rollback ordering are in .github/ROKT_SAFE_PR_GATE_SETUP.md. Start with audit mode for one to two weeks before enabling the required status check.

Validation

trunk check on all eight modified files
node syntax check
13 Gate unit tests
YAML parsing for both modified workflows
JSON policy parsing
prior branch validation: iOS build plus Objective-C and Swift test suites

Notes

The size-report comment is intentionally skipped for fork PRs so its read-only GITHUB_TOKEN cannot make Pull request fail. Its job summary and measurement still run.

Summary by CodeRabbit

New Features
- Added the Rokt Safe PR Gate to evaluate pull requests against file-path, size, workflow, and review requirements.
- Added scheduled, manual, and pull-request-triggered evaluations with visible check results.
- Added audit mode and SDK-team review validation.
Documentation
- Added setup, validation, configuration, and rollback guidance for the PR gate.
Bug Fixes
- Limited pull-request notifications and size-report comments to eligible non-draft, non-fork pull requests.

cursor · 2026-08-28T20:13:50Z

PR Summary

High Risk
Introduces merge-gating logic, pull_request_target, and org membership checks; misconfiguration or premature ruleset enforcement could block merges or widen the “safe” surface.

Overview
Adds a Rokt Safe PR Gate that posts a required-style check run for a narrow allowlist of Markdown-only PRs, using trusted default-branch policy and separate GitHub App tokens for mParticle (checks/PR/Actions) and ROKT (employee team membership).

The new composite action and rokt-safe-pr-gate workflow evaluate path/size/workflow/review rules: verified Rokt employees on eligible diffs can get Gate success after Pull request CI succeeds; others need a fresh sdk-team approval on the current SHA; mixed or source changes get Gate success while the documented ruleset still requires SDK review. Audit mode reports neutral “would have” outcomes until admins flip to enforce. Triggers include pull_request_target, completion of the main PR workflow, a five-minute schedule (bounded API use for fork rechecks), and manual re-run by PR number.

Also ships .github/rokt-safe-pr-gate-policy.json, setup/rollback docs (ROKT_SAFE_PR_GATE_SETUP.md), gate unit tests on every PR workflow run, and skips fork PR comments for size-report and pr-notify so read-only tokens cannot break fork CI.

^{Reviewed by Cursor Bugbot for commit 34be4a8. Bugbot is set up for automated code reviews on this repo. Configure here.}

mattbodle · 2026-08-28T20:52:13Z

Design review — Rokt Safe PR Gate

Reviewed the plan, the setup doc and the implementation at 653970a2. The shape is right and
the risky parts are handled well: pull_request_target is used correctly (permissions: {},
checkout pinned to default_branch, persist-credentials: false, actions pinned by SHA, no PR
content checked out/executed/interpolated), the App token is pull-requests: read rather than
the write the plan asked for, identity comes from IdP-synced team membership rather than
author_association, and classifyFiles covers the whole file-operation surface in four lines
(added|modified, mode === 100644 so symlinks/submodules/exec bits are out, required patch
so binaries are out). The check is pinned to the App id and the approval to the exact head SHA.

Three blockers before the ruleset is touched, plus one number I think should be decided first.

B1 — As a required check, this permanently blocks every fork PR

size-report / measure-size fails on this PR at "Create or update PR comment" with
Resource not accessible by integration (Secret source: None, job 98974973862) — a fork
PR's GITHUB_TOKEN is read-only regardless of the workflow's permissions: block. That fails
the Pull request run → evaluateWorkflows returns failed → the Gate posts failure → as a
required strict check the PR is unmergeable, with no path to green. Fork PRs are the entire
external-contributor path on a public SDK, and this PR is itself a fork PR, so the Gate could
not pass its own gate.

Not just forks: of the last 100 pull-request.yml runs on pull_request events —
33 success / 24 failure / 41 cancelled / 1 startup_failure. So ~42% of completed PR CI runs
are red today, on a repo whose main ruleset currently requires zero status checks
(confirmed: ruleset 6260587 has deletion, non_fast_forward and pull_request rules only).
And because evaluateWorkflows runs before classifyFiles, the Gate check would apply that
failure to source PRs too — so this quietly turns "CI is advisory" into "CI is mandatory"
repo-wide. tvOS native tests are red on this PR right now.

The required check also buys nothing for the feature: the Gate already waits for CI to succeed
before approving. The only thing it adds is "CI must be green for everything", which is a
separate repo-wide policy decision that deserves its own PR naming the real job names as
required checks.

Suggestion: drop the required-check half and ship the approver. That also removes
checks: write, the source-pinning requirement, the in_progress parking, the ruleset edit,
and every deadlock in B2 below. The failure mode degrades to "you waited for a human", i.e. the
status quo.

Also worth flagging: strict_required_status_checks_policy: true combined with this repo's
dismiss_stale_reviews_on_push: true means an approved-but-behind PR has to update its branch,
which dismisses the approval that let it merge. Harmless for safe PRs (the Gate re-approves),
a standing tax on every source PR.

B2 — The fail-closed paths are fail-silent, and they deadlock a required check

Each of these leaves a required check non-terminal, which is worse than red:

Pending parks forever if the workflow name drifts. The only retrigger for a parked
in_progress check is a workflow_run completion for a workflow whose name string is
exactly Pull request. Rename name: in pull-request.yml, or add a run-name: (which
overrides run.name), and every PR in the repo parks silently. Keying the requirement on
run.path == '.github/workflows/pull-request.yml' can't drift.
No check is posted at all if anything fails before activeGate is set — App token
minting, checkout, policy/event parse, the PR fetch, the /commits/{sha}/pulls pagination.
main().catch has nothing to write to.
Cancellation and timeout write nothing. cancel-in-progress: true groups both event types
for one SHA together, so they can cancel each other, and cancellation isn't a JS exception.
Same for timeout-minutes: 5.
ROKT_SAFE_PR_GATE_ENABLED != 'true' skips the job, so no check appears. This makes the
documented rollback order wrong: the setup doc says set the variable false then remove the
check, which is a total merge freeze on main in between. Reverse it — remove the required
check first — and state the ordering in the doc.

A workflow_dispatch entry point (PR number as input) would let a parked check be re-driven
without a force-push.

B3 — A base-branch retarget keeps a stale Gate approval

The Gate approves the diff against the PR's current base and never dismisses: the ineligible
and non-employee paths post success and leave any existing approval standing.
dismiss_stale_reviews_on_push fires on a push; changing a PR's base isn't a push and doesn't
change the head SHA.

Push a branch B containing a source change.
Open a PR from B + README.md targeting B → diff is README-only → Gate approves.
Retarget the base to main. The diff now contains the source change and the machine user's
approval still satisfies require_code_owner_review for *.

That lands arbitrary source in main with no SDK-team review. It needs employee-team membership
plus write access, so it's an insider path rather than an anonymous one — but "trusted to edit a
README" is a much wider population than sdk-team, which is the premise of the whole feature.

Two small fixes, both worth doing regardless: add edited to the pull_request_target
types (GitHub sends it with changes.base), and make the ineligible/non-employee paths
dismiss an existing Gate approval instead of only posting success. The invariant should be
a Gate approval exists iff the PR qualifies right now, not iff it qualified once. I couldn't
test a retarget against this ruleset, so treat the dismissal semantics as unconfirmed — but
please add it as PoC fixture #6.

Should-fix

The sdk-team machine user deserves an explicit justification in the PR body. The plan
deliberately avoided it (require_code_owner_review: false + path-scoped
required_reviewers, "it need not join sdk-team because safe paths are excluded from that
team requirement"). Retaining global code-owner enforcement is the safer half of that swap,
but it means the ruleset no longer pins the safe surface — lib/gate.js is the only thing
doing it, and ROKT_SAFE_PR_GATE_REVIEWER_TOKEN becomes a standing "one code-owner approval
on any path" capability. It can't merge or push, so exploitation needs a second actor, but
the plan's defense in depth is gone. If it stays: scope the PAT to this repo with Pull
requests: write only, calendar its expiry (an expired PAT is a Gate failure, which under B1
is a merge freeze), and check the team's review auto-assignment so the bot is never
round-robined onto real PRs.
validatePolicy blocks globs but not source paths. safePaths: ["mParticle-Apple-SDK/MPBackendController.m"] passes validation today, so the safe surface is
one policy edit away from source. Requiring every safe path to end in .md is one line. Also
validate the caps exist: files.length > undefined is false, so dropping maxFiles or
maxChangedLines from the policy silently removes the limit rather than failing; a missing
gateCheckName would POST name: undefined.
The glob-rejection test asserts the wrong error. validatePolicy({ safePaths: ["*.md"] })
throws on the missing requiredWorkflows (/required workflow/), not on the glob — so
path.includes("*"), the most security-relevant line in the file, has zero coverage and would
survive being deleted. { ...policy, safePaths: ["*.md"] } asserting /explicit/ fixes it.
Plan condition 5 isn't implemented — nothing checks for a blocking CHANGES_REQUESTED or
an unresolved thread, so the Gate will approve over a maintainer's change request and re-post
on every synchronize. The reviews are already fetched; bailing on a CHANGES_REQUESTED newer
than the last Gate approval, plus a no-auto-approve label, gives humans a durable override.
There's no audit mode, but rollout step 1 and PoC step 1 both require 1–2 weeks of
audit-only operation. ROKT_SAFE_PR_GATE_ENABLED=false doesn't run the job, so there's
nothing to review. A ROKT_SAFE_PR_GATE_MODE=audit|enforce variable (audit evaluates, logs,
posts a neutral check, never approves) is ~10 lines and is a stated prerequisite.
Token scope: create-github-app-token is called without repositories:, so the mParticle
token covers every repo the App is installed on in that org — worth pinning to this
repository. And one App/one private key serves both installations, so the key held in this
public repo's secrets also mints the org-membership-read token; two Apps keep those blast
radii separate.

Smaller notes

X-GitHub-Api-Version: 2026-03-10 isn't a published version (only 2022-11-28 is). It
returns 200 today because unknown values are ignored, but if that's ever validated every call
400s. Suggest 2022-11-28.
evaluateWorkflows maps cancelled to failed. Cancellations are routine here (41 of the
last 100 runs, from cancel-in-progress) and a re-run fixes them — cancelled/stale/
skipped read better as pending-with-reason.
getOpenPullRequestNumber takes the first open PR for a SHA; check runs are per-SHA, so with
two open PRs on one head SHA the Gate evaluates one and decorates both.
tree.truncated → hard failure. 1,849 tracked files, so unreachable today; noted only as
another fail-closed hard block while the check is required.
The ruleset has require_extra_approval_for_unattributed_changes: true, so a Gate success
won't always mean mergeable. Worth a line in the setup doc so the first PoC surprise isn't
chased as a Gate bug.
Rokt Safe PR gate tests runs the PR's own copy of the test file, so its green tick is
PR-controlled. The Gate itself correctly reads its action and policy from the default branch,
so this is cosmetic — just not a control.

The number I'd want decided first

Replaying the last 300 first-parent commits on main (2024-11-27 → 2026-08-28, ~21 months)
against the shipped policy:

Allowlist	Would have qualified
The 5 paths in `rokt-safe-pr-gate-policy.json`	2 of 300 (#741, #310)
Every `*.md` outside `.github/`	10 of 300 — and 2 of those touched 28–29 files, which `maxFiles: 10` rejects

18 commits touched an allowlisted file but were mixed with code, and the Gate correctly declines
all of those. So the current allowlist is worth roughly one automated approval every ten months,
and the widest plausible allowlist about one every ten weeks. CHANGELOG.md — the most
frequently edited doc here — is excluded and is written by release automation anyway, and the two
kit-README sweeps in the window both exceed the file cap.

That's set against a GitHub App installed in two orgs, a private key and a machine-user PAT in a
public repo's secrets, a machine account that is a code owner for every path, and the repo's
first-ever required status check. Worth confirming with the SDK team that the trade is one they
want, because the cheaper alternative is the plan's original design: path-scoped
required_reviewers negations and nothing else — no credentials, no machine user, no required
check. What that gives up is author-conditionality (a public contributor's docs PR would also
clear with any collaborator approval), which rulesets genuinely cannot express and which is the
honest argument for this Gate existing. If you go that route, verify GitHub actually honours
! negation in required_reviewers.file_patterns first — the plan depends on it and I couldn't
confirm it.

PoC additions

On top of the plan's five fixtures: (6) base retarget after approval; (7) a fork PR, to see what
the Gate concludes when size-report can't comment; (8) a rollback drill with the variable
flipped and the check still required; (9) a workflow name: rename; (10) a PR carrying
CHANGES_REQUESTED.

The pre-change ruleset JSON is worth capturing before any edit, as the plan says.

mattbodle · 2026-08-28T21:32:55Z

Re: the design review, the ruleset-first revision is pushed in ffed9db.

Removed the machine reviewer, PAT, review submission, and global Code Owner enforcement. The ruleset now pins SDK-team review to all paths except the explicit safe Markdown files, while the App emits only the required status check.
Safe employee PRs wait for the configured Pull request workflow; non-safe and mixed PRs now receive Gate success before CI so this does not convert existing advisory CI into a repository-wide requirement.
Fork size-report comment creation is skipped, so a read-only fork token cannot make the workflow fail. Fork manual-review rechecks use the trusted five-minute schedule or dispatch instead of a secret-bearing review event.
Added audit and enforce modes, base-retarget handling, no cancellation overlap, path-based workflow verification, safe policy caps and Markdown validation, blocking team change-request handling, and a shared-head fail-closed guard.
Split credentials into a repository-scoped mParticle Gate App and a Rokt Members-read App; the API version is now the published 2022-11-28 value.

The setup guide now describes the audit period, PoC fixtures, atomic ruleset change, and rollback order. Focused validation is green: trunk, 13 Node tests, workflow YAML, and policy JSON.

cursor