review-pull-request

Files (1)

• SKILL.md

SKILL.md (first lines)

# Review a pull request from ar2

Most of them merge themselves. A run generates `versions/<escaped version>/registry-1.json`
for each version a package is missing, opens one pull request per version, and turns on
auto-merge; CI downloads every asset the files describe, on a machine of the
environment each entry is for, and opens it. A pull request that merges without anyone
reading it is the normal case, and the trust in it comes from that check rather than
from anyone's judgement.

So a pull request waiting for a human is a pull request ar2 decided it could not answer
for -- or one a person asked for, which `ar2 regenerate` opens and never auto-merges. Its
body says which case it is. This is what to do about each.

## What not to check

CI has already established, for every entry in the pull request:

- the asset exists, downloads, and its checksum is the one recorded
- the archive opens, and `files[].src` names something inside it
- every signature the entry claims has been verified with the tool the entry names
- an entry for `linux/amd64` was checked on a Linux amd64 machine, and so on for the
  six environments

None of that needs reading again. The generated files are indented, so a diff says which
field moved, but what CI established about them isn't learned again by reading them.

## The version is verifiable with less than the one before it

> These versions can be verified with less than the version before them

The release stopped carrying a signature that the previous version had. That is also
what an attacker publishing a release would look like from here, which is why it stops.

Check upstream: a project that moved from cosign to attestations, or retired a key, says
so in its release notes or its workflow. A project that says nothing about it is the
case this exists for.

## The version covers platforms the one before it didn't

> These versions cover platforms the version before them didn't

The asset names are read from the release, so an asset placed on a platform it wasn't on
before is either a build the release started publishing, or another program of the release
whose name doesn't tell it apart. The first merges as it is. The second is what
`all_assets_filter` is for: see below.

## `files[].src` and environments left out

When the path the definition gives isn't in the archive, ar2 looks for the file of that name
that is an executable -- read from the file's own header, ELF, Mach-O or PE, or a script
naming its interpreter -- and records where it is. Exactly one such file is the command
wherever it is, so that merges itself and doesn't arrive here.

None, or several, is an environment the definition can't be read against. It is left out of
the version, and the package's issue labelled `missing-environment` says which and why; so
does an environment whose assets nothing in their names tells apart. The version's other
environments merge without it. What to fix is the definition -- `files`, or a filter -- and
then [generate the version again](../regenerate-versions/SKILL.md).

A version where no environment could be read against the definition is a pull request of
its own instead, which is the next section.

## The pull request is versions waiting for a definition

> Generated by ar2, and waiting for the definition to say more

Labelled `needs-definition`, and the one kind of pull request here that can't merge as it
stands: the entry names a file the archive doesn't hold anywhere inside it, so what it says
isn't true, and the checks say so.

It holds the versions of one era of the release and no others, on the branch
`ar2_<id>__needs-definition`. The versions the same run generated that were right have pull
requests of their own, which merged without waiting for these.

What they need is the definition to describe that era — a `version_overrides` entry with its
own `files` — and the definition goes on this branch rather than straight onto `main`, so that
the versions and what makes them true arrive together. Then
[generate them again from it](../regenerate-versions/SKILL.md#what-is-waiting-for-a-definition)
and let the checks decide.

Closing it says the versions aren't worth a definition. Nothing opens it again: the run that
finds them next sees the package already has versions waiting only while the pull request is
open, so a later run will offer them again — decide once and, if the answer is no twice,
[ignore the package](../ignore-package/SKILL.md) or leave the era alone.

## The archive couldn't be opened

No machine in the matrix has a tool for the format. The checksum is still recorded, so
the entry is complete; what is missing is the confirmation that the files are where the
entry says. Rare, and a reason to look at the format rather than the package.

## A version_constraint couldn't be turned into a boundary

> These version_constraints of the definition couldn't be turned into boundaries, so
> the overrides are carried over in the order aqua-registry had them

This is the common one, and the only one where the thing to check is the definition
rather than the release.

aqua-registry lists its overrides oldest first, each bounded above, and reads them top
down. This registry reads them the other way: the conversion reverses the list and
gives each entry the lower bound of the range it covers, so that an entry means the same
thing read from either end. A constraint that names versions rather than bounding them
— `Version in ["v0.3.6", "v0.4.0"]`, or `semver("<= 0.1.7") or Version == "v0.2.1"` —
can't give the entry above it a bound, so nothing can be derived and the list is carried
over in the order it had.

Carried over, the order still resolves the same way, because both registries read their
overrides in order and take the first match. The pull request is asking for that to be
confirmed rather than reporting a problem.

Four steps:

1. Read the constraints the body names.
2. Open `registry.yaml` in the package's directory and check its overrides are
   aqua-registry's, in aqua-registry's order. The top-level `version_constraint` is
   gone, which is expected: there is none here, and it was `"false"` there.
3. For each version the body lists, work out which override matches first in that
   order, and confirm aqua-registry picks the same one.

From aquaproj/aqua-registry-g2, MIT.

More from aquaproj/aqua-registry-g2