From 65a017b62326b8fb070434da9d470653c0946d68 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Tue, 31 Mar 2026 19:45:52 +0000 Subject: [PATCH 1/5] fix(publish): distinguish auth failures from merge conflicts in post-publish merge The post-publish merge warning always assumed merge conflicts, even when the real cause was an expired GitHub App token (common for publishes exceeding the 1-hour token lifetime, e.g. sentry-native). Add an isAuthError() helper that pattern-matches git credential errors and use it to provide an accurate diagnosis in the warning message. --- AGENTS.md | 21 +++++----- src/commands/__tests__/publish.test.ts | 53 +++++++++++++++++++++++++- src/commands/publish.ts | 16 +++++++- 3 files changed, 76 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 955135396..fa229994e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,26 +113,23 @@ Some operations need explicit `isDryRun()` checks: - User experience optimizations (e.g., skipping sleep timers) - ## Long-term Knowledge ### Architecture - - -- **Registry target: repo_url auto-derived from git remote, not user-configurable**: \`repo_url\` in registry manifests is always set by Craft as \`https://github.com/${owner}/${repo}\`. Resolution: (1) explicit \`github: { owner, repo }\` in \`.craft.yml\` (rare), (2) fallback: auto-detect from git \`origin\` remote URL via \`git-url-parse\` library (\`git.ts:194-217\`, \`config.ts:286-316\`). Works with HTTPS and SSH remote URLs. Always overwritten on every publish — existing manifest values are replaced (\`registry.ts:417-418\`). Result is cached globally with \`Object.freeze\`. If remote isn't \`github.com\` and no explicit config exists, throws \`ConfigurationError\`. Most repos need no configuration — the git origin remote is sufficient. - - - -- **Registry target: urlTemplate generates artifact download URLs in manifest**: \`urlTemplate\` in the registry target config generates download URLs for release artifacts in the registry manifest's \`files\` field. Uses Mustache rendering with variables \`{{version}}\`, \`{{file}}\`, \`{{revision}}\`. Primarily useful for apps (standalone binaries) and CDN-hosted assets — SDK packages published to public registries (npm, PyPI, gem) typically don't need it. If neither \`urlTemplate\` nor \`checksums\` is configured, Craft skips adding file data entirely (warns at \`registry.ts:341-349\`). Real-world pattern: \`https://downloads.sentry-cdn.com/\/{{version}}/{{file}}\`. + +* **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. ### Gotcha - + +* **GitHub App tokens expire after 1 hour — breaks long-running CI publishes**: GitHub App installation tokens expire after 1 hour (non-configurable). For publish jobs exceeding this (e.g., sentry-native's ~1h 23m symbol upload), the token expires before Craft's post-publish \`git push\` for the release branch merge. Git fails with \`could not read Username for 'https://github.com': No such device or address\` — which looks like a credential config issue but is actually token expiration. No code change in Craft alone can fix this — the \`GITHUB\_TOKEN\` env var, git \`http.extraheader\`, and Octokit all use the same expired token. The real fix requires the CI workflow (\`getsentry/publish\`) to generate a fresh token after the Docker container exits, before the merge step. -- **ESM modules prevent vi.spyOn of child_process.spawnSync — use test subclass pattern**: In ESM (Vitest or Bun), you cannot \`vi.spyOn\` exports from Node built-in modules — throws 'Module namespace is not configurable'. Workaround: create a test subclass that overrides the method calling the built-in and injects controllable values. \`vi.mock\` at module level works but affects all tests in the file. + +* **prepare-dry-run e2e tests fail without EDITOR in dumb terminals**: The 7 tests in \`src/\_\_tests\_\_/prepare-dry-run.e2e.test.ts\` fail in environments where \`TERM=dumb\` and \`EDITOR\` is unset (e.g., inside agent shells or minimal CI containers). The error is \`Terminal is dumb, but EDITOR unset\` from git commit. This is a pre-existing environment issue, not a code defect. These tests pass in normal CI (Node.js 20/22 runners) where terminal capabilities are available. - +### Pattern -- **pnpm overrides with >= can cross major versions — use ^ to constrain**: pnpm overrides gotchas: (1) \`>=\` crosses major versions — use \`^\` to constrain within same major. (2) Version-range selectors don't reliably force re-resolution of compatible transitive deps; use blanket overrides when safe. (3) Overrides become stale — audit with \`pnpm why \\` after dependency changes. (4) Never manually resolve pnpm-lock.yaml conflicts — \`git checkout --theirs\` then \`pnpm install\` to regenerate deterministically. + +* **Craft Docker container auth: credentials come from volume-mounted git config**: When Craft runs inside Docker in CI (via \`getsentry/craft:latest\`), git authentication comes from \`actions/checkout\`'s \`http.extraheader\` in the local \`.git/config\`, which is volume-mounted into the container at \`/github/workspace\`. The container sets \`HOME=/root\`, so global git config from the host runner isn't available. The \`GITHUB\_TOKEN\` env var is passed separately. Craft's \`handleReleaseBranch()\` doesn't set up its own auth — it relies on whatever git config is present. Other targets (registry, commitOnGitRepository) explicitly inject tokens into clone URLs via \`GitHubRemote.getRemoteStringWithAuth()\` or URL manipulation. diff --git a/src/commands/__tests__/publish.test.ts b/src/commands/__tests__/publish.test.ts index e358b0cc5..f1ed0d225 100644 --- a/src/commands/__tests__/publish.test.ts +++ b/src/commands/__tests__/publish.test.ts @@ -1,7 +1,11 @@ import { vi, describe, test, expect, beforeEach, type Mock } from 'vitest'; import { join as pathJoin } from 'path'; import { spawnProcess, hasExecutable } from '../../utils/system'; -import { runPostReleaseCommand, handleReleaseBranch } from '../publish'; +import { + runPostReleaseCommand, + handleReleaseBranch, + isAuthError, +} from '../publish'; import type { SimpleGit } from 'simple-git'; vi.mock('../../utils/system'); @@ -282,3 +286,50 @@ describe('handleReleaseBranch', () => { expect(git.push).toHaveBeenCalledWith('origin', 'master'); }); }); + +describe('isAuthError', () => { + test('detects "could not read Username" as auth error', () => { + expect( + isAuthError( + new Error( + "fatal: could not read Username for 'https://github.com': No such device or address", + ), + ), + ).toBe(true); + }); + + test('detects "Authentication failed" as auth error', () => { + expect( + isAuthError( + new Error( + "fatal: Authentication failed for 'https://github.com/org/repo.git/'", + ), + ), + ).toBe(true); + }); + + test('detects HTTP 401 as auth error', () => { + expect(isAuthError(new Error('HTTP 401 Unauthorized'))).toBe(true); + }); + + test('detects HTTP 403 as auth error', () => { + expect(isAuthError(new Error('HTTP 403 Forbidden'))).toBe(true); + }); + + test('does not flag merge conflicts as auth error', () => { + expect( + isAuthError( + new Error('CONFLICT (content): Merge conflict in CHANGELOG.md'), + ), + ).toBe(false); + }); + + test('does not flag generic git errors as auth error', () => { + expect(isAuthError(new Error('fatal: not a git repository'))).toBe(false); + }); + + test('handles non-Error values', () => { + expect(isAuthError('could not read Username')).toBe(true); + expect(isAuthError('some other error')).toBe(false); + }); +}); diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 3674af2cb..4e14da0a1 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -386,6 +386,17 @@ async function checkRevisionStatus( await statusProvider.waitForTheBuildToSucceed(revision); } +/** + * Checks whether an error from a git operation is an authentication failure + * (e.g., expired token) rather than a merge conflict or other git error. + */ +export function isAuthError(error: unknown): boolean { + const msg = error instanceof Error ? error.message : String(error); + return /could not read Username|Authentication failed|HTTP 401|HTTP 403|Invalid credentials|token.*expired/i.test( + msg, + ); +} + /** * Deals with the release branch after publishing is done * @@ -734,10 +745,13 @@ export async function publishMain(argv: PublishOptions): Promise { // signal for a fully-published release. Report to Sentry for // observability but don't fail the command. captureException(mergeError); + const diagnosis = isAuthError(mergeError) + ? `This is likely due to an expired authentication token (common for long-running publishes > 1 hour).` + : `This is likely due to a merge conflict (e.g., in CHANGELOG.md).`; logger.warn( [ `Failed to merge release branch "${branchName}" into the target branch.`, - `This is likely due to a merge conflict (e.g., in CHANGELOG.md).`, + diagnosis, `All publish targets completed successfully — only the post-publish merge failed.`, ``, `To resolve manually:`, From 395f48572d8a0b058ae9cdd692cfe5afe45c0894 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 1 Apr 2026 10:31:46 +0000 Subject: [PATCH 2/5] style: format AGENTS.md with prettier --- AGENTS.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fa229994e..1b1d8175a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,23 +113,28 @@ Some operations need explicit `isDryRun()` checks: - User experience optimizations (e.g., skipping sleep timers) + ## Long-term Knowledge ### Architecture -* **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 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. ### Gotcha -* **GitHub App tokens expire after 1 hour — breaks long-running CI publishes**: GitHub App installation tokens expire after 1 hour (non-configurable). For publish jobs exceeding this (e.g., sentry-native's ~1h 23m symbol upload), the token expires before Craft's post-publish \`git push\` for the release branch merge. Git fails with \`could not read Username for 'https://github.com': No such device or address\` — which looks like a credential config issue but is actually token expiration. No code change in Craft alone can fix this — the \`GITHUB\_TOKEN\` env var, git \`http.extraheader\`, and Octokit all use the same expired token. The real fix requires the CI workflow (\`getsentry/publish\`) to generate a fresh token after the Docker container exits, before the merge step. + +- **GitHub App tokens expire after 1 hour — breaks long-running CI publishes**: GitHub App installation tokens expire after 1 hour (non-configurable). For publish jobs exceeding this (e.g., sentry-native's ~1h 23m symbol upload), the token expires before Craft's post-publish \`git push\` for the release branch merge. Git fails with \`could not read Username for 'https://github.com': No such device or address\` — which looks like a credential config issue but is actually token expiration. No code change in Craft alone can fix this — the \`GITHUB_TOKEN\` env var, git \`http.extraheader\`, and Octokit all use the same expired token. The real fix requires the CI workflow (\`getsentry/publish\`) to generate a fresh token after the Docker container exits, before the merge step. -* **prepare-dry-run e2e tests fail without EDITOR in dumb terminals**: The 7 tests in \`src/\_\_tests\_\_/prepare-dry-run.e2e.test.ts\` fail in environments where \`TERM=dumb\` and \`EDITOR\` is unset (e.g., inside agent shells or minimal CI containers). The error is \`Terminal is dumb, but EDITOR unset\` from git commit. This is a pre-existing environment issue, not a code defect. These tests pass in normal CI (Node.js 20/22 runners) where terminal capabilities are available. + +- **prepare-dry-run e2e tests fail without EDITOR in dumb terminals**: The 7 tests in \`src/\_\_tests\_\_/prepare-dry-run.e2e.test.ts\` fail in environments where \`TERM=dumb\` and \`EDITOR\` is unset (e.g., inside agent shells or minimal CI containers). The error is \`Terminal is dumb, but EDITOR unset\` from git commit. This is a pre-existing environment issue, not a code defect. These tests pass in normal CI (Node.js 20/22 runners) where terminal capabilities are available. ### Pattern -* **Craft Docker container auth: credentials come from volume-mounted git config**: When Craft runs inside Docker in CI (via \`getsentry/craft:latest\`), git authentication comes from \`actions/checkout\`'s \`http.extraheader\` in the local \`.git/config\`, which is volume-mounted into the container at \`/github/workspace\`. The container sets \`HOME=/root\`, so global git config from the host runner isn't available. The \`GITHUB\_TOKEN\` env var is passed separately. Craft's \`handleReleaseBranch()\` doesn't set up its own auth — it relies on whatever git config is present. Other targets (registry, commitOnGitRepository) explicitly inject tokens into clone URLs via \`GitHubRemote.getRemoteStringWithAuth()\` or URL manipulation. + +- **Craft Docker container auth: credentials come from volume-mounted git config**: When Craft runs inside Docker in CI (via \`getsentry/craft:latest\`), git authentication comes from \`actions/checkout\`'s \`http.extraheader\` in the local \`.git/config\`, which is volume-mounted into the container at \`/github/workspace\`. The container sets \`HOME=/root\`, so global git config from the host runner isn't available. The \`GITHUB_TOKEN\` env var is passed separately. Craft's \`handleReleaseBranch()\` doesn't set up its own auth — it relies on whatever git config is present. Other targets (registry, commitOnGitRepository) explicitly inject tokens into clone URLs via \`GitHubRemote.getRemoteStringWithAuth()\` or URL manipulation. From 1471ff74a5a6df474dd7e9d0a0ae5a9861f17633 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 1 Apr 2026 13:33:11 +0000 Subject: [PATCH 3/5] fix: provide context-specific resolution steps for auth vs merge errors --- src/commands/publish.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 4e14da0a1..5ec842a94 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -745,9 +745,20 @@ export async function publishMain(argv: PublishOptions): Promise { // signal for a fully-published release. Report to Sentry for // observability but don't fail the command. captureException(mergeError); - const diagnosis = isAuthError(mergeError) + const authFailure = isAuthError(mergeError); + const diagnosis = authFailure ? `This is likely due to an expired authentication token (common for long-running publishes > 1 hour).` : `This is likely due to a merge conflict (e.g., in CHANGELOG.md).`; + const resolutionSteps = authFailure + ? [ + ` 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}`, + ] + : [ + ` 1. Merge the release branch into the target branch, resolving conflicts`, + ` 2. Delete the release branch: git push ${argv.remote} --delete ${branchName}`, + ]; logger.warn( [ `Failed to merge release branch "${branchName}" into the target branch.`, @@ -755,8 +766,7 @@ export async function publishMain(argv: PublishOptions): Promise { `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}`, + ...resolutionSteps, ``, `Error: ${mergeError instanceof Error ? mergeError.message : String(mergeError)}`, ].join('\n'), From fc03d85e7b087c492b0e1ee4afdebc66e620a66b Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 1 Apr 2026 13:45:26 +0000 Subject: [PATCH 4/5] refactor: replace regex-based isAuthError with typed MergeConflictError/PushError Classify errors deterministically by WHERE they occur in handleReleaseBranch, not by pattern-matching the message: - Merge fails (both ort + resolve strategies): throws MergeConflictError with the list of conflicted files from git status - Push fails after successful merge: throws PushError The caller uses instanceof to provide targeted diagnostics: - MergeConflictError: lists the conflicting files - PushError: explains the merge succeeded but push failed (likely expired token) --- src/commands/__tests__/publish.test.ts | 108 +++++++++++++--------- src/commands/publish.ts | 123 ++++++++++++++++++------- 2 files changed, 157 insertions(+), 74 deletions(-) diff --git a/src/commands/__tests__/publish.test.ts b/src/commands/__tests__/publish.test.ts index f1ed0d225..d9aeb3e94 100644 --- a/src/commands/__tests__/publish.test.ts +++ b/src/commands/__tests__/publish.test.ts @@ -4,7 +4,8 @@ import { spawnProcess, hasExecutable } from '../../utils/system'; import { runPostReleaseCommand, handleReleaseBranch, - isAuthError, + MergeConflictError, + PushError, } from '../publish'; import type { SimpleGit } from 'simple-git'; @@ -120,6 +121,7 @@ describe('handleReleaseBranch', () => { mockGit.remote = makeChainable(); mockGit.revparse = makeChainable('main'); mockGit.raw = makeChainable(''); + mockGit.status = makeChainable({ conflicted: [] }); return mockGit as unknown as SimpleGit & Record; } @@ -209,7 +211,7 @@ describe('handleReleaseBranch', () => { expect(git.push).toHaveBeenCalledWith('origin', 'main'); }); - test('throws when both merge strategies fail and aborts both', async () => { + test('throws MergeConflictError with file list when both strategies fail', async () => { const git = createMockGit(); const defaultError = new Error('CONFLICT with default'); const resolveError = new Error('CONFLICT with resolve'); @@ -220,19 +222,54 @@ describe('handleReleaseBranch', () => { .mockImplementationOnce(() => Promise.reject(resolveError)) // resolve merge .mockImplementationOnce(() => Promise.resolve()); // abort after resolve - await expect( - handleReleaseBranch(git, 'origin', 'release/1.0.0', 'main'), - ).rejects.toThrow('CONFLICT with resolve'); + // Simulate conflicted files in git status + (git.status as Mock).mockImplementationOnce(() => + Promise.resolve({ conflicted: ['CHANGELOG.md', 'package.json'] }), + ); + + const error = await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + 'main', + ).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(MergeConflictError); + expect((error as MergeConflictError).message).toBe('CONFLICT with resolve'); + expect((error as MergeConflictError).conflictedFiles).toEqual([ + 'CHANGELOG.md', + 'package.json', + ]); expect(git.merge).toHaveBeenCalledTimes(4); - // First abort: after default strategy failure expect(git.merge).toHaveBeenNthCalledWith(2, ['--abort']); - // Second abort: after resolve strategy failure expect(git.merge).toHaveBeenNthCalledWith(4, ['--abort']); + expect(git.status).toHaveBeenCalledTimes(1); // push should NOT be called expect(git.push).not.toHaveBeenCalledWith('origin', 'main'); }); + test('throws PushError when push fails after successful merge', async () => { + const git = createMockGit(); + const pushError = new Error( + "fatal: could not read Username for 'https://github.com': No such device or address", + ); + + (git.push as Mock).mockImplementationOnce(() => Promise.reject(pushError)); + + const error = await handleReleaseBranch( + git, + 'origin', + 'release/1.0.0', + 'main', + ).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(PushError); + expect((error as PushError).message).toContain('could not read Username'); + // Merge should have been called (and succeeded) + expect(git.merge).toHaveBeenCalledTimes(1); + }); + test('aborts rebase when pull --rebase fails', async () => { const git = createMockGit(); const pullError = new Error('CONFLICT during rebase'); @@ -287,49 +324,34 @@ describe('handleReleaseBranch', () => { }); }); -describe('isAuthError', () => { - test('detects "could not read Username" as auth error', () => { - expect( - isAuthError( - new Error( - "fatal: could not read Username for 'https://github.com': No such device or address", - ), - ), - ).toBe(true); +describe('MergeConflictError', () => { + test('is instanceof Error', () => { + const err = new MergeConflictError('conflict', ['a.txt']); + expect(err).toBeInstanceOf(Error); + expect(err).toBeInstanceOf(MergeConflictError); }); - test('detects "Authentication failed" as auth error', () => { - expect( - isAuthError( - new Error( - "fatal: Authentication failed for 'https://github.com/org/repo.git/'", - ), - ), - ).toBe(true); + test('carries conflictedFiles', () => { + const err = new MergeConflictError('msg', ['CHANGELOG.md', 'package.json']); + expect(err.message).toBe('msg'); + expect(err.conflictedFiles).toEqual(['CHANGELOG.md', 'package.json']); }); - test('detects HTTP 401 as auth error', () => { - expect(isAuthError(new Error('HTTP 401 Unauthorized'))).toBe(true); - }); - - test('detects HTTP 403 as auth error', () => { - expect(isAuthError(new Error('HTTP 403 Forbidden'))).toBe(true); - }); - - test('does not flag merge conflicts as auth error', () => { - expect( - isAuthError( - new Error('CONFLICT (content): Merge conflict in CHANGELOG.md'), - ), - ).toBe(false); + test('works with empty conflictedFiles', () => { + const err = new MergeConflictError('msg', []); + expect(err.conflictedFiles).toEqual([]); }); +}); - test('does not flag generic git errors as auth error', () => { - expect(isAuthError(new Error('fatal: not a git repository'))).toBe(false); +describe('PushError', () => { + test('is instanceof Error', () => { + const err = new PushError('push failed'); + expect(err).toBeInstanceOf(Error); + expect(err).toBeInstanceOf(PushError); }); - test('handles non-Error values', () => { - expect(isAuthError('could not read Username')).toBe(true); - expect(isAuthError('some other error')).toBe(false); + test('carries message', () => { + const err = new PushError('could not read Username'); + expect(err.message).toBe('could not read Username'); }); }); diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 5ec842a94..27b20418b 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -387,14 +387,34 @@ async function checkRevisionStatus( } /** - * Checks whether an error from a git operation is an authentication failure - * (e.g., expired token) rather than a merge conflict or other git error. + * Error thrown when the release branch merge fails due to conflicts. + * Contains the list of conflicting file paths for diagnostics. */ -export function isAuthError(error: unknown): boolean { - const msg = error instanceof Error ? error.message : String(error); - return /could not read Username|Authentication failed|HTTP 401|HTTP 403|Invalid credentials|token.*expired/i.test( - msg, - ); +export class MergeConflictError extends Error { + public __proto__: Error; + + public constructor( + message: string, + public readonly conflictedFiles: string[], + ) { + const trueProto = new.target.prototype; + super(message); + this.__proto__ = trueProto; + } +} + +/** + * Error thrown when the post-merge push fails (e.g., expired token). + * The merge itself succeeded — only the push to the remote failed. + */ +export class PushError extends Error { + public __proto__: Error; + + public constructor(message: string) { + const trueProto = new.target.prototype; + super(message); + this.__proto__ = trueProto; + } } /** @@ -435,6 +455,7 @@ export async function handleReleaseBranch( throw pullError; } + // Stage 1: Merge — if this fails, it's a merge conflict logger.debug(`Merging ${branch} into: ${mergeTarget}`); try { await git.merge(['--no-ff', '--no-edit', branch]); @@ -455,17 +476,36 @@ export async function handleReleaseBranch( try { await git.merge(['-s', 'resolve', '--no-ff', '--no-edit', branch]); } catch (resolveError) { - // Resolve also failed — abort to leave repo in a clean state + // Resolve also failed — capture conflicting files before aborting + let conflictedFiles: string[] = []; + try { + const status = await git.status(); + conflictedFiles = status.conflicted; + } catch (_statusError) { + logger.trace('git status failed while collecting conflict info'); + } try { await git.merge(['--abort']); } catch (_abortError) { logger.trace('git merge --abort failed after resolve strategy'); } - throw resolveError; + throw new MergeConflictError( + resolveError instanceof Error + ? resolveError.message + : String(resolveError), + conflictedFiles, + ); } } - await git.push(remoteName, mergeTarget); + // Stage 2: Push — merge succeeded, any error here is auth/network + try { + await git.push(remoteName, mergeTarget); + } catch (pushError) { + throw new PushError( + pushError instanceof Error ? pushError.message : String(pushError), + ); + } if (keepBranch) { logger.info('Not deleting the release branch.'); @@ -745,32 +785,53 @@ export async function publishMain(argv: PublishOptions): Promise { // signal for a fully-published release. Report to Sentry for // observability but don't fail the command. captureException(mergeError); - const authFailure = isAuthError(mergeError); - const diagnosis = authFailure - ? `This is likely due to an expired authentication token (common for long-running publishes > 1 hour).` - : `This is likely due to a merge conflict (e.g., in CHANGELOG.md).`; - const resolutionSteps = authFailure - ? [ - ` 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}`, - ] - : [ - ` 1. Merge the release branch into the target branch, resolving conflicts`, - ` 2. Delete the release branch: git push ${argv.remote} --delete ${branchName}`, - ]; - logger.warn( - [ - `Failed to merge release branch "${branchName}" into the target branch.`, - diagnosis, + + const lines = [ + `Failed to merge release branch "${branchName}" into the target branch.`, + ]; + if (mergeError instanceof MergeConflictError) { + 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}`); + } + } + lines.push( + ``, `All publish targets completed successfully — only the post-publish merge failed.`, ``, `To resolve manually:`, - ...resolutionSteps, + ` 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( + `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.`, ``, - `Error: ${mergeError instanceof Error ? mergeError.message : String(mergeError)}`, - ].join('\n'), + `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}`, + ); + } else { + 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`, + ` 2. Delete the release branch: git push ${argv.remote} --delete ${branchName}`, + ); + } + lines.push( + ``, + `Error: ${mergeError instanceof Error ? mergeError.message : String(mergeError)}`, ); + logger.warn(lines.join('\n')); } // XXX(BYK): intentionally DO NOT await unlinking as we do not want From fb18a40ddc2011de0a8c31a17d1aa0e1ed40e82c Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Wed, 1 Apr 2026 14:01:43 +0000 Subject: [PATCH 5/5] feat: include conflict diff in MergeConflictError diagnostics Capture git diff of conflicted files before aborting the failed merge. The diff shows the actual conflict markers (<<<<<<< / ======= / >>>>>>>) so operators can see exactly what conflicted without having to reproduce the merge locally. --- src/commands/__tests__/publish.test.ts | 29 +++++++++++++++++++------- src/commands/publish.ts | 17 +++++++++++++-- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/commands/__tests__/publish.test.ts b/src/commands/__tests__/publish.test.ts index d9aeb3e94..617d89770 100644 --- a/src/commands/__tests__/publish.test.ts +++ b/src/commands/__tests__/publish.test.ts @@ -122,6 +122,7 @@ describe('handleReleaseBranch', () => { mockGit.revparse = makeChainable('main'); mockGit.raw = makeChainable(''); mockGit.status = makeChainable({ conflicted: [] }); + mockGit.diff = makeChainable(''); return mockGit as unknown as SimpleGit & Record; } @@ -227,6 +228,10 @@ describe('handleReleaseBranch', () => { Promise.resolve({ conflicted: ['CHANGELOG.md', 'package.json'] }), ); + const fakeDiff = + '<<<<<<< HEAD\nold content\n=======\nnew content\n>>>>>>> release/1.0.0'; + (git.diff as Mock).mockImplementationOnce(() => Promise.resolve(fakeDiff)); + const error = await handleReleaseBranch( git, 'origin', @@ -235,16 +240,19 @@ describe('handleReleaseBranch', () => { ).catch((e: unknown) => e); expect(error).toBeInstanceOf(MergeConflictError); - expect((error as MergeConflictError).message).toBe('CONFLICT with resolve'); - expect((error as MergeConflictError).conflictedFiles).toEqual([ + const conflictError = error as MergeConflictError; + expect(conflictError.message).toBe('CONFLICT with resolve'); + expect(conflictError.conflictedFiles).toEqual([ 'CHANGELOG.md', 'package.json', ]); + expect(conflictError.diff).toBe(fakeDiff); expect(git.merge).toHaveBeenCalledTimes(4); expect(git.merge).toHaveBeenNthCalledWith(2, ['--abort']); expect(git.merge).toHaveBeenNthCalledWith(4, ['--abort']); expect(git.status).toHaveBeenCalledTimes(1); + expect(git.diff).toHaveBeenCalledWith(['CHANGELOG.md', 'package.json']); // push should NOT be called expect(git.push).not.toHaveBeenCalledWith('origin', 'main'); }); @@ -326,20 +334,27 @@ describe('handleReleaseBranch', () => { describe('MergeConflictError', () => { test('is instanceof Error', () => { - const err = new MergeConflictError('conflict', ['a.txt']); + const err = new MergeConflictError('conflict', ['a.txt'], ''); expect(err).toBeInstanceOf(Error); expect(err).toBeInstanceOf(MergeConflictError); }); - test('carries conflictedFiles', () => { - const err = new MergeConflictError('msg', ['CHANGELOG.md', 'package.json']); + test('carries conflictedFiles and diff', () => { + const diff = '<<<<<<< HEAD\nold\n=======\nnew\n>>>>>>>'; + const err = new MergeConflictError( + 'msg', + ['CHANGELOG.md', 'package.json'], + diff, + ); expect(err.message).toBe('msg'); expect(err.conflictedFiles).toEqual(['CHANGELOG.md', 'package.json']); + expect(err.diff).toBe(diff); }); - test('works with empty conflictedFiles', () => { - const err = new MergeConflictError('msg', []); + test('works with empty conflictedFiles and no diff', () => { + const err = new MergeConflictError('msg', [], ''); expect(err.conflictedFiles).toEqual([]); + expect(err.diff).toBe(''); }); }); diff --git a/src/commands/publish.ts b/src/commands/publish.ts index 27b20418b..5979d31d1 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -388,7 +388,7 @@ async function checkRevisionStatus( /** * Error thrown when the release branch merge fails due to conflicts. - * Contains the list of conflicting file paths for diagnostics. + * Contains the list of conflicting file paths and a unified diff for diagnostics. */ export class MergeConflictError extends Error { public __proto__: Error; @@ -396,6 +396,7 @@ export class MergeConflictError extends Error { public constructor( message: string, public readonly conflictedFiles: string[], + public readonly diff: string, ) { const trueProto = new.target.prototype; super(message); @@ -476,14 +477,22 @@ export async function handleReleaseBranch( try { await git.merge(['-s', 'resolve', '--no-ff', '--no-edit', branch]); } catch (resolveError) { - // Resolve also failed — capture conflicting files before aborting + // Resolve also failed — capture conflict details before aborting let conflictedFiles: string[] = []; + let conflictDiff = ''; try { const status = await git.status(); conflictedFiles = status.conflicted; } catch (_statusError) { logger.trace('git status failed while collecting conflict info'); } + if (conflictedFiles.length > 0) { + try { + conflictDiff = await git.diff(conflictedFiles); + } catch (_diffError) { + logger.trace('git diff failed while collecting conflict diff'); + } + } try { await git.merge(['--abort']); } catch (_abortError) { @@ -494,6 +503,7 @@ export async function handleReleaseBranch( ? resolveError.message : String(resolveError), conflictedFiles, + conflictDiff, ); } } @@ -798,6 +808,9 @@ export async function publishMain(argv: PublishOptions): Promise { lines.push(` - ${file}`); } } + if (mergeError.diff) { + lines.push(``, `Diff:`, mergeError.diff); + } lines.push( ``, `All publish targets completed successfully — only the post-publish merge failed.`,