diff --git a/AGENTS.md b/AGENTS.md index 955135396..1b1d8175a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -118,21 +118,23 @@ Some operations need explicit `isDryRun()` checks: ### 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. +- **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 -- **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}}\`. + -### 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. + + - +- **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. -- **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. +### 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..617d89770 100644 --- a/src/commands/__tests__/publish.test.ts +++ b/src/commands/__tests__/publish.test.ts @@ -1,7 +1,12 @@ 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, + MergeConflictError, + PushError, +} from '../publish'; import type { SimpleGit } from 'simple-git'; vi.mock('../../utils/system'); @@ -116,6 +121,8 @@ describe('handleReleaseBranch', () => { mockGit.remote = makeChainable(); mockGit.revparse = makeChainable('main'); mockGit.raw = makeChainable(''); + mockGit.status = makeChainable({ conflicted: [] }); + mockGit.diff = makeChainable(''); return mockGit as unknown as SimpleGit & Record; } @@ -205,7 +212,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'); @@ -216,19 +223,61 @@ 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 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', + 'release/1.0.0', + 'main', + ).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(MergeConflictError); + 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); - // 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); + expect(git.diff).toHaveBeenCalledWith(['CHANGELOG.md', 'package.json']); // 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'); @@ -282,3 +331,42 @@ describe('handleReleaseBranch', () => { expect(git.push).toHaveBeenCalledWith('origin', 'master'); }); }); + +describe('MergeConflictError', () => { + test('is instanceof Error', () => { + const err = new MergeConflictError('conflict', ['a.txt'], ''); + expect(err).toBeInstanceOf(Error); + expect(err).toBeInstanceOf(MergeConflictError); + }); + + 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 and no diff', () => { + const err = new MergeConflictError('msg', [], ''); + expect(err.conflictedFiles).toEqual([]); + expect(err.diff).toBe(''); + }); +}); + +describe('PushError', () => { + test('is instanceof Error', () => { + const err = new PushError('push failed'); + expect(err).toBeInstanceOf(Error); + expect(err).toBeInstanceOf(PushError); + }); + + 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 3674af2cb..5979d31d1 100644 --- a/src/commands/publish.ts +++ b/src/commands/publish.ts @@ -386,6 +386,38 @@ async function checkRevisionStatus( await statusProvider.waitForTheBuildToSucceed(revision); } +/** + * Error thrown when the release branch merge fails due to conflicts. + * Contains the list of conflicting file paths and a unified diff for diagnostics. + */ +export class MergeConflictError extends Error { + public __proto__: Error; + + public constructor( + message: string, + public readonly conflictedFiles: string[], + public readonly diff: 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; + } +} + /** * Deals with the release branch after publishing is done * @@ -424,6 +456,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]); @@ -444,17 +477,45 @@ 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 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) { logger.trace('git merge --abort failed after resolve strategy'); } - throw resolveError; + throw new MergeConflictError( + resolveError instanceof Error + ? resolveError.message + : String(resolveError), + conflictedFiles, + conflictDiff, + ); } } - 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.'); @@ -734,19 +795,56 @@ 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); - 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).`, + + 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}`); + } + } + 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( + `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