diff --git a/.github/workflows/dispatcher-publish.yml b/.github/workflows/dispatcher-publish.yml index fba735529e..3acd5ddf3d 100644 --- a/.github/workflows/dispatcher-publish.yml +++ b/.github/workflows/dispatcher-publish.yml @@ -477,6 +477,10 @@ jobs: # not earlier. The App token is required: a merge made with GITHUB_TOKEN # starts no follow-up workflow run, so the package release would never be # tagged (docs/adr/0007-separate-release-prs-and-package-auto-merge.md). + # The package release pull request stays draft until this step lifts the + # draft itself, so draft is not a reason to wait here. An already open + # release pull request for the next dispatcher cycle is not a reason to + # wait either: this package pin records the dispatcher published above. - name: Merge the Unity package release pull request now that the pin records this dispatcher if: needs.build.outputs.should_publish == 'true' && needs.build.outputs.release_prerelease != 'true' working-directory: cli/release-automation diff --git a/.github/workflows/release-please.yml b/.github/workflows/release-please.yml index 54a53cf5fd..c1c297c0e4 100644 --- a/.github/workflows/release-please.yml +++ b/.github/workflows/release-please.yml @@ -111,6 +111,7 @@ jobs: cache-dependency-path: '**/go.sum' - name: Dispatch release PR checks + id: release_pr_checks if: steps.target.outputs.branch == 'main' && steps.release_commit.outputs.skip != 'true' && steps.package_release_sync.outputs.ready != 'false' && steps.release.outputs.prs_created == 'true' working-directory: cli/release-automation env: @@ -118,3 +119,41 @@ jobs: TARGET_BRANCH: ${{ steps.target.outputs.branch }} GITHUB_REPOSITORY: ${{ github.repository }} run: go run ./cmd/dispatch-release-please-pr-checks + + # A merge made with GITHUB_TOKEN starts no follow-up workflow run, so the + # package release would never be tagged; the App token is what makes the + # merge observable to release-please + # (docs/adr/0007-separate-release-prs-and-package-auto-merge.md). + # The dispatch step above fails when any component's checks fail, and a + # failing runner release must not strand a package-only release, so these + # steps also run after that failure. The merge helper verifies the package + # head's own runs, so another component's failure cannot make it merge + # something unchecked. + - name: Mint package release merge token + id: package-merge-token + if: steps.target.outputs.branch == 'main' && steps.release_commit.outputs.skip != 'true' && steps.package_release_sync.outputs.ready != 'false' && steps.release.outputs.prs_created == 'true' && !cancelled() && (steps.release_pr_checks.outcome == 'success' || steps.release_pr_checks.outcome == 'failure') + uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 + with: + app-id: ${{ vars.DISPATCHER_PIN_APP_ID }} + private-key: ${{ secrets.DISPATCHER_PIN_APP_PRIVATE_KEY }} + permission-contents: write + permission-pull-requests: write + + # This step leaves the package release pull request draft while a + # dispatcher release pull request is open, and while the pin at its head + # still disagrees with the dispatcher version the base branch manifest + # releases. Both windows would otherwise ship a package that pins an + # older dispatcher than the one main is about to release. + - name: Merge the Unity package release pull request when no dispatcher release is pending + if: steps.target.outputs.branch == 'main' && steps.release_commit.outputs.skip != 'true' && steps.package_release_sync.outputs.ready != 'false' && steps.release.outputs.prs_created == 'true' && !cancelled() && (steps.release_pr_checks.outcome == 'success' || steps.release_pr_checks.outcome == 'failure') + working-directory: cli/release-automation + env: + GH_TOKEN: ${{ steps.package-merge-token.outputs.token }} + GITHUB_REPOSITORY: ${{ github.repository }} + TARGET_BRANCH: ${{ steps.target.outputs.branch }} + run: >- + go run ./cmd/merge-package-release-pr + --repo "${GITHUB_REPOSITORY}" + --base-branch "${TARGET_BRANCH}" + --require-no-open-dispatcher-pr + --no-wait diff --git a/AGENTS.md b/AGENTS.md index 3a5789af32..1fc7b37479 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -114,8 +114,8 @@ inputs outside the package roots (non-test `cli/common/**` sources, `scripts/ins run in the same PR; description-only catalog regenerations are exempt. CI (`check-release-triggers`) fails otherwise. Rules and rationale: `docs/shared-release-inputs.md`. -release-please opens one release pull request per component; merge the dispatcher one first, and -leave the unity-package one to be merged by `post-publish` after the pin stamp. Details: +release-please opens one release pull request per component; merge the dispatcher one first. The +unity-package one stays draft and is merged only by automation — never merge it by hand. Details: `docs/dispatcher-pin-release-order.md`. ## Broken CLI Releases diff --git a/cli/release-automation/internal/automation/package_release_pr_gate.go b/cli/release-automation/internal/automation/package_release_pr_gate.go new file mode 100644 index 0000000000..01788262f6 --- /dev/null +++ b/cli/release-automation/internal/automation/package_release_pr_gate.go @@ -0,0 +1,123 @@ +package automation + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "net/url" + "strings" +) + +const dispatcherReleasePRComponent = "dispatcher" + +// githubFileContentAtRef reads a repository file through the contents API at an +// exact ref. The merge gate reasons about commits that are not in this +// checkout, so the file has to come from GitHub rather than from the worktree. +func githubFileContentAtRef( + ctx context.Context, + runOutput func(context.Context, string, ...string) (string, error), + repository string, + path string, + ref string, +) ([]byte, error) { + // The ref is a query parameter, so an unescaped one would be cut short at + // the first & and read a different commit than the caller asked for. + output, err := runOutput( + ctx, + "gh", + "api", + "repos/"+repository+"/contents/"+path+"?ref="+url.QueryEscape(ref), + "--jq", + ".content", + ) + if err != nil { + return nil, err + } + + // The contents API wraps base64 at a fixed width, so every whitespace + // character has to go before decoding. + encoded := strings.Join(strings.Fields(output), "") + decoded, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + return nil, fmt.Errorf("%s: failed to decode %s at %s: %w", + mergePackageReleasePRCommandName, path, ref, err) + } + return decoded, nil +} + +// dispatcherReleaseTagFromManifestAtRef reports the dispatcher release tag the +// base branch tip publishes. The package pin must record that exact tag, so +// this is the value the merge gate compares the pin against when the caller +// does not name a tag itself. +func dispatcherReleaseTagFromManifestAtRef( + ctx context.Context, + runOutput func(context.Context, string, ...string) (string, error), + repository string, + ref string, +) (string, error) { + content, err := githubFileContentAtRef(ctx, runOutput, repository, releasePleaseManifestRelativePath, ref) + if err != nil { + return "", err + } + + manifest := map[string]string{} + err = json.Unmarshal(content, &manifest) + if err != nil { + return "", fmt.Errorf("%s: failed to parse %s at %s: %w", + mergePackageReleasePRCommandName, releasePleaseManifestRelativePath, ref, err) + } + + version := manifest[releasePleaseDispatcherPackage] + if strings.TrimSpace(version) == "" { + return "", fmt.Errorf("%s: %s at %s has no %q version", + mergePackageReleasePRCommandName, releasePleaseManifestRelativePath, ref, releasePleaseDispatcherPackage) + } + // A padded version would build a tag no release ever carries, so the gate + // would wait out its whole timeout instead of reporting the bad manifest. + if version != strings.TrimSpace(version) { + return "", fmt.Errorf("%s: %s at %s releases %q as %q, which is not a bare version", + mergePackageReleasePRCommandName, releasePleaseManifestRelativePath, ref, releasePleaseDispatcherPackage, version) + } + return dispatcherReleaseTagPrefix + version, nil +} + +// openDispatcherReleasePullRequestExists reports whether a dispatcher release +// is still pending. The label is deliberately not part of the query: a pending +// release pull request that lost its label is still a dispatcher release the +// package must not overtake. +func openDispatcherReleasePullRequestExists( + ctx context.Context, + runOutput func(context.Context, string, ...string) (string, error), + repository string, + baseBranch string, +) (bool, error) { + headBranch := "release-please--branches--" + baseBranch + "--components--" + dispatcherReleasePRComponent + output, err := runOutput( + ctx, + "gh", + "pr", + "list", + "--repo", + repository, + "--state", + "open", + "--base", + baseBranch, + "--head", + headBranch, + "--json", + "number", + ) + if err != nil { + return false, err + } + + releasePRs := []mergePackageReleasePullRequest{} + err = json.Unmarshal([]byte(output), &releasePRs) + if err != nil { + return false, fmt.Errorf("%s: failed to parse dispatcher release PR list: %w", + mergePackageReleasePRCommandName, err) + } + return len(releasePRs) > 0, nil +} diff --git a/cli/release-automation/internal/automation/package_release_pr_merge.go b/cli/release-automation/internal/automation/package_release_pr_merge.go index 6d6ead97cf..2145a74335 100644 --- a/cli/release-automation/internal/automation/package_release_pr_merge.go +++ b/cli/release-automation/internal/automation/package_release_pr_merge.go @@ -2,13 +2,10 @@ package automation import ( "context" - "encoding/base64" "encoding/json" "flag" "fmt" "io" - "strconv" - "strings" "time" ) @@ -20,12 +17,14 @@ const ( ) type mergePackageReleasePRConfig struct { - repository string - baseBranch string - dispatcherTag string - workflows []string - timeout time.Duration - interval time.Duration + repository string + baseBranch string + dispatcherTag string + requireNoOpenDispatcherPR bool + waitForMergeable bool + workflows []string + timeout time.Duration + interval time.Duration } type mergePackageReleasePRDeps struct { @@ -71,6 +70,31 @@ func RunMergePackageReleasePRWithDeps( return waitAndMergePackageReleasePR(ctx, stdout, stderr, config, deps) } +// mergePackageReleasePRDispatcherTag reports the dispatcher release the package +// pin must record. When the caller names no tag it is read from the base branch +// manifest on every pass, and never before the open-dispatcher gate: a +// dispatcher release pull request merged in between would otherwise leave a +// tag read from the old manifest paired with an already empty open list, which +// together read as "nothing pending" for a head that still pins the old +// dispatcher. +func mergePackageReleasePRDispatcherTag( + ctx context.Context, + stdout io.Writer, + config mergePackageReleasePRConfig, + deps mergePackageReleasePRDeps, +) (string, error) { + if config.dispatcherTag != "" { + return config.dispatcherTag, nil + } + dispatcherTag, err := dispatcherReleaseTagFromManifestAtRef(ctx, deps.runOutput, config.repository, config.baseBranch) + if err != nil { + return "", err + } + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "Resolved dispatcher release tag %s from the %s release manifest.", dispatcherTag, config.baseBranch)) + return dispatcherTag, nil +} + func defaultMergePackageReleasePRDeps() mergePackageReleasePRDeps { return mergePackageReleasePRDeps{ now: releasePRCheckNow, @@ -83,7 +107,9 @@ func parseMergePackageReleasePRFlags(args []string) (mergePackageReleasePRConfig flagSet := flag.NewFlagSet(mergePackageReleasePRCommandName, flag.ContinueOnError) repository := flagSet.String("repo", "", "GitHub repository that owns the release pull requests") baseBranch := flagSet.String("base-branch", "main", "base branch of the release pull requests") - dispatcherTag := flagSet.String("dispatcher-tag", "", "dispatcher release tag the package pin must record") + dispatcherTag := flagSet.String("dispatcher-tag", "", "dispatcher release tag the package pin must record (default: the tag the base branch manifest releases)") + requireNoOpenDispatcherPR := flagSet.Bool("require-no-open-dispatcher-pr", false, "refuse to merge while a dispatcher release pull request is open") + noWait := flagSet.Bool("no-wait", false, "decide once and leave the pull request draft instead of polling") timeoutMinutes := flagSet.Int("timeout-minutes", defaultMergePackageReleasePRTimeoutMinutes, "how long to wait for the pull request to become mergeable") intervalSeconds := flagSet.Int("interval-seconds", defaultMergePackageReleasePRIntervalSeconds, "how long to wait between polls") err := flagSet.Parse(args) @@ -94,9 +120,6 @@ func parseMergePackageReleasePRFlags(args []string) (mergePackageReleasePRConfig if *repository == "" { return mergePackageReleasePRConfig{}, fmt.Errorf("%s: --repo is required", mergePackageReleasePRCommandName) } - if *dispatcherTag == "" { - return mergePackageReleasePRConfig{}, fmt.Errorf("%s: --dispatcher-tag is required", mergePackageReleasePRCommandName) - } if *baseBranch == "" { return mergePackageReleasePRConfig{}, fmt.Errorf("%s: --base-branch must not be empty", mergePackageReleasePRCommandName) } @@ -110,12 +133,14 @@ func parseMergePackageReleasePRFlags(args []string) (mergePackageReleasePRConfig } return mergePackageReleasePRConfig{ - repository: *repository, - baseBranch: *baseBranch, - dispatcherTag: *dispatcherTag, - workflows: workflows, - timeout: time.Duration(*timeoutMinutes) * time.Minute, - interval: time.Duration(*intervalSeconds) * time.Second, + repository: *repository, + baseBranch: *baseBranch, + dispatcherTag: *dispatcherTag, + requireNoOpenDispatcherPR: *requireNoOpenDispatcherPR, + waitForMergeable: !*noWait, + workflows: workflows, + timeout: time.Duration(*timeoutMinutes) * time.Minute, + interval: time.Duration(*intervalSeconds) * time.Second, }, nil } @@ -132,10 +157,17 @@ func waitAndMergePackageReleasePR( if settled { return exitCode } + // The release-please path runs on every push to the base branch, so a + // pull request that is not mergeable now is reconsidered by the next + // push rather than being waited out inside one workflow run. + if !config.waitForMergeable { + writeMergePackageReleasePRLine(stdout, "The Unity package release pull request is not mergeable yet; leaving it draft.") + return 0 + } if !deps.now().Before(deadline) { writeMergePackageReleasePRLine(stderr, fmt.Errorf( "%s: waiting for the Unity package release pull request to record %s timed out; merge the pull request by hand once its checks pass", - mergePackageReleasePRCommandName, config.dispatcherTag)) + mergePackageReleasePRCommandName, mergePackageReleasePRAwaitedTag(config))) return 1 } err := deps.sleep(ctx, config.interval) @@ -146,6 +178,16 @@ func waitAndMergePackageReleasePR( } } +// mergePackageReleasePRAwaitedTag names what the wait was for. The tag is +// resolved per pass, so the timeout report has to describe the source rather +// than a value when the caller named no tag. +func mergePackageReleasePRAwaitedTag(config mergePackageReleasePRConfig) string { + if config.dispatcherTag != "" { + return config.dispatcherTag + } + return "the dispatcher release the " + config.baseBranch + " manifest publishes" +} + // attemptMergePackageReleasePR runs one pass of the wait loop. settled is false // while the pull request is still expected to reach a mergeable state, so the // caller keeps polling until the deadline. @@ -166,26 +208,43 @@ func attemptMergePackageReleasePR( return true, 0 } + if config.requireNoOpenDispatcherPR { + dispatcherPRIsOpen, err := openDispatcherReleasePullRequestExists(ctx, deps.runOutput, config.repository, config.baseBranch) + if err != nil { + writeMergePackageReleasePRLine(stderr, err) + return true, 1 + } + // Waiting here cannot help: the dispatcher release has to be merged, + // published and stamped, which takes far longer than this command runs. + if dispatcherPRIsOpen { + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "PR #%d stays draft: a dispatcher release pull request is open; the dispatcher must be released and stamped first.", + releasePR.Number)) + return true, 0 + } + } + + dispatcherTag, err := mergePackageReleasePRDispatcherTag(ctx, stdout, config, deps) + if err != nil { + writeMergePackageReleasePRLine(stderr, err) + return true, 1 + } + pinnedTag, err := packageReleasePullRequestPinnedTag(ctx, config, releasePR, deps) if err != nil { writeMergePackageReleasePRLine(stderr, err) return true, 1 } - if pinnedTag != config.dispatcherTag { + if pinnedTag != dispatcherTag { writeMergePackageReleasePRLine(stdout, fmt.Sprintf( "PR #%d head %s still pins %s; waiting for release-please to rebase it onto the stamp.", releasePR.Number, releasePR.HeadRefOID, pinnedTag)) return false, 0 } - // release-please updates the head and the release PR check automation marks - // the pull request draft in separate steps, so a rebased head is briefly - // non-draft with no checks of its own. Both the draft flag and this head's - // own runs must be consulted before merging. - if releasePR.IsDraft { - writeMergePackageReleasePRLine(stdout, fmt.Sprintf("PR #%d is draft while release checks run; waiting.", releasePR.Number)) - return false, 0 - } - + // The draft flag is not consulted as evidence: it exists to keep people from + // merging this pull request, not to prove its checks ran. What proves that + // is a successful run of every required workflow for this exact head SHA, + // held to that head by --match-head-commit at the merge. checksPassed, exitCode := packageReleasePullRequestChecksPassed(ctx, stdout, stderr, config, releasePR, deps) if exitCode != 0 { return true, exitCode @@ -194,14 +253,7 @@ func attemptMergePackageReleasePR( return false, 0 } - err = squashMergePackageReleasePR(ctx, config, releasePR, deps) - if err != nil { - writeMergePackageReleasePRLine(stderr, err) - return true, 1 - } - writeMergePackageReleasePRLine(stdout, fmt.Sprintf( - "Merged Unity package release PR #%d at %s; it pins %s.", releasePR.Number, releasePR.HeadRefOID, pinnedTag)) - return true, 0 + return readyAndMergePackageReleasePR(ctx, stdout, stderr, config, releasePR, pinnedTag, deps) } func findPackageReleasePullRequest( @@ -266,27 +318,12 @@ func packageReleasePullRequestPinnedTag( releasePR mergePackageReleasePullRequest, deps mergePackageReleasePRDeps, ) (string, error) { - output, err := deps.runOutput( - ctx, - "gh", - "api", - "repos/"+config.repository+"/contents/"+unityPackageCliPinFile+"?ref="+releasePR.HeadRefOID, - "--jq", - ".content", - ) + decoded, err := githubFileContentAtRef( + ctx, deps.runOutput, config.repository, unityPackageCliPinFile, releasePR.HeadRefOID) if err != nil { return "", err } - // The contents API wraps base64 at a fixed width, so every whitespace - // character has to go before decoding. - encoded := strings.Join(strings.Fields(output), "") - decoded, err := base64.StdEncoding.DecodeString(encoded) - if err != nil { - return "", fmt.Errorf("%s: failed to decode %s at %s: %w", - mergePackageReleasePRCommandName, unityPackageCliPinFile, releasePR.HeadRefOID, err) - } - pin := packagePinConsistencyPin{} err = json.Unmarshal(decoded, &pin) if err != nil { @@ -379,30 +416,6 @@ func packageReleasePullRequestRun( return mergePackageReleasePRRun{}, false, nil } -// squashMergePackageReleasePR pins the merge to the head this command -// verified, so a head moved between the check and the merge fails instead of -// releasing an unchecked commit. -func squashMergePackageReleasePR( - ctx context.Context, - config mergePackageReleasePRConfig, - releasePR mergePackageReleasePullRequest, - deps mergePackageReleasePRDeps, -) error { - _, err := deps.runOutput( - ctx, - "gh", - "pr", - "merge", - strconv.Itoa(releasePR.Number), - "--repo", - config.repository, - "--squash", - "--match-head-commit", - releasePR.HeadRefOID, - ) - return err -} - func writeMergePackageReleasePRLine(writer io.Writer, values ...any) { // CI status output failures cannot be recovered after the command outcome is known. _, _ = fmt.Fprintln(writer, values...) diff --git a/cli/release-automation/internal/automation/package_release_pr_merge_test.go b/cli/release-automation/internal/automation/package_release_pr_merge_test.go index 0a709adff5..aa131fbe06 100644 --- a/cli/release-automation/internal/automation/package_release_pr_merge_test.go +++ b/cli/release-automation/internal/automation/package_release_pr_merge_test.go @@ -10,7 +10,10 @@ import ( "time" ) -const packageReleasePRHeadBranch = "release-please--branches--main--components--unity-package" +const ( + packageReleasePRHeadBranch = "release-please--branches--main--components--unity-package" + packageReleasePRNumber = 2002 +) // mergePackageReleasePRPoll describes what the stubbed gh reports on one pass of // the wait loop, so a test can walk the pull request through the states it @@ -21,6 +24,18 @@ type mergePackageReleasePRPoll struct { // pin read at the pull request head from one read at any other ref. pinnedTagsByRef map[string]string runs map[string]string + // openDispatcherPRListJSON answers the listing that asks whether a + // dispatcher release is still pending. + openDispatcherPRListJSON string + // manifestByRef answers the contents API for the release manifest, so a + // test can pin the dispatcher version the base branch tip releases. + manifestByRef map[string]string + // failReady and failMerge make the two writes fail the way they do when + // another run reached the pull request first. + failReady bool + failMerge bool + // stateJSON answers the re-read that follows a failed write. + stateJSON string } type mergePackageReleasePRStub struct { @@ -43,12 +58,57 @@ func (stub *mergePackageReleasePRStub) runOutput(ctx context.Context, name strin commandLine := strings.Join(append([]string{name}, args...), " ") stub.commandLog = append(stub.commandLog, commandLine) + output, answered, err := stub.answerPullRequestCommand(commandLine) + if answered { + return output, err + } + return stub.answerContentCommand(commandLine) +} + +// answerPullRequestCommand covers the pull request reads and writes. answered +// is false for anything it does not own, so the caller falls through to the +// content and run reads. +func (stub *mergePackageReleasePRStub) answerPullRequestCommand(commandLine string) (string, bool, error) { switch { + case strings.Contains(commandLine, "--components--dispatcher"): + // The dispatcher listing is a gate question inside one pass, so unlike + // the package listing it must not advance the stubbed state. + if stub.activePoll.openDispatcherPRListJSON == "" { + return "[]", true, nil + } + return stub.activePoll.openDispatcherPRListJSON, true, nil case strings.HasPrefix(commandLine, "gh pr list "): // The listing opens each pass, so it is what advances the stubbed state; // the rest of the pass must keep reading the same poll. stub.activePoll = stub.nextPoll() - return stub.activePoll.prListJSON, nil + return stub.activePoll.prListJSON, true, nil + case strings.HasPrefix(commandLine, "gh pr ready "): + if stub.activePoll.failReady { + return "", true, fmt.Errorf("gh pr ready failed") + } + return "", true, nil + case strings.HasPrefix(commandLine, "gh pr view "): + return stub.activePoll.stateJSON, true, nil + case strings.Contains(commandLine, " --match-head-commit "): + if stub.activePoll.failMerge { + return "", true, fmt.Errorf("gh pr merge failed") + } + return "", true, nil + } + return "", false, nil +} + +// answerContentCommand covers the contents API reads and the workflow run +// listing, and rejects anything the command is not expected to run. +func (stub *mergePackageReleasePRStub) answerContentCommand(commandLine string) (string, error) { + switch { + case strings.Contains(commandLine, releasePleaseManifestRelativePath): + ref := mergePackageReleasePRRequestedRef(commandLine) + manifest, known := stub.manifestAt(ref) + if !known { + return "", fmt.Errorf("no stubbed manifest for ref %q", ref) + } + return base64.StdEncoding.EncodeToString([]byte(manifest)) + "\n", nil case strings.HasPrefix(commandLine, "gh api "): ref := mergePackageReleasePRRequestedRef(commandLine) pinnedTag, known := stub.activePoll.pinnedTagsByRef[ref] @@ -64,12 +124,17 @@ func (stub *mergePackageReleasePRStub) runOutput(ctx context.Context, name strin } } return "[]", nil - case strings.Contains(commandLine, " --match-head-commit "): - return "", nil } return "", fmt.Errorf("unexpected command: %s", commandLine) } +// manifestAt answers the release manifest read of the pass that is open, so a +// test can change what the base branch releases between passes. +func (stub *mergePackageReleasePRStub) manifestAt(ref string) (string, bool) { + manifest, known := stub.activePoll.manifestByRef[ref] + return manifest, known +} + func mergePackageReleasePRRequestedRef(commandLine string) string { _, ref, found := strings.Cut(commandLine, "?ref=") if !found { @@ -80,6 +145,15 @@ func mergePackageReleasePRRequestedRef(commandLine string) string { func runMergePackageReleasePRCase(t *testing.T, polls []mergePackageReleasePRPoll) (int, string, string, *mergePackageReleasePRStub) { t.Helper() + return runMergePackageReleasePRCaseWithArgs(t, polls, []string{"--dispatcher-tag", "dispatcher-v3.4.0"}) +} + +func runMergePackageReleasePRCaseWithArgs( + t *testing.T, + polls []mergePackageReleasePRPoll, + extraArgs []string, +) (int, string, string, *mergePackageReleasePRStub) { + t.Helper() stub := &mergePackageReleasePRStub{polls: polls} now := time.Date(2026, 9, 8, 1, 0, 0, 0, time.UTC) @@ -98,20 +172,20 @@ func runMergePackageReleasePRCase(t *testing.T, polls []mergePackageReleasePRPol stdout := bytes.Buffer{} stderr := bytes.Buffer{} - exitCode := RunMergePackageReleasePRWithDeps(context.Background(), &stdout, &stderr, []string{ + args := append([]string{ "--repo", "owner/repository", "--base-branch", "main", - "--dispatcher-tag", "dispatcher-v3.4.0", "--timeout-minutes", "45", "--interval-seconds", "30", - }, deps) + }, extraArgs...) + exitCode := RunMergePackageReleasePRWithDeps(context.Background(), &stdout, &stderr, args, deps) return exitCode, stdout.String(), stderr.String(), stub } func packageReleasePRListJSON(headSHA string, isDraft bool) string { return fmt.Sprintf( - `[{"number":2002,"headRefName":%q,"headRefOid":%q,"isDraft":%t}]`, - packageReleasePRHeadBranch, headSHA, isDraft) + `[{"number":%d,"headRefName":%q,"headRefOid":%q,"isDraft":%t}]`, + packageReleasePRNumber, packageReleasePRHeadBranch, headSHA, isDraft) } func packageReleasePRWorkflows() []string { @@ -136,16 +210,16 @@ func packageReleasePRPinAt(headSHA string, pinnedTag string) map[string]string { return map[string]string{headSHA: pinnedTag} } -// Verifies the merge waits for the rebased pin, for the draft window, and for the rebased head's own check runs, then merges exactly once against that head — reading the pin, the runs, and the merge target from the same head SHA. -func TestMergePackageReleasePRWaitsForPinDraftAndChecks(t *testing.T) { +// Verifies the merge waits for the rebased pin and for the rebased head's own check runs, then lifts the draft and merges exactly once against that head — reading the pin, the runs, and the merge target from the same head SHA. +func TestMergePackageReleasePRReadiesDraftPullRequestOnceChecksPass(t *testing.T) { exitCode, stdout, stderr, stub := runMergePackageReleasePRCase(t, []mergePackageReleasePRPoll{ { - prListJSON: packageReleasePRListJSON("stale123", false), + prListJSON: packageReleasePRListJSON("stale123", true), pinnedTagsByRef: packageReleasePRPinAt("stale123", "dispatcher-v3.3.1"), runs: packageReleasePRRunsAt("stale123"), }, { - prListJSON: packageReleasePRListJSON("rebased456", false), + prListJSON: packageReleasePRListJSON("rebased456", true), pinnedTagsByRef: packageReleasePRPinAt("rebased456", "dispatcher-v3.4.0"), runs: map[string]string{}, }, @@ -154,11 +228,6 @@ func TestMergePackageReleasePRWaitsForPinDraftAndChecks(t *testing.T) { pinnedTagsByRef: packageReleasePRPinAt("rebased456", "dispatcher-v3.4.0"), runs: packageReleasePRRunsAt("rebased456"), }, - { - prListJSON: packageReleasePRListJSON("rebased456", false), - pinnedTagsByRef: packageReleasePRPinAt("rebased456", "dispatcher-v3.4.0"), - runs: packageReleasePRRunsAt("rebased456"), - }, }) if exitCode != 0 { @@ -166,15 +235,224 @@ func TestMergePackageReleasePRWaitsForPinDraftAndChecks(t *testing.T) { } assertReleasePRCheckLogContains(t, stdout, "still pins dispatcher-v3.3.1") assertReleasePRCheckLogContains(t, stdout, "has not finished for this head") - assertReleasePRCheckLogContains(t, stdout, "is draft while release checks run") + assertReleasePRCheckLogContains(t, stdout, "Marked Unity package release PR #2002 ready before merging it.") assertReleasePRCheckLogContains(t, stdout, "Merged Unity package release PR #2002 at rebased456; it pins dispatcher-v3.4.0.") commandLogText := strings.Join(stub.commandLog, "\n") assertReleasePRCheckLogContains(t, commandLogText, "?ref=stale123") assertReleasePRCheckLogContains(t, commandLogText, "?ref=rebased456") + assertMergePackageReleasePRReadyPrecedesMerge(t, stub) assertMergePackageReleasePRMergeCount(t, stub, "rebased456", 1) } +// Verifies a pull request that is already out of draft is merged without a redundant gh pr ready, which would fail on it. +func TestMergePackageReleasePRMergesReadyPullRequestWithoutReadying(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCase(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", false), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: packageReleasePRRunsAt("package123"), + }, + }) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogDoesNotContain(t, stdout, "ready before merging it") + assertReleasePRCheckLogDoesNotContain(t, strings.Join(stub.commandLog, "\n"), "gh pr ready") + assertMergePackageReleasePRMergeCount(t, stub, "package123", 1) +} + +// Verifies the release-please path leaves the pull request draft while a dispatcher release is still pending, and settles at once rather than polling for a state only a later workflow can reach. +func TestMergePackageReleasePRLeavesDraftWhileDispatcherReleaseIsPending(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCaseWithArgs(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: packageReleasePRRunsAt("package123"), + openDispatcherPRListJSON: `[{"number":2001}]`, + }, + }, []string{"--dispatcher-tag", "dispatcher-v3.4.0", "--require-no-open-dispatcher-pr"}) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogContains(t, stdout, "PR #2002 stays draft: a dispatcher release pull request is open") + commandLogText := strings.Join(stub.commandLog, "\n") + assertReleasePRCheckLogDoesNotContain(t, commandLogText, "gh pr ready") + assertMergePackageReleasePRNeverMerged(t, stub) + assertMergePackageReleasePRListCount(t, stub, 2) +} + +// Verifies the post-publish path still merges while the next cycle's dispatcher release pull request is open, because that release is not the one this package pin records. +func TestMergePackageReleasePRMergesWithOpenDispatcherPRWhenGateIsOff(t *testing.T) { + exitCode, _, stderr, stub := runMergePackageReleasePRCase(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: packageReleasePRRunsAt("package123"), + openDispatcherPRListJSON: `[{"number":2001}]`, + }, + }) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertMergePackageReleasePRMergeCount(t, stub, "package123", 1) +} + +// Verifies an omitted --dispatcher-tag is resolved from the base branch release manifest and compared against the head pin. +func TestMergePackageReleasePRResolvesDispatcherTagFromManifest(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCaseWithArgs(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.5.0"), + runs: packageReleasePRRunsAt("package123"), + manifestByRef: map[string]string{"main": `{"Packages/src":"3.6.0","cli/dispatcher":"3.5.0"}`}, + }, + }, nil) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogContains(t, stdout, "Resolved dispatcher release tag dispatcher-v3.5.0 from the main release manifest.") + assertMergePackageReleasePRMergeCount(t, stub, "package123", 1) +} + +// Verifies a base branch manifest without a dispatcher version stops the command instead of merging against a guessed tag. +func TestMergePackageReleasePRFailsWhenManifestHasNoDispatcherVersion(t *testing.T) { + exitCode, _, stderr, stub := runMergePackageReleasePRCaseWithArgs(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.5.0"), + runs: packageReleasePRRunsAt("package123"), + manifestByRef: map[string]string{"main": `{"Packages/src":"3.6.0"}`}, + }, + }, nil) + + if exitCode != 1 { + t.Fatalf("expected exit code 1, got %d", exitCode) + } + assertReleasePRCheckLogContains(t, stderr, `has no "cli/dispatcher" version`) + assertMergePackageReleasePRNeverMerged(t, stub) +} + +// Verifies --no-wait decides once on a stale pin and exits without polling, so the release-please run does not hold a runner open. +func TestMergePackageReleasePRWithNoWaitLeavesStalePinDraftAfterOnePass(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCaseWithArgs(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.3.1"), + runs: packageReleasePRRunsAt("package123"), + }, + }, []string{"--dispatcher-tag", "dispatcher-v3.4.0", "--no-wait"}) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogContains(t, stdout, "is not mergeable yet; leaving it draft.") + assertMergePackageReleasePRNeverMerged(t, stub) + assertMergePackageReleasePRListCount(t, stub, 1) +} + +// Verifies losing the merge race to the other automated path is a success rather than a failed job: a merge that fails against an already merged pull request reports it and exits 0. +func TestMergePackageReleasePRAcceptsAPullRequestAnotherRunMerged(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCase(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", false), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: packageReleasePRRunsAt("package123"), + failMerge: true, + stateJSON: `{"state":"MERGED","isDraft":false}`, + }, + }) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogContains(t, stdout, "PR #2002 was already merged by another run") + assertMergePackageReleasePRMergeCount(t, stub, "package123", 1) +} + +// Verifies a gh pr ready that the other path won is absorbed and the merge still runs, so the release is not left half-done. +func TestMergePackageReleasePRContinuesWhenAnotherRunLiftedTheDraft(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCase(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: packageReleasePRRunsAt("package123"), + failReady: true, + stateJSON: `{"state":"OPEN","isDraft":false}`, + }, + }) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogContains(t, stdout, "PR #2002 is already out of draft; continuing to merge it.") + assertMergePackageReleasePRMergeCount(t, stub, "package123", 1) +} + +// Verifies a merge that fails against a pull request nothing else merged is a failure: reporting success there would leave the package release unmade with a green job. +func TestMergePackageReleasePRFailsWhenTheMergeFailsAndThePullRequestIsStillOpen(t *testing.T) { + exitCode, stdout, stderr, _ := runMergePackageReleasePRCase(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", false), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: packageReleasePRRunsAt("package123"), + failMerge: true, + stateJSON: `{"state":"OPEN","isDraft":false}`, + }, + }) + + if exitCode != 1 { + t.Fatalf("expected exit code 1, got %d\nstdout: %s", exitCode, stdout) + } + assertReleasePRCheckLogContains(t, stderr, "gh pr merge failed") + assertReleasePRCheckLogDoesNotContain(t, stdout, "already merged by another run") +} + +// Verifies a failed write that no concurrent run explains still fails the command, so a real permission or ruleset error is not swallowed as a lost race. +func TestMergePackageReleasePRFailsWhenAFailedWriteIsNotARace(t *testing.T) { + exitCode, _, stderr, stub := runMergePackageReleasePRCase(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: packageReleasePRRunsAt("package123"), + failReady: true, + stateJSON: `{"state":"OPEN","isDraft":true}`, + }, + }) + + if exitCode != 1 { + t.Fatalf("expected exit code 1, got %d", exitCode) + } + assertReleasePRCheckLogContains(t, stderr, "gh pr ready failed") + assertMergePackageReleasePRNeverMerged(t, stub) +} + +// Verifies the release manifest is never read before the open-dispatcher gate: reading it first would pair a tag from the pre-merge manifest with an already empty open list, which together read as "nothing pending" for a head that still pins the old dispatcher. +func TestMergePackageReleasePRReadsTheManifestOnlyAfterTheDispatcherGate(t *testing.T) { + exitCode, stdout, stderr, stub := runMergePackageReleasePRCaseWithArgs(t, []mergePackageReleasePRPoll{ + { + prListJSON: packageReleasePRListJSON("package123", true), + pinnedTagsByRef: packageReleasePRPinAt("package123", "dispatcher-v3.4.0"), + runs: packageReleasePRRunsAt("package123"), + manifestByRef: map[string]string{"main": `{"cli/dispatcher":"3.4.0"}`}, + openDispatcherPRListJSON: `[{"number":2001}]`, + }, + }, []string{"--require-no-open-dispatcher-pr"}) + + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) + } + assertReleasePRCheckLogContains(t, stdout, "PR #2002 stays draft: a dispatcher release pull request is open") + commandLogText := strings.Join(stub.commandLog, "\n") + assertReleasePRCheckLogDoesNotContain(t, commandLogText, releasePleaseManifestRelativePath) + assertReleasePRCheckLogDoesNotContain(t, stdout, "Resolved dispatcher release tag") + assertMergePackageReleasePRNeverMerged(t, stub) +} + // Verifies a run that completed on an older head does not count as this head's check result. func TestMergePackageReleasePRIgnoresRunsFromAnotherHead(t *testing.T) { exitCode, stdout, _, stub := runMergePackageReleasePRCase(t, []mergePackageReleasePRPoll{ @@ -306,16 +584,22 @@ func TestMergePackageReleasePRFailsWhenSeveralPullRequestsMatch(t *testing.T) { assertMergePackageReleasePRNeverMerged(t, stub) } +// assertMergePackageReleasePRMergeCount counts merges of the package release +// pull request against one head. The pull request number is part of the match: +// a merge command naming another pull request must not read as this one. func assertMergePackageReleasePRMergeCount(t *testing.T, stub *mergePackageReleasePRStub, headSHA string, expected int) { t.Helper() + expectedMerge := fmt.Sprintf( + "gh pr merge %d --repo owner/repository --squash --match-head-commit %s", packageReleasePRNumber, headSHA) mergeCount := 0 for _, commandLine := range stub.commandLog { - if strings.Contains(commandLine, "--match-head-commit "+headSHA) { + if commandLine == expectedMerge { mergeCount++ } } if mergeCount != expected { - t.Fatalf("expected %d merges against %s, got %d\n%s", expected, headSHA, mergeCount, strings.Join(stub.commandLog, "\n")) + t.Fatalf("expected %d merges matching %q, got %d\n%s", + expected, expectedMerge, mergeCount, strings.Join(stub.commandLog, "\n")) } } @@ -323,3 +607,33 @@ func assertMergePackageReleasePRNeverMerged(t *testing.T, stub *mergePackageRele t.Helper() assertReleasePRCheckLogDoesNotContain(t, strings.Join(stub.commandLog, "\n"), "--match-head-commit") } + +func assertMergePackageReleasePRListCount(t *testing.T, stub *mergePackageReleasePRStub, expected int) { + t.Helper() + listCount := 0 + for _, commandLine := range stub.commandLog { + if strings.HasPrefix(commandLine, "gh pr list ") { + listCount++ + } + } + if listCount != expected { + t.Fatalf("expected %d pull request listings, got %d\n%s", expected, listCount, strings.Join(stub.commandLog, "\n")) + } +} + +func assertMergePackageReleasePRReadyPrecedesMerge(t *testing.T, stub *mergePackageReleasePRStub) { + t.Helper() + readyIndex := -1 + for index, commandLine := range stub.commandLog { + if strings.HasPrefix(commandLine, "gh pr ready ") { + readyIndex = index + } + if strings.Contains(commandLine, "--match-head-commit") { + if readyIndex == -1 || readyIndex > index { + t.Fatalf("expected gh pr ready before the merge\n%s", strings.Join(stub.commandLog, "\n")) + } + return + } + } + t.Fatalf("expected a merge command\n%s", strings.Join(stub.commandLog, "\n")) +} diff --git a/cli/release-automation/internal/automation/package_release_pr_ready_merge.go b/cli/release-automation/internal/automation/package_release_pr_ready_merge.go new file mode 100644 index 0000000000..607b1891fd --- /dev/null +++ b/cli/release-automation/internal/automation/package_release_pr_ready_merge.go @@ -0,0 +1,190 @@ +package automation + +import ( + "context" + "encoding/json" + "fmt" + "io" + "strconv" +) + +// mergePackageReleasePRMergedState is the state gh reports for a pull request +// another run already merged. +const mergePackageReleasePRMergedState = "MERGED" + +type mergePackageReleasePRState struct { + State string `json:"state"` + IsDraft bool `json:"isDraft"` +} + +// readyAndMergePackageReleasePR performs the two writes that release the +// package. Both paths that call this command wait for the same head to go +// green, so the release-please run started by the pin stamp can reach here +// while post-publish is still polling. Losing that race is a success: a failed +// write is re-read, and a pull request the other run already merged reports +// what happened instead of failing the job. +func readyAndMergePackageReleasePR( + ctx context.Context, + stdout io.Writer, + stderr io.Writer, + config mergePackageReleasePRConfig, + releasePR mergePackageReleasePullRequest, + pinnedTag string, + deps mergePackageReleasePRDeps, +) (settled bool, exitCode int) { + if releasePR.IsDraft { + settled, exitCode := readyPackageReleasePRBeforeMerge(ctx, stdout, stderr, config, releasePR, deps) + if settled { + return true, exitCode + } + } + + mergeErr := squashMergePackageReleasePR(ctx, config, releasePR, deps) + if mergeErr != nil { + return true, resolvePackageReleasePRMergeFailure(ctx, stdout, stderr, config, releasePR, mergeErr, deps) + } + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "Merged Unity package release PR #%d at %s; it pins %s.", releasePR.Number, releasePR.HeadRefOID, pinnedTag)) + return true, 0 +} + +// readyPackageReleasePRBeforeMerge lifts the draft. settled is false only when +// the merge is still worth attempting: either the draft was lifted here, or the +// re-read shows another run lifted it first. +func readyPackageReleasePRBeforeMerge( + ctx context.Context, + stdout io.Writer, + stderr io.Writer, + config mergePackageReleasePRConfig, + releasePR mergePackageReleasePullRequest, + deps mergePackageReleasePRDeps, +) (settled bool, exitCode int) { + readyErr := markPackageReleasePRReady(ctx, config, releasePR, deps) + if readyErr == nil { + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "Marked Unity package release PR #%d ready before merging it.", releasePR.Number)) + return false, 0 + } + + state, err := packageReleasePullRequestState(ctx, config, releasePR, deps) + if err != nil { + writeMergePackageReleasePRLine(stderr, readyErr) + writeMergePackageReleasePRLine(stderr, err) + return true, 1 + } + if state.State == mergePackageReleasePRMergedState { + writeMergePackageReleasePRLine(stdout, packageReleasePRAlreadyMergedMessage(releasePR)) + return true, 0 + } + // Still draft means the command genuinely could not lift it -- a permission + // or ruleset failure, not a lost race. + if state.IsDraft { + writeMergePackageReleasePRLine(stderr, readyErr) + return true, 1 + } + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "Unity package release PR #%d is already out of draft; continuing to merge it.", releasePR.Number)) + return false, 0 +} + +// resolvePackageReleasePRMergeFailure decides what a failed merge means. Only +// a pull request another run already merged is a success here: any other state +// leaves the package unreleased, so reporting anything but a failure would let +// the release silently not happen. +func resolvePackageReleasePRMergeFailure( + ctx context.Context, + stdout io.Writer, + stderr io.Writer, + config mergePackageReleasePRConfig, + releasePR mergePackageReleasePullRequest, + mergeErr error, + deps mergePackageReleasePRDeps, +) int { + state, err := packageReleasePullRequestState(ctx, config, releasePR, deps) + if err != nil { + writeMergePackageReleasePRLine(stderr, mergeErr) + writeMergePackageReleasePRLine(stderr, err) + return 1 + } + if state.State == mergePackageReleasePRMergedState { + writeMergePackageReleasePRLine(stdout, packageReleasePRAlreadyMergedMessage(releasePR)) + return 0 + } + writeMergePackageReleasePRLine(stderr, mergeErr) + return 1 +} + +func packageReleasePRAlreadyMergedMessage(releasePR mergePackageReleasePullRequest) string { + return fmt.Sprintf( + "Unity package release PR #%d was already merged by another run; nothing left to do.", releasePR.Number) +} + +// packageReleasePullRequestState re-reads the state a write may have raced +// with. Draft and merged are the two outcomes a concurrent run produces, so +// both are read in one call. +func packageReleasePullRequestState( + ctx context.Context, + config mergePackageReleasePRConfig, + releasePR mergePackageReleasePullRequest, + deps mergePackageReleasePRDeps, +) (mergePackageReleasePRState, error) { + output, err := deps.runOutput( + ctx, + "gh", + "pr", + "view", + strconv.Itoa(releasePR.Number), + "--repo", + config.repository, + "--json", + "state,isDraft", + ) + if err != nil { + return mergePackageReleasePRState{}, err + } + + state := mergePackageReleasePRState{} + err = json.Unmarshal([]byte(output), &state) + if err != nil { + return mergePackageReleasePRState{}, fmt.Errorf( + "%s: failed to parse the state of PR #%d: %w", mergePackageReleasePRCommandName, releasePR.Number, err) + } + return state, nil +} + +// markPackageReleasePRReady lifts the draft state the release PR check +// automation leaves in place. Only draft pull requests are readied, because +// gh pr ready fails on one that is already ready. +func markPackageReleasePRReady( + ctx context.Context, + config mergePackageReleasePRConfig, + releasePR mergePackageReleasePullRequest, + deps mergePackageReleasePRDeps, +) error { + _, err := deps.runOutput(ctx, "gh", "pr", "ready", strconv.Itoa(releasePR.Number), "--repo", config.repository) + return err +} + +// squashMergePackageReleasePR pins the merge to the head this command +// verified, so a head moved between the check and the merge fails instead of +// releasing an unchecked commit. +func squashMergePackageReleasePR( + ctx context.Context, + config mergePackageReleasePRConfig, + releasePR mergePackageReleasePullRequest, + deps mergePackageReleasePRDeps, +) error { + _, err := deps.runOutput( + ctx, + "gh", + "pr", + "merge", + strconv.Itoa(releasePR.Number), + "--repo", + config.repository, + "--squash", + "--match-head-commit", + releasePR.HeadRefOID, + ) + return err +} diff --git a/cli/release-automation/internal/automation/release_pr_checks.go b/cli/release-automation/internal/automation/release_pr_checks.go index 03e19a8ef9..662efda2dd 100644 --- a/cli/release-automation/internal/automation/release_pr_checks.go +++ b/cli/release-automation/internal/automation/release_pr_checks.go @@ -237,6 +237,17 @@ func finalizeReleasePRChecks( return 0 } + // Readying the Unity package release PR would let a person merge it before + // the dispatcher it must ship is released and stamped. It stays draft, and + // the merge automation lifts the draft itself right before merging + // (docs/dispatcher-pin-release-order.md). + if releasePRCheckComponentFromHeadRef(releasePR.HeadRefName, config.targetBranch) == mergePackageReleasePRComponent { + writeReleasePRCheckLine(stdout, fmt.Sprintf( + "Leaving release PR #%d in draft: the Unity package release pull request is merged only by automation (docs/dispatcher-pin-release-order.md).", + releasePR.Number)) + return 0 + } + err = markReleasePRCheckReady(ctx, config, releasePR, deps) if err != nil { writeReleasePRCheckLine(stderr, err) diff --git a/cli/release-automation/internal/automation/release_pr_checks_test.go b/cli/release-automation/internal/automation/release_pr_checks_test.go index 0420d9450e..9df8c1c9e4 100644 --- a/cli/release-automation/internal/automation/release_pr_checks_test.go +++ b/cli/release-automation/internal/automation/release_pr_checks_test.go @@ -870,8 +870,8 @@ func multiReleasePRCheckHeadSHA(commandLine string) string { return "dispatcher123" } -// Verifies that every pending per-component release PR is drafted, checked, and marked ready. -func TestReleasePRChecksMarkEveryComponentPullRequestReady(t *testing.T) { +// Verifies that the dispatcher release PR is marked ready after its checks pass while the Unity package one stays draft, so only automation can merge the package release. +func TestReleasePRChecksLeaveUnityPackagePullRequestDraft(t *testing.T) { prListJSON := `[` + `{"number":2001,"headRefName":"release-please--branches--main--components--dispatcher","headRefOid":"dispatcher123","title":"chore(main): release dispatcher 3.6.0","url":"https://example.test/pr/2001"},` + `{"number":2002,"headRefName":"release-please--branches--main--components--unity-package","headRefOid":"package123","title":"chore(main): release 3.6.0","url":"https://example.test/pr/2002"}` + @@ -883,13 +883,14 @@ func TestReleasePRChecksMarkEveryComponentPullRequestReady(t *testing.T) { t.Fatalf("expected exit code 0, got %d\nstderr: %s", exitCode, stderr) } assertReleasePRCheckLogContains(t, stdout, "Marked release PR #2001 as ready after checks passed.") - assertReleasePRCheckLogContains(t, stdout, "Marked release PR #2002 as ready after checks passed.") + assertReleasePRCheckLogContains(t, stdout, "Leaving release PR #2002 in draft: the Unity package release pull request is merged only by automation") + assertReleasePRCheckLogDoesNotContain(t, stdout, "Marked release PR #2002 as ready after checks passed.") commandLogText := strings.Join(stub.commandLog, "\n") assertReleasePRCheckLogContainsLine(t, commandLogText, "gh pr ready 2001 --repo owner/repository") - assertReleasePRCheckLogContainsLine(t, commandLogText, "gh pr ready 2002 --repo owner/repository") + assertReleasePRCheckLogDoesNotContainLine(t, commandLogText, "gh pr ready 2002 --repo owner/repository") } -// Verifies that one component's failing checks still let the other component's release PR be marked ready, and the command reports the failure. +// Verifies that one component's failing checks still let the other component's release PR be finalized, and the command reports the failure. func TestReleasePRChecksContinueAfterOneComponentFails(t *testing.T) { prListJSON := `[` + `{"number":2001,"headRefName":"release-please--branches--main--components--dispatcher","headRefOid":"dispatcher123","title":"chore(main): release dispatcher 3.6.0","url":"https://example.test/pr/2001"},` + @@ -902,10 +903,9 @@ func TestReleasePRChecksContinueAfterOneComponentFails(t *testing.T) { t.Fatalf("expected exit code 1, got %d\nstderr: %s", exitCode, stderr) } assertReleasePRCheckLogContains(t, stderr, "gh run watch 11 failed") - assertReleasePRCheckLogContains(t, stdout, "Marked release PR #2002 as ready after checks passed.") + assertReleasePRCheckLogContains(t, stdout, "Leaving release PR #2002 in draft") commandLogText := strings.Join(stub.commandLog, "\n") assertReleasePRCheckLogDoesNotContainLine(t, commandLogText, "gh pr ready 2001 --repo owner/repository") - assertReleasePRCheckLogContainsLine(t, commandLogText, "gh pr ready 2002 --repo owner/repository") } // Verifies that a non-package component's release PR body is left alone, so its bare version summary is not relabeled as the Unity package. diff --git a/docs/adr/0007-separate-release-prs-and-package-auto-merge.md b/docs/adr/0007-separate-release-prs-and-package-auto-merge.md index 2a779da7a4..677e0e9a17 100644 --- a/docs/adr/0007-separate-release-prs-and-package-auto-merge.md +++ b/docs/adr/0007-separate-release-prs-and-package-auto-merge.md @@ -113,3 +113,46 @@ becomes reproducible so the pin's asset digests can be computed before the release exists. In either case the components can share a release pull request again and the wait-and-merge step becomes unnecessary. Finding three release pull requests noisier than one is not a reversal condition. + +## Amendment (2026-09-08) + +The original decision left the Unity package release pull request mergeable by +hand: the release PR check automation marked every component's pull request +ready once its checks passed. That reopened the lag this ADR removes — a person +could merge the package release before the dispatcher one — and it also exposed +a narrower window in which `check-package-release-pin` is green on a head that +predates the dispatcher merge, so merging there would ship an already stale pin. + +**Decision.** The unity-package release pull request stays draft. Only +automation merges it, and each path lifts the draft immediately before merging: + +- `release-please.yml`, after the release PR checks, merges it only while **no + dispatcher release pull request is open** and the pin at the package head + records the dispatcher version `main`'s manifest releases. Otherwise it leaves + the pull request draft and reconsiders on the next push. This is the path a + package release with no dispatcher bump takes. +- `dispatcher-publish.yml`'s `post-publish`, after the pin stamp, merges it when + the head pin records the tag it just published. It does **not** require the + absence of an open dispatcher pull request: by then the next cycle's + dispatcher release can already be pending, and requiring its absence would + make the step wait out its timeout on every second release. + +Neither path treats the draft flag as evidence. What proves a head was checked +is a successful run of every required workflow for that exact head SHA, held to +that head by `--match-head-commit`. + +**Why the two conditions differ.** An open dispatcher pull request disappearing +is not proof the stamp landed: the pull request closes on merge, the stamp +arrives 15–25 minutes later, and in between `main`'s manifest names the new +dispatcher while the package head still pins the old one. Requiring the pin to +match the manifest closes that window; requiring only "no open dispatcher pull +request" would not. + +**Limits.** This prevents a misplaced click, not a determined one. Anyone with +write access can press "Ready for review" and merge. It is a guard rail, not +access control; the enforcement of correctness stays with +`check-package-pin-consistency`, which fails the release commit itself. + +`release-please.yml` therefore mints the same GitHub App token +`post-publish` uses. A merge made with `GITHUB_TOKEN` starts no follow-up +workflow run, so the package release would never be tagged. diff --git a/docs/dispatcher-pin-release-order.md b/docs/dispatcher-pin-release-order.md index 176fbca999..33c5aebb5b 100644 --- a/docs/dispatcher-pin-release-order.md +++ b/docs/dispatcher-pin-release-order.md @@ -31,21 +31,38 @@ any later push to `main`. ## Release pull requests release-please opens **one release pull request per component**: `unity-package`, -`dispatcher`, and `uloop-project-runner`. Merge order matters for the first two. +`dispatcher`, and `uloop-project-runner`. The **unity-package** one stays draft +and is merged only by automation; the other two are the ones people merge. 1. Merge the **dispatcher** release pull request and approve the `cli-release` environment. Publishing the dispatcher is the only human decision. -2. `post-publish` stamps the pin on `main` and then merges the **unity-package** - release pull request itself, once that pull request's head records the - dispatcher tag just published, is out of draft, and has a successful run of - every required workflow for that exact head commit. -3. The **project runner** release pull request carries no cross-component +2. `post-publish` stamps the pin on `main`, then marks the **unity-package** + release pull request ready and merges it, once that pull request's head + records the dispatcher tag just published and has a successful run of every + required workflow for that exact head commit. +3. A package release that carries no dispatcher bump never reaches step 2, so + `release-please.yml` merges it instead: after the release PR checks, it + marks the package pull request ready and merges it, but only while no + dispatcher release pull request is open **and** the pin at the package head + already records the dispatcher version `main`'s manifest releases. Otherwise + it leaves the pull request draft and reconsiders on the next push to `main`. +4. The **project runner** release pull request carries no cross-component ordering constraint and can be merged at any time. -Merging the package pull request before the dispatcher one does not break -anything — that commit's manifest and pin agree, so the release is valid — but -the package then ships the *previous* dispatcher, which is the lag this order -exists to remove. +Merging the package pull request before the dispatcher one would ship the +*previous* dispatcher, which is the lag this order exists to remove, so it is +not offered: the pull request is left in draft and the merge button is not +available. Doing it anyway takes two deliberate actions — "Ready for review", +then merge — rather than one misplaced click. + +Both automated paths reason about the pin at the pull request head, never about +the draft flag. "No dispatcher release pull request is open" alone does not mean +the stamp landed: the pull request disappears from the open list the moment it +merges, while the stamp arrives 15–25 minutes later. That window is why the +`release-please.yml` path also requires the head pin to match `main`'s manifest. +The `post-publish` path deliberately does *not* require the absence of an open +dispatcher pull request, because the next cycle's dispatcher release can already +be pending by the time the stamp lands. The config sets `always-update: true` alongside `separate-pull-requests`. Without it release-please pushes a release branch only when the pull request body @@ -74,7 +91,9 @@ says nothing about the pull request. Before merging, confirm all four: - the pin freshness gate on `main` is green, so the stamp reached `main`; - the pull request's head commit records the dispatcher tag just published in `Packages/src/project-runner-pin.json`; -- the pull request is not a draft; +- no dispatcher release pull request is open, or the one that is open belongs to + the *next* cycle and the head pin already records the dispatcher `main` + releases; - every required workflow has a completed successful run for that exact head SHA — not for an earlier head. @@ -82,6 +101,15 @@ Merging without those is how a stale or unvalidated package release gets published, which is the failure this whole order exists to prevent. Decision record: `docs/adr/0007-separate-release-prs-and-package-auto-merge.md`. +Once all four hold, take the pull request out of draft yourself ("Ready for +review") and merge it. A **pre-release** dispatcher is never stamped, so a +package release waiting on one stays draft indefinitely: publish a stable +dispatcher, or merge by hand as above. + +When the stamp itself fails, the package pull request simply stays draft. Re-run +the `post-publish` job of the `dispatcher-publish` run: it stamps the new tip and +merges the pull request in the same job. + ## Why the stamp is pushed without a pull request Until 2026-09 the stamp travelled as an automated pull request that a human