Skip to content

Require a signed interactive review record - #610

Merged
jserv merged 3 commits into
mainfrom
review-gate
Nov 7, 2026
Merged

jserv merged 3 commits into
mainfrom
review-gate

Conversation

@jserv

@jserv jserv commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Problem and behavior

The interactive-review check could not tell whether a contributor ran the
interactive review. It waited for the explained label, and on fork pull
requests it failed outright (run 42251304697). Only the pre-push hook enforced
the review, so a contributor without the hooks, or pushing past them, reached a
green check.

After this change, the pre-push hook signs a review record on the author's
machine and pushes it with the branch as the git note
refs/notes/codetrial-review. The signature covers a fingerprint of the
project, head, base, diff hash and answer digest; the note carries none of the
answers. The check measures the diff. Below the thresholds it passes. Above
them it verifies the record against the author's public SSH keys on GitHub. The
PR description and labels play no part.

Exemptions:

  • a pull request whose opener and every commit author are in the base branch's
    CODEOWNERS;
  • Dependabot;
  • the maintainer's ruleset bypass.

Every failure names its fix:

  • a missing key prints the three setup steps;
  • a key outage asks for a retry, not new setup;
  • a branch that is already pushed can send its record alone with
    sh scripts/git-pre-push.sh --publish-review <fork-remote> [<branch>].

Approach

  • The check stays on pull_request_target, so a pull request cannot edit the
    check that judges it.
  • The checker runs from the base branch against a bare fetch of the head; no
    file from the pull request is on disk.
  • GitHub restricts that trigger from 2026-12-02, so an Actions event policy
    scoped to pr-description.yml now allows it (created as id 11911).
  • docs/large-change-explanation.md owns the rules and every recovery path.
    CONTRIBUTING and the skills link to it.
  • The approver's "explain one passage" rule is withdrawn until it can be
    enforced; a FIXME in docs/development.md and one in the checker track it.
  • A merge group passes at once: the queue takes a pull request only after this
    check passed on its own head (the ruleset requires it).
  • check-queue-review.py drops its --guard, --branch and --repo options,
    which nothing calls any more.
  • The check also reruns when a pull request is edited, so a pull request moved
    to another base is measured again. The token is unset before anything reads
    the pull request's data.

Validation

  • scripts/run-python-tests.py on the review-hook and hashchain tests: 104
    pass. Mutations of the base-ancestor check, the commit-list match, the
    push-URL lookup and the record restore after the notes merge each fail a
    test.
  • ./scripts/test.sh portable: passes except test-gate-tests, which needs
    cryptography in the local Python and does not touch this change.
  • scripts/check-skill-refs.py: passes.
  • scripts/hashchain.py verify: passes.
  • make indent: clean.
  • actionlint and shellcheck on the workflow: pass.
  • Merges cleanly with Show what a force push changed since a review #605; the tests on the merged tree pass.

Not checked: a real notes push and signature check from a fork pull request. A
contributor's next large-change run after merge is the first live test.

@jserv
jserv requested a review from ColtenOuO as a code owner November 7, 2026 07:27
@jserv jserv added the explained label Nov 7, 2026
cubic-dev-ai[bot]

This comment was marked as resolved.

jserv added 3 commits November 7, 2026 16:18
The interactive-review check waited for a maintainer's label and failed
on fork pull requests, and only the pre-push hook enforced the review,
so a contributor without the hooks reached a green check.

The hook now signs a record of the completed review with the
contributor's SSH key and pushes it with the branch as a git note. The
check runs the base's checker on a bare fetch of the head, measures the
change and, over the thresholds, verifies the record against the GitHub
keys of the opener and the commit authors. A code owner's own work and
Dependabot's are exempt, an outage is told apart from missing setup,
and --publish-review sends the record for a branch already pushed.

Change-Id: I27c8fdd0a9da5bab38f02afe8daa37fd347b50c7
Contributors and maintainers read the large-change rules in several
places, which had drifted from what the hook and the check enforce and
still promised the removed label and the approver's one-passage
question. docs/large-change-explanation.md now owns the thresholds, the
exemptions, the record and a fix for each refusal; CONTRIBUTING.md
keeps the one-time key setup and links to it, as do the skills, and
development.md lists the admin setup. The one-passage question is
withdrawn until it can be enforced, with a FIXME where maintainers and
the checker discuss it.

Change-Id: I6ceead0d9d0d62dcb1f515b1f75985ee9745f931
From December 2, 2026, GitHub blocks pull_request_target in public
repositories unless an Actions event policy allows it, and the
interactive-review check would stop running. The trigger stays, since
it keeps a pull request from editing its own check; record the one
policy, scoped to this workflow and its two events, that an admin
creates to keep it, and note in the workflow that renaming the file
needs the policy changed with it.

Change-Id: I4b5d325011648485751cbaeeac5079072da6f60a
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