Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,21 +118,23 @@ Some operations need explicit `isDryRun()` checks:

### Architecture

<!-- lore:019cb31a-14ce-7892-b22a-0327cfcebc13 -->
<!-- lore:019d4479-fd88-7a7d-96b1-2a6b668dfc45 -->

- **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.

<!-- lore:019cb31a-14c8-7ba9-b1c4-81b2e8bf7e85 -->
### 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/\<product>/{{version}}/{{file}}\`.
<!-- lore:019d4479-fd93-7328-a4b5-f9405e4aad8b -->

### 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.

<!-- lore:019d4479-fd9a-7a97-931f-8f9a18e5752e -->

<!-- lore:019c9f57-aa0c-7a2a-8a10-911b13b48fc0 -->
- **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

<!-- lore:019c9be1-33d1-7b6e-b107-ae7ad42a4ea4 -->
<!-- lore:019d4479-fd97-764e-a7ec-0d32360b0f16 -->

- **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 \<pkg>\` 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.
<!-- End lore-managed section -->
102 changes: 95 additions & 7 deletions src/commands/__tests__/publish.test.ts
Original file line number Diff line number Diff line change
@@ -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');
Expand Down Expand Up @@ -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<string, Mock>;
}
Expand Down Expand Up @@ -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');
Expand All @@ -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');
Expand Down Expand Up @@ -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');
});
});
116 changes: 107 additions & 9 deletions src/commands/publish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,38 @@
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
*
Expand Down Expand Up @@ -418,12 +450,13 @@
// Pull --rebase failure can leave the repo in an active rebase state
try {
await git.raw(['rebase', '--abort']);
} catch (_abortError) {

Check warning on line 453 in src/commands/publish.ts

View workflow job for this annotation

GitHub Actions / Lint fixes

[@typescript-eslint/no-unused-vars] '_abortError' is defined but never used.
logger.trace('git rebase --abort failed (may be no rebase in progress)');
}
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]);
Expand All @@ -434,7 +467,7 @@
);
try {
await git.merge(['--abort']);
} catch (_abortError) {

Check warning on line 470 in src/commands/publish.ts

View workflow job for this annotation

GitHub Actions / Lint fixes

[@typescript-eslint/no-unused-vars] '_abortError' is defined but never used.
// merge --abort can fail if no merge in progress (e.g. pull failed)
logger.trace('git merge --abort failed (may be no merge in progress)');
}
Expand All @@ -444,17 +477,45 @@
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) {

Check warning on line 486 in src/commands/publish.ts

View workflow job for this annotation

GitHub Actions / Lint fixes

[@typescript-eslint/no-unused-vars] '_statusError' is defined but never used.
logger.trace('git status failed while collecting conflict info');
}
if (conflictedFiles.length > 0) {
try {
conflictDiff = await git.diff(conflictedFiles);
} catch (_diffError) {

Check warning on line 492 in src/commands/publish.ts

View workflow job for this annotation

GitHub Actions / Lint fixes

[@typescript-eslint/no-unused-vars] '_diffError' is defined but never used.
logger.trace('git diff failed while collecting conflict diff');
}
}
try {
await git.merge(['--abort']);
} catch (_abortError) {

Check warning on line 498 in src/commands/publish.ts

View workflow job for this annotation

GitHub Actions / Lint fixes

[@typescript-eslint/no-unused-vars] '_abortError' is defined but never used.
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(
Comment thread
sentry[bot] marked this conversation as resolved.
pushError instanceof Error ? pushError.message : String(pushError),
);
}

if (keepBranch) {
logger.info('Not deleting the release branch.');
Expand Down Expand Up @@ -734,19 +795,56 @@
// 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
Expand Down
Loading