Skip to content

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

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

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

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-java/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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-18T06:20:18.668826Z 014eb29 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

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 (openai, openai-astra)

Review of the human-review-policy caller workflows: no defects found in the changed lines.

This PR adds two workflows:

  • .github/workflows/human-review-policy-trigger.yaml does nothing itself. It runs on pull_request, pull_request_review and merge_group events.
  • .github/workflows/human-review-policy.yaml runs on workflow_run when the trigger completes. It calls rollbar/human-review-policy/.github/workflows/human-review-policy.yaml@main with contents: read, pull-requests: write and statuses: write.

What I checked in this checkout:

  • Names match. The trigger is named Human review policy trigger (.github/workflows/human-review-policy-trigger.yaml:1), and the workflow_run filter uses that exact name (.github/workflows/human-review-policy.yaml:11).
  • The trigger is safe. It sets permissions: {} (.github/workflows/human-review-policy-trigger.yaml:14). Its only step interpolates github.event.pull_request.number (a number) and github.event_name (.github/workflows/human-review-policy-trigger.yaml:20), so there is nothing a PR author could inject. On merge_group the PR number is just empty in the log line.
  • Permissions are scoped. The caller sets them on the job, not the whole workflow (.github/workflows/human-review-policy.yaml:16-19), and the trigger's conclusion isn't checked. That matches the stated intent: the evaluation runs even if a PR breaks the trigger.
  • The default branch is master. ci.yml filters on master (.github/workflows/ci.yml:14-16), which fits the rollout note that workflow_run only fires once the caller is on the default branch.

Stated dependencies, not checked:

  • The description says rollbar/human-review-policy must get its first content merged first (pr-description.md:17). It also mentions rollbar/github-workflows#130 for private repos, which doesn't affect this repo. Neither repo is checked out, so I could not check either.
  • So I couldn't confirm what the reusable workflow does with this token. Things to check there before step 3 (making the status required):
    • Are contents: read, pull-requests: write and statuses: write enough? Adding or creating the needs-post-merge-review label and posting comments may need issues: write, and looking up workflow runs may need actions: read.
    • Does it ever put the fork-controlled github.event.workflow_run.head_branch directly into a run: script? That would allow script injection.
    • Does it ever check out PR code while it has write scopes?

Non-blocking notes (no change required in this PR):

  • @main is a moving target. The reusable workflow is referenced by branch (.github/workflows/human-review-policy.yaml:20), so any push to that repo's main changes what runs here with write scopes. This repo's existing actions use tags, not SHAs (.github/workflows/ci.yml:25-26), and updating the policy centrally may be the point. Pinning to a tag would still narrow the blast radius.
  • Commit statuses can be forged. Any workflow in a same-repo PR branch that grants itself statuses: write could post a human-review-policy success itself. A required status check can't tell the two apart if both come from GitHub Actions. The post-merge needs-post-merge-review labelling is the backstop. This concerns the ruleset and the shared design, not these files.
  • No concurrency group in the caller. Back-to-back events, such as an approval and then a dismissal, start separate evaluations that could finish out of order. Whether that can leave a stale status depends on how the shared workflow reads review state, which I couldn't check.
  • Merge queue. Nothing in this repo shows a merge queue is in use. The merge_group trigger (.github/workflows/human-review-policy-trigger.yaml:11-12) is harmless either way.

As the description says (pr-description.md:30-31), this hasn't been run end to end, and nothing in the diff shows it running.

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.

1 participant