From 5a997fb7d435e0498c82d02ae895257dc0ad375f Mon Sep 17 00:00:00 2001 From: Myasnikov Daniil Date: Mon, 9 Nov 2026 15:19:04 +0000 Subject: [PATCH] feat(release): post an immutable report of every classification and write record version 2 Each run rewrote the batch body whole, so the only account of what a run found was gone at the next run, and a merged batch kept only its last one. Nothing said what a classification was made of either: not the heads it read, the policy, the tool or the flags. A run now makes a report: every original merged into main since the line branched, in exactly one category, with what the run read. Its ID hashes the classification and names no pull request, so a new batch's report is whole before GitHub numbers the pull request, and a run that finds the same as the last one makes the same report, which is posted once and keeps its first bytes. The body carries the latest report under the record; a comment the tool never edits keeps each one, so report reads any of them back, from merged batches too. The comment goes up before the push and the body edit on an open batch, and after the pull request a new batch opens with the report in its body; a later run posts a report the body kept but no comment did, byte for byte, before its own. Every write is preceded by a read of the pull request, and nothing is written once it is merged or closed. Both the comment and the body are rendered and measured before the first write, so an oversized one stops the run with nothing written. The record becomes version 2: what it was built from, with the run's --no-workflow-files, which --defer keeps rather than resets, and the report it goes with. Assisted-By: LLM Signed-off-by: Myasnikov Daniil --- cmd/backport-batch/README.md | 21 +- cmd/backport-batch/assemble.go | 153 +++++- cmd/backport-batch/body.go | 32 +- cmd/backport-batch/body_test.go | 20 +- cmd/backport-batch/defer.go | 7 + cmd/backport-batch/defer_test.go | 4 +- cmd/backport-batch/fixture_test.go | 105 +++- cmd/backport-batch/github.go | 35 +- cmd/backport-batch/main.go | 46 +- cmd/backport-batch/notify.go | 5 +- cmd/backport-batch/notify_test.go | 21 + cmd/backport-batch/overrides_test.go | 16 +- cmd/backport-batch/record.go | 21 + cmd/backport-batch/record_test.go | 74 +++ cmd/backport-batch/report.go | 734 ++++++++++++++++++++++++++ cmd/backport-batch/report_test.go | 687 ++++++++++++++++++++++++ cmd/backport-batch/workflows_test.go | 3 +- docs/release/backport-tasks/README.md | 4 +- docs/release/backports.md | 8 +- 19 files changed, 1898 insertions(+), 98 deletions(-) create mode 100644 cmd/backport-batch/report.go create mode 100644 cmd/backport-batch/report_test.go diff --git a/cmd/backport-batch/README.md b/cmd/backport-batch/README.md index 945d6fac1..7a1acabf1 100644 --- a/cmd/backport-batch/README.md +++ b/cmd/backport-batch/README.md @@ -13,12 +13,13 @@ go build ./cmd/backport-batch ./backport-batch fold --line release-1.7 --original 4321 --commit SHA [--policy REF] ./backport-batch verify --line release-1.7 [--head SHA] ./backport-batch notify --line release-1.7 [--mention org/team|login] [--push] +./backport-batch report --pr 5246 [--id ID] [--json] ./backport-batch lines [--policy REF] ``` [`hack/backport-batch-scheduled.sh`](../../hack/backport-batch-scheduled.sh) runs `lines`, then `run --continue --push`, `check` and `notify` for each line; [`backport-batch.yaml`](../../.github/workflows/backport-batch.yaml) runs it on schedule as the cozystack-ci App, with `--no-workflow-files`, which it hands to `run`. -Requires `git` and an authenticated `gh` on PATH, and a remote called `origin`. Every command but `notify` fetches `origin` first; `notify` reads only GitHub. The batch runs only for the lines `lines` prints. +Requires `git` and an authenticated `gh` on PATH, and a remote called `origin`. Every command but `notify` and `report` fetches `origin` first; those two read only GitHub. The batch runs only for the lines `lines` prints. `run`, `check`, `fold` and `lines` read the `backport-policy` block of the policy page from `origin/main`, or from the ref `--policy` names, which is how a change to the page is tried before it merges; `verify` and `notify` do not read it and take no `--policy`. A line the block lists takes nothing merged into `main` before the day it gives, unless the original carries `backport/include`; any other line starts at its fork point. A page without exactly one readable block stops the run with exit 2. @@ -28,7 +29,7 @@ Build the binary wherever an exit code matters: `go run` turns any non-zero exit Switches the current working tree to `backport-batch/-`, created at `origin/`, and picks onto it one original at a time, in the order they merged into `main`, as one unit each on the branch's first-parent line. A pull request merged into `main` with a merge commit `M` goes on as its own commits, `git rev-list --reverse --topo-order --no-merges M^1..M^2`, each picked with `git cherry-pick -x --signoff`, so each keeps its author, message and sign-offs and gains the `(cherry picked from commit …)` line; the merges that brought `main` into its branch are left out, since `main` already has what they brought. A merge commit then joins the picks to the batch: its first parent is the batch before the original, its second the last pick, and its message is `M`'s with a paragraph of its own, `Backported-from: M`. A pull request squash-merged into `main` goes on as one pick. When any commit of an original conflicts, the pick is aborted, the branch goes back to where it was before that original, and the original is recorded with the commit that failed and its files; the run goes on with the next one. A commit the branch already carries is skipped as empty, and an original none of whose commits changes anything is reported as carried. The tree has to be clean. A local branch of that name is reset to where the run starts; when that drops commits only it has, the run first prints its tip and the `git branch` command that restores them, and so does `--defer`. Signing follows your git configuration; hooks are off for the picks and the merge, so each commit carries its message as git writes it. -The pull request body goes to stdout, built from the line's own `.github/PULL_REQUEST_TEMPLATE.md`: the originals on the branch, each with how many commits it brought and which of them differ from the commit they were picked from by `git patch-id`, a resolution or a pick git had to fit, the deferred conflicts with the commit that failed and its files, the skipped pull requests counted by rule with the lists collapsed, the originals' release notes, and a `backport-batch` block of JSON that lists the deferred conflicts for `check`. The title is `fix(backport): batch for , `. A body that would pass GitHub's 65536-byte limit leaves out its longest skipped lists, and the run prints them on stderr instead. +The pull request body goes to stdout, built from the line's own `.github/PULL_REQUEST_TEMPLATE.md`: the originals on the branch, each with how many commits it brought and which of them differ from the commit they were picked from by `git patch-id`, a resolution or a pick git had to fit, the deferred conflicts with the commit that failed and its files, the skipped pull requests counted by rule with the lists collapsed, the originals' release notes, and two blocks of JSON: the [line's record](#the-lines-record-and-reports) in `backport-batch`, which later runs, `check` and `notify` act on, and the run's report in `backport-batch-report`. The title is `fix(backport): batch for , `. A body that would pass GitHub's 65536-byte limit leaves out its longest skipped lists, and the run prints them on stderr instead; one that what it has to carry still puts over the limit is refused before anything is pushed. `--onto SHA` continues a batch already pushed instead of starting a new one. SHA has to be the head the remote has for a batch branch of the line right now, read with `git ls-remote`, so a head someone pushed onto since is refused. What that branch carries stays, and only what it does not carry yet is picked on top, so the next push is a fast-forward. `--continue` takes SHA from the open batch pull request, and starts a new batch when none is open, which is what the scheduled run does. @@ -36,11 +37,11 @@ The pull request body goes to stdout, built from the line's own `.github/PULL_RE `--defer N[,N] --reason TEXT` takes originals out of the open batch pull request, usually the ones a failing check was pinned on. It builds the batch branch again off `origin/` without their units: while the batch still sits on the line's head, the units before the first one it drops stay as they are, and every later one, a pushed resolution included, is picked again in its own shape, a merge's picks one by one and then the merge with its message. When dropping one leaves a later commit that no longer applies, the run stops and names that commit's original, to defer as well. It adds no new original. It also takes out an original that got `backport/skip` after the batch picked it, which a run never removes on its own and the body lists as still on the branch. The body records each deferred original with the reason in the `backport-batch` block; a run that continues this batch keeps them out, and the next batch tries them again. A batch that carries a merge commit other than an original's, as GitHub's update-branch button makes, is refused, since picking such a merge again would lose what it resolved. -`--push` pushes the branch, refusing anything but a fast-forward of what the remote has, then opens the draft pull request against the line with `gh pr create --draft`, or updates the open batch pull request with `gh pr edit`, and prints its URL on stderr. When the branch it refuses to push over has no pull request open on it, as a run that stopped between its push and opening the pull request leaves it, the refusal names the `run --onto --push` that continues it. While a batch pull request is open on another branch it refuses, so a line never has two. It never pushes to or edits a batch pull request whose body has no `backport-batch` block either: someone wrote that body by hand, and the batch is theirs to merge or close. After `--defer` alone it replaces the branch, with `--force-with-lease=refs/heads/:`, where head is what `git ls-remote` reads for the branch and has to be the head the rebuild started from: a commit pushed since is never dropped, by the tool's check or, if one lands between that check and the push, by the remote. +`--push` pushes the branch, refusing anything but a fast-forward of what the remote has, then opens the draft pull request against the line with `gh pr create --draft`, or updates the open batch pull request with `gh pr edit`, and prints its URL on stderr. On the open batch it first posts the run's report as a comment, so a body never names a report nobody can read; a new batch carries the report in the body it opens with, and the comment follows. Before every write to the pull request it reads the pull request again, and stops with exit 2 once it is merged or closed. When the branch it refuses to push over has no pull request open on it, as a run that stopped between its push and opening the pull request leaves it, the refusal names the `run --onto --push` that continues it. While a batch pull request is open on another branch it refuses, so a line never has two. It never pushes to or edits a batch pull request whose body has no `backport-batch` block either: someone wrote that body by hand, and the batch is theirs to merge or close. After `--defer` alone it replaces the branch, with `--force-with-lease=refs/heads/:`, where head is what `git ls-remote` reads for the branch and has to be the head the rebuild started from: a commit pushed since is never dropped, by the tool's check or, if one lands between that check and the push, by the remote. ### check -Exits 1 and prints the URL of every pull request the rules take that is neither on the line nor on the open batch pull request, 0 when there is none, and 2 when it cannot tell. A pull request that the [line's record](#the-lines-record) lists as deferred, a conflict or one taken out with `--defer`, prints as a warning and does not fail the check: deferring is a maintainer's call. That holds once the batch that recorded it merged or closed too. So does one the open batch lists as waiting for a maintainer because it changes workflows; once that batch is gone, nothing lists it any more, and it is missing. +Exits 1 and prints the URL of every pull request the rules take that is neither on the line nor on the open batch pull request, 0 when there is none, and 2 when it cannot tell. A pull request that the [line's record](#the-lines-record-and-reports) lists as deferred, a conflict or one taken out with `--defer`, prints as a warning and does not fail the check: deferring is a maintainer's call. That holds once the batch that recorded it merged or closed too. So does one the open batch lists as waiting for a maintainer because it changes workflows; once that batch is gone, nothing lists it any more, and it is missing. `--cut` makes it the gate before a patch release or an rc is cut from a line. A cut tags the line, so a pull request that is only on the open batch pull request fails it too, with one line each: `#N is only on open batch PR #M, merge it before the cut`. One that waits there for a maintainer fails it as well, since nobody decided to leave it out, also when it is all the batch has left: bring it over, or take it out with `run --defer` and a reason. Everything else is as without it, and a cut that passes says `no change the policy takes is missing before the cut`. The scheduled run checks without `--cut`, since its batch is open by design. @@ -56,12 +57,16 @@ Holds every unit of a batch to the shape fold checks for one, the clean picks as ### notify -Reads the [line's record](#the-lines-record) first, as the other commands do, and exits 2 when it cannot read it or when it says the open batch is being closed, with a batch open or not. Then it reads the open batch pull request of the line, trusted as described below, and prints one line for each thing that waits on it: a batch open for three days or more, the shortest gap between two scheduled runs, so one the previous run already saw; the conflicts it deferred; and the originals waiting for a maintainer because they change workflows. Nothing open prints nothing. +Reads the [line's record](#the-lines-record-and-reports) first, as the other commands do, and exits 2 when it cannot read it or when it says the open batch is being closed, with a batch open or not. Then it reads the open batch pull request of the line, trusted as described below, and prints one line for each thing that waits on it: a batch open for three days or more, the shortest gap between two scheduled runs, so one the previous run already saw; the conflicts it deferred; and the originals waiting for a maintainer because they change workflows. Nothing open prints nothing. With `--push` it writes the same into one comment on the batch pull request: the conflicts, each to be resolved as the [conflict task](../../docs/release/backport-tasks/resolve-conflict.md) describes, and the originals that wait because they change workflows. The comment starts with `` and is one per pull request: a later run edits it in place rather than adding another, and once nothing waits it says so. Only a comment whose author the batch trusts counts as it, so one someone else wrote with the same first line is neither edited nor believed. It mentions `--mention`, an `org/team` or a login, with or without `@`, only while something waits. A failed batch CI is not among what it reports: the run has no trigger on the batch's CI finishing. +### report + +Prints a report a batch pull request keeps: the one its body carries, or the one `--id` names. It reads the report comments the batch trusts and falls back to the body, recomputes the report's ID from what it says, and exits 2 when a trusted comment does not match its ID, when a report says anything a run does not write or is for another line than the pull request's, or when the pull request keeps no such report. A report comment counts only when its author is one the batch trusts with a record, so anyone else's comment, whatever ID it carries, is not read. It prints what the classification was made of, whether the pull request's head or body shows the report was published, which it calls unconfirmed when neither does, and one line per original with its category and what the report recorded about it; `--json` prints the report as stored. A report whose comment the run never got to post is printed from the body, with a note that the next `run --push` posts it, or, on a merged or closed batch, that it is kept in the body only. It reads GitHub alone and writes nothing. + ### lines Prints the release lines the batch runs for, newest first: of the `release-X.Y` branches on `origin`, the newest that have a stable `vX.Y.Z` tag there, as many as `released-lines` in the policy block says, two when it says nothing, and every branch newer than the newest of them. The tags are read with `git ls-remote`; a tag with a pre-release suffix does not count. `run`, `check` and `fold` refuse any other line, and rule 6 holds each line to the next newer one among them. @@ -96,7 +101,7 @@ Only the line's own commits count, the ones after the point where it branched fr Titles and bodies are read from the merge commits on `main`, which hold them as a maintainer merged them; the live pull request stays editable by its author. -## The line's record +## The line's record and reports The line's record is the `backport-batch` block of its newest batch pull request: the newest pull request against the line from a `backport-batch/-` branch of this repository, opened by an author the batch trusts as above, whether it is open, merged or closed. `run`, `check`, `fold` and `notify` find it in GitHub's list of the line's pull requests, newest first, reading at most ten pages of 100. A batch pull request a person opened without the block is passed over. A block whose author the batch does not trust, a bot's batch without its block, a block that cannot be read whole, a closed record newer than the open batch, or ten pages with no record stop the command with exit 2, naming the pull request, rather than fall back to an older record. @@ -104,10 +109,12 @@ The record is written only while its pull request is open: a merged or closed ba The record is only as trustworthy as the body it sits in. Anyone with write access can edit any body, the bot's included, and neither the bot's authorship nor anything in the block proves who edited it last. A hand edit that leaves a readable record is believed; one that breaks it stops every command with exit 2, naming the pull request. When the newest batch pull request is deleted and no later one exists, nothing shows it was there: the older record governs, and what was recorded since is lost. +The record holds only what later commands act on. Every run that classifies the line also makes a report, one line of JSON in a `backport-batch-report` block: every original merged into `main` since the line branched in exactly one category, picked, missing, deferred with what it was deferred for, or skipped with the rule that skipped it, the commits on `main` that merged no pull request, the labels, and what the classification was made of: the heads of `main`, the line and the newer line, the policy, the batch head it describes, the tool's commit, the Actions run and the flags. Its ID is a hash of all that but the time and the run, so two runs that find the same make the same report, and it names no pull request, so a new batch's report is whole before GitHub numbers the pull request. The body carries the latest report beside the record, and a comment on the open batch pull request keeps each one: `` and the same block, folded away. A report already posted is not posted again, and the tool never edits or deletes such a comment, so a report the next run replaces in the body is still read back by `report`, also after the batch merged. A comment that looks like one but whose author the batch does not trust is not read. When a run stops after writing the body but before posting its comment, the next `run --push` on the open batch posts that report first, byte for byte; once the batch is closed nothing is written to it, and the report stays in its body. A run that picks nothing while no batch is open keeps no report. + ## What CI verifies The tool does not build or test anything. CI on the batch pull request runs the line's own build and tests against what was picked, and `verify` against its shape, and a maintainer reviews and merges it. A deferred conflict waits for the next batch unless someone resolves it, checks the resolution with `fold` and pushes it, and then CI runs again. ## Untrusted text -Titles, release notes and file names in the body come from whoever opened the originals. A title, a reason or a file name goes into a code span whose fence no backtick inside can close, and the release notes go into the `release-note` block with every run of three backticks or tildes defused, so GitHub acts on no mention, reference, link or HTML in them. Inside that block, in case a note is copied out of it, links lose any target outside this repository, and closing keywords, cross-repository references and mentions have their `@`, `#` and `://` swapped for fullwidth signs, which act on nothing however the backticks around them pair up. Zero-width, bidi and control characters are dropped first, and everything the tool prints passes the same filter, so nothing it relays can drive a terminal. Commit messages are read through `git cat-file --batch`, which gives each object's length, and end at their first NUL as `git log` shows them, so a message cannot pass for a second commit or hide a trailer from a reader of the log. Every value handed to git comes after `--end-of-options` or `--`, refs pass `git check-ref-format`, SHAs have to be 40 hex digits, and nothing runs through a shell. +Titles, release notes and file names in the body come from whoever opened the originals, and a reason from whoever ran `--defer`. A title, a reason or a file name goes into a code span whose fence no backtick inside can close, and the release notes go into the `release-note` block with every run of three backticks or tildes defused, so GitHub acts on no mention, reference, link or HTML in them. Inside that block, in case a note is copied out of it, links lose any target outside this repository, and closing keywords, cross-repository references and mentions have their `@`, `#` and `://` swapped for fullwidth signs, which act on nothing however the backticks around them pair up. The record and the report are one line of JSON each, whose encoding leaves no newline, `<`, `>` or `&` in them, so nothing in a file name or a reason ends their block or opens HTML, and a report comment names no one: numbers are bare, and its only text outside the block is the line, the date and the report's ID. Zero-width, bidi and control characters are dropped first, and everything the tool prints passes the same filter, so nothing it relays can drive a terminal. Commit messages are read through `git cat-file --batch`, which gives each object's length, and end at their first NUL as `git log` shows them, so a message cannot pass for a second commit or hide a trailer from a reader of the log. Every value handed to git comes after `--end-of-options` or `--`, refs pass `git check-ref-format`, SHAs have to be 40 hex digits, and nothing runs through a shell. diff --git a/cmd/backport-batch/assemble.go b/cmd/backport-batch/assemble.go index ad6de0a1b..a2adc5c1d 100644 --- a/cmd/backport-batch/assemble.go +++ b/cmd/backport-batch/assemble.go @@ -158,7 +158,9 @@ func (a *app) run(args []string) int { } r := newReport(b) + r.opts = runOptions{NoWorkflowFiles: *noWorkflows, Mode: modeNew} if carried != nil { + r.opts.Mode = modeContinue if r.stale, err = keptOut(b, carried); err != nil { return a.fail(err) } @@ -264,12 +266,25 @@ func (a *app) finish(b *batch, r *report, branch string, date time.Time, push, r if r.shapes, err = summarize(b, head, r.onBranch); err != nil { return a.fail(err) } - title := prTitle(b.line, date.Format(time.DateOnly)) + r.built = builtFrom{Main: b.mainHead, Line: b.lineHead, Newer: b.newerHead, Policy: b.policyAt, Batch: head, Tool: a.tool, + Run: runURL(os.LookupEnv), At: a.now().UTC().Format(time.RFC3339), Options: r.opts} + doc, err := buildReport(b, r) + if err != nil { + return a.fail(err) + } + p := publication{branch: branch, head: head, title: prTitle(b.line, date.Format(time.DateOnly)), report: storedReport{doc: doc, raw: reportJSON(doc)}} + p.record = r.lineRecord(doc) + // A record the next command would refuse is never written. + if err := p.record.validate(b.line); err != nil { + return a.fail(fmt.Errorf("the record this run made %w", err)) + } + r.blocks = recordBlock(p.record) + "\n\n" + reportBlock(p.report.raw) template, err := lineTemplate(b.lineHead) if err != nil { return a.fail(err) } body, left := r.body(template) + p.body = body fmt.Fprint(a.stdout, body) skipped := 0 for _, items := range r.skipped { @@ -283,8 +298,8 @@ func (a *app) finish(b *batch, r *report, branch string, date time.Time, push, r for _, p := range r.waiting { fmt.Fprintf(a.stderr, "waiting for a maintainer: %s %s changes %s\n", p.v.o.url(), terminalSafe(p.v.o.title), terminalSafe(strings.Join(p.files, ", "))) } - fmt.Fprintf(a.stderr, "%s at %.12s: %d on the branch, %d deferred, %d waiting for a maintainer, %d skipped\ntitle: %s\n", - branch, head, len(r.onBranch), len(r.deferred)+len(r.held), len(r.waiting), skipped, title) + fmt.Fprintf(a.stderr, "%s at %.12s: %d on the branch, %d deferred, %d waiting for a maintainer, %d skipped\ntitle: %s\nreport: %s\n", + branch, head, len(r.onBranch), len(r.deferred)+len(r.held), len(r.waiting), skipped, p.title, doc.ID) if !push { return 0 } @@ -296,12 +311,19 @@ func (a *app) finish(b *batch, r *report, branch string, date time.Time, push, r fmt.Fprintf(a.stderr, "no batch pull request is open on %s; this run's classification is not retained\n", b.line.branch()) return 0 } - if err := a.publish(b, branch, head, title, body, rebuilt); err != nil { + if err := a.publish(b, p, rebuilt); err != nil { return a.fail(err) } return 0 } +// lineRecord is the record a run leaves for the line: what later commands +// act on, with the full classification left to the report it names. +func (r *report) lineRecord(doc reportDoc) lineRecord { + built := doc.Built + return lineRecord{Version: 2, Line: doc.Line, Built: &built, Report: doc.ID, Deferred: slices.Clone(doc.Primary.Deferred), Dropped: []droppedEntry{}, Stale: slices.Clone(doc.Overlays.Stale)} +} + // branchDate is the date a batch branch was opened under. func branchDate(branch string) (time.Time, error) { m := batchBranchRE.FindStringSubmatch(branch) @@ -446,51 +468,64 @@ func lineTemplate(lineHead string) (string, error) { return git("show", "--end-of-options", lineHead+":"+template) } +// publication is what a run publishes: the branch at head, and the body that +// carries the record and the report. +type publication struct { + branch, head, title, body string + report storedReport + record lineRecord +} + // publish pushes the batch branch, then opens the draft batch pull request or // updates the open one. A run pushes only a fast-forward of what the remote // has. A rebuild replaces the branch, and only the head it was rebuilt from: // the lease is that head, as ls-remote reads it now, so a commit pushed since // is never dropped. -func (a *app) publish(b *batch, branch, head, title, body string, rebuilt bool) error { - if b.pr != nil && b.pr.Head.Ref != branch { +// +// The report goes up as a comment before the push and the body that names +// it, so no write leaves a body naming a report nobody can read; a new batch +// carries the report in the body it is opened with, and the comment follows. +// Everything is rendered and measured before the first write. +func (a *app) publish(b *batch, p publication, rebuilt bool) error { + if b.pr != nil && b.pr.Head.Ref != p.branch { return fmt.Errorf("the open batch pull request #%d is on %s; continue it with --onto %s", b.pr.Number, b.pr.Head.Ref, b.prHead) } // A body without the block was written by hand, and the tool's would // replace it whole, title included; such a batch is the maintainer's. - if b.pr != nil && !strings.Contains("\n"+strings.ReplaceAll(b.pr.Body, "\r\n", "\n")+"\n", "\n"+deferredFence+"\n") { + if b.pr != nil && !b.src.open { return fmt.Errorf("batch pull request #%d has no %s block, so backport-batch did not write it; merge or close it, and the next run opens a batch of its own", b.pr.Number, deferredFence) } - if head == b.lineHead { + if p.head == b.lineHead { if rebuilt { - return fmt.Errorf("without what is deferred, %s carries nothing beyond %s; close batch pull request #%d instead", branch, b.line.branch(), b.pr.Number) + return fmt.Errorf("without what is deferred, %s carries nothing beyond %s; close batch pull request #%d instead", p.branch, b.line.branch(), b.pr.Number) } - return fmt.Errorf("%s carries nothing beyond %s; nothing to push", branch, b.line.branch()) + return fmt.Errorf("%s carries nothing beyond %s; nothing to push", p.branch, b.line.branch()) } heads, err := remoteBatchHeads(b.line) if err != nil { return err } - pushed, onRemote := heads[branch] + pushed, onRemote := heads[p.branch] lease := "" switch { case rebuilt: if !onRemote || pushed != b.prHead { - return fmt.Errorf("%s on %s is at %.12s, not at %.12s, which the rebuild started from; run again", branch, remote, pushed, b.prHead) + return fmt.Errorf("%s on %s is at %.12s, not at %.12s, which the rebuild started from; run again", p.branch, remote, pushed, b.prHead) } lease = pushed - case onRemote && pushed != head: + case onRemote && pushed != p.head: have, err := lookup(pushed) if err != nil { return err } descends := false if have != "" { - if descends, err = isAncestor(pushed, head); err != nil { + if descends, err = isAncestor(pushed, p.head); err != nil { return err } } if !descends { - refusal := fmt.Sprintf("%s on %s is at %.12s, which %.12s does not descend from; refusing a non-fast-forward push", branch, remote, pushed, head) + refusal := fmt.Sprintf("%s on %s is at %.12s, which %.12s does not descend from; refusing a non-fast-forward push", p.branch, remote, pushed, p.head) // A run that stopped between its push and opening the pull request // leaves the branch, and the next run that day builds it anew. if b.pr == nil { @@ -499,38 +534,100 @@ func (a *app) publish(b *batch, branch, head, title, body string, rebuilt bool) return errors.New(refusal) } } - if pushed != head { - if _, err := git(pushArgs(branch, head, lease)...); err != nil { + if err := p.fits(); err != nil { + return err + } + if b.pr != nil { + if err := a.keepReports(b, &p); err != nil { return err } } - f, err := os.CreateTemp("", "backport-batch-body-*.md") + if pushed != p.head { + if _, err := git(pushArgs(p.branch, p.head, lease)...); err != nil { + return err + } + } + name, err := writeTemp("backport-batch-body-*.md", p.body) if err != nil { return err } - defer os.Remove(f.Name()) - if _, err := f.WriteString(body); err != nil { - f.Close() - return err - } - if err := f.Close(); err != nil { - return err - } + defer os.Remove(name) if b.pr == nil { - url, err := a.gh.createPull(b.line.branch(), branch, title, f.Name()) + n, url, err := a.gh.createPull(b.line.branch(), p.branch, p.title, name) if err != nil { return err } fmt.Fprintf(a.stderr, "opened draft %s\n", url) + if err := a.postReport(n, p.report); err != nil { + return fmt.Errorf("posting report %.12s on #%d: %w; the body keeps it, and the next run --push posts it", p.report.doc.ID, n, err) + } return nil } - if err := a.gh.editPull(b.pr.Number, title, f.Name()); err != nil { + if err := a.stillOpen(b.pr.Number); err != nil { + return err + } + if err := a.gh.editPull(b.pr.Number, p.title, name); err != nil { return err } fmt.Fprintf(a.stderr, "updated %s\n", pullURL(b.pr)) return nil } +// fits says why GitHub would not take the comment or the body. +func (p *publication) fits() error { + if _, err := reportComment(p.report); err != nil { + return err + } + if len(p.body) > bodyLimit { + return fmt.Errorf("the body is %d bytes with only what it cannot leave out, over GitHub's %d; nothing is published", len(p.body), bodyLimit) + } + return nil +} + +// keepReports makes sure the open batch's comments keep both the report its +// body carries now and the one this run made, in that order, each once. +// When this run's report is one already kept, the body carries the kept +// bytes, which differ only in when and in which job it was made. +func (a *app) keepReports(b *batch, p *publication) error { + pr := b.pr + posted, bad, err := findReports(a.gh, pr.Number, b.line) + if err != nil { + return err + } + for _, why := range bad { + a.warn("batch pull request #%d: %s; it does not count as a report", pr.Number, why) + } + earlier, err := bodyReport(pr.Body, b.line) + if err != nil { + return fmt.Errorf("batch pull request #%d: %w", pr.Number, err) + } + kept, ok := posted[p.report.doc.ID] + if !ok && earlier != nil && earlier.doc.ID == p.report.doc.ID { + kept, ok = *earlier, true + } + if ok { + p.body = strings.Replace(p.body, reportBlock(p.report.raw), reportBlock(kept.raw), 1) + p.report = kept + if err := p.fits(); err != nil { + return err + } + } + if earlier != nil { + if _, ok := posted[earlier.doc.ID]; !ok { + if err := a.postReport(pr.Number, *earlier); err != nil { + return fmt.Errorf("posting the earlier report %.12s on #%d: %w", earlier.doc.ID, pr.Number, err) + } + posted[earlier.doc.ID] = *earlier + } + } + if _, ok := posted[p.report.doc.ID]; !ok { + if err := a.postReport(pr.Number, p.report); err != nil { + return fmt.Errorf("posting report %.12s on #%d: %w", p.report.doc.ID, pr.Number, err) + } + } + return nil +} + // pullURL is where a pull request is on GitHub. func pullURL(pr *pullRequest) string { if pullURLRE.MatchString(pr.HTMLURL) { diff --git a/cmd/backport-batch/body.go b/cmd/backport-batch/body.go index bfb908d2d..ae34be876 100644 --- a/cmd/backport-batch/body.go +++ b/cmd/backport-batch/body.go @@ -190,25 +190,11 @@ type deferredRecord struct { Deferred []deferredEntry `json:"deferred"` } -// deferredBlock is the machine-readable list of what the batch defers. One -// line of JSON: encoding/json escapes newlines and <, > and &, so nothing in a -// file name or a reason can end the fence or open HTML. It leaves bidi and -// zero-width characters as they are, so file names go through clean first, -// as the reason does. +// deferredBlock is a version 1 record, which batches carried before records +// had a version. encoding/json leaves bidi and zero-width characters as they +// are, so file names go through clean first, as the reason does. func deferredBlock(l line, conflicts, held, waiting []pick) string { - rec := deferredRecord{Line: l.branch(), Deferred: []deferredEntry{}} - for _, p := range conflicts { - files := cleanFiles(p.files[:min(len(p.files), filesKept)]) - rec.Deferred = append(rec.Deferred, deferredEntry{Original: p.v.o.number, Commit: p.failed, Files: files, More: len(p.files) - len(files) + p.more}) - } - for _, p := range held { - rec.Deferred = append(rec.Deferred, deferredEntry{Original: p.v.o.number, Files: []string{}, Reason: capped(oneLine(clean(p.reason, false)), titleLimit)}) - } - for _, p := range waiting { - files := cleanFiles(p.files[:min(len(p.files), filesKept)]) - rec.Deferred = append(rec.Deferred, deferredEntry{Original: p.v.o.number, Files: files, More: len(p.files) - len(files) + p.more, Workflows: true}) - } - data, _ := json.Marshal(rec) + data, _ := json.Marshal(deferredRecord{Line: l.branch(), Deferred: deferredEntries(conflicts, held, waiting)}) return deferredFence + "\n" + string(data) + "\n```" } @@ -254,6 +240,14 @@ type report struct { start time.Time // shapes says, for an original on the branch, what it brought. shapes map[*verdict]string + // missing are originals the policy takes that the run neither tried nor + // found anywhere. + missing []*verdict + // opts is how the run was asked to classify, and built what it read. + opts runOptions + built builtFrom + // blocks are the record and the report the body ends with. + blocks string } // ruleName is how output names what kept an original out: the policy's @@ -368,7 +362,7 @@ func (r *report) what(details map[int]bool) string { b.WriteString("\n\n") } } - b.WriteString("\n" + deferredBlock(r.line, r.deferred, r.held, r.waiting) + "\n") + b.WriteString("\n" + r.blocks + "\n") return b.String() } diff --git a/cmd/backport-batch/body_test.go b/cmd/backport-batch/body_test.go index 07f2f8582..3f746d17d 100644 --- a/cmd/backport-batch/body_test.go +++ b/cmd/backport-batch/body_test.go @@ -286,7 +286,7 @@ func TestBodyFollowsTheTemplate(t *testing.T) { l, _ := parseLine("release-1.6") template := "\n\n## What this PR does\n\n\n### Screenshots\n\n\n\n### Downstream repositories\n\n- [ ] No downstream repository is affected by this change\n\n### Release note\n\n```release-note\n\n```\n" o := &original{number: 7, title: "fix(x): y", body: "```release-note\nfix(x): note\n```"} - r := &report{line: l, onBranch: []*verdict{{o: o}}, skipped: map[int][]*verdict{}} + r := &report{line: l, onBranch: []*verdict{{o: o}}, skipped: map[int][]*verdict{}, blocks: deferredBlock(l, nil, nil, nil)} body, _ := r.body(template) for _, want := range []string{"## What this PR does\n\nBackports to", "- [#7](https://github.com/cozystack/cozystack/pull/7) `fix(x): y`", "### Downstream repositories\n\n- [ ] No downstream", "```release-note\nfix(x): note\n```", deferredFence} { if !strings.Contains(body, want) { @@ -305,7 +305,7 @@ func TestBodyFollowsTheTemplate(t *testing.T) { for i := range 40 { many = append(many, &verdict{o: &original{number: i + 1, title: "fix(x): y", body: "```release-note\n" + long + "\n```"}}) } - r = &report{line: l, onBranch: many, skipped: map[int][]*verdict{ruleType: many}} + r = &report{line: l, onBranch: many, skipped: map[int][]*verdict{ruleType: many}, blocks: deferredBlock(l, nil, nil, nil)} if body, _ := r.body(template); len(body) > bodyLimit || !strings.Contains(body, "truncated") || !strings.Contains(body, deferredFence) { t.Errorf("a body over the limit is %d bytes (limit %d), truncated=%v", len(body), bodyLimit, strings.Contains(body, "truncated")) } @@ -322,7 +322,7 @@ func TestBodyFollowsTheTemplate(t *testing.T) { for i := range 80 { conflict.files = append(conflict.files, fmt.Sprintf("chart/f%02d.yaml", i)) } - r = &report{line: l, onBranch: many[:1], deferred: []pick{conflict}, skipped: map[int][]*verdict{ruleType: lots, ruleOnLine: few}} + r = &report{line: l, onBranch: many[:1], deferred: []pick{conflict}, skipped: map[int][]*verdict{ruleType: lots, ruleOnLine: few}, blocks: deferredBlock(l, []pick{conflict}, nil, nil)} body, left := r.body(template) if !slices.Equal(left, []int{ruleType}) { t.Errorf("left out the lists of rules %v, want only rule 2", left) @@ -366,3 +366,17 @@ func TestBodyLinksTaskPagesThatExist(t *testing.T) { } } } + +// A body that what it has to carry pushes over GitHub's limit is refused +// before anything is written, the replay of an earlier report included, +// although the report alone would fit a comment. +func TestBodyRefusesMandatoryContentOverTheLimit(t *testing.T) { + f, gh, head := bigWaits(t, 4) + code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--continue", "--no-workflow-files", "--push") + if code != 2 || !strings.Contains(stderr, "with only what it cannot leave out, over GitHub's 65536") { + t.Errorf("run --push = %d, want a refusal of the body:\n%s", code, stderr) + } + if f.remoteHead(batchBranch1) != head || len(gh.issueComments) != 0 || len(gh.edited) != 0 { + t.Errorf("the refused run moved the batch to %.12s, wrote %d comments and edited %q", f.remoteHead(batchBranch1), len(gh.issueComments), gh.edited) + } +} diff --git a/cmd/backport-batch/defer.go b/cmd/backport-batch/defer.go index 5f2b29c6a..cdc17a78c 100644 --- a/cmd/backport-batch/defer.go +++ b/cmd/backport-batch/defer.go @@ -114,6 +114,9 @@ func (a *app) rebuild(b *batch, numbers []int, reason string, push bool) int { return a.fail(err) } r := newReport(b) + // Taking an original out changes nothing about how the batch is + // assembled, so the record keeps the flag the batch was assembled with. + r.opts = runOptions{NoWorkflowFiles: b.src.savedNoWorkflowFiles(), Mode: modeDefer} if r.stale, err = keptOut(b, picks); err != nil { return a.fail(err) } @@ -128,6 +131,10 @@ func (a *app) rebuild(b *batch, numbers []int, reason string, push bool) int { r.waiting = append(r.waiting, pick{v: v, status: "waiting", files: e.Files, more: e.More}) case len(e.Files) > 0: r.deferred = append(r.deferred, pick{v: v, status: "conflict", files: e.Files, more: e.More, failed: e.Commit}) + default: + // Merged into main since the batch was assembled, or recorded + // nowhere: a rebuild adds no original, the next run tries it. + r.missing = append(r.missing, v) } } return a.finish(b, r, branch, date, push, true) diff --git a/cmd/backport-batch/defer_test.go b/cmd/backport-batch/defer_test.go index 856c9909a..59c736083 100644 --- a/cmd/backport-batch/defer_test.go +++ b/cmd/backport-batch/defer_test.go @@ -261,8 +261,8 @@ func TestDeferLeasesTheHeadItRebuilt(t *testing.T) { pr := f.batchPR(90, "release-1.1", batchBranch1, gh.prs[0].Body) a := &app{gh: gh} - b := &batch{line: l, lineHead: lineHead, pr: &pr, prHead: head} - err := a.publish(b, batchBranch1, rebuilt, "title", "body", true) + b := &batch{line: l, lineHead: lineHead, pr: &pr, prHead: head, src: recordSource{carrier: &pr, open: true, code: &pr}} + err := a.publish(b, publication{branch: batchBranch1, head: rebuilt, title: "title", body: "body"}, true) if err == nil || !strings.Contains(err.Error(), "which the rebuild started from") { t.Errorf("publish over a head pushed since = %v, want a refusal", err) } diff --git a/cmd/backport-batch/fixture_test.go b/cmd/backport-batch/fixture_test.go index 23e157f9f..1a633134f 100644 --- a/cmd/backport-batch/fixture_test.go +++ b/cmd/backport-batch/fixture_test.go @@ -18,6 +18,7 @@ package main import ( "bytes" + "cmp" "fmt" "os" "os/exec" @@ -36,6 +37,8 @@ type fixture struct { dir string remote string now time.Time + // tool is the commit the tool says it was built from. + tool string } func newFixture(t *testing.T) *fixture { @@ -256,6 +259,48 @@ type fakeGH struct { // closedWrites names every write the tool attempted on a pull request // that is no longer open; exec fails the test on one. closedWrites []string + // fail makes the next call of an operation, by its method name, fail. + fail map[string]error + // nextPR is the number createPull gives, 90 when unset. + nextPR int + // remoteHead reads a branch's head off the fixture's remote: GitHub + // reports an open pull request at what its branch has. + remoteHead func(string) string + // commented lists the pull request each comment went to, in order. + commented []int + // after runs once an operation, by its method name, has succeeded: what + // someone else does on GitHub meanwhile. + after map[string]func() +} + +// live is pr as GitHub reports it now: an open one at the head its branch +// has on the remote. +func (g *fakeGH) live(pr pullRequest) pullRequest { + if g.remoteHead != nil && (pr.State == "" || pr.State == "open") { + if head := g.remoteHead(pr.Head.Ref); head != "" { + pr.Head.SHA = head + } + } + return pr +} + +// inject is the failure a test set for the next call of op, once. +func (g *fakeGH) inject(op string) error { + err := g.fail[op] + delete(g.fail, op) + return err +} + +func (g *fakeGH) pull(n int) (pullRequest, error) { + if err := g.inject("pull"); err != nil { + return pullRequest{}, err + } + for _, pr := range g.prs { + if pr.Number == n { + return g.live(pr), nil + } + } + return pullRequest{}, fmt.Errorf("HTTP 404: no pull request #%d", n) } // isOpen says whether the fake has pull request n open, or does not know it. @@ -296,6 +341,11 @@ func (g *fakeGH) recentPulls(base string, page int) ([]pullRequest, error) { func (g *fakeGH) batchBot() string { return g.bot } func (g *fakeGH) comments(int) ([]issueComment, error) { + defer func() { + if f := g.after["comments"]; f != nil { + f() + } + }() return slices.Clone(g.issueComments), nil } @@ -303,6 +353,15 @@ func (g *fakeGH) createComment(number int, bodyFile string) error { if err := g.write(number, "a comment"); err != nil { return err } + if err := g.inject("createComment"); err != nil { + return err + } + g.commented = append(g.commented, number) + defer func() { + if f := g.after["createComment"]; f != nil { + f() + } + }() data, err := os.ReadFile(bodyFile) var c issueComment c.ID, c.Body = int64(1000+len(g.issueComments)), string(data) @@ -346,23 +405,56 @@ func (g *fakeGH) openPulls(base string) ([]pullRequest, error) { var out []pullRequest for _, pr := range g.prs { if pr.Base.Ref == base && (pr.State == "" || pr.State == "open") { - out = append(out, pr) + out = append(out, g.live(pr)) } } return out, g.err } -func (g *fakeGH) createPull(base, head, title, bodyFile string) (string, error) { +// createPull opens the pull request on the fake, as the maintainer, at the +// head the remote has, numbered nextPR. +func (g *fakeGH) createPull(base, head, title, bodyFile string) (int, string, error) { + if err := g.inject("createPull"); err != nil { + return 0, "", err + } g.created = append(g.created, base+" "+head+" "+title) - return fmt.Sprintf("https://github.com/%s/pull/90", upstreamRepo), g.readBody(bodyFile) + if err := g.readBody(bodyFile); err != nil { + return 0, "", err + } + n := cmp.Or(g.nextPR, 90) + var pr pullRequest + pr.Number, pr.Body, pr.State = n, g.bodies[len(g.bodies)-1], "open" + pr.HTMLURL = fmt.Sprintf("https://github.com/%s/pull/%d", upstreamRepo, n) + pr.User.Login, pr.User.Type = "maintainer", "User" + pr.Head.Ref = head + if g.remoteHead != nil { + pr.Head.SHA = g.remoteHead(head) + } + pr.Head.Repo = &struct { + FullName string `json:"full_name"` + }{upstreamRepo} + pr.Base.Ref, pr.Base.Repo.FullName = base, upstreamRepo + g.prs = append(slices.DeleteFunc(g.prs, func(p pullRequest) bool { return p.Number == n }), pr) + return n, pr.HTMLURL, nil } func (g *fakeGH) editPull(number int, title, bodyFile string) error { if err := g.write(number, "an edit"); err != nil { return err } + if err := g.inject("editPull"); err != nil { + return err + } g.edited = append(g.edited, fmt.Sprintf("#%d %s", number, title)) - return g.readBody(bodyFile) + if err := g.readBody(bodyFile); err != nil { + return err + } + for i := range g.prs { + if g.prs[i].Number == number { + g.prs[i].Body = g.bodies[len(g.bodies)-1] + } + } + return nil } func (g *fakeGH) readBody(path string) error { @@ -423,8 +515,11 @@ func (f *fixture) exec(gh *fakeGH, args ...string) (int, string, string) { if gh == nil { gh = &fakeGH{} } + if gh.remoteHead == nil { + gh.remoteHead = f.remoteHead + } var stdout, stderr bytes.Buffer - a := &app{gh: gh, stdout: &stdout, stderr: &stderr, now: func() time.Time { return f.now }} + a := &app{gh: gh, stdout: &stdout, stderr: &stderr, now: func() time.Time { return f.now }, tool: f.tool} written := len(gh.closedWrites) code := a.main(args) if len(gh.closedWrites) > written { diff --git a/cmd/backport-batch/github.go b/cmd/backport-batch/github.go index 3a7fa39d4..b1f3b5ce3 100644 --- a/cmd/backport-batch/github.go +++ b/cmd/backport-batch/github.go @@ -52,7 +52,7 @@ var ( batchBranchRE = regexp.MustCompile(`^backport-batch/(release-\d+\.\d+)-(\d{8})$`) loginRE = regexp.MustCompile(`^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?$`) botLoginRE = regexp.MustCompile(`^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?\[bot\]$`) - pullURLRE = regexp.MustCompile(`^https://[^/\s]+/[^/\s]+/[^/\s]+/pull/[1-9][0-9]*$`) + pullURLRE = regexp.MustCompile(`^https://[^/\s]+/[^/\s]+/[^/\s]+/pull/([1-9][0-9]{0,8})$`) ) // batchBotEnv names the GitHub App bot the scheduled batch opens its pull @@ -122,8 +122,10 @@ type github interface { permission(login string) (string, error) // labelled lists the pull requests merged into main that carry label. labelled(label string) ([]int, error) - // createPull opens the draft and returns its URL. - createPull(base, head, title, bodyFile string) (string, error) + // pull reads one pull request as it is now. + pull(number int) (pullRequest, error) + // createPull opens the draft and returns its number and URL. + createPull(base, head, title, bodyFile string) (int, string, error) editPull(number int, title, bodyFile string) error comments(number int) ([]issueComment, error) createComment(number int, bodyFile string) error @@ -223,16 +225,33 @@ func (ghCLI) labelled(label string) ([]int, error) { return numbers, nil } -func (ghCLI) createPull(base, head, title, bodyFile string) (string, error) { +func (ghCLI) pull(number int) (pullRequest, error) { + var pr pullRequest + out, err := capture("gh", "api", "repos/{owner}/{repo}/pulls/"+strconv.Itoa(number)) + if err != nil { + return pr, err + } + if err := json.Unmarshal([]byte(out), &pr); err != nil { + return pr, fmt.Errorf("reading pull request #%d: %w", number, err) + } + if pr.Number != number { + return pr, fmt.Errorf("asked for pull request #%d, GitHub answered with #%d", number, pr.Number) + } + return pr, nil +} + +func (ghCLI) createPull(base, head, title, bodyFile string) (int, string, error) { out, err := capture("gh", "pr", "create", "--draft", "--base", base, "--head", head, "--title", title, "--body-file", bodyFile) if err != nil { - return "", err + return 0, "", err } url := strings.TrimSpace(out) - if !pullURLRE.MatchString(url) { - return "", fmt.Errorf("gh pr create printed %q, not the URL of a pull request", url) + m := pullURLRE.FindStringSubmatch(url) + if m == nil { + return 0, "", fmt.Errorf("gh pr create printed %q, not the URL of a pull request", url) } - return url, nil + n, err := strconv.Atoi(m[1]) + return n, url, err } func (ghCLI) editPull(number int, title, bodyFile string) error { diff --git a/cmd/backport-batch/main.go b/cmd/backport-batch/main.go index b921656f7..98ed2b2ac 100644 --- a/cmd/backport-batch/main.go +++ b/cmd/backport-batch/main.go @@ -38,6 +38,7 @@ Usage: backport-batch fold --line release-X.Y --original N --commit SHA [--policy REF] backport-batch verify --line release-X.Y [--head SHA] backport-batch notify --line release-X.Y [--mention org/team|login] [--push] + backport-batch report --pr N [--id ID] [--json] backport-batch lines [--policy REF] run Builds backport-batch/- in this working tree, off @@ -69,11 +70,15 @@ notify Prints what waits on the open batch PR for a maintainer: a batch open for 3 days or more, its deferred conflicts, and the originals that change workflows. --push keeps one comment on the PR that says what waits, mentioning --mention. +report Prints a report batch PR N keeps: the one its body carries, or the + one --id names, read from its comments or its body. --json prints the + report as it is stored. It writes nothing, so it reads merged and + closed batches too. lines Prints the release lines the batch runs for, newest first. The policy is docs/release/backports.md. run, check, fold and lines read its policy block from that page on origin/main, or on --policy REF. Every command -but notify, which reads only GitHub, fetches origin first. +but notify and report, which read only GitHub, fetches origin first. BACKPORT_BATCH_BOT names the bot the scheduled batch writes as, [bot], cozystack-ci[bot] when unset: a batch pull request or a notify @@ -93,10 +98,12 @@ type app struct { now func() time.Time // policyRef is where the policy page is read from; origin/main when empty. policyRef string + // tool is the commit backport-batch was built from, for the record. + tool string } func main() { - a := &app{gh: ghCLI{bot: batchBotLogin(os.LookupEnv)}, stdout: os.Stdout, stderr: os.Stderr, now: time.Now} + a := &app{gh: ghCLI{bot: batchBotLogin(os.LookupEnv)}, stdout: os.Stdout, stderr: os.Stderr, now: time.Now, tool: toolVersion()} os.Exit(a.main(os.Args[1:])) } @@ -121,6 +128,8 @@ func (a *app) main(args []string) int { return a.verify(args[1:]) case "notify": return a.notify(args[1:]) + case "report": + return a.report(args[1:]) case "lines": return a.lines(args[1:]) case "-h", "--help", "help": @@ -199,11 +208,15 @@ type batch struct { newer *line lineHead string mainHead string - base string - other []string - verdicts []*verdict - pr *pullRequest - prHead string + // newerHead is the newer line's head, and policyAt the commit the + // policy was read from. + newerHead string + policyAt string + base string + other []string + verdicts []*verdict + pr *pullRequest + prHead string // picks is what the open batch pull request carries beyond the line. picks *history // src is the line's record and where it was found. @@ -229,18 +242,20 @@ func fetch() error { return err } -func (a *app) policy(mainHead string) (*policy, error) { +// policy reads the policy and says from which commit. +func (a *app) policy(mainHead string) (*policy, string, error) { at := mainHead if a.policyRef != "" { if strings.HasPrefix(a.policyRef, "-") { - return nil, fmt.Errorf("--policy %q is not a ref", a.policyRef) + return nil, "", fmt.Errorf("--policy %q is not a ref", a.policyRef) } var err error if at, err = resolve(a.policyRef); err != nil { - return nil, err + return nil, "", err } } - return readPolicy(at) + pol, err := readPolicy(at) + return pol, at, err } // prepare reads the line, main and the open batch pull request, each pinned to @@ -251,7 +266,7 @@ func (a *app) prepare(l line) (*batch, error) { if b.mainHead, err = resolve("refs/remotes/" + remote + "/main"); err != nil { return nil, err } - pol, err := a.policy(b.mainHead) + pol, policyAt, err := a.policy(b.mainHead) if err != nil { return nil, err } @@ -265,7 +280,7 @@ func (a *app) prepare(l line) (*batch, error) { if b.lineHead, err = resolve("refs/remotes/" + remote + "/" + l.branch()); err != nil { return nil, err } - b.start, b.textUntil = pol.starts[l.branch()], pol.batchOnly + b.start, b.textUntil, b.policyAt = pol.starts[l.branch()], pol.batchOnly, policyAt out, err := git("merge-base", "--end-of-options", b.mainHead, b.lineHead) if err != nil { return nil, fmt.Errorf("%s shares no history with main: %w", l.branch(), err) @@ -282,11 +297,10 @@ func (a *app) prepare(l line) (*batch, error) { } var newerHist *history if b.newer != nil { - newerHead, err := resolve("refs/remotes/" + remote + "/" + b.newer.branch()) - if err != nil { + if b.newerHead, err = resolve("refs/remotes/" + remote + "/" + b.newer.branch()); err != nil { return nil, err } - if newerHist, err = readHistory(newerHead, b.mainHead, b.textUntil); err != nil { + if newerHist, err = readHistory(b.newerHead, b.mainHead, b.textUntil); err != nil { return nil, err } } diff --git a/cmd/backport-batch/notify.go b/cmd/backport-batch/notify.go index 80c5aa1da..7191f42b4 100644 --- a/cmd/backport-batch/notify.go +++ b/cmd/backport-batch/notify.go @@ -182,6 +182,9 @@ func (a *app) comment(pr *pullRequest, mention string, today time.Time, text []s if err := f.Close(); err != nil { return a.fail(err) } + if err := a.stillOpen(pr.Number); err != nil { + return a.fail(err) + } if ours != nil { err = a.gh.editComment(ours.ID, f.Name()) } else { @@ -207,7 +210,7 @@ func (a *app) lines(args []string) int { if err != nil { return a.fail(err) } - pol, err := a.policy(mainHead) + pol, _, err := a.policy(mainHead) if err != nil { return a.fail(err) } diff --git a/cmd/backport-batch/notify_test.go b/cmd/backport-batch/notify_test.go index 6b91a4159..4bfb650e9 100644 --- a/cmd/backport-batch/notify_test.go +++ b/cmd/backport-batch/notify_test.go @@ -313,3 +313,24 @@ func TestRunWithNothingToPick(t *testing.T) { t.Errorf("pushed %q and opened %q with nothing picked", f.remoteHead(batchBranch1), gh.created) } } + +// notify writes its comment only after GitHub says the batch is still open, +// so a batch closed while notify reads it is not written to, whether the +// comment would be new or an update of its own. +func TestNotifyWritesNothingOnABatchClosedMeanwhile(t *testing.T) { + for _, update := range []bool{false, true} { + t.Run(map[bool]string{false: "a new comment", true: "an update"}[update], func(t *testing.T) { + gh := &fakeGH{prs: []pullRequest{notifyPR("20261023", recordBody(5))}} + if update { + ours := issueComment{ID: 7, Body: notifyMarker + "\nearlier"} + ours.User.Login, ours.User.Type = "maintainer", "User" + gh.issueComments = []issueComment{ours} + } + gh.after = map[string]func(){"comments": func() { gh.find(90).State = "closed" }} + code, _, stderr := notifyRun(gh, "--push") + if code != 2 || !strings.Contains(stderr, "batch pull request #90 is closed now; nothing is written to it") || len(gh.closedWrites) != 0 || update && gh.issueComments[0].Body != notifyMarker+"\nearlier" { + t.Errorf("notify = %d, closed writes %q, comments %+v; want 2 and nothing written:\n%s", code, gh.closedWrites, gh.issueComments, stderr) + } + }) + } +} diff --git a/cmd/backport-batch/overrides_test.go b/cmd/backport-batch/overrides_test.go index 740713359..a759fd59c 100644 --- a/cmd/backport-batch/overrides_test.go +++ b/cmd/backport-batch/overrides_test.go @@ -238,16 +238,26 @@ func TestGHCLI(t *testing.T) { // A batch pull request carries no label: the release workflows act on a // pull request cozystack-ci[bot] opened only when it has the release label. write("https://github.com/cozystack/cozystack/pull/5300\n") - if url, err := (ghCLI{}).createPull("release-1.7", batchBranch1, "fix(backport): batch", "/tmp/body.md"); err != nil || url != "https://github.com/cozystack/cozystack/pull/5300" { - t.Errorf("createPull = %q, %v; want the URL gh printed", url, err) + if n, url, err := (ghCLI{}).createPull("release-1.7", batchBranch1, "fix(backport): batch", "/tmp/body.md"); err != nil || n != 5300 || url != "https://github.com/cozystack/cozystack/pull/5300" { + t.Errorf("createPull = %d, %q, %v; want the pull request gh printed", n, url, err) } if want := "pr create --draft --base release-1.7 --head " + batchBranch1 + " --title fix(backport): batch --body-file /tmp/body.md"; called() != want { t.Errorf("gh %q, want gh %q", called(), want) } write("Warning: 1 uncommitted change\n") - if _, err := (ghCLI{}).createPull("release-1.7", batchBranch1, "fix(backport): batch", "/tmp/body.md"); err == nil { + if _, _, err := (ghCLI{}).createPull("release-1.7", batchBranch1, "fix(backport): batch", "/tmp/body.md"); err == nil { t.Error("createPull took output that is no pull request URL") } + write(`{"number":90,"state":"closed","merged_at":"2026-10-26T12:00:00Z","head":{"ref":"x","sha":"y"}}`) + if pr, err := (ghCLI{}).pull(90); err != nil || pr.State != "closed" || pr.MergedAt == nil { + t.Errorf("pull = %+v, %v; want #90 merged", pr, err) + } + if want := "api repos/{owner}/{repo}/pulls/90"; called() != want { + t.Errorf("gh %q, want gh %q", called(), want) + } + if _, err := (ghCLI{}).pull(91); err == nil { + t.Error("pull took GitHub's answer about another pull request") + } write("") if err := (ghCLI{}).editPull(90, "fix(backport): batch", "/tmp/body.md"); err != nil { t.Fatal(err) diff --git a/cmd/backport-batch/record.go b/cmd/backport-batch/record.go index 648b000ba..4f504ee55 100644 --- a/cmd/backport-batch/record.go +++ b/cmd/backport-batch/record.go @@ -285,6 +285,27 @@ func (b *builtFrom) validate() error { var runURLRE = regexp.MustCompile(`^https://[^/\s]+/[^/\s]+/[^/\s]+/actions/runs/[0-9]+(/attempts/[0-9]+)?$`) +// recordBlock is the block that holds rec. One line of JSON: encoding/json +// escapes newlines and <, > and &, so nothing in a file name or a reason can +// end the fence or open HTML. +func recordBlock(rec lineRecord) string { + data, _ := json.Marshal(rec) + return deferredFence + "\n" + string(data) + "\n```" +} + +// savedNoWorkflowFiles is the --no-workflow-files the open batch was last +// assembled with, which an operation that only changes the record keeps. A +// record from before it was saved says so by listing a wait. +func (s recordSource) savedNoWorkflowFiles() bool { + if !s.open || s.rec == nil { + return false + } + if s.rec.Built != nil { + return s.rec.Built.Options.NoWorkflowFiles + } + return slices.ContainsFunc(s.rec.Deferred, func(d deferredEntry) bool { return d.Workflows }) +} + // deferred maps the originals the record defers to their entries. func (rec *lineRecord) deferred() map[int]deferredEntry { out := map[int]deferredEntry{} diff --git a/cmd/backport-batch/record_test.go b/cmd/backport-batch/record_test.go index b512a2cda..830685474 100644 --- a/cmd/backport-batch/record_test.go +++ b/cmd/backport-batch/record_test.go @@ -242,3 +242,77 @@ func TestRecordAsksForAPermissionOnce(t *testing.T) { t.Errorf("asked GitHub for %q, want the author once", gh.asked) } } + +// The record and the report name what the classification was made of: the +// heads it read, the policy, the tool, the job, the time and the flags. +func TestRecordNamesItsInputs(t *testing.T) { + f := newFixture(t) + f.lines() + mc5 := f.pr(5, "fix(app): five", "", map[string]string{"app/five.txt": "5\n"}) + f.onLine("release-1.1", "fix(app): five\n\n(cherry picked from commit "+mc5+")", map[string]string{"app/five.txt": "5\n"}) + f.tool = strings.Repeat("c", 40) + "+dirty" + t.Setenv("GITHUB_SERVER_URL", "https://github.com") + t.Setenv("GITHUB_REPOSITORY", "cozystack/cozystack") + t.Setenv("GITHUB_RUN_ID", "123") + t.Setenv("GITHUB_RUN_ATTEMPT", "2") + gh := &fakeGH{} + stdout := mustRun(t, f, gh, "run", "--line", "release-1.0", "--no-workflow-files", "--push") + l, _ := parseLine("release-1.0") + const branch = "backport-batch/release-1.0-20261026" + rec, err := parseRecord(stdout, l) + if err != nil { + t.Fatal(err) + } + want := builtFrom{Main: f.git("rev-parse", "origin/main"), Line: f.git("rev-parse", "origin/release-1.0"), Newer: f.git("rev-parse", "origin/release-1.1"), + Policy: f.git("rev-parse", "origin/main"), Batch: f.remoteHead(branch), Tool: f.tool, Run: "https://github.com/cozystack/cozystack/actions/runs/123/attempts/2", + At: "2026-10-26T06:00:00Z", Options: runOptions{NoWorkflowFiles: true, Mode: modeNew}} + if rec.Version != 2 || *rec.Built != want || bodyReportOf(t, stdout, l).doc.Built != want || rec.Report != bodyReportOf(t, stdout, l).doc.ID { + t.Errorf("record = %+v, built %+v; want built %+v and the report it names", rec, rec.Built, want) + } + + // A policy from another ref, a run by hand that continues the batch. + f.git("push", "-q", "origin", "origin/main:refs/heads/policy-draft") + f.git("fetch", "-q", "origin") + t.Setenv("GITHUB_RUN_ID", "") + gh.prs = []pullRequest{f.batchPR(90, "release-1.0", branch, stdout)} + mc6 := f.pr(6, "fix(app): six", "", map[string]string{"app/six.txt": "6\n"}) + f.onLine("release-1.1", "fix(app): six\n\n(cherry picked from commit "+mc6+")", map[string]string{"app/six.txt": "6\n"}) + stdout = mustRun(t, f, gh, "run", "--line", "release-1.0", "--continue", "--policy", "origin/policy-draft", "--push") + if rec, err = parseRecord(stdout, l); err != nil { + t.Fatal(err) + } + if b := rec.Built; b.Policy != f.git("rev-parse", "origin/policy-draft") || b.Main != f.git("rev-parse", "origin/main") || b.Run != "" || b.Options != (runOptions{Mode: modeContinue}) || b.Batch != f.remoteHead(branch) { + t.Errorf("built %+v; want the draft's policy, no job and a continuation without the flag", b) + } +} + +// What a run assembled with --no-workflow-files stays saved through the +// operations that only change the record, which take no such flag: check +// keeps failing the cut on the wait they leave in place. +func TestMetadataOperationsKeepTheMode(t *testing.T) { + for _, v1 := range []bool{false, true} { + t.Run(fmt.Sprintf("an open record of version 1: %v", v1), func(t *testing.T) { + f := workflowWorld(t) + f.pr(7, "fix(app): seven", "", map[string]string{"app/seven.txt": "7\n"}) + f.pr(8, "fix(app): eight", "", map[string]string{"app/eight.txt": "8\n"}) + gh := &fakeGH{} + stdout := mustRun(t, f, gh, "run", "--line", "release-1.1", "--no-workflow-files", "--push") + reopen(f, gh) + if v1 { + waits := []pick{{v: &verdict{o: &original{number: 5}}, files: []string{".github/workflows/five.yaml"}}} + gh.prs[0].Body = strings.Replace(stdout, strings.Split(strings.Split(stdout, deferredFence+"\n")[1], "\n")[0], strings.Split(deferredBlock(notifyLine, nil, nil, waits), "\n")[1], 1) + } else if rec, err := parseRecord(stdout, notifyLine); err != nil || !rec.Built.Options.NoWorkflowFiles { + t.Fatalf("the scheduled run saved %+v, %v", rec, err) + } + stdout = mustRun(t, f, gh, "run", "--line", "release-1.1", "--defer", "7", "--reason", "fails the e2e", "--push") + rec, err := parseRecord(stdout, notifyLine) + if err != nil || rec.Built.Options != (runOptions{NoWorkflowFiles: true, Mode: modeDefer}) { + t.Errorf("after --defer the record is %+v, %v; want the flag kept", rec.Built, err) + } + reopen(f, gh) + if code, stdout, _ := f.exec(gh, "check", "--line", "release-1.1", "--cut"); code != 1 || !strings.Contains(stdout, "#5 changes .github/workflows/") { + t.Errorf("check --cut = %d, want #5 still waiting:\n%s", code, stdout) + } + }) + } +} diff --git a/cmd/backport-batch/report.go b/cmd/backport-batch/report.go new file mode 100644 index 000000000..78c460479 --- /dev/null +++ b/cmd/backport-batch/report.go @@ -0,0 +1,734 @@ +/* +Copyright 2026 The Cozystack Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "cmp" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "os" + "regexp" + "runtime/debug" + "slices" + "strconv" + "strings" + "time" +) + +// A report is one classification of a line: where every original merged into +// main since the line branched stands, and what it was made of. The batch +// pull request's body carries the latest one beside the record, and an issue +// comment the tool never edits keeps each one, so a later run that replaces +// the body loses none of them. Its ID hashes the classification, not the pull +// request it lands on, which a new batch does not have yet when the report is +// made. + +const ( + reportFence = "```backport-batch-report" + reportMarker = "\n" + + "
Backport batch report for " + s.doc.Line + ", " + date + " (" + s.doc.ID[:12] + ")\n\n" + + reportBlock(s.raw) + "\n
\n" + if len(c) > reportLimit { + return "", fmt.Errorf("the report comment is %d bytes, over the %d it may have", len(c), reportLimit) + } + return c, nil +} + +var reportMarkerRE = regexp.MustCompile(`^` + regexp.QuoteMeta(reportMarker) + `([0-9a-f]{64}) -->\n`) + +// findReports reads the report comments on batch pull request n, against l, +// by authors the batch trusts, as it trusts a record's author, by ID. One +// whose content does not match the ID its marker gives, or is not what a run +// writes, is returned apart, with why; a look-alike by anyone else is not +// read, whatever it says. +func findReports(gh github, n int, l line) (map[string]storedReport, map[string]string, error) { + comments, err := gh.comments(n) + if err != nil { + return nil, nil, err + } + found, bad := map[string]storedReport{}, map[string]string{} + for _, c := range comments { + if !strings.HasPrefix(c.Body, reportMarker) { + continue + } + why, err := trustedAuthor(gh, c.User.Login, c.User.Type) + if err != nil { + return nil, nil, err + } + if why != "" { + continue + } + m := reportMarkerRE.FindStringSubmatch(c.Body) + if m == nil { + bad[strconv.FormatInt(c.ID, 10)] = fmt.Sprintf("comment %d starts like a report but names none", c.ID) + continue + } + blocks, err := fencedLines(c.Body, reportFence) + if err == nil && len(blocks) != 1 { + err = fmt.Errorf("it holds %d reports", len(blocks)) + } + var doc reportDoc + if err == nil { + doc, err = parseReport(blocks[0], l) + } + if err == nil && doc.ID != m[1] { + err = fmt.Errorf("its marker names %.12s", m[1]) + } + if err != nil { + bad[m[1]] = fmt.Sprintf("comment %d: %v", c.ID, err) + continue + } + found[m[1]] = storedReport{doc: doc, raw: blocks[0]} + } + return found, bad, nil +} + +// toolVersion is the commit this binary was built from, with +dirty when its +// tree had changes, or "" when the build did not record it, as go run does +// not. +func toolVersion() string { + info, ok := debug.ReadBuildInfo() + if !ok { + return "" + } + rev, dirty := "", false + for _, s := range info.Settings { + switch s.Key { + case "vcs.revision": + rev = s.Value + case "vcs.modified": + dirty = s.Value == "true" + } + } + if checkSHA(rev) != nil { + return "" + } + if dirty { + rev += "+dirty" + } + return rev +} + +// runURL is the Actions run the command runs in, from the variables GitHub +// sets for it, or "" outside one. +func runURL(lookup func(string) (string, bool)) string { + get := func(key string) string { v, _ := lookup(key); return v } + url := get("GITHUB_SERVER_URL") + "/" + get("GITHUB_REPOSITORY") + "/actions/runs/" + get("GITHUB_RUN_ID") + if attempt := get("GITHUB_RUN_ATTEMPT"); attempt != "" && attempt != "1" { + url += "/attempts/" + attempt + } + if !runURLRE.MatchString(url) { + return "" + } + return url +} + +// report prints a report a batch pull request keeps, reading GitHub alone and +// writing nothing, so it works on merged and closed batches too. +func (a *app) report(args []string) int { + fs := a.flags("report") + number := fs.Int("pr", 0, "the batch pull request") + id := fs.String("id", "", "the report, by its ID (default: the one the body carries)") + asJSON := fs.Bool("json", false, "print the report's JSON") + if err := parse(fs, args); err != nil { + return 2 + } + if *number <= 0 { + return a.fail(fmt.Errorf("--pr %d is not a pull request number", *number)) + } + if *id != "" && !reportIDRE.MatchString(*id) { + return a.fail(fmt.Errorf("--id %q is not a report ID", *id)) + } + pr, err := a.gh.pull(*number) + if err != nil { + return a.fail(err) + } + l, err := parseLine(pr.Base.Ref) + if m := batchBranchRE.FindStringSubmatch(pr.Head.Ref); err != nil || m == nil || m[1] != l.branch() || pr.Head.Repo == nil || pr.Head.Repo.FullName != pr.Base.Repo.FullName { + return a.fail(fmt.Errorf("#%d is not a batch pull request of this repository", pr.Number)) + } + why, err := trusted(a.gh, pr) + if err != nil { + return a.fail(err) + } + if why != "" { + return a.fail(fmt.Errorf("batch pull request #%d was %s, so what it says is not the batch's", pr.Number, why)) + } + inBody, err := bodyReport(pr.Body, l) + if err != nil { + return a.fail(fmt.Errorf("batch pull request #%d: %w", pr.Number, err)) + } + if *id == "" { + if inBody == nil { + return a.fail(fmt.Errorf("batch pull request #%d carries no report in its body; name one with --id", pr.Number)) + } + *id = inBody.doc.ID + } + posted, bad, err := findReports(a.gh, pr.Number, l) + if err != nil { + return a.fail(err) + } + if why := bad[*id]; why != "" { + return a.fail(fmt.Errorf("batch pull request #%d: the report %.12s is not what its comment says: %s", pr.Number, *id, why)) + } + s, inComment := posted[*id] + switch { + case inComment: + case inBody != nil && inBody.doc.ID == *id: + s = *inBody + default: + return a.fail(fmt.Errorf("batch pull request #%d keeps no report %.12s", pr.Number, *id)) + } + if *asJSON { + fmt.Fprintln(a.stdout, s.raw) + return 0 + } + a.printReport(&pr, s, inBody, inComment) + return 0 +} + +// printReport prints a report. It was checked when it was read, and every +// field still goes through terminalSafe, so nothing it carries spans lines +// or drives a terminal. +func (a *app) printReport(pr *pullRequest, s storedReport, inBody *storedReport, inComment bool) { + d, b := s.doc, s.doc.Built + safe := terminalSafe + fmt.Fprintf(a.stdout, "report %s for %s, made %s by a %s run", safe(d.ID), safe(d.Line), safe(b.At), safe(b.Options.Mode)) + if b.Options.NoWorkflowFiles { + fmt.Fprint(a.stdout, " with --no-workflow-files") + } + fmt.Fprintln(a.stdout) + fmt.Fprintf(a.stdout, " main %s, line %s, newer line %s, policy %s\n batch %s, tool %s, run %s\n", safe(b.Main), safe(b.Line), safe(or(b.Newer, "none")), safe(b.Policy), safe(b.Batch), safe(or(b.Tool, "unknown")), safe(or(b.Run, "none"))) + current := "none" + if inBody != nil { + current = safe(inBody.doc.ID) + } + switch { + case inBody != nil && inBody.doc.ID == d.ID: + fmt.Fprintf(a.stdout, " #%d's body carries this report\n", pr.Number) + case pr.Head.SHA == b.Batch: + fmt.Fprintf(a.stdout, " #%d's head is %.12s, the batch this report describes, but its body carries report %.12s\n", pr.Number, safe(b.Batch), current) + default: + fmt.Fprintf(a.stdout, " publication unconfirmed: #%d's head is %.12s, not %.12s, and its body carries report %.12s\n", pr.Number, safe(pr.Head.SHA), safe(b.Batch), current) + } + if !inComment { + if prState(pr) == "open" { + fmt.Fprintln(a.stdout, " comment not posted; the next backport-batch run --push posts it") + } else { + fmt.Fprintf(a.stdout, " comment not posted; kept in the body only, since nothing is written to a %s pull request\n", prState(pr)) + } + } + type line struct { + n int + text string + } + var lines []line + for _, n := range d.Primary.Picked { + lines = append(lines, line{n, "picked"}) + } + for _, n := range d.Primary.Missing { + lines = append(lines, line{n, "missing"}) + } + for _, e := range d.Primary.Deferred { + text := "deferred: " + switch { + case e.Workflows: + text += "waits for a maintainer, changes " + strings.Join(e.Files, ", ") + case e.Reason != "": + text += e.Reason + default: + text += fmt.Sprintf("conflict at %.12s in %s", e.Commit, strings.Join(e.Files, ", ")) + } + if e.More > 0 { + text += fmt.Sprintf(" and %d more", e.More) + } + lines = append(lines, line{e.Original, text}) + } + for _, l := range d.Primary.Skipped.lists() { + for _, n := range *l.list { + lines = append(lines, line{n, "skipped." + l.name}) + } + } + slices.SortFunc(lines, func(x, y line) int { return cmp.Compare(x.n, y.n) }) + for _, l := range lines { + fmt.Fprintf(a.stdout, "#%d %s\n", l.n, terminalSafe(l.text)) + } + overlay := func(name string, list []int) { + if len(list) > 0 { + fmt.Fprintf(a.stdout, "%s: %s\n", name, strings.Trim(fmt.Sprint(list), "[]")) + } + } + overlay("stale, still on the batch", d.Overlays.Stale) + for _, e := range d.Overlays.Dropped { + fmt.Fprintf(a.stdout, "dropped: #%d, merged as %.12s, on #%d: %s\n", e.Original, terminalSafe(e.Merge), e.On, terminalSafe(e.Reason)) + } + overlay("changed in the pick", d.Overlays.Changed) + if len(d.Other) > 0 { + other := make([]string, len(d.Other)) + for i, sha := range d.Other { + other[i] = safe(sha) + } + fmt.Fprintf(a.stdout, "commits on main that merged no pull request: %s\n", strings.Join(other, ", ")) + } +} + +func or(s, otherwise string) string { + if s == "" { + return otherwise + } + return s +} + +// postReport writes a report comment on pull request n, after GitHub says +// it is still open: nothing is written to a merged or closed one. +func (a *app) postReport(n int, s storedReport) error { + if err := a.stillOpen(n); err != nil { + return err + } + c, err := reportComment(s) + if err != nil { + return err + } + name, err := writeTemp("backport-batch-report-*.md", c) + if err != nil { + return err + } + defer os.Remove(name) + return a.gh.createComment(n, name) +} + +// stillOpen reads pull request n again and refuses to go on unless it is +// open. +func (a *app) stillOpen(n int) error { + pr, err := a.gh.pull(n) + if err != nil { + return err + } + if st := prState(&pr); st != "open" { + return fmt.Errorf("batch pull request #%d is %s now; nothing is written to it", n, st) + } + return nil +} + +func writeTemp(pattern, content string) (string, error) { + f, err := os.CreateTemp("", pattern) + if err != nil { + return "", err + } + if _, err := f.WriteString(content); err != nil { + f.Close() + os.Remove(f.Name()) + return "", err + } + if err := f.Close(); err != nil { + os.Remove(f.Name()) + return "", err + } + return f.Name(), nil +} diff --git a/cmd/backport-batch/report_test.go b/cmd/backport-batch/report_test.go new file mode 100644 index 000000000..a0b2942ff --- /dev/null +++ b/cmd/backport-batch/report_test.go @@ -0,0 +1,687 @@ +/* +Copyright 2026 The Cozystack Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + "unicode" +) + +// reportsPosted lists the report IDs the comments carry, in the order they +// were posted, whoever wrote them. +func reportsPosted(gh *fakeGH) []string { + var ids []string + for _, c := range gh.issueComments { + if m := reportMarkerRE.FindStringSubmatch(c.Body); m != nil { + ids = append(ids, m[1]) + } + } + return ids +} + +// bodyReportOf is the report a body on release-1.1, or on the line given, +// carries, failing the test without one. +func bodyReportOf(t *testing.T, body string, on ...line) storedReport { + t.Helper() + l := notifyLine + if len(on) > 0 { + l = on[0] + } + s, err := bodyReport(body, l) + if err != nil || s == nil { + t.Fatalf("the body carries no readable report: %v\n%s", err, body) + } + return *s +} + +func (g *fakeGH) find(n int) *pullRequest { + for i := range g.prs { + if g.prs[i].Number == n { + return &g.prs[i] + } + } + return nil +} + +// refusePushes makes the remote refuse every push, as a lost connection or +// a protected branch would, until it is called with false. +func (f *fixture) refusePushes(on bool) { + f.t.Helper() + hook := filepath.Join(f.remote, "hooks", "pre-receive") + if !on { + if err := os.Remove(hook); err != nil { + f.t.Fatal(err) + } + return + } + if err := os.WriteFile(hook, []byte("#!/bin/sh\necho refused >&2\nexit 1\n"), 0o755); err != nil { + f.t.Fatal(err) + } +} + +func mustRun(t *testing.T, f *fixture, gh *fakeGH, args ...string) string { + t.Helper() + code, stdout, stderr := f.exec(gh, args...) + if code != 0 { + t.Fatalf("%q = %d\n%s", args, code, stderr) + } + f.git("switch", "-q", "main") + return stdout +} + +// A report comment keeps each classification: one the next run replaced in +// the body is still read back by its ID, also once the batch merged, which +// nothing writes to. +func TestReportSurvivesContinuation(t *testing.T) { + f := newFixture(t) + f.lines() + f.onLine("release-1.1", "fix(app): a line-only five", map[string]string{"app/five.txt": "line\n"}) + mc5 := f.pr(5, "fix(app): five", "", map[string]string{"app/five.txt": "5\n"}) + f.pr(6, "fix(app): six", "", map[string]string{"app/six.txt": "6\n"}) + gh := &fakeGH{} + openBatch(t, f, gh) + r1 := bodyReportOf(t, gh.prs[0].Body).doc.ID + f.onLine("release-1.1", "fix(app): five, by hand\n\n(cherry picked from commit "+mc5+")", map[string]string{"app/five.txt": "5\n"}) + f.now = f.now.AddDate(0, 0, 1) + mustRun(t, f, gh, "run", "--line", "release-1.1", "--continue", "--push") + r2 := bodyReportOf(t, gh.find(90).Body).doc.ID + if r1 == r2 || !slices.Equal(reportsPosted(gh), []string{r1, r2}) { + t.Fatalf("reports %q; want R1 %.12s, then R2 %.12s", reportsPosted(gh), r1, r2) + } + read := func(id, want string) { + t.Helper() + code, stdout, stderr := f.exec(gh, "report", "--pr", "90", "--id", id) + if code != 0 || !strings.Contains(stdout, want) { + t.Errorf("report --id %.12s = %d, want %q:\n%s%s", id, code, want, stdout, stderr) + } + } + for _, merged := range []bool{false, true} { + if merged { + mergeBatch(f, gh) + } + read(r1, "#5 deferred: conflict at ") + read(r2, "#5 skipped.on-line") + read(r2, "#6 picked") + } +} + +// Two runs that find the same post one report, and the body carries the one +// kept, whatever time and job the second ran in. A look-alike someone else +// wrote is no report, and a kept report edited since stops the reader. +func TestReportDedupAndTrust(t *testing.T) { + f := newFixture(t) + f.lines() + f.pr(5, "fix(app): five", "", map[string]string{"app/five.txt": "5\n"}) + f.pr(6, "fix(app): six", "", map[string]string{"app/six.txt": "6\n"}) + gh := &fakeGH{} + openBatch(t, f, gh) + t.Setenv("GITHUB_SERVER_URL", "https://github.com") + t.Setenv("GITHUB_REPOSITORY", "cozystack/cozystack") + t.Setenv("GITHUB_RUN_ID", "100") + + // The report the next run makes, read off a run that publishes nothing, + // planted by someone the batch does not trust. + preview := bodyReportOf(t, mustRun(t, f, gh, "run", "--line", "release-1.1", "--continue")) + planted, err := reportComment(preview) + if err != nil { + t.Fatal(err) + } + look := issueComment{ID: 7, Body: planted} + look.User.Login, look.User.Type = "someone", "User" + gh.issueComments = append(gh.issueComments, look) + + f.now = f.now.AddDate(0, 0, 1) + mustRun(t, f, gh, "run", "--line", "release-1.1", "--continue", "--push") + kept := bodyReportOf(t, gh.find(90).Body) + if kept.doc.ID != preview.doc.ID { + t.Fatalf("the run made %.12s, the preview %.12s", kept.doc.ID, preview.doc.ID) + } + f.now = f.now.AddDate(0, 0, 1) + t.Setenv("GITHUB_RUN_ID", "101") + mustRun(t, f, gh, "run", "--line", "release-1.1", "--continue", "--push") + posted := reportsPosted(gh) + if n := len(slices.DeleteFunc(slices.Clone(posted), func(id string) bool { return id != kept.doc.ID })); n != 2 || len(posted) != 3 { + t.Errorf("reports %q; want the first, the look-alike and the run's own once", posted) + } + body := gh.find(90).Body + again := bodyReportOf(t, body) + rec, err := parseRecord(body, notifyLine) + if err != nil || again.raw != kept.raw || rec.Report != again.doc.ID || !strings.Contains(again.raw, `"run":"https://github.com/cozystack/cozystack/actions/runs/100"`) { + t.Errorf("the second run's body carries %s with record report %v (%v); want the kept bytes of run 100", again.raw, rec, err) + } + + // The reader takes the run's own comment, not the look-alike. + if code, stdout, stderr := f.exec(gh, "report", "--pr", "90", "--id", kept.doc.ID); code != 0 || !strings.Contains(stdout, "#6 picked") { + t.Errorf("report = %d:\n%s%s", code, stdout, stderr) + } + for i := range gh.issueComments { + c := &gh.issueComments[i] + if c.User.Login == "maintainer" && strings.Contains(c.Body, kept.doc.ID) { + c.Body = strings.Replace(c.Body, `"picked":[5,6]`, `"picked":[5]`, 1) + } + } + if code, stdout, stderr := f.exec(gh, "report", "--pr", "90", "--id", kept.doc.ID); code != 2 || !strings.Contains(stderr, "does not match") { + t.Errorf("report of an edited comment = %d, want 2:\n%s%s", code, stdout, stderr) + } +} + +// The report holds no pull request number, so a new batch's report is whole +// before GitHub numbers the pull request, and the reader finds it on +// whatever number it got. +func TestFirstReportIDIsKnownBeforeTheNumber(t *testing.T) { + f := newFixture(t) + f.lines() + f.pr(5, "fix(app): five", "", map[string]string{"app/five.txt": "5\n"}) + gh := &fakeGH{nextPR: 4242} + mustRun(t, f, gh, "run", "--line", "release-1.1", "--push") + inBody := bodyReportOf(t, gh.bodies[0]) + // Every list is one, empty or not, so equal reports are equal bytes: + // main here has no commit that merged no pull request. + if !strings.Contains(inBody.raw, `"other":[]`) { + t.Errorf("the report writes its empty list of other commits as something else:\n%s", inBody.raw) + } + if !slices.Equal(gh.commented, []int{4242}) || !slices.Equal(reportsPosted(gh), []string{inBody.doc.ID}) { + t.Fatalf("commented on %v with %q, want #4242 with %.12s", gh.commented, reportsPosted(gh), inBody.doc.ID) + } + found, bad, err := findReports(gh, 4242, notifyLine) + if err != nil || len(bad) != 0 || found[inBody.doc.ID].raw != inBody.raw { + t.Errorf("findReports = %v, %v, %v; want the body's report", found, bad, err) + } + if code, stdout, stderr := f.exec(gh, "report", "--pr", "4242"); code != 0 || !strings.Contains(stdout, "report "+inBody.doc.ID) || !strings.Contains(stdout, "#4242's body carries this report") { + t.Errorf("report --pr 4242 = %d:\n%s%s", code, stdout, stderr) + } +} + +// Every original main merged since the line branched is in exactly one +// category of the report, a commit that merged no pull request is counted +// apart, and the report says so whatever the run had to do. +func TestReportPartition(t *testing.T) { + f := newFixture(t) + f.lines() + gh := &fakeGH{labels: map[string][]int{labelSkip: {9}}} + // onNewer puts an original on release-1.1, as the newer line's batch + // would, so rule 6 lets it onto release-1.0. + onNewer := func(mc string, files map[string]string) { + f.onLine("release-1.1", "fix: by the newer batch\n\n(cherry picked from commit "+mc+")", files) + } + pr := func(n int, title string, files map[string]string, newer bool) { + mc := f.pr(n, title, "", files) + if newer { + onNewer(mc, files) + } + } + t.Setenv("GIT_COMMITTER_DATE", "2026-01-01T00:00:00Z") + f.pr(4, "fix(app): four, before the start", "", map[string]string{"app/four.txt": "4\n"}) + os.Unsetenv("GIT_COMMITTER_DATE") + f.setPolicy("release-1.0: 2026-06-01\n") + f.onLine("release-1.0", "fix(app): a line-only eleven", map[string]string{"app/eleven.txt": "line\n"}) + pr(5, "fix(app): five", map[string]string{"app/five.txt": "5\n"}, true) + pr(6, "fix(app): six, not on the newer line", map[string]string{"app/six.txt": "6\n"}, false) + pr(7, "fix(app)!: seven", map[string]string{"app/seven.txt": "7\n"}, true) + pr(8, "fix(platform): eight", map[string]string{migrationsDir + "42": "#!/bin/sh\n"}, true) + pr(9, "fix(app): nine", map[string]string{"app/nine.txt": "9\n"}, true) + mc10 := f.pr(10, "fix(app): ten", "", map[string]string{"app/ten.txt": "10\n"}) + onNewer(mc10, map[string]string{"app/ten.txt": "10\n"}) + f.onLine("release-1.0", "fix(app): ten\n\n(cherry picked from commit "+mc10+")", map[string]string{"app/ten.txt": "10\n"}) + pr(11, "fix(app): eleven", map[string]string{"app/eleven.txt": "11\n"}, true) + pr(12, "fix(ci): twelve", map[string]string{".github/workflows/twelve.yaml": "on: push\n"}, true) + pr(13, "fix(app): thirteen", map[string]string{"app/thirteen.txt": "13\n"}, true) + f.git("switch", "-q", "main") + f.write("app/direct.txt", "pushed\n") + f.git("add", "-A") + f.git("commit", "-q", "-m", "chore: a direct push") + direct := f.git("rev-parse", "HEAD") + f.git("push", "-q", "origin", "main") + + stdout := mustRun(t, f, gh, "run", "--line", "release-1.0", "--no-workflow-files", "--push") + const branch = "backport-batch/release-1.0-20261026" + gh.prs = []pullRequest{f.batchPR(90, "release-1.0", branch, stdout)} + pr(14, "fix(app): fourteen, after the batch", map[string]string{"app/fourteen.txt": "14\n"}, true) + stdout = mustRun(t, f, gh, "run", "--line", "release-1.0", "--defer", "13", "--reason", "fails the e2e", "--push") + + l, _ := parseLine("release-1.0") + s := bodyReportOf(t, stdout, l) + got, err := s.doc.categories() + if err != nil { + t.Fatal(err) + } + want := map[int]string{1: "skipped.type", 4: "skipped.before-start", 5: "picked", 6: "skipped.newer", 7: "skipped.breaking", + 8: "skipped.touches", 9: "skipped.skip-label", 10: "skipped.on-line", 11: "deferred", 12: "deferred", 13: "deferred", + 14: "missing", 9000: "skipped.type"} + for n, category := range want { + if got[n] != category { + t.Errorf("#%d is %q, want %q", n, got[n], category) + } + delete(got, n) + } + if len(got) != 0 { + t.Errorf("the report also names %v", got) + } + if !slices.Contains(s.doc.Other, direct) { + t.Errorf("the report does not count the direct push %.12s: %v", direct, s.doc.Other) + } + kinds := map[int]string{} + for _, e := range s.doc.Primary.Deferred { + kinds[e.Original] = fmt.Sprintf("commit=%v reason=%q workflows=%v", e.Commit != "", e.Reason, e.Workflows) + } + if kinds[11] != `commit=true reason="" workflows=false` || kinds[12] != `commit=false reason="" workflows=true` || kinds[13] != `commit=false reason="fails the e2e" workflows=false` { + t.Errorf("the deferred entries are %v", kinds) + } +} + +// A report too big for a comment is refused before anything is written: no +// push, no comment, no edit, and no replay of the earlier report either. +func TestReportTooLargeIsRefusedBeforePush(t *testing.T) { + f, gh, head := bigWaits(t, 7) + code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--continue", "--no-workflow-files", "--push") + if code != 2 || !strings.Contains(stderr, "the report comment is") { + t.Errorf("run --push = %d, want a refusal of the report:\n%s", code, stderr) + } + if f.remoteHead(batchBranch1) != head || len(gh.issueComments) != 0 || len(gh.edited) != 0 { + t.Errorf("the refused run moved the batch to %.12s, wrote %d comments and edited %q", f.remoteHead(batchBranch1), len(gh.issueComments), gh.edited) + } +} + +// bigWaits opens a batch whose report comment failed, so its body keeps a +// report no comment does, and then merges n originals that each wait on 50 +// workflow files with long names, which both the record and the report list +// in full. +func bigWaits(t *testing.T, n int) (*fixture, *fakeGH, string) { + t.Helper() + f := newFixture(t) + f.lines() + f.pr(5, "fix(app): five", "", map[string]string{"app/five.txt": "5\n"}) + gh := &fakeGH{fail: map[string]error{"createComment": errString("HTTP 502")}} + if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--no-workflow-files", "--push"); code != 2 || len(gh.created) != 1 { + t.Fatalf("the first run = %d, want its comment to fail after the pull request opened\n%s", code, stderr) + } + f.git("switch", "-q", "main") + for i := range n { + files := map[string]string{} + for j := range filesKept { + files[fmt.Sprintf(".github/workflows/%s-%02d-%02d.yaml", strings.Repeat("w", 170), i, j)] = "on: push\n" + } + f.pr(10+i, fmt.Sprintf("fix(ci): wait %d", i), "", files) + } + f.now = f.now.AddDate(0, 0, 1) + return f, gh, f.remoteHead(batchBranch1) +} + +// A failure between any two writes leaves what was written readable, and the +// next run, whatever changed meanwhile, replays a report the body kept but no +// comment did, byte for byte, and posts nothing twice. A batch closed in +// between is never written again. +func TestReportReplayAfterEachFailure(t *testing.T) { + setup := func(t *testing.T) (*fixture, *fakeGH) { + f := newFixture(t) + f.lines() + f.pr(5, "fix(app): five", "", map[string]string{"app/five.txt": "5\n"}) + f.pr(6, "fix(app): six", "", map[string]string{"app/six.txt": "6\n"}) + return f, &fakeGH{} + } + // change moves a label, main, the clock and the run's flags between + // the failure and the retry, so the retry's report is another one. + change := func(f *fixture, gh *fakeGH) []string { + gh.labels = map[string][]int{labelSkip: {6}} + f.pr(7, "fix(app): seven", "", map[string]string{"app/seven.txt": "7\n"}) + f.now = f.now.AddDate(0, 0, 1) + return []string{"run", "--line", "release-1.1", "--continue", "--no-workflow-files", "--push"} + } + once := func(t *testing.T, gh *fakeGH, ids ...string) { + t.Helper() + posted := reportsPosted(gh) + for _, id := range ids { + if n := len(slices.DeleteFunc(slices.Clone(posted), func(p string) bool { return p != id })); n != 1 { + t.Errorf("report %.12s is posted %d times: %q", id, n, posted) + } + } + } + + t.Run("a new batch whose pull request was not opened", func(t *testing.T) { + f, gh := setup(t) + gh.fail = map[string]error{"createPull": errString("HTTP 502")} + if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--push"); code != 2 { + t.Fatalf("run = %d\n%s", code, stderr) + } + f.git("switch", "-q", "main") + if len(gh.prs) != 0 || len(gh.issueComments) != 0 || f.remoteHead(batchBranch1) == "" { + t.Errorf("after the failure: %d pull requests, %d comments, branch %q; want the branch alone", len(gh.prs), len(gh.issueComments), f.remoteHead(batchBranch1)) + } + mustRun(t, f, gh, change(f, gh)...) + if len(gh.prs) != 1 || len(reportsPosted(gh)) != 1 || bodyReportOf(t, gh.prs[0].Body).doc.ID != reportsPosted(gh)[0] { + t.Errorf("the retry opened %d pull requests with reports %q", len(gh.prs), reportsPosted(gh)) + } + }) + t.Run("a new batch whose report comment failed", func(t *testing.T) { + f, gh := setup(t) + gh.fail = map[string]error{"createComment": errString("HTTP 502")} + if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--push"); code != 2 || !strings.Contains(stderr, "the body keeps it") { + t.Fatalf("run = %d\n%s", code, stderr) + } + f.git("switch", "-q", "main") + first := bodyReportOf(t, gh.find(90).Body) + mustRun(t, f, gh, change(f, gh)...) + second := bodyReportOf(t, gh.find(90).Body) + want, err := reportComment(first) + if err != nil || len(gh.issueComments) != 2 || gh.issueComments[0].Body != want || reportsPosted(gh)[1] != second.doc.ID || first.doc.ID == second.doc.ID { + t.Errorf("comments %q; want the first report replayed as it was, then the retry's", reportsPosted(gh)) + } + }) + t.Run("a continuation whose push failed", func(t *testing.T) { + f, gh := setup(t) + openBatch(t, f, gh) + first := reportsPosted(gh)[0] + f.pr(8, "fix(app): eight", "", map[string]string{"app/eight.txt": "8\n"}) + f.refusePushes(true) + if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--continue", "--push"); code != 2 { + t.Fatalf("run = %d\n%s", code, stderr) + } + f.git("switch", "-q", "main") + f.refusePushes(false) + attempted := reportsPosted(gh)[1] + if bodyReportOf(t, gh.find(90).Body).doc.ID != first { + t.Error("the body moved on although the push failed") + } + mustRun(t, f, gh, change(f, gh)...) + once(t, gh, first, attempted) + if len(reportsPosted(gh)) != 3 { + t.Errorf("reports %q; want the first, the attempted and the retry's", reportsPosted(gh)) + } + }) + t.Run("a continuation whose body edit failed", func(t *testing.T) { + f, gh := setup(t) + openBatch(t, f, gh) + first := reportsPosted(gh)[0] + before := f.remoteHead(batchBranch1) + f.pr(8, "fix(app): eight", "", map[string]string{"app/eight.txt": "8\n"}) + gh.fail = map[string]error{"editPull": errString("HTTP 502")} + if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--continue", "--push"); code != 2 { + t.Fatalf("run = %d\n%s", code, stderr) + } + f.git("switch", "-q", "main") + attempted := reportsPosted(gh)[1] + if f.remoteHead(batchBranch1) == before || bodyReportOf(t, gh.find(90).Body).doc.ID != first { + t.Error("the push or the body is not where the failure left them") + } + mustRun(t, f, gh, change(f, gh)...) + once(t, gh, first, attempted) + if last := reportsPosted(gh)[len(reportsPosted(gh))-1]; bodyReportOf(t, gh.find(90).Body).doc.ID != last || len(reportsPosted(gh)) != 3 { + t.Errorf("reports %q; want the body to carry the retry's, posted last", reportsPosted(gh)) + } + }) + t.Run("a batch closed while the run writes to it", func(t *testing.T) { + f, gh := setup(t) + openBatch(t, f, gh) + f.pr(8, "fix(app): eight", "", map[string]string{"app/eight.txt": "8\n"}) + gh.after = map[string]func(){"createComment": func() { gh.find(90).State = "closed" }} + code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--continue", "--push") + if code != 2 || !strings.Contains(stderr, "batch pull request #90 is closed now; nothing is written to it") || len(gh.edited) != 0 { + t.Errorf("run = %d, edited %q; want 2 and no edit of the closed pull request:\n%s", code, gh.edited, stderr) + } + }) + t.Run("a batch closed before its report was posted", func(t *testing.T) { + f, gh := setup(t) + gh.fail = map[string]error{"createComment": errString("HTTP 502")} + if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--push"); code != 2 { + t.Fatalf("run = %d\n%s", code, stderr) + } + f.git("switch", "-q", "main") + kept := bodyReportOf(t, gh.find(90).Body) + gh.find(90).State = "closed" + gh.nextPR = 91 + mustRun(t, f, gh, change(f, gh)...) + if slices.Contains(gh.commented, 90) || slices.Contains(reportsPosted(gh), kept.doc.ID) { + t.Errorf("commented on %v, posting %q; want nothing on the closed #90", gh.commented, reportsPosted(gh)) + } + code, stdout, stderr := f.exec(gh, "report", "--pr", "90") + if code != 0 || !strings.Contains(stdout, "report "+kept.doc.ID) || !strings.Contains(stdout, "kept in the body only") { + t.Errorf("report --pr 90 = %d:\n%s%s", code, stdout, stderr) + } + }) + // Every comment is posted only after GitHub says the batch is still + // open, the replay's included, so a close between the two stops the + // second. + t.Run("a batch closed between the replay and the new report", func(t *testing.T) { + f, gh := setup(t) + gh.fail = map[string]error{"createComment": errString("HTTP 502")} + if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--push"); code != 2 { + t.Fatalf("run = %d\n%s", code, stderr) + } + f.git("switch", "-q", "main") + kept := bodyReportOf(t, gh.find(90).Body) + f.pr(8, "fix(app): eight", "", map[string]string{"app/eight.txt": "8\n"}) + gh.after = map[string]func(){"createComment": func() { gh.find(90).State = "closed" }} + code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--continue", "--push") + if code != 2 || !strings.Contains(stderr, "batch pull request #90 is closed now; nothing is written to it") || !slices.Equal(reportsPosted(gh), []string{kept.doc.ID}) || len(gh.edited) != 0 { + t.Errorf("run = %d, posted %q, edited %q; want 2 after the replay alone:\n%s", code, reportsPosted(gh), gh.edited, stderr) + } + }) +} + +// The reader says how a report stands against the pull request as GitHub +// shows it now, and claims publication only when the head or the body shows +// it: a failed push, a push whose answer was lost, a body that was not +// updated and a rebuild since all read as they are. +func TestReportPublicationStatus(t *testing.T) { + setup := func(t *testing.T) (*fixture, *fakeGH, string) { + f := newFixture(t) + f.lines() + f.pr(5, "fix(app): five", "", map[string]string{"app/five.txt": "5\n"}) + f.pr(6, "fix(app): six", "", map[string]string{"app/six.txt": "6\n"}) + gh := &fakeGH{} + openBatch(t, f, gh) + return f, gh, reportsPosted(gh)[0] + } + status := func(t *testing.T, f *fixture, gh *fakeGH, id, want string) { + t.Helper() + code, stdout, stderr := f.exec(gh, "report", "--pr", "90", "--id", id) + if code != 0 || !strings.Contains(stdout, want) { + t.Errorf("report --id %.12s = %d, want %q:\n%s%s", id, code, want, stdout, stderr) + } + } + t.Run("a push the remote refused, and one it took whose answer was lost", func(t *testing.T) { + f, gh, first := setup(t) + f.pr(7, "fix(app): seven", "", map[string]string{"app/seven.txt": "7\n"}) + f.refusePushes(true) + if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--continue", "--push"); code != 2 { + t.Fatalf("run = %d\n%s", code, stderr) + } + f.refusePushes(false) + attempted := reportsPosted(gh)[1] + status(t, f, gh, attempted, "publication unconfirmed: #90's head is ") + status(t, f, gh, attempted, "its body carries report "+first[:12]) + // The same head reaching the remote is what a push GitHub took looks + // like when its answer never came back. + f.git("push", "-q", "origin", "HEAD:refs/heads/"+batchBranch1) + f.git("switch", "-q", "main") + status(t, f, gh, attempted, "the batch this report describes, but its body carries report "+first[:12]) + }) + t.Run("a push followed by a failed body edit", func(t *testing.T) { + f, gh, first := setup(t) + f.pr(7, "fix(app): seven", "", map[string]string{"app/seven.txt": "7\n"}) + gh.fail = map[string]error{"editPull": errString("HTTP 502")} + if code, _, stderr := f.exec(gh, "run", "--line", "release-1.1", "--continue", "--push"); code != 2 { + t.Fatalf("run = %d\n%s", code, stderr) + } + f.git("switch", "-q", "main") + status(t, f, gh, reportsPosted(gh)[1], "the batch this report describes, but its body carries report "+first[:12]) + status(t, f, gh, first, "#90's body carries this report") + }) + t.Run("a rebuild since", func(t *testing.T) { + f, gh, first := setup(t) + mustRun(t, f, gh, "run", "--line", "release-1.1", "--defer", "6", "--reason", "x", "--push") + second := reportsPosted(gh)[1] + status(t, f, gh, first, "publication unconfirmed: #90's head is ") + status(t, f, gh, second, "#90's body carries this report") + }) +} + +// forged is a report on release-1.1 with what change sets, under an ID that +// matches it: only checking what it says stands in its way. +func forged(change func(d *reportDoc)) storedReport { + sha := strings.Repeat("a", 40) + d := reportDoc{Format: reportFormat, Line: "release-1.1", Diag: map[string][]string{}, Other: []string{sha}, + Built: builtFrom{Main: sha, Line: sha, Policy: sha, Batch: sha, At: "2026-10-26T06:00:00Z", Options: runOptions{Mode: modeNew}}} + d.Primary.Picked = []int{5} + change(&d) + d.ID = reportID(d) + return storedReport{doc: d, raw: reportJSON(d)} +} + +// reportRun runs report with gh faked; it reads nothing but GitHub. +func reportRun(gh *fakeGH, args ...string) (int, string, string) { + var stdout, stderr strings.Builder + a := &app{gh: gh, stdout: &stdout, stderr: &stderr, now: func() time.Time { return time.Date(2026, 10, 26, 6, 0, 0, 0, time.UTC) }} + code := a.main(append([]string{"report"}, args...)) + return code, stdout.String(), stderr.String() +} + +// A report is read only when everything it says is what a run writes, and +// it is for the line of the pull request it sits on: an ID that matches its +// content proves only that nobody changed it since it was hashed. +func TestReportRefusesWhatItCannotVouchFor(t *testing.T) { + for name, change := range map[string]func(d *reportDoc){ + "a line that is no release line": func(d *reportDoc) { d.Line = "release-1.1\n\x1b[2Jpicked" }, + "another line": func(d *reportDoc) { d.Line = "release-1.0" }, + "a main that is no commit": func(d *reportDoc) { d.Built.Main = "main\n#5 picked" }, + "a time that is none": func(d *reportDoc) { d.Built.At = "2026-10-26T" }, + "an unknown operation": func(d *reportDoc) { d.Built.Options.Mode = "new\x1b[2J" }, + "a run that is no Actions run": func(d *reportDoc) { d.Built.Run = "javascript:alert(1)" }, + "a tool that is no commit": func(d *reportDoc) { d.Built.Tool = "v1\n" }, + "a commit on main that is none": func(d *reportDoc) { d.Other = []string{"HEAD\x1b]0;x\x07"} }, + "a deferred commit that is none": func(d *reportDoc) { + d.Primary.Deferred = []deferredEntry{{Original: 6, Commit: "-x", Files: []string{"a"}}} + }, + "an original named twice": func(d *reportDoc) { d.Primary.Missing = []int{5} }, + "an original that is none": func(d *reportDoc) { d.Primary.Skipped.Type = []int{-1} }, + // Fields that speak of one another have to agree. + "a drop category with no drop": func(d *reportDoc) { d.Primary.Skipped.Dropped = []int{6} }, + "a stale original it has missing": func(d *reportDoc) { + d.Primary.Missing, d.Overlays.Stale = []int{7}, []int{7} + }, + "a stale original it puts nowhere": func(d *reportDoc) { d.Overlays.Stale = []int{8} }, + "a changed original it defers": func(d *reportDoc) { + d.Primary.Deferred = []deferredEntry{{Original: 9, Files: []string{}, Reason: "fails the e2e"}} + d.Overlays.Changed = []int{9} + }, + "a skip label on a picked original": func(d *reportDoc) { d.Overrides.Skip = []int{5} }, + "a skip-label category with no label": func(d *reportDoc) { d.Primary.Skipped.SkipLabel = []int{10} }, + "an include label on a rule 2 skip": func(d *reportDoc) { + d.Primary.Skipped.Type, d.Overrides.Include = []int{11}, []int{11} + }, + "both labels on one original": func(d *reportDoc) { + d.Primary.Skipped.SkipLabel, d.Overrides.Include, d.Overrides.Skip = []int{12}, []int{12}, []int{12} + }, + "a label on no original": func(d *reportDoc) { d.Overrides.Include = []int{13} }, + "a diagnostic of an original it puts nowhere": func(d *reportDoc) { d.Diag = map[string][]string{"14": {"incomplete"}} }, + "a workflow wait without the flag": func(d *reportDoc) { + d.Primary.Deferred = []deferredEntry{{Original: 15, Files: []string{".github/workflows/ci.yaml"}, Workflows: true}} + }, + "a close that still picks": func(d *reportDoc) { d.Built.Options.Mode = modeClose }, + "a parked original": func(d *reportDoc) { d.Primary.Parked = []json.RawMessage{json.RawMessage(`{"original":16}`)} }, + } { + s := forged(change) + c, _ := reportComment(storedReport{doc: forged(func(*reportDoc) {}).doc, raw: s.raw}) + comment := issueComment{ID: 1, Body: strings.Replace(c, forged(func(*reportDoc) {}).doc.ID, s.doc.ID, 1)} + comment.User.Login, comment.User.Type = "maintainer", "User" + for where, gh := range map[string]*fakeGH{ + "in the body": {prs: []pullRequest{notifyPR("20261026", "text\n\n"+reportBlock(s.raw)+"\n")}}, + "in a comment": {prs: []pullRequest{notifyPR("20261026", "text")}, issueComments: []issueComment{comment}}, + } { + if code, stdout, stderr := reportRun(gh, "--pr", "90", "--id", s.doc.ID); code != 2 || stdout != "" { + t.Errorf("%s, %s: report = %d, want 2 and nothing printed:\n%s%s", name, where, code, stdout, stderr) + } + } + } + if code, stdout, stderr := reportRun(&fakeGH{prs: []pullRequest{notifyPR("20261026", "text\n\n"+reportBlock(forged(func(*reportDoc) {}).raw)+"\n")}}, "--pr", "90"); code != 0 || !strings.Contains(stdout, "#5 picked") { + t.Errorf("report of a sound one = %d:\n%s%s", code, stdout, stderr) + } +} + +// What report prints stays one line per field and carries nothing a +// terminal acts on, whatever reached it. +func TestReportPrintsNothingThatDrivesATerminal(t *testing.T) { + const evil = "\n\x1b[2Jfake: #9 picked\u202e" + s := forged(func(d *reportDoc) { + d.Line, d.Built.At, d.Built.Options.Mode = "release-1.1"+evil, "2026-10-26"+evil, "new"+evil + d.Built.Main, d.Built.Tool, d.Built.Run, d.Built.Newer = "main"+evil, "tool"+evil, "run"+evil, "newer"+evil + d.Other = []string{"other" + evil} + d.Primary.Deferred = []deferredEntry{{Original: 6, Reason: "why" + evil}} + }) + var stdout strings.Builder + a := &app{stdout: &stdout} + pr := notifyPR("20261026", "") + a.printReport(&pr, s, nil, true) + for _, ln := range strings.Split(stdout.String(), "\n") { + if strings.HasPrefix(ln, "fake") || strings.ContainsFunc(ln, func(r rune) bool { return r != '\t' && unicode.IsControl(r) || unicode.Is(unicode.Cf, r) }) { + t.Errorf("report printed %q", ln) + } + } +} + +// The comment is made of what was checked: a line, a time and an ID that +// carry no markup into the summary GitHub renders. +func TestReportCommentIsBuiltFromCheckedFields(t *testing.T) { + for name, change := range map[string]func(d *reportDoc){ + "markup in the line": func(d *reportDoc) { d.Line = "release-1.1" }, + "markup in the time": func(d *reportDoc) { d.Built.At = "2026-10-26T06:00:00Z" }, + "a mention": func(d *reportDoc) { d.Line = "release-1.1 @cozystack/maintainers" }, + } { + if c, err := reportComment(forged(change)); err == nil { + t.Errorf("%s: reportComment made\n%s", name, c) + } + } + if _, err := reportComment(forged(func(*reportDoc) {})); err != nil { + t.Errorf("reportComment of a sound report: %v", err) + } +} + +// Anyone can comment on a public pull request: the reader takes a report +// from a comment only when the batch trusts its author, whatever ID it +// carries. +func TestReportReaderTakesOnlyTrustedComments(t *testing.T) { + s := forged(func(*reportDoc) {}) + c, err := reportComment(s) + if err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + login, kind string + read bool + }{ + {"someone", "User", false}, + {"renovate[bot]", "Bot", false}, + {"github-actions[bot]", "Bot", false}, + {"maintainer", "User", true}, + {defaultBatchBot, "Bot", true}, + } { + comment := issueComment{ID: 1, Body: c} + comment.User.Login, comment.User.Type = tc.login, tc.kind + gh := &fakeGH{prs: []pullRequest{notifyPR("20261026", "text")}, issueComments: []issueComment{comment}, bot: defaultBatchBot} + code, stdout, stderr := reportRun(gh, "--pr", "90", "--id", s.doc.ID) + if read := code == 0 && strings.Contains(stdout, "#5 picked"); read != tc.read || !tc.read && (code != 2 || !strings.Contains(stderr, "keeps no report")) { + t.Errorf("a report by %s: report = %d, read %v, want %v:\n%s%s", tc.login, code, read, tc.read, stdout, stderr) + } + } +} diff --git a/cmd/backport-batch/workflows_test.go b/cmd/backport-batch/workflows_test.go index 35f2d9f14..97547f6b5 100644 --- a/cmd/backport-batch/workflows_test.go +++ b/cmd/backport-batch/workflows_test.go @@ -110,7 +110,8 @@ func TestWorkflowChangesWaitUntilOnTheLine(t *testing.T) { if code != 0 || !strings.Contains(stdout, "1 originals on #90 change .github/workflows/ and wait for a maintainer") || strings.Contains(stdout, "deferred conflicts") { t.Errorf("notify = %d, %q; want #5 for a maintainer and no conflict\n%s", code, stdout, stderr) } - if len(gh.issueComments) != 1 || !strings.Contains(gh.issueComments[0].Body, "[#5](") || !strings.Contains(gh.issueComments[0].Body, "#changes-to-workflows") || strings.Contains(gh.issueComments[0].Body, "These conflicts") { + notes := slices.DeleteFunc(slices.Clone(gh.issueComments), func(c issueComment) bool { return !strings.HasPrefix(c.Body, notifyMarker) }) + if len(notes) != 1 || !strings.Contains(notes[0].Body, "[#5](") || !strings.Contains(notes[0].Body, "#changes-to-workflows") || strings.Contains(notes[0].Body, "These conflicts") { t.Errorf("the comment does not list #5 for a maintainer: %+v", gh.issueComments) } diff --git a/docs/release/backport-tasks/README.md b/docs/release/backport-tasks/README.md index eba2c3738..e4395480e 100644 --- a/docs/release/backport-tasks/README.md +++ b/docs/release/backport-tasks/README.md @@ -12,7 +12,7 @@ The [backport batch](../backports.md) is built by [`backport-batch`](../../../cm - Push nothing but the branch of the open batch pull request, `backport-batch/-` in this repository, and only what the task's check accepted. Never push to `main` or a `release-*` branch, and never open a second batch pull request: the scheduled run continues the open one. - Change no file the original pull request does not change. A conflict that cannot be resolved inside those files is not resolved: the original stays deferred. - Never rewrite the batch branch by hand. A plain push to it is a fast-forward; only `backport-batch run --defer --push` replaces it, leased on the head it rebuilt. -- Leave the batch pull request's title and body to the tool: it rewrites both on every run, and a hand edit is lost or, inside the `backport-batch` block, makes `check` refuse the body. +- Leave the batch pull request's title and body to the tool: it rewrites both on every run, and a hand edit is lost or, inside the `backport-batch` block, makes `run`, `check`, `fold` and `notify` refuse the body, and inside the `backport-batch-report` block makes `run --push` and `backport-batch report` refuse it. Leave the tool's report comments as they are too: each keeps one classification of the line, and an edited one makes `backport-batch report` refuse it. - When the task cannot be done within these rules, push nothing and leave a comment on the batch pull request saying why, so the next maintainer to look finds it. Nothing is lost by waiting: a red batch does not merge, and a deferred original goes to the next batch. ### Getting the tool @@ -26,4 +26,4 @@ cd ../backport-task go build -o /tmp/backport-batch ./cmd/backport-batch ``` -Run it from that worktree. Every command but `notify` fetches `origin` first, and `run`, `check`, `fold` and `lines` read the policy from `origin/main`. +Run it from that worktree. Every command but `notify` and `report` fetches `origin` first, and `run`, `check`, `fold` and `lines` read the policy from `origin/main`. diff --git a/docs/release/backports.md b/docs/release/backports.md index f63b6e089..0cc8db3b6 100644 --- a/docs/release/backports.md +++ b/docs/release/backports.md @@ -81,15 +81,17 @@ A run by hand trusts the open batch pull request the scheduled run opened as `co `./backport-batch lines` prints the lines, newest first. `run`, `check`, `fold` and `lines` read the policy block from this page on `main`, so until the page with its block is on `main` they need `--policy ` naming a ref that has it, and the scheduled run, which passes none, fails. -`run` picks what the rules take onto `backport-batch/-` and opens a draft pull request against the line. A pull request merged into `main` with a merge commit goes onto the batch as its own commits, oldest first, each picked with `git cherry-pick -x --signoff` so it keeps its author, its message and its sign-offs and gains the sign-off of whoever runs the batch; commits that merged `main` into the pull request's branch are left out. A merge commit joins them to the batch: it carries the original's merge message from `main` and ends in a `Backported-from: ` trailer. A pull request squash-merged into `main` goes on as one pick. When any commit of an original conflicts, nothing of that original stays on the batch. The batch pull request is merged into the line with a merge commit, so all of this reaches the line as it is. Its body lists the originals with how many commits each brought and which of them differ from the commit they came from, the conflicts it deferred with the commit that failed and its files, and what it skipped under which rule. While that pull request is open, the next run continues it, `--continue` reading its head, rather than opening another. CI on the batch pull request runs the line's build and tests, and a maintainer merges it. Since rule 6 reads the newer line itself, an older line takes a fix in the batch after the newer line's batch with that fix has merged. +`run` picks what the rules take onto `backport-batch/-` and opens a draft pull request against the line. A pull request merged into `main` with a merge commit goes onto the batch as its own commits, oldest first, each picked with `git cherry-pick -x --signoff` so it keeps its author, its message and its sign-offs and gains the sign-off of whoever runs the batch; commits that merged `main` into the pull request's branch are left out. A merge commit joins them to the batch: it carries the original's merge message from `main` and ends in a `Backported-from: ` trailer. A pull request squash-merged into `main` goes on as one pick. When any commit of an original conflicts, nothing of that original stays on the batch. The batch pull request is merged into the line with a merge commit, so all of this reaches the line as it is. Its body lists the originals with how many commits each brought and which of them differ from the commit they came from, the conflicts it deferred with the commit that failed and its files, and what it skipped under which rule, and ends with the [line's record and the run's report](#the-lines-record-and-reports); `--push` prints the pull request's URL. While that pull request is open, the next run continues it, `--continue` reading its head, rather than opening another. CI on the batch pull request runs the line's build and tests, and a maintainer merges it. Since rule 6 reads the newer line itself, an older line takes a fix in the batch after the newer line's batch with that fix has merged. A deferred conflict waits for the next batch. To land it sooner, pick the original's commits onto the batch head with `git cherry-pick -x`, resolving each conflict, and join them to the batch head with a merge commit that names the original; an original squash-merged into `main` is one pick of its commit there, with no merge. Check that merge or that pick with `./backport-batch fold --line --original --commit `, and push it onto the batch branch, as [resolving a conflict](./backport-tasks/resolve-conflict.md) describes. CI runs `backport-batch verify` on every push to a batch pull request, which holds every original on it to the same shape. When CI on the batch pull request fails, the original it comes from is taken out with `./backport-batch run --line --defer --reason --push` and goes to the next batch, as [attributing a failed batch CI](./backport-tasks/failed-ci.md) describes; the rest of the batch merges without it. Before a patch release or an rc is cut from a line, `./backport-batch check --line --cut` has to exit 0. A cut tags the line, so the check fails while a pull request the rules take is not on the line, including one that is only on the open batch pull request: merge the batch pull request first. It prints the URLs, and prints what the batch deferred as warnings, since deferring is a maintainer's call; so is a pull request labelled `backport/skip` that the open batch still carries. An original that waits on the batch for a maintainer fails the cut, also when it is all the batch has left. A cut that passes says so. Without `--cut`, as the scheduled run uses it, a change on the open batch pull request counts as carried. -### The line's record +### The line's record and reports -What the batch decided about a line, the conflicts it deferred, the originals taken out with `--defer` and those waiting for a maintainer, is kept in a `backport-batch` block in the body of the line's newest batch pull request, its record. `backport-batch` reads the record whether that pull request is open, merged or closed, and writes it only while the pull request is open: a merged or closed batch pull request is never edited or commented on again. So a conflict the batch deferred is still a warning in `check` after the batch merged, until a later batch tries it again. A run that picks nothing while no batch is open writes nothing, and says so. A record `backport-batch` cannot read, or one written by someone it does not trust, stops it rather than let it fall back to an older one; how it decides is in [its README](../../cmd/backport-batch/README.md#the-lines-record). +What the batch decided about a line, the conflicts it deferred, the originals taken out with `--defer` and those waiting for a maintainer, is kept in a `backport-batch` block in the body of the line's newest batch pull request, its record. `backport-batch` reads the record whether that pull request is open, merged or closed, and writes it only while the pull request is open: a merged or closed batch pull request is never edited or commented on again. So a conflict the batch deferred is still a warning in `check` after the batch merged, until a later batch tries it again. A run that picks nothing while no batch is open writes nothing, and says so. A record `backport-batch` cannot read, or one written by someone it does not trust, stops it rather than let it fall back to an older one; how it decides is in [its README](../../cmd/backport-batch/README.md#the-lines-record-and-reports). + +Each run also reports where every pull request merged into `main` since the line branched stands, and what it read to decide. The body carries the latest report under the record, and a comment the tool posts on the open batch pull request keeps each one, so an earlier classification is still there after a later run replaced the body, and after the batch merged. `./backport-batch report --pr ` prints the report the body carries, and `--id ` an earlier one; it reads GitHub only and writes nothing. A run that picks nothing while no batch is open has nowhere to keep its report: the next batch's report has the line's state. ### Changes to workflows