Alternate more flexible code owners mechanism, soon to avoid mass pings (#336261)

.github/CODEOWNERS

@@ -15,6 +15,8 @@
 /.github/workflows @NixOS/Security @Mic92 @zowoq
 /.github/workflows/check-nix-format.yml @infinisil
 /.github/workflows/nixpkgs-vet.yml @infinisil @philiptaron
+/.github/workflows/codeowners.yml @infinisil
+/.github/OWNERS @infinisil
 /ci @infinisil @philiptaron @NixOS/Security
 
 # Development support

.github/OWNERS

@@ -0,0 +1,19 @@
+#
+# Currently unused! Use CODEOWNERS for now, see workflows/codeowners.yml
+#
+####################
+#
+# This file is used to describe who owns what in this repository.
+# Users/teams will get review requests for PRs that change their files.
+#
+# This file does not replace `meta.maintainers`
+# but is instead used for other things than derivations and modules,
+# like documentation, package sets, and other assets.
+#
+# This file uses the same syntax as the natively supported CODEOWNERS file,
+# see https://help.github.com/articles/about-codeowners/ for documentation.
+# However it comes with some notable differences:
+# - There is no need for user/team listed here to have write access.
+# - No reviews will be requested for PRs that target the wrong base branch.
+#
+# Processing of this file is implemented in workflows/codeowners.yml

.github/workflows/codeowners.yml

@@ -0,0 +1,88 @@
+name: Codeowners
+
+# This workflow depends on a GitHub App with the following permissions:
+# - Repository > Administration: read-only
+# - Organization > Members: read-only
+# - Repository > Pull Requests: read-write
+# The App needs to be installed on this repository
+# the OWNER_APP_ID repository variable needs to be set
+# the OWNER_APP_PRIVATE_KEY repository secret needs to be set
+
+on:
+  pull_request_target:
+    types: [opened, ready_for_review, synchronize, reopened, edited]
+
+env:
+  # TODO: Once confirmed that this works by seeing that the action would request
+  # reviews from the same people (or refuse for wrong base branches),
+  # move all entries from CODEOWNERS to OWNERS and change this value here
+  # OWNERS_FILE: .github/OWNERS
+  OWNERS_FILE: .github/CODEOWNERS
+  # Also remove this
+  DRY_MODE: 1
+
+jobs:
+  # Check that code owners is valid
+  check:
+    name: Check
+    runs-on: ubuntu-latest
+    steps:
+    - uses: cachix/install-nix-action@08dcb3a5e62fa31e2da3d490afc4176ef55ecd72 # v30
+
+    # Important: Because we use pull_request_target, this checks out the base branch of the PR, not the PR itself.
+    # We later build and run code from the base branch with access to secrets,
+    # so it's important this is not the PRs code.
+    - uses: actions/checkout@d632683dd7b4114ad314bca15554477dd762a938 # v4.2.0
+      with:
+        path: base
+
+    - name: Build codeowners validator
+      run: nix-build base/ci -A codeownersValidator
+
+    - uses: actions/create-github-app-token@5d869da34e18e7287c1daad50e0b8ea0f506ce69 # v1.11.0
+      id: app-token
+      with:
+        app-id: ${{ vars.OWNER_APP_ID }}
+        private-key: ${{ secrets.OWNER_APP_PRIVATE_KEY }}
+
+    - uses: actions/checkout@d632683dd7b4114ad314bca15554477dd762a938 # v4.2.0
+      with:
+        ref: refs/pull/${{ github.event.number }}/merge
+        path: pr
+
+    - name: Validate codeowners
+      run: result/bin/codeowners-validator
+      env:
+        OWNERS_FILE: pr/${{ env.OWNERS_FILE }}
+        GITHUB_ACCESS_TOKEN: ${{ steps.app-token.outputs.token }}
+        REPOSITORY_PATH: pr
+        OWNER_CHECKER_REPOSITORY: ${{ github.repository }}
+        # Set this to "notowned,avoid-shadowing" to check that all files are owned by somebody
+        EXPERIMENTAL_CHECKS: "avoid-shadowing"
+
+  # Request reviews from code owners
+  request:
+    name: Request
+    runs-on: ubuntu-latest
+    steps:
+    - uses: cachix/install-nix-action@08dcb3a5e62fa31e2da3d490afc4176ef55ecd72 # v30
+
+    # Important: Because we use pull_request_target, this checks out the base branch of the PR, not the PR head.
+    # This is intentional, because we need to request the review of owners as declared in the base branch.
+    - uses: actions/checkout@d632683dd7b4114ad314bca15554477dd762a938 # v4.2.0
+
+    - uses: actions/create-github-app-token@5d869da34e18e7287c1daad50e0b8ea0f506ce69 # v1.11.0
+      id: app-token
+      with:
+        app-id: ${{ vars.OWNER_APP_ID }}
+        private-key: ${{ secrets.OWNER_APP_PRIVATE_KEY }}
+
+    - name: Build review request package
+      run: nix-build ci -A requestReviews
+
+    - name: Request reviews
+      run: result/bin/request-reviews.sh ${{ github.repository }} ${{ github.event.number }} "$OWNERS_FILE"
+      env:
+        GH_TOKEN: ${{ steps.app-token.outputs.token }}
+        # Don't do anything on draft PRs
+        DRY_MODE: ${{ github.event.pull_request.draft && '1' || '' }}

ci/codeowners-validator/default.nix

@@ -0,0 +1,31 @@
+{
+  buildGoModule,
+  fetchFromGitHub,
+  fetchpatch,
+}:
+buildGoModule {
+  name = "codeowners-validator";
+  src = fetchFromGitHub {
+    owner = "mszostok";
+    repo = "codeowners-validator";
+    rev = "f3651e3810802a37bd965e6a9a7210728179d076";
+    hash = "sha256-5aSmmRTsOuPcVLWfDF6EBz+6+/Qpbj66udAmi1CLmWQ=";
+  };
+  patches = [
+    # https://github.com/mszostok/codeowners-validator/pull/222
+    (fetchpatch {
+      name = "user-write-access-check";
+      url = "https://github.com/mszostok/codeowners-validator/compare/f3651e3810802a37bd965e6a9a7210728179d076...840eeb88b4da92bda3e13c838f67f6540b9e8529.patch";
+      hash = "sha256-t3Dtt8SP9nbO3gBrM0nRE7+G6N/ZIaczDyVHYAG/6mU=";
+    })
+    # Undoes part of the above PR: We don't want to require write access
+    # to the repository, that's only needed for GitHub's native CODEOWNERS.
+    # Furthermore, it removes an unneccessary check from the code
+    # that breaks tokens generated for GitHub Apps.
+    ./permissions.patch
+    # Allows setting a custom CODEOWNERS path using the OWNERS_FILE env var
+    ./owners-file-name.patch
+  ];
+  postPatch = "rm -r docs/investigation";
+  vendorHash = "sha256-R+pW3xcfpkTRqfS2ETVOwG8PZr0iH5ewroiF7u8hcYI=";
+}

ci/codeowners-validator/owners-file-name.patch

@@ -0,0 +1,15 @@
+diff --git a/pkg/codeowners/owners.go b/pkg/codeowners/owners.go
+index 6910bd2..e0c95e9 100644
+--- a/pkg/codeowners/owners.go
++++ b/pkg/codeowners/owners.go
+@@ -39,6 +39,10 @@ func NewFromPath(repoPath string) ([]Entry, error) {
+ // openCodeownersFile finds a CODEOWNERS file and returns content.
+ // see: https://help.github.com/articles/about-code-owners/#codeowners-file-location
+ func openCodeownersFile(dir string) (io.Reader, error) {
++	if file, ok := os.LookupEnv("OWNERS_FILE"); ok {
++		return fs.Open(file)
++	}
++
+ 	var detectedFiles []string
+ 	for _, p := range []string{".", "docs", ".github"} {
+ 		pth := path.Join(dir, p)

ci/codeowners-validator/permissions.patch

@@ -0,0 +1,36 @@
+diff --git a/internal/check/valid_owner.go b/internal/check/valid_owner.go
+index a264bcc..610eda8 100644
+--- a/internal/check/valid_owner.go
++++ b/internal/check/valid_owner.go
+@@ -16,7 +16,6 @@ import (
+ const scopeHeader = "X-OAuth-Scopes"
+ 
+ var reqScopes = map[github.Scope]struct{}{
+-	github.ScopeReadOrg: {},
+ }
+ 
+ type ValidOwnerConfig struct {
+@@ -223,10 +222,7 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr
+ 	for _, t := range v.repoTeams {
+ 		// GitHub normalizes name before comparison
+ 		if strings.EqualFold(t.GetSlug(), team) {
+-			if t.Permissions["push"] {
+-				return nil
+-			}
+-			return newValidateError("Team %q cannot review PRs on %q as neither it nor any parent team has write permissions.", team, v.orgRepoName)
++			return nil
+ 		}
+ 	}
+ 
+@@ -245,10 +241,7 @@ func (v *ValidOwner) validateGitHubUser(ctx context.Context, name string) *valid
+ 	for _, u := range v.repoUsers {
+ 		// GitHub normalizes name before comparison
+ 		if strings.EqualFold(u.GetLogin(), userName) {
+-			if u.Permissions["push"] {
+-				return nil
+-			}
+-			return newValidateError("User %q cannot review PRs on %q as they don't have write permissions.", userName, v.orgRepoName)
++			return nil
+ 		}
+ 	}
+ 

ci/default.nix

@@ -0,0 +1,29 @@
+let
+  pinnedNixpkgs = builtins.fromJSON (builtins.readFile ./pinned-nixpkgs.json);
+in
+{
+  system ? builtins.currentSystem,
+
+  nixpkgs ? null,
+}:
+let
+  nixpkgs' =
+    if nixpkgs == null then
+      fetchTarball {
+        url = "https://github.com/NixOS/nixpkgs/archive/${pinnedNixpkgs.rev}.tar.gz";
+        sha256 = pinnedNixpkgs.sha256;
+      }
+    else
+      nixpkgs;
+
+  pkgs = import nixpkgs' {
+    inherit system;
+    config = { };
+    overlays = [ ];
+  };
+in
+{
+  inherit pkgs;
+  requestReviews = pkgs.callPackage ./request-reviews { };
+  codeownersValidator = pkgs.callPackage ./codeowners-validator { };
+}

ci/request-reviews/default.nix

@@ -0,0 +1,43 @@
+{
+  lib,
+  stdenvNoCC,
+  makeWrapper,
+  coreutils,
+  codeowners,
+  jq,
+  curl,
+  github-cli,
+  gitMinimal,
+}:
+stdenvNoCC.mkDerivation {
+  name = "request-reviews";
+  src = lib.fileset.toSource {
+    root = ./.;
+    fileset = lib.fileset.unions [
+      ./get-reviewers.sh
+      ./request-reviews.sh
+      ./verify-base-branch.sh
+      ./dev-branches.txt
+    ];
+  };
+  nativeBuildInputs = [ makeWrapper ];
+  dontBuild = true;
+  installPhase = ''
+    mkdir -p $out/bin
+    mv dev-branches.txt $out/bin
+    for bin in *.sh; do
+      mv "$bin" "$out/bin"
+      wrapProgram "$out/bin/$bin" \
+        --set PATH ${
+          lib.makeBinPath [
+            coreutils
+            codeowners
+            jq
+            curl
+            github-cli
+            gitMinimal
+          ]
+        }
+    done
+  '';
+}

ci/request-reviews/dev-branches.txt

@@ -0,0 +1,7 @@
+# Trusted development branches:
+# These generally require PRs to update and are built by Hydra.
+master
+staging
+release-*
+staging-*
+haskell-updates

ci/request-reviews/get-reviewers.sh

@@ -0,0 +1,87 @@
+#!/usr/bin/env bash
+
+# Get the code owners of the files changed by a PR,
+# suitable to be consumed by the API endpoint to request reviews:
+# https://docs.github.com/en/rest/pulls/review-requests?apiVersion=2022-11-28#request-reviewers-for-a-pull-request
+
+set -euo pipefail
+
+log() {
+    echo "$@" >&2
+}
+
+if (( "$#" < 5 )); then
+    log "Usage: $0 GIT_REPO BASE_REF HEAD_REF OWNERS_FILE PR_AUTHOR"
+    exit 1
+fi
+
+gitRepo=$1
+baseRef=$2
+headRef=$3
+ownersFile=$4
+prAuthor=$5
+
+tmp=$(mktemp -d)
+trap 'rm -rf "$tmp"' exit
+
+git -C "$gitRepo" diff --name-only --merge-base "$baseRef" "$headRef" > "$tmp/touched-files"
+readarray -t touchedFiles < "$tmp/touched-files"
+log "This PR touches ${#touchedFiles[@]} files"
+
+# Get the owners file from the base, because we don't want to allow PRs to
+# remove code owners to avoid pinging them
+git -C "$gitRepo" show "$baseRef":"$ownersFile" > "$tmp"/codeowners
+
+# Associative arrays with the team/user as the key for easy deduplication
+declare -A teams users
+
+for file in "${touchedFiles[@]}"; do
+    result=$(codeowners --file "$tmp"/codeowners "$file")
+
+    read -r file owners <<< "$result"
+    if [[ "$owners" == "(unowned)" ]]; then
+        log "File $file is unowned"
+        continue
+    fi
+    log "File $file is owned by $owners"
+
+    # Split up multiple owners, separated by arbitrary amounts of spaces
+    IFS=" " read -r -a entries <<< "$owners"
+
+    for entry in "${entries[@]}"; do
+        # GitHub technically also supports Emails as code owners,
+        # but we can't easily support that, so let's not
+        if [[ ! "$entry" =~ @(.*) ]]; then
+            warn -e "\e[33mCodeowner \"$entry\" for file $file is not valid: Must start with \"@\"\e[0m" >&2
+            # Don't fail, because the PR for which this script runs can't fix it,
+            # it has to be fixed in the base branch
+            continue
+        fi
+        # The first regex match is everything after the @
+        entry=${BASH_REMATCH[1]}
+        if [[ "$entry" =~ .*/(.*) ]]; then
+            # Teams look like $org/$team, where we only need $team for the API
+            # call to request reviews from teams
+            teams[${BASH_REMATCH[1]}]=
+        else
+            # Everything else is a user
+            users[$entry]=
+        fi
+    done
+
+done
+
+# Cannot request a review from the author
+if [[ -v users[$prAuthor] ]]; then
+    log "One or more files are owned by the PR author, ignoring"
+    unset 'users[$prAuthor]'
+fi
+
+# Turn it into a JSON for the GitHub API call to request PR reviewers
+jq -n \
+    --arg users "${!users[*]}" \
+    --arg teams "${!teams[*]}" \
+    '{
+      reviewers: $users | split(" "),
+      team_reviewers: $teams | split(" ")
+    }'

ci/request-reviews/request-reviews.sh

@@ -0,0 +1,97 @@
+#!/usr/bin/env bash
+
+# Requests reviews for a PR after verifying that the base branch is correct
+
+set -euo pipefail
+tmp=$(mktemp -d)
+trap 'rm -rf "$tmp"' exit
+SCRIPT_DIR=$(dirname "$0")
+
+log() {
+    echo "$@" >&2
+}
+
+effect() {
+    if [[ -n "${DRY_MODE:-}" ]]; then
+        log "Skipping in dry mode:" "${@@Q}"
+    else
+        "$@"
+    fi
+}
+
+if (( $# < 3 )); then
+    log "Usage: $0 GITHUB_REPO PR_NUMBER OWNERS_FILE"
+    exit 1
+fi
+baseRepo=$1
+prNumber=$2
+ownersFile=$3
+
+log "Fetching PR info"
+prInfo=$(gh api \
+  -H "Accept: application/vnd.github+json" \
+  -H "X-GitHub-Api-Version: 2022-11-28" \
+  "/repos/$baseRepo/pulls/$prNumber")
+
+baseBranch=$(jq -r .base.ref <<< "$prInfo")
+log "Base branch: $baseBranch"
+prRepo=$(jq -r .head.repo.full_name <<< "$prInfo")
+log "PR repo: $prRepo"
+prBranch=$(jq -r .head.ref <<< "$prInfo")
+log "PR branch: $prBranch"
+prAuthor=$(jq -r .user.login <<< "$prInfo")
+log "PR author: $prAuthor"
+
+extraArgs=()
+if pwdRepo=$(git rev-parse --show-toplevel 2>/dev/null); then
+    # Speedup for local runs
+    extraArgs+=(--reference-if-able "$pwdRepo")
+fi
+
+log "Fetching Nixpkgs commit history"
+# We only need the commit history, not the contents, so we can do a tree-less clone using tree:0
+# https://github.blog/open-source/git/get-up-to-speed-with-partial-clone-and-shallow-clone/#user-content-quick-summary
+git clone --bare --filter=tree:0 --no-tags --origin upstream "${extraArgs[@]}" https://github.com/"$baseRepo".git "$tmp"/nixpkgs.git
+
+log "Fetching the PR commit history"
+# Fetch the PR
+git -C "$tmp/nixpkgs.git" remote add fork https://github.com/"$prRepo".git
+# This remote config is the same as --filter=tree:0 when cloning
+git -C "$tmp/nixpkgs.git" config remote.fork.partialclonefilter tree:0
+git -C "$tmp/nixpkgs.git" config remote.fork.promisor true
+
+# This should not conflict with any refs in Nixpkgs
+headRef=refs/remotes/fork/pr
+# Only fetch into a remote ref, because the local ref namespace is used by Nixpkgs, don't want any conflicts
+git -C "$tmp/nixpkgs.git" fetch --no-tags fork "$prBranch":"$headRef"
+
+log "Checking correctness of the base branch"
+if ! "$SCRIPT_DIR"/verify-base-branch.sh "$tmp/nixpkgs.git" "$headRef" "$baseRepo" "$baseBranch" "$prRepo" "$prBranch" | tee "$tmp/invalid-base-error" >&2; then
+    log "Posting error as comment"
+    if ! response=$(effect gh api \
+        --method POST \
+        -H "Accept: application/vnd.github+json" \
+        -H "X-GitHub-Api-Version: 2022-11-28" \
+        "/repos/$baseRepo/issues/$prNumber/comments" \
+        -F "body=@$tmp/invalid-base-error"); then
+        log "Failed to post the comment: $response"
+    fi
+    exit 1
+fi
+
+log "Getting code owners to request reviews from"
+"$SCRIPT_DIR"/get-reviewers.sh "$tmp/nixpkgs.git" "$baseBranch" "$headRef" "$ownersFile" "$prAuthor" > "$tmp/reviewers.json"
+
+log "Requesting reviews from: $(<"$tmp/reviewers.json")"
+
+if ! response=$(effect gh api \
+    --method POST \
+    -H "Accept: application/vnd.github+json" \
+    -H "X-GitHub-Api-Version: 2022-11-28" \
+    "/repos/$baseRepo/pulls/$prNumber/requested_reviewers" \
+    --input "$tmp/reviewers.json"); then
+    log "Failed to request reviews: $response"
+    exit 1
+fi
+
+log "Successfully requested reviews"

ci/request-reviews/verify-base-branch.sh

@@ -0,0 +1,103 @@
+#!/usr/bin/env bash
+
+# Check that a PR doesn't include commits from other development branches.
+# Fails with next steps if it does
+
+set -euo pipefail
+tmp=$(mktemp -d)
+trap 'rm -rf "$tmp"' exit
+SCRIPT_DIR=$(dirname "$0")
+
+log() {
+    echo "$@" >&2
+}
+
+# Small helper to check whether an element is in a list
+# Usage: `elementIn foo "${list[@]}"`
+elementIn() {
+    local e match=$1
+    shift
+    for e; do
+        if [[ "$e" == "$match" ]]; then
+            return 0
+        fi
+    done
+    return 1
+}
+
+if (( $# < 6 )); then
+    log "Usage: $0 LOCAL_REPO HEAD_REF BASE_REPO BASE_BRANCH PR_REPO PR_BRANCH"
+    exit 1
+fi
+localRepo=$1
+headRef=$2
+baseRepo=$3
+baseBranch=$4
+prRepo=$5
+prBranch=$6
+
+# All development branches
+devBranchPatterns=()
+while read -r pattern; do
+    if [[ "$pattern" != '#'* ]]; then
+        devBranchPatterns+=("$pattern")
+    fi
+done < "$SCRIPT_DIR/dev-branches.txt"
+
+git -C "$localRepo" branch --list --format "%(refname:short)" "${devBranchPatterns[@]}" > "$tmp/dev-branches"
+readarray -t devBranches < "$tmp/dev-branches"
+
+if [[ "$baseRepo" == "$prRepo" ]] && elementIn "$prBranch" "${devBranches[@]}"; then
+    log "This PR merges $prBranch into $baseBranch, no commit check necessary"
+    exit 0
+fi
+
+# The current merge base of the PR
+prMergeBase=$(git -C "$localRepo" merge-base "$baseBranch" "$headRef")
+log "The PR's merge base with the base branch $baseBranch is $prMergeBase"
+
+# This is purely for debugging
+git -C "$localRepo" rev-list --reverse "$baseBranch".."$headRef" > "$tmp/pr-commits"
+log "The PR includes these $(wc -l < "$tmp/pr-commits") commits:"
+cat <"$tmp/pr-commits" >&2
+
+for testBranch in "${devBranches[@]}"; do
+
+    if [[ -z "$(git -C "$localRepo" rev-list -1 --since="1 month ago" "$testBranch")" ]]; then
+        log "Not checking $testBranch, was inactive for the last month"
+        continue
+    fi
+    log "Checking if commits from $testBranch are included in the PR"
+
+    # We need to check for any commits that are in the PR which are also in the test branch.
+    # We could check each commit from the PR individually, but that's unnecessarily slow.
+    #
+    # This does _almost_ what we want: `git rev-list --count headRef testBranch ^baseBranch`,
+    # except that it includes commits that are reachable from _either_ headRef or testBranch,
+    # instead of restricting it to ones reachable by both
+
+    # Easily fixable though, because we can use `git merge-base testBranch headRef`
+    # to get the least common ancestor (aka merge base) commit reachable by both.
+    # If the branch being tested is indeed the right base branch,
+    # this is then also the commit from that branch that the PR is based on top of.
+    testMergeBase=$(git -C "$localRepo" merge-base "$testBranch" "$headRef")
+
+    # And then use the `git rev-list --count`, but replacing the non-working
+    # `headRef testBranch` with the merge base of the two.
+    extraCommits=$(git -C "$localRepo" rev-list --count "$testMergeBase" ^"$baseBranch")
+
+    if (( extraCommits != 0 )); then
+        log -e "\e[33m"
+        echo "The PR's base branch is set to $baseBranch, but $extraCommits commits from the $testBranch branch are included. Make sure you know the [right base branch for your changes](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#branch-conventions), then:"
+        echo "- If the changes should go to the $testBranch branch, [change the base branch](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request) to $testBranch"
+        echo "- If the changes should go to the $baseBranch branch, rebase your PR onto the merge base with the $testBranch branch:"
+        echo "  \`\`\`"
+        echo "  git rebase --onto $prMergeBase $testMergeBase"
+        echo "  git push --force-with-lease"
+        echo "  \`\`\`"
+        log -e "\e[m"
+        exit 1
+    fi
+done
+
+log "Base branch is correct, no commits from development branches are included"

shell.nix

@@ -11,23 +11,12 @@
 #
 #     nix-shell --arg nixpkgs ./.
 #
-let
-  pinnedNixpkgs = builtins.fromJSON (builtins.readFile ci/pinned-nixpkgs.json);
-in
 {
   system ? builtins.currentSystem,
-
+  nixpkgs ? null,
-  nixpkgs ? fetchTarball {
-    url = "https://github.com/NixOS/nixpkgs/archive/${pinnedNixpkgs.rev}.tar.gz";
-    sha256 = pinnedNixpkgs.sha256;
-  },
 }:
 let
-  pkgs = import nixpkgs {
+  inherit (import ./ci { inherit nixpkgs system; }) pkgs;
-    inherit system;
-    config = { };
-    overlays = [ ];
-  };
 in
 pkgs.mkShellNoCC {
   packages = with pkgs; [