diff --git a/.github/MAINTAINERS.md b/.github/MAINTAINERS.md index 6487cbfd0f..c0eba167bb 100644 --- a/.github/MAINTAINERS.md +++ b/.github/MAINTAINERS.md @@ -141,9 +141,10 @@ lists, and approve ([runbook](../apps/cli/docs/release-process.md#claude-resolved-develop-into-next)). Otherwise merge the target into the sync branch, resolve, and push; then **approve** the PR. Approval fast-forwards the -target and deletes the branch. Never use the merge button. If the target or the -source moved, the bot merges the latest target and source into the approved head and lands -it directly if the merge is clean, as for a clean sync (the merge commit is +target and deletes the branch. Never use the merge button. A moved source does +not block landing; the next sync run brings its newer commits in. If the target +moved, the bot merges the latest target into the approved head and lands it +directly if the merge is clean, as for a clean sync (the merge commit is untested); only a new conflict, or a target that keeps moving, sends the PR back for a new approval. Only two sync pairs exist (`sync/main-into-develop` into `develop`, `sync/develop-into-next` into diff --git a/.github/scripts/fast-forward.test.ts b/.github/scripts/fast-forward.test.ts index acebc80566..008058b89e 100644 --- a/.github/scripts/fast-forward.test.ts +++ b/.github/scripts/fast-forward.test.ts @@ -733,8 +733,11 @@ describe("runFastForward sync", () => { expect(() => repo.remoteTip("sync/main-into-develop")).toThrow(); }); - test("lands the approved head merged with a non-conflicting target move", async () => { + test("lands the approved head merged with a target move without pulling in the moved source", async () => { const { repo, head, io, comments } = resolvedSyncScenario(); + git(repo.seed, "switch", "main"); + const movedMain = repo.commit(repo.seed, "late-main.txt", "late\n", "fix(cli): late main"); + git(repo.seed, "push", "origin", "main"); git(repo.seed, "switch", "develop"); const movedDevelop = repo.commit(repo.seed, "late.txt", "late\n", "feat(cli): late"); git(repo.seed, "push", "origin", "develop"); @@ -745,6 +748,7 @@ describe("runFastForward sync", () => { expect(outcome).toEqual({ status: "merged-after-resync", sha: landed }); expect(repo.isAncestor(head, landed)).toBe(true); expect(repo.isAncestor(movedDevelop, landed)).toBe(true); + expect(repo.isAncestor(movedMain, landed)).toBe(false); expect(() => repo.remoteTip("sync/main-into-develop")).toThrow(); expect(comments).toHaveLength(1); expect(comments[0]).toContain("moved since approval"); @@ -846,52 +850,17 @@ describe("runFastForward sync", () => { expect(comments[0]).toContain("- `shared.txt`"); }); - test("lands the approved head merged with a non-conflicting source move", async () => { - const { repo, head, io, comments } = resolvedSyncScenario(); - git(repo.seed, "switch", "main"); - const movedMain = repo.commit(repo.seed, "late-main.txt", "late\n", "fix(cli): late main"); - git(repo.seed, "push", "origin", "main"); - - const outcome = await runFastForward(io, { reviewCommitId: head }); - - const landed = repo.remoteTip("develop"); - expect(outcome).toEqual({ status: "merged-after-resync", sha: landed }); - expect(landed).not.toBe(head); - expect(repo.isAncestor(head, landed)).toBe(true); - expect(repo.isAncestor(movedMain, landed)).toBe(true); - expect(() => repo.remoteTip("sync/main-into-develop")).toThrow(); - expect(comments).toHaveLength(1); - expect(comments[0]).toContain("`main` moved since approval"); - }); - - test("refuses and leaves the branch alone when a moved source conflicts", async () => { + test("fast-forwards the approved head even when the source moved with a conflicting change", async () => { const { repo, head, io, comments } = resolvedSyncScenario(); - const developBefore = repo.remoteTip("develop"); git(repo.seed, "switch", "main"); repo.commit(repo.seed, "shared.txt", "conflicting main\n", "fix(cli): conflicting main"); git(repo.seed, "push", "origin", "main"); const outcome = await runFastForward(io, { reviewCommitId: head }); - expect(outcome.status).toBe("refused"); - expect(repo.remoteTip("sync/main-into-develop")).toBe(head); - expect(repo.remoteTip("develop")).toBe(developBefore); - expect(comments[0]).toContain("`main` moved since approval"); - expect(comments[0]).toContain("- `shared.txt`"); - }); - - test("dry run pushes nothing when the source moved cleanly", async () => { - const { repo, head, io, comments } = resolvedSyncScenario(); - const developBefore = repo.remoteTip("develop"); - git(repo.seed, "switch", "main"); - repo.commit(repo.seed, "late-main.txt", "late\n", "fix(cli): late main"); - git(repo.seed, "push", "origin", "main"); - - const outcome = await runFastForward(io, { reviewCommitId: head, dryRun: true }); - - expect(outcome.status).toBe("dry-run"); - expect(repo.remoteTip("develop")).toBe(developBefore); - expect(repo.remoteTip("sync/main-into-develop")).toBe(head); + expect(outcome).toEqual({ status: "fast-forwarded", sha: head }); + expect(repo.remoteTip("develop")).toBe(head); + expect(() => repo.remoteTip("sync/main-into-develop")).toThrow(); expect(comments).toEqual([]); }); }); diff --git a/.github/scripts/fast-forward.ts b/.github/scripts/fast-forward.ts index c9159fe396..77b9ff82cd 100644 --- a/.github/scripts/fast-forward.ts +++ b/.github/scripts/fast-forward.ts @@ -371,40 +371,31 @@ async function resyncSyncBranch( promotion: Promotion, headSha: string, syncBranch: string, - moved: string, ): Promise { const { git } = io; - const { target, source } = promotion; + const { target } = promotion; const targetRef = `refs/remotes/origin/${target}`; let resyncedSha = headSha; for (let attempt = 1; attempt <= MAX_PUSH_ATTEMPTS; attempt += 1) { - gitOrThrow(git, [ - "fetch", - "--no-tags", - "origin", - `+refs/heads/${target}:${targetRef}`, - `+refs/heads/${source}:refs/remotes/origin/${source}`, - ]); + gitOrThrow(git, ["fetch", "--no-tags", "origin", `+refs/heads/${target}:${targetRef}`]); const targetSha = gitOrThrow(git, ["rev-parse", targetRef]); gitOrThrow(git, ["checkout", "--detach", headSha]); - for (const ref of [target, source]) { - const merge = git(["merge", "--no-edit", `refs/remotes/origin/${ref}`]); - if (merge.status !== 0) { - const files = git(["diff", "--name-only", "--diff-filter=U"]).stdout; - git(["merge", "--abort"]); - const list = files - .split("\n") - .filter(Boolean) - .map((file) => `- \`${file}\``) - .join("\n"); - return refuse( - io, - input, - `Merging ${ref} into ${syncBranch} conflicts`, - `\`${moved}\` moved since approval and merging \`${ref}\` into \`${syncBranch}\` conflicts:\n\n${list}\n\nResolve on the branch and re-approve.`, - ); - } + const merge = git(["merge", "--no-edit", targetRef]); + if (merge.status !== 0) { + const files = git(["diff", "--name-only", "--diff-filter=U"]).stdout; + git(["merge", "--abort"]); + const list = files + .split("\n") + .filter(Boolean) + .map((file) => `- \`${file}\``) + .join("\n"); + return refuse( + io, + input, + `Merging ${target} into ${syncBranch} conflicts`, + `\`${target}\` moved since approval and merging it into \`${syncBranch}\` conflicts:\n\n${list}\n\nResolve on the branch and re-approve.`, + ); } resyncedSha = gitOrThrow(git, ["rev-parse", "HEAD"]); @@ -416,7 +407,7 @@ async function resyncSyncBranch( if (pushMergedTarget(git, target, targetSha) === "pushed") { git(["push", "origin", "--delete", syncBranch]); await io.comment( - `\`${moved}\` moved since approval, so I merged the latest \`${target}\` and \`${source}\` into the approved head and landed it as ${short(resyncedSha)}.`, + `\`${target}\` moved since approval, so I merged the latest \`${target}\` into the approved head and landed it as ${short(resyncedSha)}.`, ); return { status: "merged-after-resync", sha: resyncedSha }; } @@ -424,7 +415,7 @@ async function resyncSyncBranch( gitOrThrow(git, ["push", "origin", `${resyncedSha}:refs/heads/${syncBranch}`]); await io.comment( - `\`${target}\` kept moving, so I merged the latest \`${target}\` and \`${source}\` into \`${syncBranch}\` (${short(resyncedSha)}). Re-approve once checks pass.`, + `\`${target}\` kept moving, so I merged the latest \`${target}\` into \`${syncBranch}\` (${short(resyncedSha)}). Re-approve once checks pass.`, ); return { status: "resynced", sha: resyncedSha }; } @@ -482,9 +473,6 @@ export async function runFastForward( const fetchRefs = [ `+refs/heads/${promotion.target}:refs/remotes/origin/${promotion.target}`, `+refs/pull/${pullRequest.number}/head:refs/ff/pr-head`, - ...(promotion.kind === "sync" - ? [`+refs/heads/${promotion.source}:refs/remotes/origin/${promotion.source}`] - : []), ]; gitOrThrow(git, ["fetch", "--no-tags", "origin", ...fetchRefs]); @@ -515,18 +503,9 @@ export async function runFastForward( const canFastForward = (): boolean => git(["merge-base", "--is-ancestor", targetRef, headSha]).status === 0; - const sourceCurrent = (): boolean => - promotion.kind !== "sync" || - git(["merge-base", "--is-ancestor", `refs/remotes/origin/${promotion.source}`, headSha]) - .status === 0; - - let movedBranch: string | null = null; - if (!canFastForward()) { - movedBranch = promotion.target; - } else if (!sourceCurrent()) { - movedBranch = promotion.source; - } - if (movedBranch === null) { + // Source commits newer than the approved head reach the target through the next sync run. + let targetMoved = !canFastForward(); + if (!targetMoved) { const push = pushSha(io, input, headSha, promotion.target); if (!push.ok) { gitOrThrow(git, [ @@ -538,13 +517,13 @@ export async function runFastForward( if (canFastForward()) { throw new Error(`git push to ${promotion.target} failed: ${push.stderr}`); } - movedBranch = promotion.target; + targetMoved = true; } } - if (movedBranch !== null) { + if (targetMoved) { if (promotion.kind === "sync") { - return resyncSyncBranch(io, input, promotion, headSha, pullRequest.head.ref, movedBranch); + return resyncSyncBranch(io, input, promotion, headSha, pullRequest.head.ref); } const recovery = promotion.kind === "deploy" diff --git a/apps/cli/docs/release-process.md b/apps/cli/docs/release-process.md index 753cef6099..d089c98724 100644 --- a/apps/cli/docs/release-process.md +++ b/apps/cli/docs/release-process.md @@ -378,7 +378,7 @@ Before pushing, the job checks, in order: 2. **Approved commit is the head.** The approval's commit must equal the current PR head, and the PR must be open and not a draft. A stale approval is rejected with a comment; re-approve the new head. 3. **Required checks are green on the head.** `Check code quality`, `Run unit and integration tests`, `Run end-to-end tests`, and `Lint Pull Request` must all have succeeded. They are read from the PR's own check rollup (GraphQL), counting only GitHub Actions runs attached to this PR, so runs of another PR on the same commit cannot satisfy or mask a check. Deploy PRs and hotfix PRs into `main` run the full suite (`test.yml` also triggers on PRs into `main`), so these checks exist on the deploy PR head. 4. **Major guard (any promotion into `main`).** If the commits since the last stable tag include a breaking `!` title, the PR needs the `release-major` label. Without it the job comments and fails. -5. **Target has not moved.** The push is a plain, non-force push of the approved commit, so the server refuses anything that is not a fast-forward. For deploy and cut PRs the job comments and fails; update the PR and re-approve. When the deploy PR is refused because `main` has commits `develop` lacks, the comment names the open `sync/main-into-develop` pull request, or the sync dispatch if none is open. A sync PR also counts as stale when the current source tip (`main` or `develop`) is not in the approved head, even if the target did not move. For sync PRs the bot merges the latest target and source into the approved head and, when that merge is clean, pushes it straight to the target and comments, as for a clean sync (the merge commit is untested). A new conflict, or a target that keeps moving, sends the PR back for re-approval instead: on a conflict the bot comments and fails; if the target keeps moving it pushes the re-synced head to the sync branch and comments, and you re-approve once checks pass. +5. **Target has not moved.** The push is a plain, non-force push of the approved commit, so the server refuses anything that is not a fast-forward. For deploy and cut PRs the job comments and fails; update the PR and re-approve. When the deploy PR is refused because `main` has commits `develop` lacks, the comment names the open `sync/main-into-develop` pull request, or the sync dispatch if none is open. A sync PR lands its approved head even when its source (`main` or `develop`) has moved on; the next push to the source runs `Sync branches` again and brings the newer commits in. When the target moved, the bot merges the latest target into the approved head and, when that merge is clean, pushes it straight to the target and comments, as for a clean sync (the merge commit is untested). A new conflict, or a target that keeps moving, sends the PR back for re-approval instead: on a conflict the bot comments and fails; if the target keeps moving it pushes the re-synced head to the sync branch and comments, and you re-approve once checks pass. When all checks pass and no major is involved, a deploy PR behaves as before: the App pushes the approved commit to `main` and `push: main` triggers the stable release.