diff --git a/.claude/skills/codetrial-contribute/SKILL.md b/.claude/skills/codetrial-contribute/SKILL.md index 09718745..8c64faac 100644 --- a/.claude/skills/codetrial-contribute/SKILL.md +++ b/.claude/skills/codetrial-contribute/SKILL.md @@ -172,16 +172,22 @@ a PR is not ready for review until CI is green. Before handing the draft to gh-submit or opening it directly, apply [the large-change explanation requirement](../../../docs/large-change-explanation.md) to the complete committed PR diff and every proposed commit. If any commit -has at least 200 reviewable added plus deleted lines, or the whole PR has more -than 500, the local review must include the contributor's explanation and +has at least 200 added plus deleted text lines, or the whole PR has more than +500 (raw counts, as the hook and CI measure them), and the pull request is not +a code owner's own work (opener and every commit's author in CODEOWNERS) +or Dependabot's alone, the local review must include the contributor's explanation and follow-up answers checked against this base and head. If absent, incorrect, or stale, ask for the missing account and keep submission pending while continuing independent preparation. An approval to publish or an agent-written summary does not replace the answers. Preserve the useful rationale and reading order in the PR body, but keep the private report and transcript out. The pull request has no review block to fill in: CI measures the change itself, and a -change over the thresholds passes once a maintainer adds the `explained` label. -A later code or +change over the thresholds still needs the interactive review before the push; +the check fails unless the pre-push hook sent its completed record, signed with +an SSH key on the contributor's GitHub account. For a refusal by the hook or +the check, quote the fix from [the troubleshooting +list](../../../docs/large-change-explanation.md#troubleshooting) rather than +restating it. A later code or base change requires rechecking the count, and a new review session: the evidence is bound to the exact base, head and diff, so the earlier session is refused and the answers are written and sent again, which can start from the diff --git a/.claude/skills/codetrial-review/SKILL.md b/.claude/skills/codetrial-review/SKILL.md index 14f5ab2e..ef54182f 100644 --- a/.claude/skills/codetrial-review/SKILL.md +++ b/.claude/skills/codetrial-review/SKILL.md @@ -294,7 +294,8 @@ same lines, keeping the strongest wording and the highest level any confirmed version earned; note how many reviewers raised it, but agreement ranks nothing, a concrete failure does. -A diff past about 500 changed lines gets one more round, and only one: a +A diff past about 500 changed lines (a review-effort budget, not the +interactive-review threshold) gets one more round, and only one: a reviewer handed the confirmed findings ("do not repeat these") and the boundaries and protected subjects above that no verdict covers yet, including those in files that already produced a finding, since one finding does not diff --git a/.claude/skills/codetrial-review/workbench.md b/.claude/skills/codetrial-review/workbench.md index a9568cdc..7af388b7 100644 --- a/.claude/skills/codetrial-review/workbench.md +++ b/.claude/skills/codetrial-review/workbench.md @@ -12,17 +12,17 @@ adds layout styles and does not become a route in the interview application. Count over the whole intended PR diff, even when the review reads a smaller `--since` or explicit-base delta. Use the manifest's per-file additions and -deletions as the raw totals only. The reviewable total follows the counting -rule of [the large-change requirement](../../../docs/large-change-explanation.md): -subtract generated, vendored, lockfile or mechanical-move paths only when each -is listed with how its origin was checked, and report every exclusion and its -evidence. A null count or incomplete manifest is unknown coverage, never zero. +deletions as the totals, raw, as [the large-change +requirement](../../../docs/large-change-explanation.md) counts them: nothing is +subtracted. Generated, vendored, lockfile or mechanical-move paths may be named +for the reviewer with how their origin was checked, which does not lower the +count. A null count or incomplete manifest is unknown coverage, never zero. Check every proposed commit against its parent as well as the whole PR. Local bundles with an explicit or resolved branch base carry `commit_sizes`, collected by `commit-sizes.py` without network access. Missing or incomplete commit-size coverage, including a PR bundle without local history, needs the full base/head checkout before the threshold can be cleared; never infer small commits from a -small net diff. Verify exclusions per commit and record triggering full SHAs. +small net diff. Record the triggering full SHAs. When the threshold is reached, pick the single most consequential modification in the pinned diff: a changed behavior, trust boundary, failure path or @@ -30,8 +30,9 @@ consequential design choice. Read related callers first; line count alone does not decide. If the large commit was reverted before the head, so that no path or passage of it remains in the pinned diff, ask about the largest change that does remain and say in the report that the triggering commit has no passage -left; if the final diff is empty, interactive evidence cannot be certified and -a maintainer decides. The question asks why this change is needed or why this +left; if the final diff is empty there is nothing to explain, and the +contributor drops or squashes the commit and its revert so no commit reaches +the threshold. The question asks why this change is needed or why this approach serves the contributor's goal. Do not reveal a reference answer before the author submits; afterward give @@ -60,7 +61,7 @@ explaining; never generate or silently infer an answer. ## Track readiness Record explanation readiness separately from defect severity: `not required`, -`pending` or `complete`, with the base/head, totals, exclusions, covered areas, +`pending` or `complete`, with the base/head, totals, covered areas, answer location and unresolved questions, in the private report's coverage and questions fields. No new report schema or reviewed-head marker certifies explanation completion. A dirty-tree explanation is provisional until checked @@ -77,8 +78,7 @@ check. Use the complete intended PR diff from step 1, not a fix-only delta, counted as [above](#decide-whether-it-applies). If its manifest is dirty, incomplete, or lists undiffed untracked files, the workspace shows that limitation and its -explanation remains provisional; its raw total is never a reviewed exclusion -count. For a dirty preview, supply the full PR base to `commit-sizes.py`. +explanation remains provisional. For a dirty preview, supply the full PR base to `commit-sizes.py`. Prepare `$bundle/questions.json` with **one core key-change question**, chosen after reading the diff and relevant callers. Include a manifest `path`, a short `quote` @@ -217,8 +217,7 @@ Once the reflection is confirmed the page offers **Finish and close**. It asks the server to stop (`PUT /api/finish`, refused until the explanation is confirmed), which also stops the managed worker, removes the token from the address bar, and replaces the page with what comes next: push the branch, then -open the pull request, where a maintainer adds the `explained` label to a large -change. A browser lets a script close only a window the script opened, so the +open the pull request. A browser lets a script close only a window the script opened, so the tab may stay; the page then says it is safe to close. The saved explanation and confirmation are not touched, and starting the server again reopens them. @@ -411,7 +410,7 @@ revision, coverage, unresolved questions and private answer location in the normal local report. Finish the code review through its existing lifecycle, which remains independent of explanation readiness. The workspace writes no reviewed-head marker. Before submission, recheck the committed full PR diff, -counting/exclusion evidence, applicable answers, validation and final approval. +counts, applicable answers, validation and final approval. Put only useful rationale and reading order in the PR body, never this transcript. ### Git hook evidence @@ -446,8 +445,9 @@ defect freedom, test coverage or permission to merge. ## Inspect the requirement -Large-change events have a local Git note at `refs/notes/codetrial-review`, -attached to the branch head and linked to its `Change-Id`. Read it with: +Large-change events have a Git note at `refs/notes/codetrial-review`, attached +to the branch head and linked to its `Change-Id`; the pre-push hook sends the +notes ref with a large branch once its evidence verifies. Read it with: ```sh git log -1 --notes=codetrial-review @@ -465,33 +465,41 @@ head, base, triggering commit SHAs and counts. It contains no answers, feedback, tokens, transcript or private evidence paths. Dirty and provisional previews do not annotate a committed head as if they had reviewed it. -These are display snapshots, not authorization. A forged or copied `complete` -note cannot pass the hook; it verifies the actual receipt again. Use the status +Locally these are display snapshots, not authorization. A forged or copied +`complete` note cannot pass the hook; it verifies the actual receipt again. On +the pull request the signed marker is what the check requires, as a declaration +signed with the author's key, not as proof. Use the status command below to recheck against the current PR base. A rebase or amend needs new evidence even if its Change-Id stays the same. An old head's note remains -a historical record. Notes do not change commit SHAs, trees or Change-Ids, and -are not automatically pushed. No Change-Id suffix or completion trailer is +a historical record. Notes do not change commit SHAs, trees or Change-Ids. The +pre-push hook sends the notes ref with a large branch once its evidence +verifies; the `interactive-review` check reads the head's marker from there and +compares it with the diff; the record's fields and how the check verifies +them are in [the large-change +page](../../../docs/large-change-explanation.md#what-reaches-the-pull-request). +No Change-Id suffix or completion trailer is added, since changing a message to announce approval would invalidate the very commit identity the approval covers. If a note cannot be written, the command reports the failure; a completed Git commit remains intact. After resolving Git identity or note-ref locks, refresh -the marker with `python3 scripts/check-interactive-review.py mark --base origin/main` +the marker with `python3 scripts/check-interactive-review.py mark --base upstream/main` using the intended PR base. This command rechecks evidence and cannot grant approval. Notes for older heads are not copied forward as current evidence. -Run `python3 scripts/check-interactive-review.py status --base origin/main` +Run `python3 scripts/check-interactive-review.py status --base upstream/main` with the intended PR base. The JSON distinguishes `not_required`, `pending`, and `complete`, lists triggering full commit SHAs and text line counts, and binds completion to the exact branch head and merge base. This is the same size and evidence check used before pushing. A later commit undoing a large one does not remove its trigger. No commit-message trailer certifies approval. -The private evidence remains local; other clones and CI cannot infer completion -from a Change-Id or from the reviewed-head marker alone. -These are local workflow records, not signed maintainer attestations: a person -who controls the clone can alter its JSON and database. Remote enforcement must -use a maintainer-controlled required check and independently trusted review -evidence. Never publish the private answer transcript as a workaround. +The private evidence remains local. What leaves the machine is the marker, signed +with the contributor's SSH key over the review fingerprint, which the required +check verifies against the author's keys on GitHub. It is the author's signed +declaration, not proof: a person who controls the clone can alter its JSON and +database and sign the result. What the approval adds is not settled +([development.md](../../../docs/development.md#what-cannot-be-enforced-locally)). +Never publish the private answer transcript as a workaround. Size checks ignore Git replacement objects and refuse shallow history or local grafts; fetch full history rather than treating missing commits as zero lines. diff --git a/.claude/skills/codetrial-review/workbench/index.html b/.claude/skills/codetrial-review/workbench/index.html index 629b4b7d..db1a7583 100644 --- a/.claude/skills/codetrial-review/workbench/index.html +++ b/.claude/skills/codetrial-review/workbench/index.html @@ -40,9 +40,8 @@

Interactive review finished

  1. Push the branch. The pre-push hook checks the review.
  2. - Open the pull request. A large change passes its check once a - maintainer adds the explained label, after you have - explained it to them. + Open the pull request. The hook signs this review and sends its + record with the branch, which a large change's check requires.

@@ -75,9 +74,8 @@

Interactive review complete

  1. Push the branch. The pre-push hook checks the review.
  2. - Open the pull request. A large change passes its check once a - maintainer adds the explained label, after you have - explained it to them. + Open the pull request. The hook signs this review and sends its + record with the branch, which a large change's check requires.

diff --git a/.claude/skills/codetrial-triage/pulls.md b/.claude/skills/codetrial-triage/pulls.md index 8ce52fe9..16a23880 100644 --- a/.claude/skills/codetrial-triage/pulls.md +++ b/.claude/skills/codetrial-triage/pulls.md @@ -443,8 +443,15 @@ way. Two PRs that rewrite the same harness function (#376 and #396 on `json_graph`) conflict as soon as one merges: name the order. "Approved before the last push" means every maintainer whose latest verdict is -an approval left it on an older head. GitHub keeps such an approval, so -`decision` can read APPROVED for code nobody approved; it waits on a re-review. +an approval left it on an older head. Where the ruleset does not dismiss stale +approvals, GitHub keeps such an approval, so `decision` can read APPROVED for +code nobody approved; it waits on a re-review. + +A failing `interactive-review` check is the author's to fix, like any failing +check: quote the fix for its message from [the troubleshooting +list](../../../docs/large-change-explanation.md#troubleshooting) rather than +restating it. A fix that sends only the record reruns nothing until the pull +request is reopened or a maintainer reruns the job. Each `status` entry carries `waits_on`, read off the same rows as the grade: `approval` for a held run, `ci` for a pending check or a mergeability GitHub diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 1c5c6bcc..0ae26c57 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,5 +1,9 @@ # The files that decide whether a change is reviewed are the maintainer's to # approve. Pair this with "Require review from Code Owners" on main. +# +# Every login named here may also skip the interactive review of a large +# change that is wholly their own (docs/large-change-explanation.md says when); +# the check reads this file from the base, and a team is not expanded. /.github/ @jserv /scripts/ @jserv /.claude/skills/codetrial-review/ @jserv diff --git a/.github/workflows/pr-description.yml b/.github/workflows/pr-description.yml index 6fdc4881..26eb1654 100644 --- a/.github/workflows/pr-description.yml +++ b/.github/workflows/pr-description.yml @@ -1,38 +1,45 @@ -# Decide whether a pull request needs the interactive review, and whether a -# maintainer has marked it explained. +# Measure a pull request and, for a large change, check that the interactive +# review was done before it was pushed. # -# check.yml cannot re-run on every label change without paying for the whole -# matrix, so this one carries only that check. It measures the commits and the -# diff itself and reads nothing the author writes: a change over the thresholds -# passes only with the maintainer's label. The review itself is local evidence -# this job never sees. The file keeps its name from when it read the -# description. +# check.yml carries the whole matrix; this job carries only this check. It reads +# the commits and the diff and nothing the author writes in the description. A +# large change passes when the pre-push hook sent a completed review record for +# exactly its code, signed on the contributor's machine with an SSH key that +# GitHub lists for the pull request's author, read here from the fork as data, +# and fails with how to fix it when not. The answers stay local evidence this +# job never sees. The file keeps its name from when it read the description. # # PR events run on pull_request_target, so their workflow is the base branch's # and a pull request cannot edit or remove it. The same reason makes this job # the dangerous kind: it has the base repository's token. It therefore asks for -# read access only, checks the pull request out as data, and runs only the base -# branch's checker, never a script from the pull request. While the base has no -# gate (the pull request that adds it) there is nothing to compare, and this -# workflow does not exist on the base either until that pull request merges. -# Merge groups recheck each included PR with the group base's checker, but run -# the workflow file as the queue ref has it, edits from the pull request -# included, and a job its own condition skips counts as passing. So no job here -# may have an `if:`, and the pull-request run's --guard step refuses a change to -# this file (or another workflow naming this check) without an approved head. -# See docs/development.md. +# read access only, fetches the pull request's commits by identity into a bare +# repository (objects only: no working tree, no hooks, no file from the pull +# request on disk), and runs only the base branch's checker. actions/checkout +# refuses a fork's head under this trigger, so it checks out only the base. +# While the base has no checker (the pull request that adds it) there is nothing +# to compare, and this workflow does not exist on the base either until that +# pull request merges. A merge group runs the workflow file as the queue ref has +# it, edits from the pull request included, and a job its own condition skips +# counts as passing. So no job here may have an `if:`, and the pull-request +# run's guard refuses a change to this file (or another workflow naming this +# check) without an approved head. See docs/development.md. +# GitHub blocks this trigger in public repositories by default from December 2, +# 2026; a repository Actions policy allows it for this file only, so renaming +# the file needs the policy changed too (docs/development.md). # Changing the trigger or the token's scope needs the review that # .claude/skills/codetrial-triage/apply-pr-actions.sh names for this file. name: pr-description on: pull_request_target: - types: [opened, synchronize, reopened, labeled, unlabeled] + # edited: a pull request moved to another base is measured again. + types: [opened, synchronize, reopened, edited] merge_group: types: [checks_requested] permissions: contents: read + # Read only, to list the pull request's commits with their authors. pull-requests: read concurrency: @@ -47,12 +54,6 @@ jobs: runs-on: ubuntu-24.04 timeout-minutes: 10 steps: - - uses: actions/checkout@v7 - with: - ref: ${{ github.event.pull_request.head.sha || github.event.merge_group.head_sha }} - fetch-depth: 0 - persist-credentials: false - path: head - uses: actions/checkout@v7 with: ref: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }} @@ -60,35 +61,114 @@ jobs: path: base - name: Measure the change env: + # Read-only, for the public list of the author's signing keys. GH_TOKEN: ${{ github.token }} EVENT: ${{ github.event_name }} REPOSITORY: ${{ github.repository }} - BASE_REF: ${{ github.event.merge_group.base_ref }} - LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} AUTHOR_LOGIN: ${{ github.event.pull_request.user.login }} AUTHOR_TYPE: ${{ github.event.pull_request.user.type }} APPROVED_HEAD: ${{ vars.CODETRIAL_POLICY_APPROVED_HEAD }} - BASE_SHA: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }} - HEAD_SHA: ${{ github.event.pull_request.head.sha || github.event.merge_group.head_sha }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + HEAD_REPOSITORY: ${{ github.event.pull_request.head.repo.full_name }} + PR_NUMBER: ${{ github.event.pull_request.number }} run: | set -eu export GIT_TERMINAL_PROMPT=0 + if [ "$EVENT" = merge_group ]; then + # A queue head is GitHub's merge of pull requests the queue took + # only once this check passed on each one's own head; it has no + # review record of its own to read. + echo "Each pull request in this group passed this check on its own head." + exit 0 + fi if [ ! -f base/scripts/check-interactive-review.py ] \ || [ ! -f base/scripts/check-queue-review.py ] \ || [ ! -f base/.claude/skills/codetrial-review/commit-sizes.py ]; then - echo "The base branch has no interactive review gate yet; nothing to compare." + echo "The base branch has no interactive review check yet; nothing to compare." exit 0 fi - cd head - if [ "$EVENT" = merge_group ]; then - python3 ../base/scripts/check-queue-review.py \ - --base "$BASE_SHA" --head "$HEAD_SHA" \ - --branch "${BASE_REF#refs/heads/}" --repo "$REPOSITORY" + for sha in "$BASE_SHA" "$HEAD_SHA"; do + printf '%s\n' "$sha" | grep -Eqx '[0-9a-f]{40}' + done + # A fork's head is reachable from the base repository through the pull + # request. + git init -q --bare head.git + if ! git -C head.git fetch -q --no-tags \ + "https://github.com/$REPOSITORY" "$BASE_SHA" "$HEAD_SHA"; then + echo "::error title=Cannot fetch the change::Commit $HEAD_SHA is no longer available, usually because the branch was pushed again after this run started. The newer push starts its own run; if none appears, push again." + exit 1 + fi + # The pre-push hook sent the review record to the repository the branch + # came from. It is data for the base's checker, and only a large change + # needs one; the checker says so when it is missing, so a failed fetch + # is silent here. + if printf '%s\n' "$HEAD_REPOSITORY" | grep -Eqx '[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+'; then + timeout 120 git -C head.git fetch -q --no-tags \ + "https://github.com/$HEAD_REPOSITORY" \ + "+refs/notes/codetrial-review:refs/notes/codetrial-review" \ + 2> /dev/null || true + fi + # A code owner's pull request is exempt only when every commit is a + # code owner's too; GitHub links each commit's author to an account. + # A list it did not return is not passed, and the exemption then does + # not apply. + authors="$RUNNER_TEMP/commit.authors" + if printf '%s\n' "$PR_NUMBER" | grep -Eqx '[0-9]+' \ + && timeout 30 gh api "repos/$REPOSITORY/pulls/$PR_NUMBER/commits" \ + --paginate --jq '.[] | .sha + " " + (.author.login // "")' > "$authors" 2> /dev/null; then + listed=$authors else - python3 ../base/scripts/check-queue-review.py \ - --base "$BASE_SHA" --head "$HEAD_SHA" --guard \ - --approved "$APPROVED_HEAD" - python3 ../base/scripts/check-interactive-review.py pr \ - --base "$BASE_SHA" --head "$HEAD_SHA" --labels "$LABELS" \ - --author-login "$AUTHOR_LOGIN" --author-type "$AUTHOR_TYPE" + listed= + fi + # The record is signed on the contributor's machine; the keys GitHub + # publishes for the author are what the signature is checked against: + # authentication keys, and signing keys, which a contributor can add + # for this alone without touching how they push. + # A list GitHub did not return is said to be unavailable, so the + # checker does not mistake an outage for a missing key. + keys="$RUNNER_TEMP/author.keys" + : > "$keys" + unavailable= + # The opener's keys, and those of up to nine of the commits' authors, + # so a contributor whose work a code owner opened can sign its record. + valid='[A-Za-z0-9][A-Za-z0-9-]{0,38}' + opener= + printf '%s\n' "$AUTHOR_LOGIN" | grep -Eqx "$valid" && opener=$AUTHOR_LOGIN + others= + if [ -n "$listed" ]; then + others=$(awk '{ print $2 }' "$authors" | grep -Ex "$valid" \ + | grep -vixF -- "${opener:-.}" | sort -fu | head -n 9) || true fi + # Fetched side by side, each into its own file, so a slow answer + # costs one timeout, not one per login. + fetched="$RUNNER_TEMP/keys.d" + mkdir -p "$fetched" + for login in $opener $others; do + ( + curl -fsS --max-time 30 "https://github.com/$login.keys" \ + > "$fetched/$login.auth" 2> /dev/null \ + || : > "$fetched/unavailable" + timeout 30 gh api "users/$login/ssh_signing_keys" \ + --paginate --jq '.[].key' > "$fetched/$login.signing" 2> /dev/null \ + || : > "$fetched/unavailable" + ) & + done + wait + for list in "$fetched"/*.auth "$fetched"/*.signing; do + [ ! -f "$list" ] || { cat "$list"; echo; } >> "$keys" + done + # Without the commit list, the authors' keys were never fetched. + [ -n "$listed" ] && [ ! -e "$fetched/unavailable" ] \ + || unavailable=--keys-unavailable + # Only the gh calls above needed the token; nothing that reads the + # pull request's data below gets it. + unset GH_TOKEN + cd head.git + python3 ../base/scripts/check-queue-review.py \ + --base "$BASE_SHA" --head "$HEAD_SHA" --approved "$APPROVED_HEAD" + python3 ../base/scripts/check-interactive-review.py pr \ + --base "$BASE_SHA" --head "$HEAD_SHA" \ + --author-login "$AUTHOR_LOGIN" --author-type "$AUTHOR_TYPE" \ + --author-keys "$keys" $unavailable \ + ${listed:+--commit-authors "$listed"} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ed302800..2484be6a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -244,8 +244,9 @@ pull request, tells the reviewer little, and leaves open whether the issue is finished. When the issue is too large for this change, split it into sub-issues and close one with each pull request. -Working with Claude Code, run `/codetrial-review` on the committed branch -before opening the pull request. It reads what the gate cannot: the scope the +Run `/codetrial-review` (in Codex, `$codetrial-review`) on the committed +branch before opening the pull request; a large change needs Claude Code or +Codex for its interactive review. It reads what the gate cannot: the scope the issue accepted, the contracts in `docs/`, and what maintainers already decided about the same files. Fix what it confirms; its report stays out of the pull request. The pre-push hook prints one line when the head being pushed has no @@ -272,8 +273,8 @@ maintainer approval of the exact verifier commit. See [the hashchain trust boundary](docs/development.md#git-hooks). Before opening a pull request, use the interactive review web page if any -commit has at least 200 added plus deleted reviewable lines, or the whole PR -has more than 500. Explain the change and its design choices in your own words, +commit has at least 200 added plus deleted text lines, or the whole PR has more +than 500 (raw counts; nothing is excluded). Explain the change and its design choices in your own words, and answer questions grounded in the diff. Follow [the large-change explanation requirement](docs/large-change-explanation.md) for counting, coverage, and what @@ -281,8 +282,46 @@ belongs in the PR body. An agent-written summary or approval alone does not meet this requirement; submission waits for your explanation and any material corrections. The pull request carries no review block. The `interactive-review` check measures -the change itself, and a change over the thresholds passes once a maintainer adds -the `explained` label. +the change itself; over the thresholds it passes only when the pre-push hook +sent the record of a completed interactive review for that code, signed with an +SSH key on your GitHub account. Only the people `.github/CODEOWNERS` names may +skip the interactive review, and only for their own work: the account that +opens the pull request and the GitHub account of every commit's author must all +be listed there. A pull request whose every commit is Dependabot's needs no +record either, and the maintainer's ruleset bypass covers the maintainer's own +pull requests. Everyone else goes through it for every large change. When +the hook or the check refuses, [the troubleshooting +list](docs/large-change-explanation.md#troubleshooting) has the fix for each +message, including sending the record for a branch already pushed +(`--publish-review`). + +**Before your first large change**, once: + +- `make hooks`, and an `upstream` remote fetched + (`git remote add upstream https://github.com/sysprog21/codetrial && git fetch + upstream`): the hook measures the change against `upstream/main`. +- A signing key, below. The record is signed on your machine, so a large change + needs an SSH key registered on your GitHub account. A key of its own, registered as a Signing Key, leaves the way you push +(HTTPS or another SSH key) untouched: + +1. Generate it: `ssh-keygen -t ed25519 -f ~/.ssh/codetrial_signing` + ([Generating a new SSH key](https://docs.github.com/en/authentication/connecting-to-github-with-ssh/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent)). +2. Register it: on GitHub, Settings, SSH and GPG keys, New SSH key, choose + **Key type: Signing Key**, and paste `~/.ssh/codetrial_signing.pub` + ([Adding a new SSH key to your account](https://docs.github.com/en/authentication/connecting-to-github-with-ssh/adding-a-new-ssh-key-to-your-github-account)). +3. Point the hook at it: `git config codetrial.signingKey ~/.ssh/codetrial_signing` + +A key you already push with over SSH works too, without these steps when it is +in `ssh-agent`; the hook then signs with the first key the agent offers, so with more than one +key loaded, set `codetrial.signingKey` to the one on your account. The +push prints the fingerprint it signed with, and `ssh-keygen -lf ` beside +your keys on GitHub shows that it is one of them. + +`git config codetrial.codeOwner ` is for the people `.github/CODEOWNERS` +names, to tell the hook they skip the review. It is not a way around it: the +pull request's check never reads this setting, and judges the account that +opens the pull request and each commit's author instead, so on anyone else's +branch it only moves the refusal from the push to the pull request. A review thread carries a correction, a measurement, or nothing: no pasted agent walkthroughs, no severity tables, no re-summarizing a diff that GitHub diff --git a/docs/development.md b/docs/development.md index 8c2f59ad..734ec7ac 100644 --- a/docs/development.md +++ b/docs/development.md @@ -428,8 +428,8 @@ small changes get one advisory line when the head has no `/codetrial-review` record. A topic branch with any commit at least 200 added plus deleted text lines, or a net PR diff over 500, is refused without matching durable code-review and interactive-explanation evidence. The hook uses raw text counts -conservatively, without automatically exempting generated or vendored paths; -record any exclusion evidence during the review. It resolves the PR base from +conservatively and exempts no path; generated or vendored paths may be named +for the reviewer, which does not lower the count. It resolves the PR base from the canonical upstream's local `main` tracking ref, falling back to the push remote's `main` when no canonical upstream is configured. Missing history is refused when there is no remote-tracking `main` to measure against (for @@ -453,9 +453,9 @@ A commit authored by `dependabot[bot]` continues the chain without a Change-Id trailer, because the bot cannot run the hooks; the author line is unauthenticated, so locally this is a convenience and not a trust boundary. `scripts/hashchain.py` is the only place that decides who Dependabot is: the -commit-log check asks it for the commits to skip, and the description check -excuses a pull request only when GitHub reports `dependabot[bot]` (account type -`Bot`) as its opener and every commit in it is Dependabot's. A forged author +commit-log check asks it for the commits to skip, and the interactive-review +check excuses a pull request only when GitHub reports `dependabot[bot]` (account +type `Bot`) as its opener and every commit in it is Dependabot's. A forged author address therefore buys nothing there for a contributor who cannot push to Dependabot's branches, and a person's commit on the branch is read like any other. The hashchain and commit-log checks in CI still go by the author line @@ -500,9 +500,9 @@ instead is layer three things, each closing a different gap: given the same rule in its own configuration. - **CI recomputes everything it can** and is the only part a contributor cannot edit locally: the Change-Id chain, the size of every commit and of the whole - pull request, and that a change over the review thresholds carries a - maintainer's `explained` label. It reads nothing the author writes in the - description. It runs the base branch's checker, so a pull request cannot weaken it. + pull request, and, for a change over the review thresholds, that the pre-push + hook sent a completed review record for exactly that code. It reads nothing + the author writes in the description. It runs the base branch's checker, so a pull request cannot weaken it. It holds only if the repository requires it: make `check` and `interactive-review` required status checks, merge only through the merge queue, let only the maintainer bypass them, from a pull request and never by pushing @@ -511,28 +511,98 @@ instead is layer three things, each closing a different gap: `scripts/check-queue-review.py`, `.claude/skills/codetrial-review/commit-sizes.py`, `scripts/agent-guard.py`, `.claude/settings.json` and `.github/workflows/`. +Repository setup, done once by an admin; the reasons follow below: + +1. A ruleset on `main` requiring `check` and `interactive-review`, the merge + queue (merge method), one approval that a push dismisses, code-owner review, + and a maintainer bypass from a pull request only. +2. The Actions event policy that keeps `pull_request_target` running for + `pr-description.yml` (the command is below). +3. Approval required for first-time contributors' runs (Settings, Actions, + General); a maintainer grants it, through + `.claude/skills/codetrial-triage/apply-pr-actions.sh` in a sweep. +4. Before merging a pull request that edits `pr-description.yml`, + `CODETRIAL_POLICY_APPROVED_HEAD` set to its head (the guard below). +5. Once the review record replaces the old label gate: delete the label named + `explained`, and tell each open pull request over the thresholds that its next run needs the + signing key and a record ([the troubleshooting + list](large-change-explanation.md#troubleshooting)). + `pr-description.yml` runs on `pull_request_target`, so the workflow that runs is the base branch's and a pull request cannot edit or remove it. That trigger runs with the base repository's token, which is why the job asks for read access -only, checks the pull request out as data and runs only the base branch's -checker. It is also why `apply-pr-actions.sh` otherwise sends every approval of +only, fetches the pull request's commits by identity into a bare repository (no +working tree, so no file from the pull request is checked out; its objects +stay in that repository, read only by the checker) and runs only +the base branch's checker. `actions/checkout` refuses a fork's head under this +trigger, so it checks out only the base. It is also why `apply-pr-actions.sh` otherwise sends every approval of a first-time contributor's run to the confirmed list while a workflow uses that trigger: `pr-description.yml` is the one workflow it names as reviewed, and only for `pull_request_target`. A new workflow on either base-token trigger needs the same review, and the script's tests fail until it has one. +From December 2, 2026, GitHub blocks `pull_request_target` in a public +repository that has no Actions event policy allowing it, and the check would +then never run. The repository keeps the trigger, because it is what stops a +pull request from editing its own check, with one repository policy (Settings, +Actions, Policies) that targets only this workflow and allows the two events it +runs on. A repository admin creates it once: + +```sh +gh api -X POST repos/sysprog21/codetrial/actions/policies \ + -H 'X-GitHub-Api-Version: 2026-03-10' --input - << 'EOF' +{"name": "pr-description: pull_request_target", + "enforcement": "active", + "conditions": {"workflow_path": { + "include": [".github/workflows/pr-description.yml"], "exclude": []}}, + "rules": [{"type": "restrict_action_events", + "parameters": {"allowed_events": ["pull_request_target", "merge_group"]}}]} +EOF +``` + +`merge_group` is listed too because the rule reads as an allow list for the +workflows it targets. `gh api repos/sysprog21/codetrial/actions/policies` shows +it; a run that no longer carries GitHub's notice about the default policy +confirms it applies. Renaming the workflow file needs the policy's path changed +with it. + The required job is named `interactive-review` (the workflow is `pr-description`); require that job and `check` in the branch ruleset. The -workflow also handles `merge_group`: using the base branch's checker, it lists -open PRs targeting that branch, selects heads present in the group but absent -from its base, and rechecks each one's size against its own head and its group -base, and its labels. Every ordinary commit in the group must belong to one of -those heads; missing PRs, a missing label and API errors fail the check. Merge commits are -verified by the separate hashchain gate. This requires the merge method described -below; squash and rebase queues are unsupported. Queue checks use a read-only -token with `pull-requests: read` to retrieve the open PRs and their labels, never -local answers. The `explained` label is not bound to a head, so a push keeps it; -the ruleset's dismissal of stale approvals is what asks for a fresh review. +check fails when the change cannot be fetched or measured, when the guard below +refuses a workflow edit, and when a change over the thresholds has no matching +review record: the marker note the pre-push hook signs with the contributor's +SSH key (`ssh-keygen -Y sign`, namespace `codetrial-review`, over the review +fingerprint) and sends to the fork. The job fetches it from `head.repo` as data, +fetches the author's public keys, authentication keys from +`github.com/.keys` and signing keys from the public +`users//ssh_signing_keys` API with the job's read-only token, and the base's checker recomputes the diff hash and the review fingerprint and +verifies the signature against those keys. The signed record attributes the +declaration to the author's key; it is not proof of authorship or understanding, +what the approver does about that is not settled yet. + +Who needs no record, and whose keys may sign one, is +[the large-change page's](large-change-explanation.md#what-reaches-the-pull-request) +to say. For the code-owner exemption the job reads the commits' authors from +`repos//pulls//commits` (hence `pull-requests: read`), and a list that +does not name exactly the commits checked (the branch moved, or it has more +than the 250 that endpoint returns) withholds the exemption. GitHub links a +commit to an account by its author email, which anyone can write, so the +exemption keeps an honest code owner from excusing a contributor's work and is +not proof of authorship. The hook's `codetrial.codeOwner` cannot map an email to +an account either; a false claim only moves the refusal to CI. The hook sends the whole notes ref, so markers it wrote for other local +branches, pending ones included, reach the fork too; they hold no answers. A +record is read up to 64 KiB and a key list up to 256 KiB, and what the check +prints carries counts, commit identities and key fingerprints only, never text +from the branch. Passing a merge group without a record is safe only while the +ruleset makes the queue the one way to merge. A +merge group passes at once: the queue takes a pull request only after this +check passed on its own head, and the queue's merge commit has no record of its +own. Merge commits are verified by the separate hashchain gate. This requires the merge method described below; squash and +rebase queues are unsupported. The required approval, which a push dismisses, +is bound to a head; what it confirms about a large change's explanation is not +settled yet. Use GitHub's required-review rules as the independent approval boundary: require at least one non-author maintainer approval, require code-owner review for the @@ -545,8 +615,7 @@ the maintainer. A merge group runs the workflow file as the queue ref has it, edits from the pull request included, and a job its own `if:` skips counts as passing, so loading the checker from base is not enough. The pull-request run is the base branch's workflow, so -it is where such an edit is stopped: the `--guard` step of -`scripts/check-queue-review.py` fails when the pull request changes +it is where such an edit is stopped: `scripts/check-queue-review.py` fails when the pull request changes `pr-description.yml` from where its branch began, or adds another workflow that mentions `interactive-review` anywhere, or removes one of the checker scripts the workflow runs (it skips itself while the base has none of them, so a deletion would switch @@ -579,13 +648,15 @@ machines, and have CI verify the signature, issuer, policy, repository and every snapshot field against current data. Reject receipts copied between PRs or replayed after any snapshot change. Keep answers private. Such a receipt proves what the service observed, not comprehension. This service and receipt verifier -are future work; the current `explained` label remains a maintainer's word. +are future work. Today the contributor signs a review marker on their own +machine, which shows whose key declared the review and not what the service +above would observe. The chain is a checksum of each parent link and tree, not proof of review: a commit made with `--no-verify` can be restamped by `rechain` and still verify. It catches accidental history edits. Interactive-review evidence lives under -`.git/codetrial-review/` on the contributor's machine, so the pre-push check is -local deterrence and CI does not see it. +`.git/codetrial-review/` on the contributor's machine and CI does not see it; +CI sees only the signed marker the pre-push hook sends with a large branch. Each ordinary commit continues the link of its own parent, including commits on both sides of a merge. GitHub's two-parent integration commits need no @@ -670,10 +741,10 @@ large-change requirement. To inspect the interactive requirement without pushing, run: ```sh -python3 scripts/check-interactive-review.py status --base origin/main --head HEAD +python3 scripts/check-interactive-review.py status --base upstream/main --head HEAD ``` -Use the intended upstream PR base when it differs from `origin/main`. The JSON +Use the intended upstream PR base when it differs from `upstream/main`. The JSON names every triggering full commit SHA and its added plus deleted text lines, the final PR text total, and `not_required`, `pending`, or `complete`. A large commit remains a trigger when later commits undo it. `coverage_complete` is @@ -697,8 +768,10 @@ and whether the matching workbench server started. View it with Only certificate verification can produce the generated complete state, and the push hook never trusts the note as proof. Editing answers revokes the certificate and refreshes the note to pending. The note contains no private -explanation or feedback. Notes preserve commit identities and are not pushed -automatically. See [the marker lifecycle](../.claude/skills/codetrial-review/workbench.md#inspect-the-requirement). +explanation or feedback. Notes preserve commit identities. The pre-push hook +sends the notes ref to the remote a large branch goes to, once the evidence +verifies, merging what that remote already holds rather than overwriting it, so +the `interactive-review` check can read the head's marker. See [the marker lifecycle](../.claude/skills/codetrial-review/workbench.md#inspect-the-requirement). ## Releases and the embedded web tree diff --git a/docs/large-change-explanation.md b/docs/large-change-explanation.md index 580e8a37..1483b9d4 100644 --- a/docs/large-change-explanation.md +++ b/docs/large-change-explanation.md @@ -1,8 +1,12 @@ # Explain a large change before submitting it +This page owns the large-change rules: the thresholds, who is exempt, what +reaches the pull request, and how to recover from each refusal. CONTRIBUTING.md +gives the one-time setup and links here; the skills follow this page. + A large pull request needs the contributor's account of what changed and why, before it is opened. Interactive web review is required when **any proposed -commit has at least 200 added plus deleted reviewable text lines**, or when +commit has at least 200 added plus deleted text lines**, or when the **whole PR has more than 500**. Exactly 200 triggers the commit rule; exactly 500 does not trigger the PR rule unless a commit also reaches 200. Measure each commit against its parent (a merge against its first parent). @@ -13,22 +17,24 @@ when a later commit reduces the final PR diff to a few lines. This is a practica review budget, not a measure of difficulty or proof of understanding. Count hand-maintained implementation, tests, scripts, configuration, and prose: -all take attention to review. Report the raw total and the reviewable total. -Exclude generated output, vendored content, lockfile churn, or mechanical moves -only when you list the paths and explain how their origin or unchanged behavior -was checked. Count changes to their generators and inputs. Do not subtract an +all take attention to review. The hook and CI count raw text lines and grant no +exclusion, so what decides whether the review is required is the raw count. +Generated output, vendored content, lockfile churn, or mechanical moves may be +pointed out to the reviewer, with the paths and how their origin or unchanged +behavior was checked, but that does not lower the count. Count changes to their generators and inputs. Do not subtract an entire mixed file when only part is mechanical. Binary files have no text line count; name them and how they were checked separately. Unknown counts or an incomplete file list cannot establish that the branch is below the threshold. The hook therefore requires interactive review for unmeasured committed files, including paths Git marks binary. A `-diff` attribute is not a size exemption. Missing per-commit history is likewise unknown coverage, not evidence that all -commits are small. Record each triggering full commit ID, its raw and reviewable -totals, and any exclusions; splitting commits does not remove the PR-wide check. +commits are small. Splitting commits does not remove the PR-wide check. -A maintainer may choose a different budget for a particular change; record the -decision and its reason. A contributor or agent cannot silently raise the -threshold or split a single change into commits to avoid it. Smaller changes +No one sets a different budget for a single change: the thresholds are in the +checker. What gets past them is the exemptions below (Dependabot, and a code +owner's own work) and the maintainer's ruleset bypass on the maintainer's own +pull requests. A contributor or agent cannot raise the threshold or +split a single change into commits to avoid it. Smaller changes still need the usual problem, rationale, and validation in their PR description. ## What the contributor explains @@ -89,17 +95,72 @@ question-and-answer transcript and the local review report private; do not post a score or claim that a contributor has been certified. The explanation can be in the contributor's language locally. -The pull request carries no review block. CI measures the commits and the diff -itself, from the base branch's workflow and checker, so nothing in the -description can be edited out and a pull request cannot change the check. A change -under the thresholds passes. A change over them passes only once a maintainer has -added the `explained` label, which only people with triage access can add. A -pull request that Dependabot opened and whose every commit is Dependabot's needs -no label, however large; a person's commit on its branch removes the exemption. The -label means a maintainer has seen the contributor explain the change. CI cannot -see the local evidence, so the label is the maintainer's word and not a -verification. It is not tied to a head: a later push keeps it, and the ruleset's -dismissal of stale approvals is what asks for a fresh review of that push. +The pull request carries no review block and no label to ask for. CI measures +the commits and the diff itself, from the base branch's workflow and checker, so +nothing in the description can be edited out and a pull request cannot change +the check. The interactive review itself is required before the push, as above. +The pre-push hook refuses a large branch without it, and once the evidence +verifies it sends a small record with the branch: the review marker described in +[the workbench](../.claude/skills/codetrial-review/workbench.md#inspect-the-requirement), +which holds the counts, the head and base, a hash of the reviewed diff and +`complete`, and none of your answers. The hook signs it on your machine with +two fingerprints in it: + +- the **review fingerprint**, a SHA-256 over this project, the head, the base, + the diff hash and a digest of your answers, so it names exactly this code and + this explanation without carrying a word of it; and +- the **key fingerprint** (`SHA256:...`) of the SSH key that signed it, which + has to be a key on your GitHub account. + +The signature is one `ssh-keygen -Y sign` over the review fingerprint. The key +is `git config codetrial.signingKey` (a private key, or its `.pub` with the key +in `ssh-agent`), else `user.signingKey` when Git already signs with SSH, else +the first key in `ssh-agent`. Setting one up is a one-time step, listed in +[CONTRIBUTING.md](../CONTRIBUTING.md#pull-requests): a key of its own, +registered on GitHub as a Signing Key, leaves the way you push untouched. The +push prints the key fingerprint it used. + +On the pull request the required `interactive-review` check reads that record +from your fork and checks the signature against the keys GitHub publishes for +you: your authentication keys (`github.com/.keys`) and your signing keys +(`GET /users//ssh_signing_keys`). A change under the thresholds +passes. A change over them passes with a "Large change" notice when the record +is there, matches the code exactly and is signed by one of your keys, and fails, +saying how to fix it, when it is missing, unsigned, signed by another key, or +for other code. A merge that GitHub's +"Update branch" makes, bringing in only the base, keeps the record of the commit +it was made on. A pull request that Dependabot opened and whose every commit is +Dependabot's needs no record, however large; a person's commit on its branch +removes the exemption. Nor does a code owner's own pull request: when the +account that opened it and the GitHub account GitHub links to every commit's +author are all named in the base branch's `.github/CODEOWNERS`, the review is +opt-in. A contributor's commit in it, or one GitHub links to no account, +removes the exemption, so a code owner opening a pull request for someone else's +work does not excuse it; the contributor signs the record, and the check accepts +the keys of the commits' authors as well as the opener's (the opener and up +to nine other commit authors). The check reads the +record from the repository the pull request's branch is in, so the code owner +carries the contributor's record there: +`git fetch refs/notes/codetrial-review`, then +`git notes --ref=codetrial-review merge -s theirs FETCH_HEAD` (the +contributor's record wins over a pending one the code owner's own push left) and +`git push refs/notes/codetrial-review`, with the contributor's commits +unchanged. GitHub links a commit +to an account by its author email, so a code owner's commits need an email on +their account, and a pull request of more than 250 commits gets no exemption. A code owner who does the review gets the same signed +record; one who skips it tells the pre-push hook once with `git config +codetrial.codeOwner `, which the hook checks against the same base file. +The check never reads that setting, so claiming someone else's login only moves +the refusal from the push to the pull request. Everyone else goes through the +interactive review for every large change. + +The signed record shows that the holder of your key declared a completed review +of this code with these answers. It does not show who wrote the answers, or +that you understand them: anyone who reads the checker can still produce one, +but now only as a statement signed in their own name. The approval the branch +ruleset requires is separate, and a push dismisses it, so a later change is +approved again. What that approval confirms about the explanation is not +settled ([development.md](development.md#what-cannot-be-enforced-locally)). An unanswered explanation or incomplete reflection means submission is pending, even if the code review and tests pass. Continue useful review and validation @@ -112,15 +173,48 @@ who wrote an answer or whether the contributor understands it. ## Troubleshooting -Each rejection names its reason in brackets. - -- `[interactive-review/required]`: the pull request reaches a threshold, is not - Dependabot's own, and has no `explained` label. Finish the interactive review on the committed branch and - show the explanation to a maintainer, who adds the label. +- `Interactive review missing` on the `interactive-review` check: the pull + request reaches a threshold and no completed review record for its code came + with the push, usually because the hooks were not installed or were skipped. + Run `make hooks` and finish `/codetrial-review` on the branch. If that + changed the branch, push it; the record goes with it and the push reruns the + check. If the branch is already up to date, send the record alone with + `sh scripts/git-pre-push.sh --publish-review []`, then + close and reopen the pull request: a record sent without the branch does not + rerun the check. The command refuses when the fork's branch is not your + `HEAD`, and says so when there is nothing to send. A first-time + contributor's reopened run may wait for a maintainer to approve it, as the + first one did; a maintainer can also rerun the failed job, which reads the + record again. A record "made for different code" means the branch changed + after the review: review the new head. +- `Large change` notice: the record is there and matches the code. +- `No SSH key to sign the interactive review with` at push: the one-time + signing key setup is missing. Follow the three steps the message lists + (generate, register as a Signing Key, `git config codetrial.signingKey`) and + push again; nothing was pushed. +- `signed with SHA256:..., which does not verify against any SSH key on GitHub + of or the commits' authors` or `have an SSH key on GitHub` on the check: the record was + signed with a key that is not on your account, often a key `ssh-agent` offered. + Set `codetrial.signingKey` to a key you registered (the same three steps), + then run `sh scripts/git-pre-push.sh --publish-review []`, + which signs the record again and sends it without changing the branch, and + close and reopen the pull request. The push prints the key fingerprint it + signed with; `ssh-keygen -lf ` beside your keys on GitHub shows whether + it is one of them. +- `could not read the SSH keys of and the commits' authors from GitHub`: GitHub did not answer while the + check ran. Nothing is wrong with your setup; close and reopen the pull + request to run it again. +- `cannot send the interactive review record` at push: the hook could not push + `refs/notes/codetrial-review` to the remote the branch goes to. Fix access to + that remote and push again. +- `Cannot fetch the change`: the head the run was started for is gone, usually + because the branch was pushed again. The newer push starts its own run. - `Interactive review required` at push: the same thresholds, found before the pull request exists. Finish the explanation and durable report for the exact head being pushed; amending or rechaining changes the head and needs them - again. + again. When a large commit was reverted later and the final diff is empty, + there is nothing left to explain: drop or squash the commit and its revert, + so no commit reaches the threshold, and push again. - `no remote-tracking main to measure`: the hook has no base to size the change against, and does not skip the check for that. Run `git fetch` for the upstream remote (or add it) and push again. @@ -128,9 +222,8 @@ Each rejection names its reason in brackets. fully present locally. Fetch upstream `main` and retry. The required status check is the `interactive-review` job in the `pr-description` -workflow, which keeps its name from when it read the description. It reruns when a -label is added or removed. Merge groups recheck each included pull request's -current labels with the base checker. An independent maintainer approval is a -separate GitHub ruleset requirement, with stale approvals dismissed after a push. -The label and any future signature must not be described as proof of -understanding. +workflow, which keeps its name from when it read the description. It runs when a +pull request is opened, reopened or pushed, and for each merge group. An +independent maintainer approval is a separate GitHub ruleset requirement, with +stale approvals dismissed after a push. Neither the approval nor the signed +record may be described as proof of understanding. diff --git a/scripts/check-interactive-review.py b/scripts/check-interactive-review.py index 50137cfd..391adfc8 100644 --- a/scripts/check-interactive-review.py +++ b/scripts/check-interactive-review.py @@ -54,6 +54,7 @@ def resolve(ref): ) +@functools.lru_cache(maxsize=None) def common(): return Path(git("rev-parse", "--git-common-dir").decode().strip()).resolve() @@ -415,28 +416,33 @@ def status(base, head): return result +def read_marker(head): + """The marker note on `head` as a JSON object, or None. + + In CI the note comes from the fork, so it is data of any shape and size. + """ + try: + found = git("notes", "--ref=" + NOTES, "show", head) + except subprocess.CalledProcessError: + return None + if len(found) > 64 * 1024: + return None + try: + value = json.loads(found) + except ValueError: + return None + return value if isinstance(value, dict) else None + + def record_marker(base, head, *, started=False): result = status(base, head) if not result["required"]: return head, base = result["head"], result["base"] with evidence_lock(head): - old = subprocess.run( - ["git", "--no-lazy-fetch", "notes", "--ref=" + NOTES, "show", head], - capture_output=True, - env={ - **os.environ, - "GIT_NO_REPLACE_OBJECTS": "1", - "GIT_TERMINAL_PROMPT": "0", - }, - ) - try: - old = json.loads(old.stdout) if old.returncode == 0 else {} - except ValueError: - old = {} + old = read_marker(head) or {} started = started or ( - isinstance(old, dict) - and old.get("head") == head + old.get("head") == head and old.get("base") == base and old.get("workbench_started") is True ) @@ -445,31 +451,231 @@ def record_marker(base, head, *, started=False): git("show", "--no-notes", "-s", "--format=%B", head), ) marker = {k: v for k, v in result.items() if k != "evidence"} + if result["status"] == "complete": + # What CI compares with the diff it computes itself, and the + # answers by digest only, so the review fingerprint covers them + # without the note carrying a word of them. + marker["diff_hash"] = hashlib.sha256(patch(base, head)).hexdigest() + marker["answer_digest"] = load(evidence_path(head))["answer_digest"] + marker["review_fingerprint"] = review_fingerprint( + head, base, marker["diff_hash"], marker["answer_digest"] + ) marker.update( schema_version=1, change_id=ids[0].decode() if len(ids) == 1 else None, workbench_started=bool(started), ) - payload = (json.dumps(marker, indent=2) + "\n").encode() - if old == marker: - return - try: - git( - "-c", - "commit.gpgsign=false", - "notes", - "--ref=" + NOTES, - "add", - "-f", - "-F", - "-", - head, - data=payload, + if marker.get("review_fingerprint") and old.get( + "review_fingerprint" + ) == marker.get("review_fingerprint"): + # The same review of the same code: its signature still holds, and + # signing it again on every push would only move the notes ref. + for field in ("signature", "key_fingerprint"): + if field in old: + marker[field] = old[field] + if old != marker: + write_marker(head, marker) + + +def write_marker(head, marker): + payload = (json.dumps(marker, indent=2) + "\n").encode() + try: + git( + "-c", + "commit.gpgsign=false", + "notes", + "--ref=" + NOTES, + "add", + "-f", + "-F", + "-", + head, + data=payload, + ) + except subprocess.CalledProcessError as error: + raise ValueError( + "Cannot record interactive-review Git note; check Git identity and note-ref locks" + ) from error + + +# The review is signed on the contributor's machine with the SSH key already on +# their GitHub account: one signature over one fingerprint, checked in CI +# against the keys GitHub publishes for the pull request's author. +PROJECT = "sysprog21/codetrial" +NAMESPACE = "codetrial-review" +KEY_FINGERPRINT = re.compile(r"SHA256:[A-Za-z0-9+/]{43}") +LOGIN = re.compile(r"[A-Za-z0-9][A-Za-z0-9-]{0,38}") +# A key of its own, registered as a Signing Key, leaves an HTTPS push or the +# key a contributor pushes with untouched; the check accepts both kinds. +# A record sent without a push of the branch does not rerun the check. +PUBLISH = "sh scripts/git-pre-push.sh --publish-review []" +RERUN = "then close and reopen the pull request to rerun this check" +SETUP = ( + "Set up a signing key once: (1) ssh-keygen -t ed25519 -f" + " ~/.ssh/codetrial_signing; (2) on GitHub, Settings > SSH and GPG keys >" + " New SSH key, Key type: Signing Key, paste ~/.ssh/codetrial_signing.pub;" + " (3) git config codetrial.signingKey ~/.ssh/codetrial_signing." +) + + +@functools.lru_cache(maxsize=4) +def code_owners(base): + """The GitHub logins .github/CODEOWNERS names at `base`, lowercased. + + Read from the base commit, in the hook as in CI, so a branch cannot name + its own author. Teams (@org/team) are not resolved. + """ + try: + text = git("show", f"{base}:.github/CODEOWNERS").decode(errors="replace") + except subprocess.CalledProcessError: + return frozenset() + names = set() + for line in text.splitlines(): + for token in line.split("#", 1)[0].split()[1:]: + if token.startswith("@") and "/" not in token: + names.add(token[1:].lower()) + return frozenset(names) + + +def is_code_owner(login, base): + """Code owners may skip the interactive review; for them it is opt-in.""" + return bool(login and LOGIN.fullmatch(login) and login.lower() in code_owners(base)) + + +def review_fingerprint(head, base, diff_hash, answer_digest): + """What is signed: this project, this exact code, these answers.""" + return digest( + { + "project": PROJECT, + "head": head, + "base": base, + "diff_hash": diff_hash, + "answer_digest": answer_digest, + } + ) + + +def config(name): + try: + return git("config", "--get", name).decode().strip() + except subprocess.CalledProcessError: + return "" + + +def signing_key(scratch): + """The key to sign with: codetrial.signingKey, then user.signingKey when Git + already signs with SSH, then the first key in ssh-agent.""" + value = config("codetrial.signingKey") + if not value and config("gpg.format") == "ssh": + value = config("user.signingKey") + if value.startswith("key::"): + literal = scratch / "literal.pub" + literal.write_text(value[5:] + "\n") + return literal + if value: + return Path(value).expanduser() + agent = subprocess.run(["ssh-add", "-L"], capture_output=True, text=True) + if agent.returncode == 0 and agent.stdout.strip(): + first = scratch / "agent.pub" + first.write_text(agent.stdout.splitlines()[0] + "\n") + return first + raise ValueError(f"No SSH key to sign the interactive review with. {SETUP}") + + +def sign_marker(head): + """Sign the completed marker on `head` on this machine.""" + with evidence_lock(head): + marker = read_marker(head) + if not marker or marker.get("status") != "complete": + raise ValueError("No completed review marker to sign") + with tempfile.TemporaryDirectory(prefix="codetrial-sign-") as scratch: + key = signing_key(Path(scratch)) + listed = subprocess.run( + ["ssh-keygen", "-lf", str(key)], capture_output=True, text=True + ) + found = KEY_FINGERPRINT.search(listed.stdout) + if not found: + raise ValueError( + f"Cannot sign the interactive review with {key}. {SETUP}" + ) + # record_marker keeps a signature only while the review it covers + # is unchanged, so the same key need not sign it again. + if marker.get("signature") and marker.get("key_fingerprint") == found.group( + 0 + ): + return found.group(0) + made = subprocess.run( + ["ssh-keygen", "-Y", "sign", "-f", str(key), "-n", NAMESPACE], + input=marker["review_fingerprint"].encode(), + capture_output=True, + ) + if made.returncode != 0: + raise ValueError(f"Cannot sign the interactive review with {key}. {SETUP}") + marker.update(signature=made.stdout.decode(), key_fingerprint=found.group(0)) + write_marker(head, marker) + return found.group(0) + + +class Problem(str): + """Why a review record does not pass; `kind` picks the advice for it: + "outage" (GitHub did not answer), "no_key" or "mismatch" (the signing + setup), or "missing" (the review itself).""" + + def __new__(cls, text, kind="missing"): + problem = super().__new__(cls, text) + problem.kind = kind + return problem + + +def signed_by(fingerprint, signature, login, keys, unavailable=False): + """Why `signature` over `fingerprint` is not the author's, or None. + + `unavailable` says GitHub did not answer for some of the keys, so a failure + is reported as that and not as the contributor's missing setup. + """ + who = login if LOGIN.fullmatch(login or "") else "the author" + lines = [ + line.strip() + for line in (keys or "").splitlines() + if line.strip().startswith(("ssh-", "ecdsa-", "sk-")) + ] + outage = Problem( + f"could not read the SSH keys of {who} and the commits' authors from" + " GitHub to check the signature", + "outage", + ) + if not lines: + return ( + outage + if unavailable + else Problem( + f"neither {who} nor the commits' authors have an SSH key on GitHub" + " to check the signature against", + "no_key", ) - except subprocess.CalledProcessError as error: - raise ValueError( - "Cannot record interactive-review Git note; check Git identity and note-ref locks" - ) from error + ) + with tempfile.TemporaryDirectory(prefix="codetrial-verify-") as scratch: + allowed = Path(scratch) / "allowed_signers" + allowed.write_text( + "".join(f'author namespaces="{NAMESPACE}" {line}\n' for line in lines) + ) + sig = Path(scratch) / "review.sig" + sig.write_text(signature) + checked = subprocess.run( + ["ssh-keygen", "-Y", "verify", "-f", str(allowed), "-I", "author"] + + ["-n", NAMESPACE, "-s", str(sig)], + input=fingerprint.encode(), + capture_output=True, + ) + if checked.returncode == 0: + return None + if unavailable: + return outage + return Problem( + f"does not verify against any SSH key on GitHub of {who} or the" + " commits' authors", + "mismatch", + ) def record_workbench(bundle): @@ -494,31 +700,218 @@ def record_workbench(bundle): DOC = "docs/large-change-explanation.md#troubleshooting" -# A maintainer adds this label to a pull request they have seen explained. Only -# people with triage access can, so an author cannot add or strip it. -LABEL = "explained" -def check_labels(base, head, labels): - """Refuse a change that needs the interactive review and has no label. +def report(base, head): + """Why the change is large, or None when it is under the thresholds. - CI measures the change itself, from the commits and the diff, so nothing in - the description can be edited out. It cannot see the contributor's local - evidence, so the proof that the change was explained is a maintainer's label. + The text holds only counts and commit identities, never a path or other + text from the branch, because the workflow prints it as a workflow command. """ base, head, total, large, unknown = scope(base, head) if not needs_review(total, large, unknown): - return False - if LABEL not in labels: - raise ValueError( - "[interactive-review/required] This change needs the interactive " - f"review ({len(large)} commits at {LARGE_COMMIT} lines or more, " - f"PR diff {total} lines, {len(unknown)} unmeasured). A maintainer " - f"adds the '{LABEL}' label once it has been explained. See {DOC}" + return None + reasons = [] + if total > PR_TOTAL: + reasons.append(f"the pull request changes {total} lines (more than {PR_TOTAL})") + if large: + commits = ", ".join(f"{c['sha'][:12]} ({c['known_lines']})" for c in large) + reasons.append( + f"{len(large)} commit(s) change {LARGE_COMMIT} lines or more: {commits}" ) + if unknown: + reasons.append( + f"{len(unknown)} commit(s) change files that cannot be counted as text" + ) + return "This is a large change: " + "; ".join(reasons) + + +def is_ancestor(older, newer): + try: + git("merge-base", "--is-ancestor", older, newer) + except subprocess.CalledProcessError: + return False return True +def updated_from(sha, base): + """The commit `sha` merged `base`'s history into, when that is all it did. + + GitHub's "Update branch" makes such a merge, which carries no record of its + own. It counts only when its second parent is on the pull request's base + and its tree is the clean merge of its parents. + """ + chain = module(ROOT / "scripts/hashchain.py") + _, _, tree, parents = chain.commit(sha) + if len(parents) != 2 or not is_ancestor(parents[1], base): + return None + try: + chain.check_merge(tree, parents) + except (ValueError, subprocess.CalledProcessError): + return None + return parents[0] + + +def recorded(base, head, login=None, keys=None, unavailable=False): + """Why no completed review was pushed for `head`, or None when one was. + + The pre-push hook writes the marker once the local evidence verifies, + signs it and sends it with the branch. CI reads it from the fork as data: + it is a declaration, signed with a key on the author's account, of a + completed review of exactly this code. It does not show who wrote the + answers or that anyone understood them. + """ + # FIXME: nothing yet checks that the author can explain the change; the + # approver's question about one passage was withdrawn until a replacement + # is decided. + tip = head + marker = read_marker(tip) + while marker is None: + earlier = updated_from(tip, base) + if earlier is None: + return Problem(f"no review record was pushed for {head[:12]}") + tip, marker = earlier, read_marker(earlier) + start = marker.get("base") + if marker.get("head") != tip or marker.get("status") != "complete": + return Problem( + f"the record on {tip[:12]} is not a completed review of that commit" + ) + if not ( + isinstance(start, str) + and re.fullmatch("[0-9a-f]{40}", start) + and is_ancestor(start, base) + and is_ancestor(start, tip) + ): + return Problem(f"the record on {tip[:12]} was made against another base") + diff_hash = hashlib.sha256(patch(start, tip)).hexdigest() + answers = marker.get("answer_digest") + fingerprint = ( + review_fingerprint(tip, start, diff_hash, answers) + if isinstance(answers, str) and re.fullmatch("[0-9a-f]{64}", answers) + else None + ) + if ( + fingerprint is None + or marker.get("diff_hash") != diff_hash + or marker.get("review_fingerprint") != fingerprint + ): + return Problem(f"the record on {tip[:12]} was made for different code") + signature = marker.get("signature") + if ( + not isinstance(signature, str) + or "-----BEGIN SSH SIGNATURE-----" not in signature + ): + return Problem(f"the record on {tip[:12]} is not signed") + problem = signed_by(fingerprint, signature, login, keys, unavailable) + if problem is None: + return None + if problem.kind == "mismatch": + key = marker.get("key_fingerprint") + key = ( + key if isinstance(key, str) and KEY_FINGERPRINT.fullmatch(key) else "a key" + ) + return Problem( + f"the record on {tip[:12]} is signed with {key}, which {problem}", + problem.kind, + ) + return problem + + +def owner_exempt(base, head, login, authors): + """Why a code owner's pull request still needs the review, or None. + + The exemption is for a code owner's own work: the opener and the GitHub + account of every commit's author must be code owners, so a contributor's + commits do not pass by a code owner opening the pull request. `authors` + maps each commit of the pull request, as GitHub lists them, to the login + GitHub links its author to ("" for none); None when GitHub did not say. + The list must name exactly the commits checked here, so one read after the + branch moved, or cut off at GitHub's 250 commits, does not count. + + GitHub links an author by the email in the commit, which anyone can write; + this keeps an honest code owner from excusing someone else's work by + opening it, and is not proof of who wrote a commit. + """ + if authors is None: + return ( + f"{login} is a code owner, but GitHub did not list the commits'" + " authors, so the exemption could not be confirmed" + ) + commits = set(git("rev-list", f"{base}..{head}").decode().split()) + if set(authors) != commits: + return ( + f"{login} is a code owner, but the {len(authors)} commits GitHub" + f" listed are not the {len(commits)} checked here (the branch" + " moved, or it has more than GitHub lists), so the exemption could" + " not be confirmed" + ) + others = [ + name or "an author linked to no GitHub account" for name in authors.values() + ] + others = sorted({name for name in others if not is_code_owner(name, base)}) + if others: + return ( + f"{login} is a code owner, but commits here are by {', '.join(others)}," + " so the interactive review applies" + ) + return None + + +def pr_result(base, head, login=None, keys=None, unavailable=False, authors=None): + """The one line the workflow prints, and whether the check passes.""" + base, head = resolve(base), resolve(head) + reasons = report(base, head) + if reasons is None: + return "This change is under the interactive review thresholds.", True + not_exempt = None + if is_code_owner(login, base): + not_exempt = owner_exempt(base, head, login, authors) + if not_exempt is None: + return ( + "::notice title=Large change::" + + reasons + + f". {login} and every commit's author are code owners, for" + f" whom the interactive review is optional. See {DOC}" + ), True + problem = recorded(base, head, login, keys, unavailable) + if problem is not None: + if problem.kind == "outage": + advice = ( + " This is GitHub not answering, not your setup: close and reopen" + " the pull request to rerun this check." + ) + elif problem.kind in ("no_key", "mismatch"): + advice = ( + " The review is signed on your machine with an SSH key on your" + f" GitHub account. {SETUP} Then sign and send the record again" + f" with {PUBLISH}, and {RERUN}." + ) + else: + advice = ( + " A large change needs the interactive review before it is" + " pushed; the pre-push hook checks for it, signs its record and" + " sends it with the branch, so a missing record usually means the" + " hooks were not installed or were skipped. Run make hooks and" + " finish /codetrial-review on this branch. If that changed the" + " branch, push it: the record goes with it. If not, send the" + f" record with {PUBLISH}, and {RERUN}." + ) + return ( + "::error title=Interactive review missing::" + + reasons + + (f"; {not_exempt}" if not_exempt else "") + + f", and {problem}." + + advice + + f" See {DOC}" + ), False + return ( + "::notice title=Large change::" + + reasons + + ". Its interactive review was recorded at push." + f" See {DOC}" + ), True + + def dependabot_exempt(base, head, login, kind): """Whether this pull request is Dependabot's and nothing else is in it. @@ -533,23 +926,44 @@ def dependabot_exempt(base, head, login, kind): return bool(commits) and len(chain.bots(commits)) == len(commits) +def read_capped(path, limit=256 * 1024): + """A file the workflow wrote from GitHub's answers, read up to `limit`.""" + with open(path, "rb") as source: + return source.read(limit).decode(errors="replace") + + def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument( "command", - choices=["staged", "push", "certify", "status", "mark", "pr"], + choices=["staged", "push", "certify", "status", "mark", "pr", "marker"], ) parser.add_argument("--base") parser.add_argument("--head", default="HEAD") parser.add_argument("--bundle") - parser.add_argument( - "--labels", default="[]", help="pr: the pull request's labels, as a JSON list" - ) parser.add_argument("--author-login", help="pr: the pull request opener") parser.add_argument("--author-type", help="pr: the opener's account type") + parser.add_argument( + "--author-keys", + help="pr: a file of the public SSH keys of the opener and commit authors", + ) + parser.add_argument( + "--commit-authors", + help="pr: a file of ' ' lines, each commit of the pull" + " request and the GitHub login its author is linked to; absent when" + " GitHub did not list them", + ) + parser.add_argument( + "--keys-unavailable", + action="store_true", + help="pr: GitHub did not answer for some of those keys", + ) args = parser.parse_args() try: - if args.command == "staged": + if args.command == "marker": + # For the pre-push hook, which sends only a completed record. + print((read_marker(resolve(args.head)) or {}).get("status", "")) + elif args.command == "staged": count, _ = lines( git( "diff", @@ -579,14 +993,24 @@ def main(): ): print("Dependabot's own pull request; no explanation to ask for.") return 0 - labels = json.loads(args.labels) - if not isinstance(labels, list): - raise ValueError("--labels must be a JSON list of names") - if check_labels(args.base, args.head, labels): - print(f"A maintainer's '{LABEL}' label covers this change.") - else: - print("This change is under the interactive review thresholds.") - return 0 + keys = read_capped(args.author_keys) if args.author_keys else None + authors = None + if args.commit_authors: + entries = ( + line.strip().partition(" ") + for line in read_capped(args.commit_authors).splitlines() + ) + authors = {sha: name.strip() for sha, _, name in entries if sha} + line, passed = pr_result( + args.base, + args.head, + args.author_login, + keys, + args.keys_unavailable, + authors, + ) + print(line) + return 0 if passed else 1 if args.command == "mark": record_marker(args.base, args.head) else: @@ -600,12 +1024,30 @@ def main(): try: verify(head, base) except VERIFY_ERRORS as error: + owner = config("codetrial.codeOwner") + if is_code_owner(owner, base): + print( + f"Interactive review skipped: {owner} is a code owner," + " for whom it is optional; /codetrial-review still" + " runs it and this push then sends its record. The" + " pull request's check does not read this setting: it" + " passes without a record only when the account that" + " opens the pull request and the GitHub account of" + " every commit's author are code owners." + ) + return 0 triggers = ", ".join( f"{c['sha']} ({c['known_lines']} lines)" for c in large ) or ("unmeasured changes" if unknown else "PR aggregate only") raise ValueError( f"Interactive review required ({len(large)} commits >=200 lines; PR diff {total} lines). Unmeasured commits: {len(unknown)}. Trigger: {triggers}. Run /codetrial-review on the full committed branch; finish its web explanation and durable report before retrying. {error} See {DOC}" ) from error + key = sign_marker(head) + print( + f"Interactive review of {head[:12]} signed with {key}; the" + " pull request check accepts it only if this key is on your" + " GitHub account, as an authentication or a signing key." + ) except ( OSError, ValueError, @@ -615,10 +1057,20 @@ def main(): subprocess.CalledProcessError, ) as error: # Do not print Git stderr, answer text or database records from a hook. - parser.exit( - 1, - f"Interactive review check failed: {error if not isinstance(error, subprocess.CalledProcessError) else 'cannot inspect local history; fetch the PR base first'}\n", + if args.command == "pr": + # Printed as a workflow command, so nothing from the branch goes in. + parser.exit( + 1, + "::error title=Cannot check the interactive review::The change's" + " history could not be read; rerun the job, and if it fails again" + f" a maintainer looks at the job log. See {DOC}\n", + ) + detail = ( + "cannot inspect local history; fetch the PR base first" + if isinstance(error, subprocess.CalledProcessError) + else error ) + parser.exit(1, f"Interactive review check failed: {detail}\n") return 0 diff --git a/scripts/check-queue-review.py b/scripts/check-queue-review.py index 8c0d111d..965cb4ab 100644 --- a/scripts/check-queue-review.py +++ b/scripts/check-queue-review.py @@ -1,14 +1,10 @@ #!/usr/bin/env python3 -"""Recheck each pull request represented in a merge group for the -interactive review it needs, and keep a pull request from editing the workflow -that does so.""" +"""Keep a pull request from editing the workflow that the merge queue runs +for the interactive-review check.""" import argparse -import importlib.util -import json import os import subprocess -from urllib.parse import urlencode WORKFLOW = ".github/workflows/pr-description.yml" CHECK = "interactive-review" @@ -33,51 +29,6 @@ def git(*args): ).stdout.strip() -def api(path): - pages = json.loads( - subprocess.check_output(["gh", "api", "--paginate", "--slurp", path], text=True) - ) - return [item for page in pages for item in page] - - -def within(tip, parents): - """`tip` and its ancestors among the commits of the group.""" - seen, todo = set(), [tip] - while todo: - sha = todo.pop() - if sha not in seen and sha in parents: - seen.add(sha) - todo.extend(parents[sha]) - return seen - - -def check(base, head, pulls, checker): - # Only merge commits may be supplied by GitHub itself. Every ordinary - # commit must belong to a live PR whose exact head is in this group. - git("merge-base", "--is-ancestor", base, head) - parents = {} - for line in git("rev-list", "--parents", f"{base}..{head}").splitlines(): - sha, *above = line.split() - parents[sha] = above - pending = {sha for sha, above in parents.items() if len(above) < 2} - included = [] - for pull in pulls: - tip = pull["head"]["sha"] - if tip not in parents: - continue - included.append(pull) - user = pull["user"] - if not checker.dependabot_exempt(base, tip, user["login"], user.get("type")): - checker.check_labels(base, tip, [x["name"] for x in pull.get("labels", [])]) - pending.difference_update(within(tip, parents)) - if not included or pending: - raise ValueError( - "Merge group contains commits without a matching open PR head" - + (": " + ", ".join(sha[:12] for sha in sorted(pending)) if pending else "") - ) - return [pull["number"] for pull in included] - - def committed(rev, path): """The file at `rev`, or None when it is not there. Any other failure raises.""" if not git("ls-tree", "--name-only", rev, "--", path): @@ -115,7 +66,9 @@ def guard(base, head, approved): None, git("ls-tree", "-z", "--name-only", head, folder + "/").split("\0") ): if path != WORKFLOW and CHECK in (committed(head, path) or "").lower(): - problems.append(f"{path} names the {CHECK} check") + # ascii(): a name from the branch is printed, and a newline in it + # could start a workflow command. + problems.append(f"{ascii(path)} names the {CHECK} check") return problems @@ -123,51 +76,25 @@ def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--base", required=True) parser.add_argument("--head", required=True) - parser.add_argument("--branch", help="queue: the branch the group merges into") - parser.add_argument("--repo", help="queue: owner/name") - parser.add_argument( - "--guard", action="store_true", help="check the queue workflow is unchanged" - ) - parser.add_argument( - "--approved", default="", help="guard: a maintainer-approved head" - ) + parser.add_argument("--approved", default="", help="a maintainer-approved head") args = parser.parse_args() - if args.guard: - try: - problems = guard(args.base, args.head, args.approved) - except (subprocess.CalledProcessError, UnicodeDecodeError) as error: - # Not being able to compare is not a pass. - parser.exit( - 1, f"Cannot compare the queue workflow with its branch point: {error}\n" - ) - if problems: - parser.exit( - 1, - "The workflow that guards the merge queue may not change in a pull " - "request:\n " - + "\n ".join(problems) - + "\nA maintainer approves such a change by setting " - "CODETRIAL_POLICY_APPROVED_HEAD to the exact head.\n", - ) - print("The queue workflow is the base branch's.") - return - if not (args.branch and args.repo): - parser.error("the queue check requires --branch and --repo") - spec = importlib.util.spec_from_file_location( - "interactive_review", - os.path.join(os.path.dirname(__file__), "check-interactive-review.py"), - ) - checker = importlib.util.module_from_spec(spec) - spec.loader.exec_module(checker) - query = urlencode({"state": "open", "base": args.branch, "per_page": 100}) try: - pulls = api(f"repos/{args.repo}/pulls?{query}") - except (subprocess.CalledProcessError, OSError, ValueError) as error: - parser.exit(1, f"Cannot list the open pull requests: {error}\n") - print( - "Verified merge-group pull requests:", - check(args.base, args.head, pulls, checker), - ) + problems = guard(args.base, args.head, args.approved) + except (subprocess.CalledProcessError, UnicodeDecodeError) as error: + # Not being able to compare is not a pass. + parser.exit( + 1, f"Cannot compare the queue workflow with its branch point: {error}\n" + ) + if problems: + parser.exit( + 1, + "The workflow that guards the merge queue may not change in a pull " + "request:\n " + + "\n ".join(problems) + + "\nA maintainer approves such a change by setting " + "CODETRIAL_POLICY_APPROVED_HEAD to the exact head.\n", + ) + print("The queue workflow is the base branch's.") if __name__ == "__main__": diff --git a/scripts/git-pre-push.sh b/scripts/git-pre-push.sh index aaea492e..7f585f4b 100755 --- a/scripts/git-pre-push.sh +++ b/scripts/git-pre-push.sh @@ -18,7 +18,53 @@ url=${2:-} # Read the pushed refs first: the tag sync below decides from them, and the # transport must not get a chance to consume them. -refs=$(cat) +publish_only=0 +if [ "$remote" = --publish-review ]; then + + # Run by hand, for a branch already pushed: judge its head as a push would + # and send the record, which only a push of the branch carries otherwise. + [ "$#" -eq 2 ] || [ "$#" -eq 3 ] || { + echo "Usage: $0 --publish-review []" >&2 + exit 1 + } + publish_only=1 + remote=$2 + url=$(git remote get-url --push "$remote") || exit 1 + branch=$(git symbolic-ref --quiet HEAD) || { + echo "HEAD is detached; check out the topic branch the pull request is from." >&2 + exit 1 + } + target=${3:-$branch} + target=refs/heads/${target#refs/heads/} + [ "$target" != refs/heads/main ] || { + echo "A pull request comes from a topic branch, not main; name it: $0 --publish-review $remote " >&2 + exit 1 + } + head=$(git rev-parse HEAD) || exit 1 + + # The record is for the head the pull request has, so the two must agree, + # read from where the record goes: the push URL, which may differ from the + # one fetched from. + listed=$(GIT_TERMINAL_PROMPT=0 git ls-remote "$url" "$target") || { + echo "Cannot read ${target#refs/heads/} from $remote; check that you can reach it." >&2 + exit 1 + } + there=$(printf '%s\n' "$listed" | awk -v ref="$target" '$2 == ref { print $1; exit }') + if [ -z "$there" ]; then + echo "$remote has no branch ${target#refs/heads/}." >&2 + echo "If the pull request's branch has another name there, name it: $0 --publish-review $remote " >&2 + echo "If it was never pushed, push it, which sends the record with it." >&2 + exit 1 + fi + if [ "$there" != "$head" ]; then + echo "$remote has $(printf %.12s "$there") on ${target#refs/heads/}, not HEAD $(printf %.12s "$head")." >&2 + echo "Push the branch instead, which sends the record with it: git push $remote HEAD:$target" >&2 + exit 1 + fi + refs="$branch $head $target $head" +else + refs=$(cat) +fi # Release tags follow origin, and only a push to origin is held up by it, so an # origin that is slow or unreachable cannot block a push elsewhere. A push that @@ -108,10 +154,17 @@ git rev-parse --verify --quiet "$review_policy^{commit}" > /dev/null 2>&1 \ review_path=.claude/skills/codetrial-review/SKILL.md note= test_note= +review_notes=refs/notes/codetrial-review +publish_review=0 +review_heads= while read -r local_ref local_sha remote_ref remote_sha; do [ -n "${local_ref:-}" ] || continue [ "$local_sha" != "$zero" ] || continue + + # The review record this hook sends below carries no code and no commit + # message to judge, and judging it would loop back into this hook. + [ "$remote_ref" != "$review_notes" ] || continue git cat-file -e "${local_sha}^{commit}" 2> /dev/null || continue # A published upstream policy must not be reset by the outgoing branch. set -- verify --tip "$local_sha" @@ -228,6 +281,16 @@ while read -r local_ref local_sha remote_ref remote_sha; do failed=1 continue fi + + # A large change that passed has a completed, signed marker on + # its head; CI reads it from this remote. A code owner who + # skipped the review leaves only a pending one, which stays + # here. + if [ "$(python3 "$script_dir/check-interactive-review.py" marker \ + --head "$local_sha")" = complete ]; then + publish_review=1 + review_heads="$review_heads $local_sha" + fi fi if [ -z "$note" ] && [ "$review_enabled" -eq 1 ] \ && ! grep -qx "$local_sha" "$reviewed" 2> /dev/null; then @@ -253,6 +316,60 @@ done << EOF $refs EOF +# The interactive-review check reads the marker from the remote the branch goes +# to, so it is sent first and the pull request event finds it there. Records the +# remote already holds for other branches are merged in, not overwritten. A +# record that cannot be sent would turn the check red, so the push stops here. +if [ "$failed" -eq 0 ] && [ "$publish_review" -eq 1 ]; then + incoming=refs/notes/codetrial-review-incoming + + # The records signed here are kept by blob: when the remote is ahead, the + # merge fast-forwards to its notes, and -s ours never sees a conflict. + signed= + for sha in $review_heads; do + signed="$signed $sha:$(git notes --ref="$review_notes" list "$sha")" + done + if GIT_TERMINAL_PROMPT=0 git fetch -q --no-tags "$url" \ + "+$review_notes:$incoming" 2> /dev/null; then + + # A merge left half done would block every later push, so it is + # abandoned on failure; the records here are unchanged by it. + if ! git notes --ref="$review_notes" merge -q -s ours "$incoming"; then + git notes --ref="$review_notes" merge --abort 2> /dev/null + failed=1 + fi + for pair in $signed; do + sha=${pair%%:*} + [ "$failed" -eq 0 ] || break + [ "$(git notes --ref="$review_notes" list "$sha" 2> /dev/null)" = "${pair#*:}" ] \ + || git notes --ref="$review_notes" add -f -C "${pair#*:}" "$sha" \ + || failed=1 + done + git update-ref -d "$incoming" + fi + if [ "$failed" -ne 0 ] \ + || ! GIT_TERMINAL_PROMPT=0 git push -q "$remote" "$review_notes:$review_notes"; then + echo "Push rejected: cannot send the interactive review record to $remote." >&2 + echo "Check that you can push to $remote, then push again." >&2 + failed=1 + fi +fi + +# By hand, say what happened: nothing else would, and a check that has already +# failed does not rerun for a record alone. The messages above say "Push +# rejected", but here no branch was pushed. +if [ "$publish_only" -eq 1 ]; then + if [ "$failed" -ne 0 ]; then + echo "Nothing was sent; the branch on $remote is unchanged." >&2 + elif [ "$publish_review" -eq 1 ]; then + echo "Sent the interactive review record for $(printf %.12s "$head") to $remote." >&2 + echo "Close and reopen the pull request to rerun its interactive-review check." >&2 + else + # The check above said why, when a code owner skipped the review. + echo "Nothing sent: $(printf %.12s "$head") has no completed interactive review record to send, and its push check passed without one." >&2 + fi +fi + # Said once the whole push is judged, so a push refused over another ref does # not also advise a review of a head that is not going anywhere. [ "$failed" -ne 0 ] || [ -z "$note" ] || echo "$note" >&2 diff --git a/tests/browser/review-workbench.test.js b/tests/browser/review-workbench.test.js index 33316162..81633feb 100644 --- a/tests/browser/review-workbench.test.js +++ b/tests/browser/review-workbench.test.js @@ -458,7 +458,9 @@ test( ); const notice = await page.locator("#completion-notice").innerText(); assert.match(notice, /close this window and continue submitting/i); - assert.match(notice, /explained/); + assert.match(notice, /signs this review and sends its/); + assert.doesNotMatch(notice, /explain one passage/); + assert.doesNotMatch(notice, /label/); assert.equal(await page.locator("#close-window").isVisible(), true); assert.equal( await page.locator("#reflect-save").textContent(), @@ -850,7 +852,10 @@ test( await page.locator("#finished").waitFor({ state: "visible" }); assert.equal(await page.locator("#workspace").isVisible(), false); assert.doesNotMatch(page.url(), /token=/); - assert.match(await page.locator("#finished").innerText(), /explained/); + assert.match( + await page.locator("#finished").innerText(), + /signs this review and sends its/, + ); if (server.exitCode === null) await once(server, "exit", { signal: AbortSignal.timeout(8000) }); assert.equal(server.exitCode, 0); diff --git a/tests/test_hashchain.py b/tests/test_hashchain.py index 67e4e49d..0c2d3111 100644 --- a/tests/test_hashchain.py +++ b/tests/test_hashchain.py @@ -561,7 +561,7 @@ def test_the_review_check_runs_the_base_branchs_workflow(self): # A job its own condition skips counts as passing, so none may have one. self.assertNotRegex(text, r"(?m)^ if:") self.assertNotIn("continue-on-error", text) - self.assertIn("--guard", text) + self.assertIn("--approved", text) def test_one_definition_decides_who_dependabot_is(self): chain = importlib.util.module_from_spec( diff --git a/tests/test_interactive_review_hook.py b/tests/test_interactive_review_hook.py index 7cd39b2c..c1f0895b 100644 --- a/tests/test_interactive_review_hook.py +++ b/tests/test_interactive_review_hook.py @@ -11,10 +11,24 @@ import sys import tempfile import unittest -from types import SimpleNamespace + +SPEC = importlib.util.spec_from_file_location( + "support", Path(__file__).with_name("support.py") +) +support = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(support) ROOT = Path(__file__).resolve().parents[1] GATE = ROOT / "scripts/check-interactive-review.py" + + +def load_gate(name="gate_under_test"): + spec = importlib.util.spec_from_file_location(name, GATE) + gate = importlib.util.module_from_spec(spec) + spec.loader.exec_module(gate) + return gate + + SKILL = ROOT / ".claude/skills/codetrial-review" @@ -35,11 +49,25 @@ def setUp(self): self.git("config", "user.email", "review@example.test") self.git("config", "commit.gpgsign", "false") self.git("config", "codetrial.hookSuite", "true") + # The contributor's own SSH key, the one on their GitHub account. + self.key = self.root / "id_ed25519" + subprocess.run( + ["ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-C", "contributor"] + + ["-f", str(self.key)], + check=True, + ) + self.git("config", "codetrial.signingKey", str(self.key)) + self.author_keys = self.root / "contributor.keys" + self.author_keys.write_text(Path(str(self.key) + ".pub").read_text()) (self.repo / "change.txt").write_text("base\n") self.commit("Create the review fixture") self.base = self.git("rev-parse", "HEAD").stdout.strip() self.git("remote", "add", "upstream", "https://github.com/sysprog21/codetrial") self.git("update-ref", "refs/remotes/upstream/main", self.base) + # Where the branch goes: the pre-push hook sends the review record here. + self.fork = self.root / "fork.git" + self.git("init", "--bare", "-q", str(self.fork)) + self.git("remote", "add", "fork", str(self.fork)) (self.repo / "scripts").mkdir() for name in [ "git-pre-push.sh", @@ -86,7 +114,7 @@ def commit(self, title): def push(self, check=False): head = self.git("rev-parse", "HEAD").stdout.strip() return self.run_command( - ["sh", "scripts/git-pre-push.sh", "fork", "local-fixture"], + ["sh", "scripts/git-pre-push.sh", "fork", str(self.fork)], data=f"refs/heads/topic {head} refs/heads/topic {'0' * 40}\n", check=check, ) @@ -251,47 +279,680 @@ def synced(fd): self.assertLess(seen.index(parent), seen.index(str(bundle))) self.assertNotEqual(self.push().returncode, 0) - def gate(self, command, *args): + def gate(self, command, *args, base=None): return self.run_command( - [ - sys.executable, - "scripts/check-interactive-review.py", - command, - "--base", - self.base, - *args, + [sys.executable, "scripts/check-interactive-review.py", command] + + ["--base", base or self.base, *args], + check=False, + ) + + def test_a_small_change_is_under_the_thresholds(self): + self.large(10) + result = self.gate("pr") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("under the interactive review thresholds", result.stdout) + self.assertNotIn("::notice", result.stdout) + + def fork_notes(self, *args): + return self.git( + "--git-dir", + str(self.fork), + "notes", + "--ref=codetrial-review", + *args, + check=False, + ) + + def fork_note(self, head): + found = self.fork_notes("show", head) + return json.loads(found.stdout) if found.returncode == 0 else None + + def as_ci(self): + """Keep only what CI sees: the record the fork holds, not this clone's.""" + self.git("update-ref", "-d", "refs/notes/codetrial-review") + self.git( + "fetch", + "-q", + "fork", + "+refs/notes/codetrial-review:refs/notes/codetrial-review", + check=False, + ) + + def pr( + self, + base=None, + head="HEAD", + keys=None, + login="contributor", + authors=None, + unavailable=False, + ): + """`authors`: the GitHub login of each commit's author, in rev-list + order, as GitHub lists them; None when GitHub did not. Entries past the + branch's commits stand for commits it does not have.""" + extra = [] + if authors is not None: + commits = self.git( + "rev-list", f"{base or self.base}..{head}" + ).stdout.split() + commits += [f"{n:040x}" for n in range(len(authors) - len(commits))] + listed = self.root / "commit.authors" + listed.write_text( + "".join(f"{sha} {name}\n" for sha, name in zip(commits, authors)) + ) + extra = ["--commit-authors", str(listed)] + return self.gate( + "pr", + *["--head", head, "--author-login", login, "--author-type", "User"], + *["--author-keys", str(keys or self.author_keys)], + *extra, + *(["--keys-unavailable"] if unavailable else []), + base=base, + ) + + def other_key(self): + other = self.root / "other_ed25519" + if not other.exists(): + subprocess.run( + ["ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-f", str(other)], + check=True, + ) + return other + + def reviewed_push(self): + self.large() + self.prepare() + pushed = self.push() + self.assertEqual(pushed.returncode, 0, pushed.stderr) + return self.git("rev-parse", "HEAD").stdout.strip() + + def test_pushing_the_same_review_again_does_not_sign_it_again(self): + head = self.reviewed_push() + signed = self.git("rev-parse", "refs/notes/codetrial-review").stdout + self.assertEqual(self.push().returncode, 0) + self.assertEqual( + self.git("rev-parse", "refs/notes/codetrial-review").stdout, signed + ) + # Another key still signs it again. + self.git("config", "codetrial.signingKey", str(self.other_key())) + self.assertEqual(self.push().returncode, 0) + self.assertNotEqual( + self.review_note(head)["signature"], + json.loads(self.git("show", f"{signed.strip()}:{head}").stdout)[ + "signature" ], + ) + + def test_a_large_change_without_a_review_record_says_how_to_fix_it(self): + self.large() + head = self.git("rev-parse", "HEAD").stdout.strip() + result = self.pr() + self.assertEqual(result.returncode, 1, result.stdout) + error = result.stdout.strip() + self.assertEqual(len(error.splitlines()), 1, error) + self.assertTrue( + error.startswith("::error title=Interactive review missing::"), error + ) + self.assertIn(head[:12], error) + self.assertIn("make hooks", error) + self.assertIn("/codetrial-review", error) + # The record can be sent alone, but that does not rerun the check. + self.assertIn("--publish-review ", error) + self.assertIn("close and reopen the pull request", error) + self.assertNotIn("if nothing changed", error) + self.assertNotIn("Traceback", result.stderr) + + def publish(self, *args): + return self.run_command( + ["sh", "scripts/git-pre-push.sh", "--publish-review", "fork", *args], check=False, ) - def labelled(self, *names): - return self.gate("pr", "--labels", json.dumps(list(names))) + def pushed_without_record(self): + """A reviewed branch pushed with no hook installed: no record went.""" + self.large() + self.prepare() + head = self.git("rev-parse", "HEAD").stdout.strip() + self.git("push", "-q", "fork", "HEAD:refs/heads/topic") + self.assertIsNone(self.fork_note(head)) + return head + + def test_publish_review_sends_the_record_a_pushed_branch_lacks(self): + head = self.pushed_without_record() + result = self.publish("topic") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn( + f"Sent the interactive review record for {head[:12]}", result.stderr + ) + self.assertIn("Close and reopen the pull request", result.stderr) + self.assertEqual(self.fork_note(head)["status"], "complete") + self.assertEqual(self.git("rev-parse", "HEAD").stdout.strip(), head) + # The fork's branch was left alone; CI now accepts the record. + self.assertEqual( + self.git("--git-dir", str(self.fork), "rev-parse", "topic").stdout.strip(), + head, + ) + self.as_ci() + self.assertEqual(self.pr().returncode, 0) + + def test_publish_review_replaces_a_record_signed_with_another_key(self): + head = self.pushed_without_record() + self.git("config", "codetrial.signingKey", str(self.other_key())) + self.assertEqual(self.publish("topic").returncode, 0) + wrong = self.fork_note(head)["key_fingerprint"] + self.as_ci() + result = self.pr() + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("does not verify", result.stdout) + self.assertIn("--publish-review ", result.stdout) + self.assertIn("close and reopen the pull request", result.stdout) + self.git("config", "codetrial.signingKey", str(self.key)) + result = self.publish("topic") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertNotEqual(self.fork_note(head)["key_fingerprint"], wrong) + self.as_ci() + self.assertEqual(self.pr().returncode, 0) + + def test_publish_review_refuses_a_head_the_fork_does_not_have(self): + self.large() + self.prepare() + result = self.publish("topic") + self.assertEqual(result.returncode, 1) + self.assertIn("fork has no branch topic.", result.stderr) + self.assertIn("--publish-review fork ", result.stderr) + self.git("push", "-q", "fork", f"{self.base}:refs/heads/topic") + result = self.publish("topic") + self.assertEqual(result.returncode, 1) + self.assertIn(f"fork has {self.base[:12]} on topic, not HEAD", result.stderr) + self.assertIn("Push the branch instead", result.stderr) + self.assertEqual(self.fork_notes("list").stdout, "") + + def test_publish_review_finds_the_fork_branch_by_its_exact_name(self): + head = self.pushed_without_record() + # Only a branch of exactly that name counts, not one ending in it. + self.git("push", "-q", "fork", f"{head}:refs/heads/x/refs/heads/other") + self.assertIn("fork has no branch other.", self.publish("other").stderr) + # The local branch has another name; the fork's is given, as a name or + # as a full ref. + for name in ("topic", "refs/heads/topic"): + with self.subTest(name=name): + self.git("push", "fork", ":refs/notes/codetrial-review", check=False) + result = self.publish(name) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.fork_note(head)["status"], "complete") + + def test_publish_review_says_when_the_fork_cannot_be_read(self): + self.large() + self.prepare() + self.git("remote", "set-url", "fork", str(self.root / "missing.git")) + result = self.publish("topic") + self.assertEqual(result.returncode, 1) + self.assertIn("Cannot read topic from fork", result.stderr) + self.assertNotIn("has no branch", result.stderr) + + def test_publish_review_says_nothing_was_sent_when_the_check_fails(self): + self.large() + self.git("push", "-q", "fork", "HEAD:refs/heads/topic") + result = self.publish("topic") + self.assertEqual(result.returncode, 1) + self.assertIn("Interactive review required", result.stdout + result.stderr) + self.assertIn( + "Nothing was sent; the branch on fork is unchanged.", result.stderr + ) - def test_a_small_change_needs_no_label(self): + def test_publish_review_says_why_it_sends_nothing(self): self.large(10) - result = self.labelled() + self.git("push", "-q", "fork", "HEAD:refs/heads/topic") + result = self.publish("topic") self.assertEqual(result.returncode, 0, result.stderr) - self.assertIn("under the interactive review thresholds", result.stdout) + self.assertIn( + "has no completed interactive review record to send", result.stderr + ) + self.git("checkout", "-q", "--detach") + result = self.publish("topic") + self.assertEqual(result.returncode, 1) + self.assertIn("HEAD is detached", result.stderr) + self.git("checkout", "-q", "-") + result = self.publish("main") + self.assertEqual(result.returncode, 1) + self.assertIn("not main", result.stderr) + result = self.publish("fork", "topic", "extra") + self.assertEqual(result.returncode, 1) + self.assertIn("Usage:", result.stderr) + + def test_a_reviewed_push_sends_the_record_the_check_reads(self): + head = self.reviewed_push() + record = self.fork_note(head) + self.assertEqual(record["status"], "complete") + # The record says that a review happened, never what was said in it. + for private in ("answers", "feedback", "questions", "reflection"): + self.assertNotIn(private, record) + self.as_ci() + result = self.pr() + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + notice = result.stdout.strip() + self.assertEqual(len(notice.splitlines()), 1, notice) + self.assertTrue(notice.startswith("::notice title=Large change::"), notice) + self.assertIn(head[:12], notice) + self.assertIn("recorded at push", notice) + self.assertNotIn("explain one passage", notice) + + def test_a_push_keeps_the_records_the_fork_holds_for_other_branches(self): + subprocess.run( + ["git", "--git-dir", str(self.fork), "-c", "user.name=Other"] + + ["-c", "user.email=other@example.test", "notes"] + + ["--ref=codetrial-review", "add", "-m", '{"other": true}', self.base], + check=True, + capture_output=True, + env=self.env, + ) + head = self.reviewed_push() + self.assertEqual(self.fork_note(self.base), {"other": True}) + self.assertEqual(self.fork_note(head)["status"], "complete") + + def test_the_record_carries_two_fingerprints_and_a_signature(self): + head = self.reviewed_push() + record = self.fork_note(head) + self.assertRegex(record["review_fingerprint"], "^[0-9a-f]{64}$") + self.assertRegex(record["answer_digest"], "^[0-9a-f]{64}$") + listed = subprocess.run( + ["ssh-keygen", "-lf", str(self.key) + ".pub"], + capture_output=True, + text=True, + check=True, + ).stdout + self.assertIn(record["key_fingerprint"], listed) + self.assertTrue(record["signature"].startswith("-----BEGIN SSH SIGNATURE-----")) + + def test_a_record_signed_by_a_key_not_on_the_account_is_refused(self): + self.reviewed_push() + self.as_ci() + elsewhere = self.root / "elsewhere.keys" + elsewhere.write_text(Path(str(self.other_key()) + ".pub").read_text()) + result = self.pr(keys=elsewhere) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn( + "does not verify against any SSH key on GitHub of contributor", + result.stdout, + ) + self.assertIn(self.review_note()["key_fingerprint"], result.stdout) + for step in ("ssh-keygen -t ed25519", "Key type: Signing Key"): + self.assertIn(step, result.stdout) + self.assertIn("codetrial.signingKey", result.stdout) + nobody = self.root / "none.keys" + nobody.write_text("") + result = self.pr(keys=nobody) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("neither contributor nor the commits' authors", result.stdout) + + def test_signing_with_a_key_on_the_account_fixes_the_record(self): + self.git("config", "codetrial.signingKey", str(self.other_key())) + self.reviewed_push() + self.as_ci() + self.assertEqual(self.pr().returncode, 1) + self.git("config", "codetrial.signingKey", str(self.key)) + self.assertEqual(self.push().returncode, 0) + self.as_ci() + result = self.pr() + self.assertEqual(result.returncode, 0, result.stdout) + + def test_a_record_altered_after_signing_is_refused(self): + head = self.reviewed_push() + original = self.review_note() + unsigned = {k: v for k, v in original.items() if k != "signature"} + header, body, *rest = original["signature"].split("\n") + corrupt = "\n".join( + [header, ("B" if body[0] != "B" else "C") + body[1:], *rest] + ) + for record, reason in ( + (unsigned, "is not signed"), + ({**original, "answer_digest": "0" * 64}, "made for different code"), + ({**original, "review_fingerprint": "0" * 64}, "made for different code"), + ({**original, "signature": corrupt}, "does not verify"), + ): + with self.subTest(reason=reason): + self.git( + "notes", + "--ref=codetrial-review", + "add", + "-f", + "-m", + json.dumps(record), + head, + ) + result = self.pr() + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn(reason, result.stdout) + self.assertNotIn("Traceback", result.stderr) + + def test_a_push_without_a_signing_key_is_refused_with_how_to_set_one(self): + self.git("config", "--unset", "codetrial.signingKey") + self.env = {k: v for k, v in self.env.items() if k != "SSH_AUTH_SOCK"} + self.large() + self.prepare() + refused = self.push() + self.assertNotEqual(refused.returncode, 0) + # The three one-time steps, spelled out where the contributor hits it. + for step in ( + "ssh-keygen -t ed25519 -f ~/.ssh/codetrial_signing", + "Key type: Signing Key", + "git config codetrial.signingKey ~/.ssh/codetrial_signing", + ): + self.assertIn(step, refused.stderr) + self.assertIsNone(self.fork_note(self.git("rev-parse", "HEAD").stdout.strip())) + + def owners(self): + # The list counts as the base has it, so it is committed there. + (self.repo / ".github").mkdir(exist_ok=True) + (self.repo / ".github/CODEOWNERS").write_text( + "# @commented-out\n/src/ @Owner @org/team\n/web/ @owner2\n" + ) + self.git("add", "-f", ".github/CODEOWNERS") + self.git("commit", "-qm", "Name the owners", "-m", "Fixture owners.") + self.base = self.git("rev-parse", "HEAD").stdout.strip() + self.git("update-ref", "refs/remotes/upstream/main", self.base) + + def test_a_branch_cannot_name_its_own_author_a_code_owner(self): + self.owners() + (self.repo / ".github/CODEOWNERS").write_text("/src/ @contributor\n") + self.git("add", "-f", ".github/CODEOWNERS") + self.large() + commits = int(self.git("rev-list", "--count", f"{self.base}..HEAD").stdout) + # GitHub lists every commit as the contributor's, so only the owner + # list decides; the branch's own CODEOWNERS must not count. + result = self.pr(login="contributor", authors=["contributor"] * commits) + self.assertEqual(result.returncode, 1) + self.git("config", "codetrial.codeOwner", "contributor") + self.assertNotEqual(self.push().returncode, 0) + + def test_an_unreadable_key_list_is_not_reported_as_missing_setup(self): + self.reviewed_push() + self.as_ci() + empty = self.root / "none.keys" + empty.write_text("") + result = self.pr(keys=empty, unavailable=True) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("could not read the SSH keys of contributor", result.stdout) + self.assertNotIn("ssh-keygen -t ed25519", result.stdout) + # Keys that were read but do not match are still an outage while some + # were not read: the matching one may be among those. + elsewhere = self.root / "elsewhere.keys" + elsewhere.write_text(Path(str(self.other_key()) + ".pub").read_text()) + result = self.pr(keys=elsewhere, unavailable=True) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("could not read the SSH keys of contributor", result.stdout) + self.assertNotIn("ssh-keygen -t ed25519", result.stdout) + + def test_a_record_made_against_a_commit_of_the_branch_is_refused(self): + # Signed honestly, but against the branch's own first commit, so it + # covers only what came after it. + (self.repo / "early.txt").write_text("early\n") + self.git("add", "early.txt") + self.git("commit", "-qm", "Add an early file", "-m", "Fixture commit.") + start, self.base = self.base, self.git("rev-parse", "HEAD").stdout.strip() + self.git("update-ref", "refs/remotes/upstream/main", self.base) + self.reviewed_push() + self.base = start + self.git("update-ref", "refs/remotes/upstream/main", start) + self.as_ci() + result = self.pr() + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("was made against another base", result.stdout) + + def test_an_owner_list_naming_another_commit_withholds_the_exemption(self): + self.owners() + self.large() + listed = self.root / "stale.authors" + listed.write_text(f"{'0' * 40} owner\n") + result = self.gate( + "pr", + *["--head", "HEAD", "--author-login", "owner", "--author-type", "User"], + *["--author-keys", str(self.author_keys), "--commit-authors", str(listed)], + ) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("are not the 1 checked here", result.stdout) + + def test_a_push_replaces_a_different_record_the_fork_holds_for_its_head(self): + head = self.reviewed_push() + subprocess.run( + ["git", "--git-dir", str(self.fork), "-c", "user.name=Other"] + + ["-c", "user.email=other@example.test", "notes"] + + ["--ref=codetrial-review", "add", "-f", "-m", '{"stale": true}', head], + check=True, + capture_output=True, + env=self.env, + ) + self.assertEqual(self.push().returncode, 0) + self.assertEqual(self.fork_note(head)["status"], "complete") + + def test_publish_review_sends_to_the_push_url(self): + # Fetched from one repository and pushed to another, as a clone of + # upstream that pushes to a fork is set up; only the fork has the branch. + head = self.pushed_without_record() + fetched = self.root / "fetched.git" + self.git("init", "-q", "--bare", str(fetched)) + self.git("config", "remote.fork.pushurl", str(self.fork)) + self.git("config", "remote.fork.url", str(fetched)) + result = self.publish("topic") + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.fork_note(head)["status"], "complete") + + def test_a_push_whose_record_cannot_be_sent_is_refused(self): + self.large() + self.prepare() + self.git("remote", "set-url", "fork", str(self.root / "missing.git")) + refused = self.push() + self.assertNotEqual(refused.returncode, 0) + self.assertIn("cannot send the interactive review record", refused.stderr) + + def test_git_ssh_signing_key_is_used_when_none_is_set_for_the_review(self): + self.git("config", "--unset", "codetrial.signingKey") + self.git("config", "user.signingKey", str(self.key)) + self.env = {k: v for k, v in self.env.items() if k != "SSH_AUTH_SOCK"} + self.large() + self.prepare() + # Without gpg.format=ssh, user.signingKey names a GPG key: not used. + self.assertNotEqual(self.push().returncode, 0) + self.git("config", "gpg.format", "ssh") + pushed = self.push() + self.assertEqual(pushed.returncode, 0, pushed.stderr) + head = self.git("rev-parse", "HEAD").stdout.strip() + self.assertTrue(self.fork_note(head)["key_fingerprint"].startswith("SHA256:")) + self.as_ci() + self.assertEqual(self.pr().returncode, 0) + + def test_the_documents_state_what_the_checker_enforces(self): + gate = load_gate("gate_constants") + steps = ( + "ssh-keygen -t ed25519 -f ~/.ssh/codetrial_signing", + "Key type: Signing Key", + "git config codetrial.signingKey ~/.ssh/codetrial_signing", + ) + for step in steps: + self.assertIn(step, gate.SETUP) + documents = { + "CONTRIBUTING.md": ( + f"at least {gate.LARGE_COMMIT} added plus deleted", + f"more than {gate.PR_TOTAL}", + "Key type: Signing Key", + "git config codetrial.codeOwner", + "docs/large-change-explanation.md#troubleshooting", + *steps[::2], + ), + "docs/large-change-explanation.md": ( + f"at least {gate.LARGE_COMMIT} added plus deleted", + f"more than {gate.PR_TOTAL}", + gate.NAMESPACE, + "CODEOWNERS", + gate.PUBLISH, + ), + ".claude/skills/codetrial-contribute/SKILL.md": ( + "large-change-explanation.md#troubleshooting", + ), + "docs/development.md": (gate.NAMESPACE, gate.NOTES), + ".claude/skills/codetrial-review/workbench.md": (gate.NOTES,), + } + for name, phrases in documents.items(): + # Compared with whitespace folded, so rewrapping a paragraph passes. + text = " ".join((ROOT / name).read_text().split()) + for phrase in phrases: + with self.subTest(document=name, phrase=phrase): + self.assertIn(phrase, text) + # The removed label is gone from everything a contributor reads. + for path in [ROOT / "CONTRIBUTING.md", *(ROOT / "docs").glob("*.md")] + [ + *(ROOT / ".claude/skills/codetrial-review").rglob("*.*") + ]: + if path.suffix in (".md", ".html", ".js", ".py"): + with self.subTest(path=path.name): + self.assertNotIn("`explained` label", path.read_text()) + self.assertNotIn("explained", path.read_text()) - def test_a_large_change_passes_only_with_the_maintainers_label(self): + def test_a_code_owner_may_skip_the_review_and_others_may_not(self): + self.owners() + self.large() + for login in ("owner", "OWNER2"): + with self.subTest(login=login): + result = self.pr(login=login, authors=[login]) + self.assertEqual(result.returncode, 0, result.stdout) + self.assertIn("every commit's author are code owners", result.stdout) + self.assertIn("::notice title=Large change::", result.stdout) + for login in ("contributor", "commented-out", "org", "team", "own"): + with self.subTest(login=login): + self.assertEqual(self.pr(login=login, authors=[login]).returncode, 1) + + def test_a_code_owner_opening_someone_elses_work_does_not_excuse_it(self): + self.owners() + self.large() + for authors, said in ( + (["contributor"], "commits here are by contributor"), + ([""], "an author linked to no GitHub account"), + (None, "GitHub did not list the commits' authors"), + (["owner", "owner"], "the 2 commits GitHub listed are not the 1"), + ([], "the 0 commits GitHub listed are not the 1"), + ): + with self.subTest(authors=authors): + result = self.pr(login="owner", authors=authors) + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn( + "::error title=Interactive review missing::", result.stdout + ) + self.assertIn("owner is a code owner, but", result.stdout) + self.assertIn(said, result.stdout) + # Every commit by one code owner or another, in any case, is exempt. + self.assertEqual(self.pr(login="owner", authors=["OWNER2"]).returncode, 0) + # The review itself still lets it through. + self.prepare() + pushed = self.push() + self.assertEqual(pushed.returncode, 0, pushed.stderr) + self.as_ci() + result = self.pr(login="owner", authors=["contributor"]) + self.assertEqual(result.returncode, 0, result.stdout) + self.assertIn("recorded at push", result.stdout) + + def test_a_code_owner_pushes_without_the_review_after_saying_who_they_are(self): + self.owners() self.large() - for names in ((), ("bug",), ("not-explained",)): - refused = self.labelled(*names) - self.assertNotEqual(refused.returncode, 0, names) - self.assertIn("[interactive-review/required]", refused.stderr) - self.assertEqual(self.labelled("bug", "explained").returncode, 0) - malformed = self.gate("pr", "--labels", '{"explained": true}') - self.assertNotEqual(malformed.returncode, 0) - self.assertNotIn("Traceback", malformed.stderr) - - def test_the_label_does_not_replace_the_local_review_at_push(self): + refused = self.push() + self.assertNotEqual(refused.returncode, 0) + self.git("config", "codetrial.codeOwner", "someone-else") + self.assertNotEqual(self.push().returncode, 0) + self.git("config", "codetrial.codeOwner", "owner") + pushed = self.push() + self.assertEqual(pushed.returncode, 0, pushed.stderr) + self.assertIn("code owner", pushed.stdout + pushed.stderr) + # Said at the moment it could mislead: the pull request judges accounts. + self.assertIn("does not read this setting", pushed.stdout + pushed.stderr) + head = self.git("rev-parse", "HEAD").stdout.strip() + self.assertIsNone(self.fork_note(head)) + + def test_a_code_owner_who_opts_in_still_gets_a_signed_record(self): + self.owners() + self.git("config", "codetrial.codeOwner", "owner") + head = self.reviewed_push() + self.assertEqual(self.fork_note(head)["status"], "complete") + self.assertIn("signature", self.fork_note(head)) + + def test_a_record_that_does_not_match_the_change_is_refused(self): + head = self.reviewed_push() + original = self.review_note() + for field, value in ( + ("status", "pending"), + ("diff_hash", "0" * 64), + ("head", self.base), + ("base", "f" * 40), + ): + with self.subTest(field=field): + self.git( + "notes", + "--ref=codetrial-review", + "add", + "-f", + "-m", + json.dumps({**original, field: value}), + head, + ) + result = self.pr() + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn( + "::error title=Interactive review missing::", result.stdout + ) + self.assertNotIn("Traceback", result.stderr) + self.git("notes", "--ref=codetrial-review", "add", "-f", "-m", "not json", head) + self.assertEqual(self.pr().returncode, 1) + + def test_updating_the_branch_from_main_keeps_the_record(self): + head = self.reviewed_push() + # Main moves on, and GitHub's "Update branch" merges it in. + self.git("checkout", "-q", "--detach", self.base) + (self.repo / "other.txt").write_text("main moved\n") + self.git("add", "other.txt") + self.git("commit", "-qm", "Move main", "-m", "Advance the fixture main.") + main = self.git("rev-parse", "HEAD").stdout.strip() + self.git("checkout", "-q", "--detach", head) + self.git("merge", "-q", "--no-ff", "--no-edit", main) + merged = self.git("rev-parse", "HEAD").stdout.strip() + self.as_ci() + result = self.pr(base=main, head=merged) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + # A merge that also changes something of its own is not just an update. + (self.repo / "change.txt").write_text("edited in the merge\n") + self.git("add", "change.txt") + self.git("commit", "-q", "--amend", "--no-edit") + amended = self.git("rev-parse", "HEAD").stdout.strip() + self.assertEqual(self.pr(base=main, head=amended).returncode, 1) + # Nor is a merge of something that is not on main. + self.git("checkout", "-q", "--detach", head) + (self.repo / "side.txt").write_text("side\n") + self.git("add", "side.txt") + self.git("commit", "-qm", "Side work", "-m", "Not on main.") + side = self.git("rev-parse", "HEAD").stdout.strip() + self.git("checkout", "-q", "--detach", head) + self.git("merge", "-q", "--no-ff", "--no-edit", side) + self.assertEqual(self.pr(base=main).returncode, 1) + + def test_the_notice_carries_no_text_from_the_branch(self): + # The workflow prints the result as a workflow command, so a path that + # held one would be read as a second command. + name = "notes-%0A::error::forged.bin" + (self.repo / name).write_bytes(b"\0binary\0" * 10) + self.git("add", "--", name) + self.large(1) + for result in (self.pr(), self.gate("pr")): + self.assertIn("cannot be counted as text", result.stdout) + self.assertNotIn("forged", result.stdout) + self.assertEqual(len(result.stdout.strip().splitlines()), 1) + + def test_only_a_reviewed_push_produces_the_record(self): self.large() - self.assertEqual(self.labelled("explained").returncode, 0) + self.assertEqual(self.pr().returncode, 1) refused = self.push() self.assertNotEqual(refused.returncode, 0) self.assertIn("Interactive review required", refused.stderr) + self.assertIsNone(self.fork_note(self.git("rev-parse", "HEAD").stdout.strip())) self.prepare() self.assertEqual(self.push().returncode, 0) + self.as_ci() + self.assertEqual(self.pr().returncode, 0) def test_a_push_writes_no_description_block(self): self.large(10) @@ -372,6 +1033,13 @@ def test_pr_501_triggers_with_all_commits_below_200(self): self.assertNotEqual(result.returncode, 0) self.assertIn("0 commits >=200 lines; PR diff 501", result.stderr) + def test_exactly_500_with_small_commits_needs_no_record(self): + for count in [167, 334, 500]: + self.large(count) + result = self.pr() + self.assertEqual(result.returncode, 0, result.stdout) + self.assertIn("under the interactive review thresholds", result.stdout) + def test_plain_reviewed_marker_cannot_bypass_the_large_gate(self): self.large() marker = self.repo / ".git/codetrial-review/reviewed" @@ -445,11 +1113,9 @@ def test_grafts_and_shallow_history_do_not_report_small_changes(self): path.unlink() def review_status(self): - return json.loads( - self.run_command( - [sys.executable, str(GATE), "status", "--base", self.base] - ).stdout - ) + result = self.gate("status") + self.assertEqual(result.returncode, 0, result.stderr) + return json.loads(result.stdout) def review_note(self, head="HEAD"): return json.loads( @@ -655,10 +1321,10 @@ def test_real_commit_hooks_and_large_review_gate_work_together(self): self.assertEqual(self.push().returncode, 0) self.assertEqual(self.review_note()["change_id"], change_id) self.assertEqual(self.git("rev-parse", "HEAD").stdout.strip(), head) - remote = self.root / "remote.git" - self.git("init", "--bare", "-q", str(remote)) - self.git("remote", "add", "fork", str(remote)) + # Through the installed hook: the record goes with the branch, and the + # hook's own push of the notes ref does not loop back into the checks. self.git("push", "-q", "fork", "HEAD:refs/heads/topic") + self.assertEqual(self.fork_note(head)["status"], "complete") class QueueReviewTests(unittest.TestCase): @@ -681,8 +1347,6 @@ def setUp(self): self.git("config", "user.email", "queue@example.test") self.git("config", "commit.gpgsign", "false") self.base = self.commit("base") - self.first = self.commit("first") - self.second = self.commit("second") # Bind the subprocess cwd without changing the test runner's process. self.queue.git = self.git @@ -701,190 +1365,6 @@ def commit(self, value): self.git("commit", "-qm", value) return self.git("rev-parse", "HEAD") - def pull(self, number, tip, labels=("explained",)): - return { - "number": number, - "head": {"sha": tip}, - "user": {"login": "contributor", "type": "User"}, - "labels": [{"name": name} for name in labels], - } - - def checker(self): - calls = [] - - def check_labels(base, tip, labels): - calls.append((base, tip, labels)) - if "explained" not in labels: - raise ValueError("unlabeled") - - return ( - SimpleNamespace( - check_labels=check_labels, - dependabot_exempt=lambda *a: a[2] == "dependabot[bot]", - ), - calls, - ) - - def test_every_included_head_is_checked_separately(self): - checker, calls = self.checker() - result = self.queue.check( - self.base, - self.second, - [ - self.pull(1, self.first), - self.pull(2, self.second), - self.pull(3, self.base), - ], - checker, - ) - self.assertEqual(result, [1, 2]) - self.assertEqual( - calls, - [ - (self.base, self.first, ["explained"]), - (self.base, self.second, ["explained"]), - ], - ) - - def test_a_missing_label_in_any_pr_fails_the_group(self): - checker, _ = self.checker() - with self.assertRaisesRegex(ValueError, "unlabeled"): - self.queue.check( - self.base, - self.second, - [ - self.pull(1, self.first, ()), - self.pull(2, self.second), - ], - checker, - ) - - def test_missing_or_moved_pr_head_fails_closed(self): - checker, _ = self.checker() - for pulls in ([], [self.pull(1, self.first)], [self.pull(1, "0" * 40)]): - with self.subTest(pulls=pulls): - with self.assertRaisesRegex(ValueError, "matching"): - self.queue.check(self.base, self.second, pulls, checker) - - def test_a_group_that_does_not_descend_from_its_base_is_refused(self): - checker, _ = self.checker() - self.git("checkout", "-q", "--orphan", "unrelated") - self.git("commit", "-q", "--allow-empty", "-m", "unrelated") - stray = self.git("rev-parse", "HEAD") - with self.assertRaises(subprocess.CalledProcessError): - self.queue.check(self.base, stray, [self.pull(1, stray)], checker) - - def test_merge_commit_is_not_treated_as_a_pr_commit(self): - checker, calls = self.checker() - self.git("checkout", "-qb", "queue", self.base) - self.git("merge", "--no-ff", "--no-edit", self.second) - merged = self.git("rev-parse", "HEAD") - self.assertEqual( - self.queue.check( - self.base, - merged, - [ - self.pull(2, self.second), - ], - checker, - ), - [2], - ) - self.assertEqual(len(calls), 1) - - def run_queue(self, labels=("explained",), head=None, api_failure=False): - head = head or self.second - binary = self.repo / "bin" - binary.mkdir(exist_ok=True) - stub = binary / "gh" - stub.write_text( - '#!/bin/sh\nif [ "$QUEUE_API_FAILURE" = 1 ]; then exit 1; fi\n' - 'printf "%s" "$QUEUE_API_PAGES"\n' - ) - stub.chmod(0o755) - return subprocess.run( - [ - sys.executable, - str(ROOT / "scripts/check-queue-review.py"), - "--base", - self.base, - "--head", - head, - "--branch", - "main", - "--repo", - "owner/repo", - ], - cwd=self.repo, - text=True, - capture_output=True, - env={ - **self.env, - "PATH": str(binary) + os.pathsep + self.env["PATH"], - "QUEUE_API_FAILURE": "1" if api_failure else "0", - "QUEUE_API_PAGES": json.dumps([[], [self.pull(2, head, labels)]]), - }, - ) - - def big(self): - (self.repo / "file").write_text("line\n" * 600) - self.git("add", "file") - self.git("commit", "-qm", "A change over the threshold") - return self.git("rev-parse", "HEAD") - - def test_cli_checks_the_real_labels_from_later_api_page(self): - result = self.run_queue() - self.assertEqual(result.returncode, 0, result.stderr) - self.assertIn("[2]", result.stdout) - - def test_cli_rejects_a_large_change_without_the_label(self): - head = self.big() - refused = self.run_queue(labels=(), head=head) - self.assertNotEqual(refused.returncode, 0) - self.assertIn("interactive-review/required", refused.stderr) - accepted = self.run_queue(labels=("other", "explained"), head=head) - self.assertEqual(accepted.returncode, 0, accepted.stderr) - - def test_cli_does_not_pass_when_github_cannot_be_read(self): - result = self.run_queue(api_failure=True) - self.assertNotEqual(result.returncode, 0) - self.assertNotIn("Verified", result.stdout) - - def test_group_with_independent_pr_branches_checks_both(self): - checker, calls = self.checker() - self.git("checkout", "-qb", "other", self.base) - (self.repo / "other").write_text("independent change") - self.git("add", "other") - self.git("commit", "-qm", "other branch") - other = self.git("rev-parse", "HEAD") - self.git("checkout", "-qb", "queue", self.base) - self.git("merge", "--no-ff", "--no-edit", self.first) - self.git("merge", "--no-ff", "--no-edit", other) - merged = self.git("rev-parse", "HEAD") - self.assertEqual( - self.queue.check( - self.base, - merged, - [ - self.pull(1, self.first), - self.pull(2, other), - ], - checker, - ), - [1, 2], - ) - self.assertEqual([call[1] for call in calls], [self.first, other]) - - def test_dependabot_pull_request_skips_only_its_own_label_check(self): - checker, calls = self.checker() - bot = self.pull(1, self.first, ()) - bot["user"] = {"login": "dependabot[bot]", "type": "Bot"} - result = self.queue.check( - self.base, self.second, [bot, self.pull(2, self.second)], checker - ) - self.assertEqual(result, [1, 2]) - self.assertEqual([call[1] for call in calls], [self.second]) - def commit_by(self, author, parent, text, *more): tree = self.git("rev-parse", f"{parent}^{{tree}}") parents = [arg for p in (parent, *more) for arg in ("-p", p)] @@ -899,7 +1379,7 @@ def commit_by(self, author, parent, text, *more): ).stdout.strip() def test_dependabot_exemption_needs_the_opener_and_every_commit(self): - gate = self.queue_checker() + gate = self.checker() bot = "49699333+dependabot[bot]@users.noreply.github.com" mine = self.commit_by(bot, self.base, "Bump a") mixed = self.commit_by("person@example.test", mine, "Add a change") @@ -923,10 +1403,8 @@ def test_dependabot_exemption_needs_the_opener_and_every_commit(self): gate.dependabot_exempt(self.base, tip, login, kind), exempt ) - def queue_checker(self): - spec = importlib.util.spec_from_file_location("gate_under_test", GATE) - gate = importlib.util.module_from_spec(spec) - spec.loader.exec_module(gate) + def checker(self): + gate = load_gate() gate.git = lambda *args, data=None: self.git(*args).encode() gate.module(ROOT / "scripts/hashchain.py").git = lambda *a, **k: ( subprocess.run( @@ -1043,7 +1521,6 @@ def run(head, *extra): start, "--head", head, - "--guard", *extra, ], cwd=self.repo, @@ -1071,7 +1548,7 @@ def test_pr_excuses_only_a_dependabot_pull_request(self): def run(head, login, kind): return subprocess.run( [sys.executable, str(GATE), "pr", "--base", self.base] - + ["--head", head, "--labels", "[]"] + + ["--head", head] + ["--author-login", login, "--author-type", kind], cwd=self.repo, env=self.env, @@ -1088,15 +1565,15 @@ def run(head, login, kind): (theirs, "contributor", "User"), ): with self.subTest(login=login, kind=kind, human=head == mine): - # A small change needs no label, so only the message tells - # whether the exemption applied. + # A small change gets no notice either way, so only the + # message tells whether the exemption applied. result = run(head, login, kind) self.assertEqual(result.returncode, 0, result.stderr) self.assertNotIn("Dependabot's own", result.stdout) def test_the_workflow_passes_only_flags_and_variables_that_exist(self): text = (ROOT / ".github/workflows/pr-description.yml").read_text() - step = text[text.index("- name: Measure the change") :] + step = support.step(text, "Measure the change") env, run = step.split(" run: |", 1) declared = set(re.findall(r"^ (\w+):", env, re.M)) for name in set(re.findall(r"\$\{?([A-Z_]+)", run)): @@ -1110,6 +1587,38 @@ def test_the_workflow_passes_only_flags_and_variables_that_exist(self): self.assertTrue(calls, script) for flag in set(re.findall(r" (--[a-z-]+)", "".join(calls))): self.assertIn(f'"{flag}"', source.read_text(), (script, flag)) - # The guard runs on pull requests, and the branch name loses refs/heads/. - self.assertRegex(run, r"check-queue-review\.py[^\n]*\\\n[^\n]*--guard") - self.assertIn('--branch "${BASE_REF#refs/heads/}"', run) + # The guard runs on pull requests, with the head a maintainer approved. + self.assertRegex(run, r"check-queue-review\.py[^\n]*\\\n[^\n]*--approved") + # actions/checkout refuses a fork's head under pull_request_target, and + # the head is data here anyway: only the base is checked out, and the + # pull request's commits are fetched by identity into a bare repository. + self.assertEqual(text.count("uses: actions/checkout"), 1) + self.assertNotIn("head.sha", text[: text.index("- name: Measure the change")]) + self.assertIn("git init -q --bare head.git", run) + self.assertIn('"$BASE_SHA" "$HEAD_SHA"', run) + self.assertNotIn("labels", text.lower()) + # The review record comes from the repository the branch came from, by + # its validated name, and only after the change itself was fetched. + self.assertIn( + "HEAD_REPOSITORY: ${{ github.event.pull_request.head.repo.full_name }}", + text, + ) + self.assertIn('"+refs/notes/codetrial-review:refs/notes/codetrial-review"', run) + self.assertLess(run.index('"$BASE_SHA" "$HEAD_SHA"'), run.index("refs/notes/")) + # The signature is checked against the keys GitHub lists for the + # author, fetched by a validated login without a token. + self.assertIn('"https://github.com/$login.keys"', run) + self.assertIn("for login in $opener $others; do", run) + # Signing keys too, so a dedicated key leaves an HTTPS push untouched. + self.assertIn('"users/$login/ssh_signing_keys"', run) + self.assertIn('--author-keys "$keys"', run) + # A code owner's exemption needs every commit's author from GitHub. + self.assertIn('"repos/$REPOSITORY/pulls/$PR_NUMBER/commits"', run) + self.assertIn("listed=$authors", run) + self.assertIn('${listed:+--commit-authors "$listed"}', run) + # The workflow and the checker accept the same logins. + valid = re.search(r"valid='([^']+)'", run).group(1) + self.assertEqual(valid, load_gate().LOGIN.pattern) + self.assertIn('"--commit-authors"', GATE.read_text()) + # A queue head has no record of its own; each pull request had to pass. + self.assertLess(run.index('"$EVENT" = merge_group'), run.index("git init"))