From 3226ce202d600c68ecd9ae14b0894fb5b672c390 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 21:35:30 +0900 Subject: [PATCH 01/10] chore: read the release manifest and dispatcher PR state at a ref The Unity package release merge gate has to reason about commits that are not in the workflow checkout: the pin at a pull request head, and the dispatcher version the base branch tip releases. Both come from the contents API, and the base64 unwrapping was already duplicated inside the pin reader. Collect the three reads the gate needs -- file content at a ref, the dispatcher release tag from the release manifest at a ref, and whether a dispatcher release pull request is still open -- in one file, and route the existing pin reader through the shared content helper. The dispatcher listing deliberately does not filter on the pending label: a release pull request that lost its label is still a dispatcher release the package must not overtake. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- .../automation/package_release_pr_gate.go | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 cli/release-automation/internal/automation/package_release_pr_gate.go 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..976660fe69 --- /dev/null +++ b/cli/release-automation/internal/automation/package_release_pr_gate.go @@ -0,0 +1,120 @@ +package automation + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "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) { + output, err := runOutput( + ctx, + "gh", + "api", + "repos/"+repository+"/contents/"+path+"?ref="+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 +} From 8818e5681a7d0be8cc99f8c03b276e75da23b1ba Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 21:35:43 +0900 Subject: [PATCH 02/10] feat: let the package merge helper lift the draft itself The Unity package release pull request is about to stay draft permanently, so the merge helper can no longer wait for someone else to make it ready. It now marks the pull request ready itself, immediately before merging, and only when it is actually draft -- gh pr ready fails on one that is already ready. Dropping the draft wait loses nothing. Draft was never evidence that a head was checked; what proves that is a successful run of every required workflow for that exact head SHA, held to that head by --match-head-commit. The comment on that branch now says so. Two flags make the helper usable from release-please.yml as well as from post-publish. --require-no-open-dispatcher-pr refuses to merge while a dispatcher release pull request is open, and settles at once rather than polling, because that state is resolved by a release and a stamp rather than by waiting. --no-wait decides once and leaves the pull request draft, so the release-please run does not hold a runner open for the timeout. An omitted --dispatcher-tag is resolved from the base branch release manifest, which is the value the release-please path has to compare the head pin against. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- .../automation/package_release_pr_merge.go | 143 ++++++++---- .../package_release_pr_merge_test.go | 205 ++++++++++++++++-- 2 files changed, 291 insertions(+), 57 deletions(-) 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..f2381980b6 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,11 @@ package automation import ( "context" - "encoding/base64" "encoding/json" "flag" "fmt" "io" "strconv" - "strings" "time" ) @@ -20,12 +18,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 { @@ -68,9 +68,37 @@ func RunMergePackageReleasePRWithDeps( writeMergePackageReleasePRLine(stderr, err) return 1 } + config, err = resolveMergePackageReleasePRDispatcherTag(ctx, stdout, config, deps) + if err != nil { + writeMergePackageReleasePRLine(stderr, err) + return 1 + } return waitAndMergePackageReleasePR(ctx, stdout, stderr, config, deps) } +// resolveMergePackageReleasePRDispatcherTag fills in the dispatcher tag from +// the base branch tip when the caller names none. The release-please path has +// no published tag to pass, so the tag the base branch already releases is what +// the package pin has to record for the package to ship the current dispatcher. +func resolveMergePackageReleasePRDispatcherTag( + ctx context.Context, + stdout io.Writer, + config mergePackageReleasePRConfig, + deps mergePackageReleasePRDeps, +) (mergePackageReleasePRConfig, error) { + if config.dispatcherTag != "" { + return config, nil + } + dispatcherTag, err := dispatcherReleaseTagFromManifestAtRef(ctx, deps.runOutput, config.repository, config.baseBranch) + if err != nil { + return config, err + } + config.dispatcherTag = dispatcherTag + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "Resolved dispatcher release tag %s from the %s release manifest.", dispatcherTag, config.baseBranch)) + return config, nil +} + func defaultMergePackageReleasePRDeps() mergePackageReleasePRDeps { return mergePackageReleasePRDeps{ now: releasePRCheckNow, @@ -83,7 +111,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 +124,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 +137,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,6 +161,13 @@ 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", @@ -166,6 +202,22 @@ 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 + } + } + pinnedTag, err := packageReleasePullRequestPinnedTag(ctx, config, releasePR, deps) if err != nil { writeMergePackageReleasePRLine(stderr, err) @@ -177,15 +229,10 @@ func attemptMergePackageReleasePR( 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,6 +241,16 @@ func attemptMergePackageReleasePR( return false, 0 } + if releasePR.IsDraft { + err = markPackageReleasePRReady(ctx, config, releasePR, deps) + if err != nil { + writeMergePackageReleasePRLine(stderr, err) + return true, 1 + } + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "Marked Unity package release PR #%d ready before merging it.", releasePR.Number)) + } + err = squashMergePackageReleasePR(ctx, config, releasePR, deps) if err != nil { writeMergePackageReleasePRLine(stderr, err) @@ -266,27 +323,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,6 +421,19 @@ func packageReleasePullRequestRun( return mergePackageReleasePRRun{}, false, 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. 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..e6e25319c2 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 @@ -21,6 +21,12 @@ 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 } type mergePackageReleasePRStub struct { @@ -44,11 +50,27 @@ func (stub *mergePackageReleasePRStub) runOutput(ctx context.Context, name strin stub.commandLog = append(stub.commandLog, commandLine) 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 "[]", nil + } + return stub.activePoll.openDispatcherPRListJSON, 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 + case strings.HasPrefix(commandLine, "gh pr ready "): + return "", nil + 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] @@ -70,6 +92,18 @@ func (stub *mergePackageReleasePRStub) runOutput(ctx context.Context, name strin return "", fmt.Errorf("unexpected command: %s", commandLine) } +// manifestAt answers the release manifest read. The manifest is read before +// the first pull request listing, so the first poll supplies it until a pass +// has opened. +func (stub *mergePackageReleasePRStub) manifestAt(ref string) (string, bool) { + manifestByRef := stub.activePoll.manifestByRef + if manifestByRef == nil && len(stub.polls) > 0 { + manifestByRef = stub.polls[0].manifestByRef + } + manifest, known := manifestByRef[ref] + return manifest, known +} + func mergePackageReleasePRRequestedRef(commandLine string) string { _, ref, found := strings.Cut(commandLine, "?ref=") if !found { @@ -80,6 +114,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,13 +141,13 @@ 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 } @@ -136,16 +179,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 +197,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 +204,126 @@ 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 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{ @@ -323,3 +472,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")) +} From 718d6ce6d97c477f3f747ab04b33b4332df195d4 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 21:35:43 +0900 Subject: [PATCH 03/10] fix: leave the Unity package release pull request in draft Marking the package release pull request ready after its checks passed let a person merge it before the dispatcher it must ship was released and stamped, which reopens the generation lag the split release pull requests removed. It also exposed a narrower window: between the dispatcher merge and the pin stamp, the package head's checks are green on a pin that is already stale. The finalize step now leaves that one component draft and says why. The other components are unaffected -- the dispatcher and project runner release pull requests are still the ones people merge. Merging the package release is left entirely to the two automated paths, which lift the draft themselves. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- .../internal/automation/release_pr_checks.go | 11 +++++++++++ .../internal/automation/release_pr_checks_test.go | 14 +++++++------- 2 files changed, 18 insertions(+), 7 deletions(-) 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. From e4b5f4f7fd59c5fff81373229fe8a5e2f3f9c117 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 21:35:57 +0900 Subject: [PATCH 04/10] ci: merge a dispatcher-free package release from release-please A package release that carries no dispatcher bump never reaches post-publish, so with the pull request now permanently draft it would never be merged at all. release-please.yml gains that path: after the release PR checks it mints the same App token post-publish uses and runs the merge helper with --require-no-open-dispatcher-pr --no-wait. The App token is not optional -- a merge made with GITHUB_TOKEN starts no follow-up workflow run, so the package release would never be tagged. The two new steps also run when the dispatch step failed. That step reports a failure when any component's checks fail, and a failing project runner release must not strand a package-only release until the next push. The merge helper verifies the package head's own runs, so another component's failure cannot make it merge something unchecked; the job still fails on the dispatch step. post-publish keeps its arguments. It must not require the absence of an open dispatcher pull request: by the time it runs, the next cycle's dispatcher release can already be pending. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- .github/workflows/dispatcher-publish.yml | 4 +++ .github/workflows/release-please.yml | 39 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+) 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 From a777c8a8d3f28d79a9ffebb6507a50b3989e0e72 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 21:35:57 +0900 Subject: [PATCH 05/10] docs: record that the package release PR is merged only by automation State the new shape where the release order is documented: the unity-package release pull request stays draft, the two automated paths that merge it, and why their conditions differ. An open dispatcher pull request disappearing is not proof the stamp landed -- the pull request closes on merge and the stamp arrives 15-25 minutes later -- so the release-please path also requires the head pin to match what main's manifest releases, while post-publish requires only the tag it just published. The manual checklist keeps its place for the cases automation does not cover, with the draft condition turned into the two deliberate actions a person now has to take, plus what to do when a pre-release or a failed stamp leaves the pull request waiting. ADR 0007 records the amendment, including its limit: this prevents a misplaced click, not a determined one. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- AGENTS.md | 4 +- ...rate-release-prs-and-package-auto-merge.md | 43 ++++++++++++++++ docs/dispatcher-pin-release-order.md | 50 +++++++++++++++---- 3 files changed, 84 insertions(+), 13 deletions(-) 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/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 From 6673cb9ed440adf195e69ee4249017be8352bf2a Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 21:45:30 +0900 Subject: [PATCH 06/10] fix: survive the two merge paths reaching the pull request together Two review findings on the same window, both about the moment the package head goes green. The pin stamp pushes to main, which starts a release-please run whose merge step waits for the same head the polling post-publish run is waiting for. Both then lift the draft and merge within seconds of each other, and the loser failed the job on "already ready" or "already merged". A failed write is now re-read: a pull request another run merged reports that and exits 0, a draft another run already lifted continues to the merge, and a failure neither explains still fails, so a permission or ruleset error is not swallowed as a lost race. The dispatcher tag was also resolved once, before the wait loop and therefore before the open-dispatcher check. A dispatcher release pull request merged in between left a tag read from the pre-merge manifest paired with an already empty open list: together those read as "nothing pending" for a head that still pins the old dispatcher, which is the state the gate exists to catch. The tag is now resolved inside each pass, after the gate, and the timeout report names the source rather than a value when the caller named no tag. The two writes and the state re-read move to their own file, which keeps the merge helper under the file-length limit. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- .../automation/package_release_pr_merge.go | 104 ++++-------- .../package_release_pr_merge_test.go | 104 +++++++++++- .../package_release_pr_ready_merge.go | 160 ++++++++++++++++++ 3 files changed, 287 insertions(+), 81 deletions(-) create mode 100644 cli/release-automation/internal/automation/package_release_pr_ready_merge.go 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 f2381980b6..2145a74335 100644 --- a/cli/release-automation/internal/automation/package_release_pr_merge.go +++ b/cli/release-automation/internal/automation/package_release_pr_merge.go @@ -6,7 +6,6 @@ import ( "flag" "fmt" "io" - "strconv" "time" ) @@ -68,35 +67,32 @@ func RunMergePackageReleasePRWithDeps( writeMergePackageReleasePRLine(stderr, err) return 1 } - config, err = resolveMergePackageReleasePRDispatcherTag(ctx, stdout, config, deps) - if err != nil { - writeMergePackageReleasePRLine(stderr, err) - return 1 - } return waitAndMergePackageReleasePR(ctx, stdout, stderr, config, deps) } -// resolveMergePackageReleasePRDispatcherTag fills in the dispatcher tag from -// the base branch tip when the caller names none. The release-please path has -// no published tag to pass, so the tag the base branch already releases is what -// the package pin has to record for the package to ship the current dispatcher. -func resolveMergePackageReleasePRDispatcherTag( +// 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, -) (mergePackageReleasePRConfig, error) { +) (string, error) { if config.dispatcherTag != "" { - return config, nil + return config.dispatcherTag, nil } dispatcherTag, err := dispatcherReleaseTagFromManifestAtRef(ctx, deps.runOutput, config.repository, config.baseBranch) if err != nil { - return config, err + return "", err } - config.dispatcherTag = dispatcherTag writeMergePackageReleasePRLine(stdout, fmt.Sprintf( "Resolved dispatcher release tag %s from the %s release manifest.", dispatcherTag, config.baseBranch)) - return config, nil + return dispatcherTag, nil } func defaultMergePackageReleasePRDeps() mergePackageReleasePRDeps { @@ -171,7 +167,7 @@ func waitAndMergePackageReleasePR( 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) @@ -182,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. @@ -218,12 +224,18 @@ func attemptMergePackageReleasePR( } } + 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)) @@ -241,24 +253,7 @@ func attemptMergePackageReleasePR( return false, 0 } - if releasePR.IsDraft { - err = markPackageReleasePRReady(ctx, config, releasePR, deps) - if err != nil { - writeMergePackageReleasePRLine(stderr, err) - return true, 1 - } - writeMergePackageReleasePRLine(stdout, fmt.Sprintf( - "Marked Unity package release PR #%d ready before merging it.", releasePR.Number)) - } - - 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( @@ -421,43 +416,6 @@ func packageReleasePullRequestRun( return mergePackageReleasePRRun{}, false, 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 -} - 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 e6e25319c2..7f8e4a3642 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 @@ -27,6 +27,12 @@ type mergePackageReleasePRPoll struct { // 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 { @@ -63,7 +69,12 @@ func (stub *mergePackageReleasePRStub) runOutput(ctx context.Context, name strin stub.activePoll = stub.nextPoll() return stub.activePoll.prListJSON, nil case strings.HasPrefix(commandLine, "gh pr ready "): + if stub.activePoll.failReady { + return "", fmt.Errorf("gh pr ready failed") + } return "", nil + case strings.HasPrefix(commandLine, "gh pr view "): + return stub.activePoll.stateJSON, nil case strings.Contains(commandLine, releasePleaseManifestRelativePath): ref := mergePackageReleasePRRequestedRef(commandLine) manifest, known := stub.manifestAt(ref) @@ -87,20 +98,18 @@ func (stub *mergePackageReleasePRStub) runOutput(ctx context.Context, name strin } return "[]", nil case strings.Contains(commandLine, " --match-head-commit "): + if stub.activePoll.failMerge { + return "", fmt.Errorf("gh pr merge failed") + } return "", nil } return "", fmt.Errorf("unexpected command: %s", commandLine) } -// manifestAt answers the release manifest read. The manifest is read before -// the first pull request listing, so the first poll supplies it until a pass -// has opened. +// 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) { - manifestByRef := stub.activePoll.manifestByRef - if manifestByRef == nil && len(stub.polls) > 0 { - manifestByRef = stub.polls[0].manifestByRef - } - manifest, known := manifestByRef[ref] + manifest, known := stub.activePoll.manifestByRef[ref] return manifest, known } @@ -324,6 +333,85 @@ func TestMergePackageReleasePRWithNoWaitLeavesStalePinDraftAfterOnePass(t *testi 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 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{ 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..61e002dd62 --- /dev/null +++ b/cli/release-automation/internal/automation/package_release_pr_ready_merge.go @@ -0,0 +1,160 @@ +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 { + readyErr := markPackageReleasePRReady(ctx, config, releasePR, deps) + if readyErr != nil { + resolved, exitCode := resolvePackageReleasePRWriteFailure(ctx, stdout, stderr, config, releasePR, readyErr, deps) + if resolved { + return true, exitCode + } + } else { + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "Marked Unity package release PR #%d ready before merging it.", releasePR.Number)) + } + } + + mergeErr := squashMergePackageReleasePR(ctx, config, releasePR, deps) + if mergeErr != nil { + _, exitCode := resolvePackageReleasePRWriteFailure(ctx, stdout, stderr, config, releasePR, mergeErr, deps) + return true, exitCode + } + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "Merged Unity package release PR #%d at %s; it pins %s.", releasePR.Number, releasePR.HeadRefOID, pinnedTag)) + return true, 0 +} + +// resolvePackageReleasePRWriteFailure re-reads the pull request after a failed +// write and decides whether the failure was the other run getting there first. +// resolved is false only when the pull request is still open and out of draft, +// which means the draft was already lifted and the merge is still worth +// attempting; every other outcome is final and carries the exit code. +func resolvePackageReleasePRWriteFailure( + ctx context.Context, + stdout io.Writer, + stderr io.Writer, + config mergePackageReleasePRConfig, + releasePR mergePackageReleasePullRequest, + writeErr error, + deps mergePackageReleasePRDeps, +) (resolved bool, exitCode int) { + state, err := packageReleasePullRequestState(ctx, config, releasePR, deps) + if err != nil { + writeMergePackageReleasePRLine(stderr, writeErr) + writeMergePackageReleasePRLine(stderr, err) + return true, 1 + } + if state.State == mergePackageReleasePRMergedState { + writeMergePackageReleasePRLine(stdout, fmt.Sprintf( + "Unity package release PR #%d was already merged by another run; nothing left to do.", releasePR.Number)) + return true, 0 + } + if state.IsDraft { + writeMergePackageReleasePRLine(stderr, writeErr) + 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 +} + +// 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 +} From 0b0fe0a736c47e3328f4d9eb9c80c66fb2583517 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 21:56:04 +0900 Subject: [PATCH 07/10] test: split the merge stub by the command it answers The stub grew a case per new behaviour and reached cyclomatic complexity 16, which the Complexity Report fails on. scripts/check-go-cli.sh does not lint test files, so it stayed green locally. Split it along the seam it already had: the pull request reads and writes, and the contents API and run listings. No case changes behaviour. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- .../package_release_pr_merge_test.go | 41 ++++++++++++++----- 1 file changed, 30 insertions(+), 11 deletions(-) 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 7f8e4a3642..c9f2c252cb 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 @@ -55,26 +55,50 @@ 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 "[]", nil + return "[]", true, nil } - return stub.activePoll.openDispatcherPRListJSON, 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 "", fmt.Errorf("gh pr ready failed") + return "", true, fmt.Errorf("gh pr ready failed") } - return "", nil + return "", true, nil case strings.HasPrefix(commandLine, "gh pr view "): - return stub.activePoll.stateJSON, nil + 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) @@ -97,11 +121,6 @@ func (stub *mergePackageReleasePRStub) runOutput(ctx context.Context, name strin } } return "[]", nil - case strings.Contains(commandLine, " --match-head-commit "): - if stub.activePoll.failMerge { - return "", fmt.Errorf("gh pr merge failed") - } - return "", nil } return "", fmt.Errorf("unexpected command: %s", commandLine) } From 79cca4996202f40b2b89070145e600ef3b9a67e7 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 22:15:32 +0900 Subject: [PATCH 08/10] fix: fail when a merge fails and nothing else merged the pull request The race handling was shared between the two writes, and one of its outcomes is wrong for the merge. A pull request that is still open and out of draft means the draft was already lifted, which is a reason to go on to the merge -- but reached from a failed merge it reported success with the package still unreleased, so the release would silently not happen behind a green job. Split the two failures. Lifting the draft still accepts three outcomes: another run merged it, another run lifted the draft, or the write genuinely failed. A failed merge accepts only "another run already merged it"; every other state fails, because nothing else will make the release happen. Reported by CodeRabbit on PR 2710. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- .../package_release_pr_merge_test.go | 19 +++++ .../package_release_pr_ready_merge.go | 76 +++++++++++++------ 2 files changed, 72 insertions(+), 23 deletions(-) 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 c9f2c252cb..b18ff1a7f2 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 @@ -390,6 +390,25 @@ func TestMergePackageReleasePRContinuesWhenAnotherRunLiftedTheDraft(t *testing.T 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{ 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 index 61e002dd62..607b1891fd 100644 --- a/cli/release-automation/internal/automation/package_release_pr_ready_merge.go +++ b/cli/release-automation/internal/automation/package_release_pr_ready_merge.go @@ -33,55 +33,53 @@ func readyAndMergePackageReleasePR( deps mergePackageReleasePRDeps, ) (settled bool, exitCode int) { if releasePR.IsDraft { - readyErr := markPackageReleasePRReady(ctx, config, releasePR, deps) - if readyErr != nil { - resolved, exitCode := resolvePackageReleasePRWriteFailure(ctx, stdout, stderr, config, releasePR, readyErr, deps) - if resolved { - return true, exitCode - } - } else { - writeMergePackageReleasePRLine(stdout, fmt.Sprintf( - "Marked Unity package release PR #%d ready before merging it.", releasePR.Number)) + settled, exitCode := readyPackageReleasePRBeforeMerge(ctx, stdout, stderr, config, releasePR, deps) + if settled { + return true, exitCode } } mergeErr := squashMergePackageReleasePR(ctx, config, releasePR, deps) if mergeErr != nil { - _, exitCode := resolvePackageReleasePRWriteFailure(ctx, stdout, stderr, config, releasePR, mergeErr, deps) - return true, exitCode + 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 } -// resolvePackageReleasePRWriteFailure re-reads the pull request after a failed -// write and decides whether the failure was the other run getting there first. -// resolved is false only when the pull request is still open and out of draft, -// which means the draft was already lifted and the merge is still worth -// attempting; every other outcome is final and carries the exit code. -func resolvePackageReleasePRWriteFailure( +// 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, - writeErr error, deps mergePackageReleasePRDeps, -) (resolved bool, exitCode int) { +) (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, writeErr) + writeMergePackageReleasePRLine(stderr, readyErr) writeMergePackageReleasePRLine(stderr, err) return true, 1 } if state.State == mergePackageReleasePRMergedState { - writeMergePackageReleasePRLine(stdout, fmt.Sprintf( - "Unity package release PR #%d was already merged by another run; nothing left to do.", releasePR.Number)) + 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, writeErr) + writeMergePackageReleasePRLine(stderr, readyErr) return true, 1 } writeMergePackageReleasePRLine(stdout, fmt.Sprintf( @@ -89,6 +87,38 @@ func resolvePackageReleasePRWriteFailure( 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. From beb755149bf0e5b81051e173a29f7b9c63e28a09 Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 22:15:33 +0900 Subject: [PATCH 09/10] chore: escape the ref in the contents API query The ref goes into a query parameter. An unescaped one would be cut short at the first &, and the read would silently answer for a different commit than the caller asked for -- the one failure mode this helper must not have, since every merge decision rests on reading the right commit. Reported by CodeRabbit on PR 2710. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- .../internal/automation/package_release_pr_gate.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cli/release-automation/internal/automation/package_release_pr_gate.go b/cli/release-automation/internal/automation/package_release_pr_gate.go index 976660fe69..01788262f6 100644 --- a/cli/release-automation/internal/automation/package_release_pr_gate.go +++ b/cli/release-automation/internal/automation/package_release_pr_gate.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "encoding/json" "fmt" + "net/url" "strings" ) @@ -20,11 +21,13 @@ func githubFileContentAtRef( 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="+ref, + "repos/"+repository+"/contents/"+path+"?ref="+url.QueryEscape(ref), "--jq", ".content", ) From 1d204e3e0688ba2c6e5c3ae343501d8cf154d22b Mon Sep 17 00:00:00 2001 From: hatayama Date: Tue, 8 Sep 2026 22:23:56 +0900 Subject: [PATCH 10/10] test: match the merge command whole, not just its head argument The merge assertion looked for --match-head-commit anywhere in the command log, so a merge naming a different pull request would have satisfied it. Match the whole command, pull request number included. Reported by CodeRabbit on PR 2710. Claude-Session: https://claude.ai/code/session_01NAvyV1iyDTfY4NHQrEU4Jd --- .../package_release_pr_merge_test.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) 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 b18ff1a7f2..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 @@ -181,8 +184,8 @@ func runMergePackageReleasePRCaseWithArgs( 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 { @@ -581,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")) } }