diff --git a/.lore.md b/.lore.md index 4b92ea6e..1fa69eea 100644 --- a/.lore.md +++ b/.lore.md @@ -18,7 +18,7 @@ -- **Craft post-publish merge is non-blocking housekeeping**: After all publish targets complete, \`handleReleaseBranch()\` in \`src/commands/publish.ts\` merges the release branch back into the default branch and deletes it. This is a housekeeping step — failures are caught, reported to Sentry via \`captureException\`, and logged as warnings without failing the command. The merge uses \`--no-ff\` with the default (ort) strategy first, then retries with \`-s resolve\` if conflicts occur (handles criss-cross ambiguities in files like CHANGELOG.md). An \`isAuthError()\` helper distinguishes authentication failures (expired tokens) from merge conflicts to provide targeted diagnostics. +- **Craft post-publish merge failures fail the publish**: After all publish targets complete, \`handleReleaseBranch()\` in \`src/commands/publish.ts\` merges the release branch back into the merge target and deletes it. The merge uses \`--no-ff\` with the default (ort) strategy first, then retries with \`-s resolve\`, then resolves conflicts caused only by the release version bump (\`src/utils/versionConflicts.ts\`). Any remaining merge or push failure makes \`publishMain()\` throw \`ReleaseBranchMergeError\` with recovery steps; the publish state file is kept so a retry skips published targets and only merges. Only \`BranchCleanupError\` (merge pushed, branch deletion failed) stays a warning. @@ -42,7 +42,7 @@ -- **handleReleaseBranch non-blocking cleanup**: Chose non-blocking \`handleReleaseBranch()\` housekeeping over failing an otherwise completed publish because all targets have already shipped before the release-branch merge and deletion run. It attempts \`--no-ff\` with Git’s default \`ort\`, retries conflicts with \`-s resolve\` for criss-cross histories, and distinguishes authentication failures through \`isAuthError()\`; remaining failures go to Sentry and warning logs without changing publish success. +- **Release branch merge failures are fatal**: Chose failing \`craft publish\` over warning when the release-branch merge or push fails, even though every target already shipped: a warning hid broken releases (unmerged branch, skipped \`postReleaseBump\`), while a failure shows up in getsentry/publish and can be retried there. The retry skips published targets via the kept publish state. Branch cleanup failures after a pushed merge stay warnings, because retrying them cannot help. ### Decision diff --git a/docs/src/content/docs/release-workflow.md b/docs/src/content/docs/release-workflow.md index d885a039..319629d8 100644 --- a/docs/src/content/docs/release-workflow.md +++ b/docs/src/content/docs/release-workflow.md @@ -224,8 +224,14 @@ branch; override it with `--merge-target`. - If the merge target changed lines next to the version strings the release branch bumped, Craft resolves those conflicts itself: it keeps the merge - target's lines and applies the version bump to them. Any other conflict - aborts the merge and leaves it to you; the release is still published. + target's lines and applies the version bump to them. +- If the merge still fails, for example on another conflict or a rejected + push, `craft publish` fails after every target has published. It keeps the + publish state and skips the post-release bump and command. Fix the cause and + run the same `craft publish` again: it skips the published targets and only + merges. To fix a conflict, merge the merge target into the release branch + and push it, or merge the release branch into the merge target yourself + with a merge commit. - `--no-merge` skips the merge. `--keep-branch` merges but keeps the branch. - Set `noMerge: true` in `.craft.yml` to never merge. Craft turns this on automatically for compiled Node.js GitHub Actions (repositories with a diff --git a/src/commands/__tests__/publish-main.test.ts b/src/commands/__tests__/publish-main.test.ts index 6288fd0c..ec998c31 100644 --- a/src/commands/__tests__/publish-main.test.ts +++ b/src/commands/__tests__/publish-main.test.ts @@ -35,7 +35,11 @@ import { getGitClient, isRepoDirty, } from '../../utils/git'; -import { BranchCleanupError, publishMain } from '../publish'; +import { + BranchCleanupError, + publishMain, + ReleaseBranchMergeError, +} from '../publish'; vi.mock('@sentry/node', () => ({ captureException: vi.fn(), @@ -212,6 +216,45 @@ describe('publishMain release branch handling', () => { ); }); + test('fails the release and keeps the publish state when the merge conflicts', async () => { + const git = createMockGit('e'.repeat(40)); + (git.merge as Mock).mockImplementation((args: string[]) => + args[0] === '--abort' + ? Promise.resolve() + : Promise.reject(new Error('CONFLICT (content)')), + ); + vi.mocked(git.status).mockResolvedValue({ + files: [], + conflicted: ['CHANGELOG.md'], + } as never); + vi.mocked(getGitClient).mockResolvedValue(git); + + const error = await publishMain(defaultOptions).catch((e: unknown) => e); + + expect(publishTarget).toHaveBeenCalledOnce(); + expect(error).toBeInstanceOf(ReleaseBranchMergeError); + const message = (error as Error).message; + expect(message).toContain('Failed to merge release branch "stable/1.2.3"'); + expect(message).toContain(' - CHANGELOG.md'); + expect(message).toContain('Retrying the publish skips them'); + expect(safeFs.unlink).not.toHaveBeenCalled(); + expect(git.push).not.toHaveBeenCalled(); + }); + + test('fails the release when pushing the merge fails', async () => { + const git = createMockGit('f'.repeat(40)); + vi.mocked(git.push).mockRejectedValueOnce(new Error('403 Forbidden')); + vi.mocked(getGitClient).mockResolvedValue(git); + + const error = await publishMain(defaultOptions).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(ReleaseBranchMergeError); + expect((error as Error).message).toContain( + 'Retry the publish with valid credentials.', + ); + expect(safeFs.unlink).not.toHaveBeenCalled(); + }); + test('reports remote deletion failures as cleanup failures', async () => { const requestedRevision = 'release-candidate'; const approvedRevision = 'c'.repeat(40); @@ -404,6 +447,17 @@ describe('publishMain post-release bump', () => { expect(git.commit).not.toHaveBeenCalled(); }); + test('skips the bump when the merge fails', async () => { + vi.mocked(git.push).mockRejectedValueOnce(new Error('403 Forbidden')); + + await expect(publishMain(defaultOptions)).rejects.toBeInstanceOf( + ReleaseBranchMergeError, + ); + + expect(runPreReleaseCommand).not.toHaveBeenCalled(); + expect(git.commit).not.toHaveBeenCalled(); + }); + test('warns instead of failing the release when the bump fails', async () => { vi.mocked(runPreReleaseCommand).mockRejectedValue( new Error('bump script exploded'), diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 1983751e..85a200be 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -470,6 +470,70 @@ export class BranchCleanupError extends Error { } } +/** + * Error thrown when every target published but the release branch could not + * be merged into the merge target. Retrying the publish skips the published + * targets and only merges. + */ +export class ReleaseBranchMergeError extends Error { + public __proto__: Error; + + public constructor(message: string, options?: ErrorOptions) { + const trueProto = new.target.prototype; + super(message, options); + this.__proto__ = trueProto; + } +} + +/** + * Explains why the release branch merge failed and how to recover. + */ +function describeMergeFailure( + mergeError: unknown, + branchName: string, + remoteName: string, +): string { + const lines = [ + `Failed to merge release branch "${branchName}" into the target branch.`, + `All publish targets completed successfully. Retrying the publish skips them and only merges the release branch.`, + ]; + if (mergeError instanceof MergeConflictError) { + lines.push(`Merge conflict — both ort and resolve strategies failed.`); + if (mergeError.conflictedFiles.length > 0) { + lines.push(``, `Conflicting files:`); + for (const file of mergeError.conflictedFiles) { + lines.push(` - ${file}`); + } + } + if (mergeError.diff) { + lines.push(``, `Diff:`, mergeError.diff); + } + lines.push( + ``, + `To recover, do one of these, then retry the publish:`, + ` - Merge the target branch into "${branchName}", resolve the conflicts, and push it. The retry merges the new release branch head.`, + ` - Merge "${branchName}" into the target branch yourself with a merge commit (not a squash), resolving the conflicts.`, + ); + } else if (mergeError instanceof PushError) { + lines.push( + `The merge succeeded locally but pushing to the remote failed.`, + `This is likely due to an expired authentication token (common for long-running publishes > 1 hour).`, + ``, + `Retry the publish with valid credentials.`, + ); + } else { + lines.push( + ``, + `Fix the cause below and retry the publish, or merge the release branch into the target branch and delete it yourself: git push ${remoteName} --delete ${branchName}`, + ); + } + lines.push( + ``, + `Error: ${mergeError instanceof Error ? mergeError.message : String(mergeError)}`, + ); + return lines.join('\n'); +} + /** * Deals with the release branch after publishing is done * @@ -1055,89 +1119,39 @@ export async function publishMain(argv: PublishOptions): Promise { ); mergedInto = mergeTarget; } catch (mergeError) { - // The merge is a housekeeping step — it must not block the success - // signal for a fully-published release. Report to Sentry for - // observability but don't fail the command. - captureException(mergeError); - if (mergeError instanceof BranchCleanupError) { - // The merge itself was pushed; only the branch cleanup failed. - mergedInto = mergeTarget; + if (!(mergeError instanceof BranchCleanupError)) { + // Every target published, but the release didn't make it back into + // the merge target, which also skips the post-release steps. Fail + // loudly so the release can be fixed and retried. The publish state + // file stays, so the retry skips the published targets. + throw new ReleaseBranchMergeError( + describeMergeFailure(mergeError, branchName, argv.remote), + { cause: mergeError }, + ); } - const lines: string[] = []; - if (mergeError instanceof BranchCleanupError) { - lines.push( - `Failed to clean up release branch "${branchName}" after merging it into the target branch.`, - ); - if (mergeError.remoteDeleted) { - lines.push( - `The merge was pushed and the remote release branch was deleted, but deleting the local branch failed.`, - `Inspect the local branch before deleting it.`, - ``, - `To retry safely: git branch -d -- ${branchName}`, - ); - } else { - lines.push( - `The merge was pushed, but deleting the remote release branch failed.`, - `The branch may have advanced beyond the expected release revision ${revision}. Inspect it before cleanup.`, - ``, - `To retry safely: git push --force-with-lease=refs/heads/${branchName}:${revision} ${argv.remote} :refs/heads/${branchName}`, - ); - } - } else if (mergeError instanceof MergeConflictError) { - lines.push( - `Failed to merge release branch "${branchName}" into the target branch.`, - ); - lines.push(`Merge conflict — both ort and resolve strategies failed.`); - if (mergeError.conflictedFiles.length > 0) { - lines.push(``); - lines.push(`Conflicting files:`); - for (const file of mergeError.conflictedFiles) { - lines.push(` - ${file}`); - } - } - if (mergeError.diff) { - lines.push(``, `Diff:`, mergeError.diff); - } - lines.push( - ``, - `All publish targets completed successfully — only the post-publish merge failed.`, - ``, - `To resolve manually:`, - ` 1. Merge the release branch into the target branch, resolving conflicts`, - ` 2. Delete the release branch: git push ${argv.remote} --delete ${branchName}`, - ); - } else if (mergeError instanceof PushError) { - lines.push( - `Failed to merge release branch "${branchName}" into the target branch.`, - ); + // The merge itself was pushed; only the branch cleanup failed. + captureException(mergeError); + mergedInto = mergeTarget; + const lines = [ + `Failed to clean up release branch "${branchName}" after merging it into the target branch.`, + ]; + if (mergeError.remoteDeleted) { lines.push( - `The merge succeeded locally but pushing to the remote failed.`, - `This is likely due to an expired authentication token (common for long-running publishes > 1 hour).`, - ``, - `All publish targets completed successfully — only the post-publish push failed.`, + `The merge was pushed and the remote release branch was deleted, but deleting the local branch failed.`, + `Inspect the local branch before deleting it.`, ``, - `To resolve manually:`, - ` 1. Re-authenticate (e.g., generate a fresh token)`, - ` 2. Merge the release branch into the target branch`, - ` 3. Delete the release branch: git push ${argv.remote} --delete ${branchName}`, + `To retry safely: git branch -d -- ${branchName}`, ); } else { lines.push( - `Failed to merge release branch "${branchName}" into the target branch.`, - ); - lines.push( - `All publish targets completed successfully — only the post-publish merge failed.`, + `The merge was pushed, but deleting the remote release branch failed.`, + `The branch may have advanced beyond the expected release revision ${revision}. Inspect it before cleanup.`, ``, - `To resolve manually:`, - ` 1. Merge the release branch into the target branch`, - ` 2. Delete the release branch: git push ${argv.remote} --delete ${branchName}`, + `To retry safely: git push --force-with-lease=refs/heads/${branchName}:${revision} ${argv.remote} :refs/heads/${branchName}`, ); } - lines.push( - ``, - `Error: ${mergeError instanceof Error ? mergeError.message : String(mergeError)}`, - ); + lines.push(``, `Error: ${mergeError.message}`); logger.warn(lines.join('\n')); }