Skip to content

[PLA-2303] Report the human-review-policy status on PRs - #44

Closed
devtools-agent[bot] wants to merge 2 commits into
masterfrom
pla-2303-human-review-policy
Closed

devtools-agent[bot] wants to merge 2 commits into
masterfrom
pla-2303-human-review-policy

Conversation

@devtools-agent

@devtools-agent devtools-agent Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Linear: PLA-2303

What changed

Adds the two caller workflows for the shared human-review-policy check from rollbar/human-review-policy. That repo is public so SDK repos can use it; the private repos use the same code through rollbar/github-workflows#130.

  • .github/workflows/human-review-policy-trigger.yaml: a no-op that runs on PR events (opened, reopened, synchronize, ready_for_review, edited, closed) and review events (submitted, edited, dismissed).
  • .github/workflows/human-review-policy.yaml: runs on workflow_run of the trigger and calls the reusable workflow, which posts a human-review-policy commit status on the PR's head commit.

The status passes with 1 human approval besides the author on a human-authored PR and 2 distinct human approvals on a bot-authored one (e.g. devtools-agent[bot]). That way every production PR has two distinct humans on it. Approvals from bots and rollbar-circleci-machine don't count. Pushing commits to a PR doesn't stop someone from approving it.

Post-merge tracking: if a PR is merged without meeting the policy, e.g. through an admin break-glass bypass, it gets the needs-post-merge-review label and a comment saying how many approvals it still needs. Reviewers can still approve a merged PR. Once the policy is met, the label is removed and a closing comment is posted. To list what's still owed: is:pr is:merged label:needs-post-merge-review org:rollbar.

Why two files

pull_request workflows run the YAML from the PR's own branch, so a PR could rewrite the check that judges it. workflow_run workflows always run the copy on master. If a PR breaks the trigger, the status is never updated and the PR stays blocked.

Rollout order

  1. The first content of rollbar/human-review-policy has to be merged first. Until then the Human review policy runs here fail with "workflow not found". That's harmless, but noisy.
  2. Merge this PR. The status only starts appearing once human-review-policy.yaml is on master, because workflow_run only fires for workflows that are on the default branch.
  3. A repo admin then makes it required with the ruleset in ruleset.json: gh api -X POST repos/rollbar/rollbar-cli/rulesets --input ruleset.json. Org and repo admins can bypass it through a PR for incidents (break glass); post-merge review is required in those cases.

Not part of this PR: the ruleset itself (step 3).

Pull requests from forks

These work. The trigger runs on the fork PR, and the evaluation runs in this repo with this repo's token, finding the PR by its head branch. If first-time contributors need approval before their workflows run, the status appears once a maintainer approves the trigger run.

Merge queues

The trigger also runs on merge_group, so in repos with a merge queue the status is written on each merge group's commit as well, and the queue isn't left waiting for it.

Validation

  • The workflow YAML parses. The shared logic has 41 unit tests and a read-only dry run against real rollbar/mox PRs.
  • Not yet run end to end here; that happens after steps 1 and 2.

🤖 Generated with Claude Code

Adds the two caller workflows for rollbar/human-review-policy: a no-op
trigger on PR and review events, and a workflow_run workflow (always run
from master) that posts the human-review-policy commit status: 1 human
approval besides the author on a human-authored PR, 2 on a bot-authored
one. PRs merged without meeting it are labelled needs-post-merge-review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

PLA-2303

CR-192

Trigger on merge_group so a merge queue isn't left waiting for the
required status, and write the workflow name unquoted so Prettier
accepts it whatever the repo's quote style.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rollbar-circleci-machine

Copy link
Copy Markdown

AI Agent Review LGTM (openai, openai-astra)

LGTM. No blocking findings were found.

Review: no findings. Two caller workflows for the shared human-review-policy check

I read both added files and the repo's existing workflows (.github/workflows/node.js.yml, .github/workflows/publish.yml). I found no defects I could confirm.

What I checked

  • The two files are wired together correctly. human-review-policy.yaml:11 names workflows: [Human review policy trigger], which is exactly the trigger's name: at human-review-policy-trigger.yaml:1. types: [completed] (human-review-policy.yaml:12) fires whatever the trigger's conclusion, so a trigger run that fails still leads to an evaluation.
  • The trigger can't touch anything. permissions: {} (human-review-policy-trigger.yaml:14) and the only step is an echo. The run: line (human-review-policy-trigger.yaml:20) inserts only github.event.pull_request.number, a number, and github.event_name, a fixed set of values. Neither is text a PR author controls, so there is no script-injection risk. On merge_group runs the PR number is empty and the line prints PR merge_group, which is cosmetic only.
  • Permissions are narrow. The evaluating job declares permissions at job level (human-review-policy.yaml:16-19). A called workflow can only get the same or fewer permissions than the job that calls it, so the shared logic is held to contents: read, pull-requests: write and statuses: write.
  • The split design matches the description. Because the evaluation runs from workflow_run, it uses the copy on the default branch, so a PR can't rewrite the check that judges it (pr-description.md:14).

Not verified (prose only, not findings)

  • The shared workflow itself. rollbar/human-review-policy isn't checked out, so I couldn't confirm several things:
    • Whether it needs permissions beyond the three granted here. For example, it may need issues: write to create the needs-post-merge-review label or post comments, or actions: read. If it asks for more than the job grants, every policy run will fail to start.
    • Whether it needs inputs or secrets. The call passes none.
    • Whether it handles fork PRs and merge_group runs as described.
  • Unpinned ref (design note). The workflow is referenced as @main (human-review-policy.yaml:20). It runs in the privileged workflow_run context with a token that can post statuses. Anyone who can push to rollbar/human-review-policy main could change what posts the required status in this repo. That appears to be the intended trust model for a central policy, and the repo already uses moving tags (actions/checkout@v4 in node.js.yml:20). Still, consider whether a tag or SHA pin, plus a process for bumping it, fits better.
  • Declared dependencies. I didn't check either one; neither repo is in this review context:
    • The first content of rollbar/human-review-policy must merge before this PR (pr-description.md:17).
    • The private repos use the same code through rollbar/github-workflows#130 (pr-description.md:4).
  • Out of scope. The ruleset that makes the status required is explicitly not part of this PR (pr-description.md:21).
  • Merge queue. The merge_group trigger (human-review-policy-trigger.yaml:11-12) does nothing unless this repo turns on a merge queue.
  • Testing. Nothing in the diff shows these workflows have run end to end. The description says that happens after rollout steps 1 and 2 (pr-description.md:31).

@brianr

brianr commented Nov 9, 2026

Copy link
Copy Markdown
Member

Closing without merging. The human-review-policy check is moving from GitHub Actions into Caliper, as the approvals check in phase 4 of rollbar/business eng/caliper/DESIGN.md. Caliper already works out each PR's review stage, so the policy, and the lower tiers for low-risk PRs that come after it, will be defined in one place.

Caliper will post the status from its GitHub App, so this repo doesn't need any workflow files. It starts as an optional check. Tracking: PLA-2303.

@brianr brianr closed this Nov 9, 2026
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