From 8d9f1ff70f435689df74658763ac72106da73ac3 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 30 Jul 2026 23:41:01 -0500 Subject: [PATCH 1/2] Create GitHub releases for npm packages in gtb publish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit npm releases shipped untagged: changeset publish creates its release tags only in the runner's local clone, and changesets/action (which would push them and cut releases) never runs the publish phase in cd.yml — publish lives in a separate OIDC release-environment job. Add an npm channel to gtb publish mirroring the Pkl channel: after changeset publish, create a GitHub release per published package (title = tag, notes = the version's CHANGELOG section), which lands the tag server-side at --target HEAD with no git push. This keeps a manual gtb publish reproducible locally (gh auth only) and sidesteps ref-count push rules. The shared machinery (releaseTag, changelog notes, skip-if-exists, release creation) moves to lib/github-release, and the Pkl channel now also passes --target so local re-runs tag the commit actually being published. Co-Authored-By: Claude Fable 5 --- .changeset/npm-release-channel.md | 10 ++ .github/workflows/cd.yml | 9 +- AGENTS.md | 20 ++-- packages/cli/src/commands/root/publish.ts | 35 ++++-- packages/cli/src/lib/github-release.ts | 119 +++++++++++++++++++ packages/cli/src/lib/manifest-sync.ts | 12 -- packages/cli/src/lib/npm-release.ts | 49 ++++++++ packages/cli/src/lib/pkl-release.ts | 83 +++---------- packages/cli/test/github-release.test.ts | 32 +++++ packages/cli/test/helpers.ts | 31 +++++ packages/cli/test/npm-release.test.ts | 138 ++++++++++++++++++++++ packages/cli/test/pkl-release.test.ts | 54 ++++----- packages/cli/test/publish.test.ts | 16 ++- packages/test-utils/src/builders.ts | 3 + 14 files changed, 472 insertions(+), 139 deletions(-) create mode 100644 .changeset/npm-release-channel.md create mode 100644 packages/cli/src/lib/github-release.ts create mode 100644 packages/cli/src/lib/npm-release.ts create mode 100644 packages/cli/test/github-release.test.ts create mode 100644 packages/cli/test/npm-release.test.ts diff --git a/.changeset/npm-release-channel.md b/.changeset/npm-release-channel.md new file mode 100644 index 00000000..fda5dbac --- /dev/null +++ b/.changeset/npm-release-channel.md @@ -0,0 +1,10 @@ +--- +'@gtbuchanan/cli': minor +--- + +`gtb publish` now creates a GitHub release for every published npm package +(title = tag, notes = the version's CHANGELOG section), landing the release +tag on GitHub via the API — previously `changeset publish` created tags only +in the runner's local clone, so npm releases shipped untagged. Both release +channels (npm and Pkl) now also pass `--target HEAD` so a local re-run tags +the commit actually being published rather than the remote default branch. diff --git a/.github/workflows/cd.yml b/.github/workflows/cd.yml index 66994b14..5d2c08fe 100644 --- a/.github/workflows/cd.yml +++ b/.github/workflows/cd.yml @@ -26,9 +26,12 @@ jobs: # skips install on a pack cache hit. - uses: gtbuchanan/tooling/.github/actions/pnpm-tasks@main - # `gtb publish` runs `changeset publish` (npm) then every non-npm channel. - # OIDC trusted publishing + provenance flow through the ambient env set - # here (the `release` environment + `id-token: write` grant them). + # `gtb publish` runs `changeset publish` (npm), creates the GitHub + # releases that land each published package's release tag (changesets + # only tags the local clone), then every non-npm channel. OIDC trusted + # publishing + provenance flow through the ambient env set here (the + # `release` environment + `id-token: write` grant them); GH_TOKEN covers + # the release creation. - env: GH_TOKEN: ${{ github.token }} NPM_CONFIG_PROVENANCE: 'true' diff --git a/AGENTS.md b/AGENTS.md index 75412960..19a4b315 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -168,7 +168,7 @@ coverage, setupFiles, and mock reset. (root-only) `PklProject` — detection, `typecheck:pkl`, and the `*.pkl` cache inputs are all top-level, not recursive. Keep sources at the root rather than under a `src/` subdirectory. -- **Release tag mirrors changesets** (`pklReleaseTag`). The +- **Release tag mirrors changesets** (`releaseTag`). The `packageZipUrl` tag segment and the `gtb publish` release tag follow changesets' own convention by repo shape: plain `v` for a single-package (root) repo, unscoped `@` for a monorepo @@ -321,12 +321,18 @@ through `package.json` scripts backed by `gtb` leaf commands. execs it without a shell, so a `&&` chain would be passed to changesets as bogus args. - **`gtb publish`** runs `changeset publish` (npm, honoring the ambient - OIDC + `NPM_CONFIG_PROVENANCE` env) and then dispatches every non-npm - channel (the Pkl zip to a `hk-config@` GitHub release), using the - assets the `pack` step already built. Each channel is idempotent and - no-ops when the workspace ships no such package, so CD runs `gtb publish` - unconditionally; a future `.NET` channel adds just another - `executePublish*` with no workflow change. + OIDC + `NPM_CONFIG_PROVENANCE` env), then creates a GitHub release per + published npm package, and then dispatches every non-npm channel (the + Pkl zip to a `hk-config@` GitHub release), using the assets the + `pack` step already built. The API-side releases are what land the + release tags on GitHub — `changeset publish` only creates tags in the + local clone, and changesets/action (which would push them) never runs + the publish phase here. Creating releases instead of pushing tags keeps + a manual `gtb publish` reproducible locally (only `gh` auth needed, with + the `workflow` scope) and sidesteps ref-count push rules. Each channel + is idempotent and no-ops when the workspace ships no such package, so CD + runs `gtb publish` unconditionally; a future `.NET` channel adds just + another `executePublish*` with no workflow change. - The `gtb-from-source` input (default `false`) flips the gtb invocation: consumers run their installed bin (`pnpm exec gtb`); the only repo that vendors `@gtbuchanan/cli` as a workspace package (tooling itself) sets it diff --git a/packages/cli/src/commands/root/publish.ts b/packages/cli/src/commands/root/publish.ts index 558712b2..128e94b6 100644 --- a/packages/cli/src/commands/root/publish.ts +++ b/packages/cli/src/commands/root/publish.ts @@ -1,5 +1,6 @@ import { defineCommand } from 'citty'; import { createLogger } from '../../lib/logger.ts'; +import { executePublishNpmReleases } from '../../lib/npm-release.ts'; import { executePublishPkl } from '../../lib/pkl-release.ts'; import { type RunOptions, capture, run } from '../../lib/process.ts'; import { rootNames } from './names.ts'; @@ -7,40 +8,50 @@ import { rootNames } from './names.ts'; /** Injected side effects for {@link executePublish}. */ export interface PublishDeps { readonly publishNonNpm: () => Promise; + readonly releaseNpm: () => Promise; readonly run: (command: string, options?: RunOptions) => Promise; } /** * Publishes every package for the current release: `changeset publish` to the * npm registry first (honoring the ambient OIDC trusted-publishing and - * provenance env that CD sets on the step), then each non-npm channel. Both - * halves are idempotent — `changeset publish` skips versions already on the - * registry, and the non-npm channels no-op when the workspace ships no such - * package — so CD runs this unconditionally. + * provenance env that CD sets on the step), then the GitHub releases for the + * packages it published (which land the release tags — `changeset publish` + * only creates tags in the local clone), then each non-npm channel. Every + * step is idempotent — `changeset publish` skips versions already on the + * registry, and both release channels skip tags that already have a release — + * so CD runs this unconditionally and a local re-run resumes where a partial + * failure left off. */ export const executePublish = async ({ publishNonNpm, + releaseNpm, run: runCommand, }: PublishDeps): Promise => { await runCommand('pnpm', { args: ['exec', 'changeset', 'publish'] }); + await releaseNpm(); await publishNonNpm(); }; /** * `gtb publish` — publishes all packages for the current release: npm via - * changesets, then every non-npm channel. Channel dispatch for the latter - * lives in {@link executePublishPkl} (today the Pkl GitHub-release channel; a - * future channel adds its own `executePublish*`). + * changesets (plus per-package GitHub releases/tags), then every non-npm + * channel. Channel dispatch for the latter lives in {@link executePublishPkl} + * (today the Pkl GitHub-release channel; a future channel adds its own + * `executePublish*`). */ export const publish = defineCommand({ meta: { description: 'Publish all packages for the current release (idempotent)', name: rootNames.publish, }, - run: () => - executePublish({ - publishNonNpm: () => - executePublishPkl({ capture, cwd: process.cwd(), logger: createLogger(), run }), + run: () => { + const deps = { capture, cwd: process.cwd(), logger: createLogger(), run }; + + return executePublish({ + publishNonNpm: () => executePublishPkl(deps), + releaseNpm: () => executePublishNpmReleases(deps), run, - }), + }); + }, }); diff --git a/packages/cli/src/lib/github-release.ts b/packages/cli/src/lib/github-release.ts new file mode 100644 index 00000000..30ea179b --- /dev/null +++ b/packages/cli/src/lib/github-release.ts @@ -0,0 +1,119 @@ +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import type { Logger } from './logger.ts'; +import type { RunOptions } from './process.ts'; + +/** + * Side-effecting I/O the GitHub-release channels depend on. Injected so the + * orchestration (discover packages, skip-if-exists, create) is unit-testable + * without spawning gh; the citty wrapper wires the real implementations. + */ +export interface GithubReleaseDeps { + readonly capture: (command: string, args: readonly string[]) => Promise; + readonly cwd: string; + readonly logger: Logger; + readonly run: (command: string, options?: RunOptions) => Promise; +} + +/** + * Release tag following changesets' own convention by repo shape: plain + * `v` for a single-package (root) repo, `@` for a + * monorepo member (where the name disambiguates). + */ +export const releaseTag = ( + name: string, + version: string, + isMonorepo: boolean, +): string => (isMonorepo ? `${name}@${version}` : `v${version}`); + +/** + * Extracts a version's section from CHANGELOG.md content — the `## ` + * heading's body up to the next `## ` (or EOF) — so the GitHub release notes + * match what changesets writes for npm packages. Returns undefined when the + * section is absent or empty. + */ +export const extractChangelogNotes = ( + changelog: string, + version: string, +): string | undefined => { + const lines = changelog.split('\n'); + const start = lines.findIndex(line => line.trim() === `## ${version}`); + if (start === -1) { + return undefined; + } + const after = lines.slice(start + 1); + const end = after.findIndex(line => line.startsWith('## ')); + const section = (end === -1 ? after : after.slice(0, end)).join('\n').trim(); + + return section === '' ? undefined : section; +}; + +/** Reads the CHANGELOG.md section for the version, if any. */ +export const releaseNotes = ( + pkgDir: string, + version: string | undefined, +): string | undefined => { + if (version === undefined) { + return undefined; + } + try { + return extractChangelogNotes(readFileSync(path.join(pkgDir, 'CHANGELOG.md'), 'utf8'), version); + } catch { + return undefined; + } +}; + +/** True when a release already exists for the tag (`gh release view` exits 0). */ +export const releaseExists = async ( + deps: GithubReleaseDeps, + tag: string, +): Promise => { + try { + await deps.capture('gh', ['release', 'view', tag]); + + return true; + } catch { + return false; + } +}; + +/** + * Resolves the commit a new release's tag should point at: the local HEAD. + * Passing it explicitly (`--target`) matters for reproducibility — without + * it, gh targets the *remote default branch* HEAD, which is only coincidentally + * right when publishing from a fresh CI checkout and silently wrong for a + * local re-run on an older commit. + */ +export const resolveHeadSha = async (deps: GithubReleaseDeps): Promise => { + const sha = await deps.capture('git', ['rev-parse', 'HEAD']); + + return sha.trim(); +}; + +/** A planned GitHub release: one tag, its notes, and any uploadable assets. */ +export interface GithubReleasePlan { + readonly assets?: readonly string[]; + readonly notes: string; + readonly tag: string; + readonly target: string; +} + +/** + * Creates the release via gh, which also creates the tag server-side at + * `target` — no git push needed, keeping the whole flow reproducible from a + * local `gtb publish`. Mirrors changesets' release shape: title = tag, body = + * the changelog section. + */ +export const createGithubRelease = ( + deps: GithubReleaseDeps, + plan: GithubReleasePlan, +): Promise => + deps.run('gh', { + args: [ + 'release', 'create', plan.tag, + '--target', plan.target, + '--title', plan.tag, + '--notes', plan.notes, + ...(plan.assets ?? []), + ], + }); diff --git a/packages/cli/src/lib/manifest-sync.ts b/packages/cli/src/lib/manifest-sync.ts index f409a1d4..194f917f 100644 --- a/packages/cli/src/lib/manifest-sync.ts +++ b/packages/cli/src/lib/manifest-sync.ts @@ -30,18 +30,6 @@ export interface ManifestContext { readonly repoPath: string; } -/** - * Release tag for a Pkl package, mirroring changesets' convention: plain - * `v` for a single-package (root) repo, `@` for a - * monorepo member (where the name disambiguates). The asset basename stays - * `@` either way — pkl derives it from `PklProject`. - */ -export const pklReleaseTag = ( - name: string, - version: string, - isMonorepo: boolean, -): string => (isMonorepo ? `${name}@${version}` : `v${version}`); - /** * Renders the native manifest for one package "kind". `render` may read the * existing file and patch it (e.g. a `.csproj`), but the Pkl writer fully diff --git a/packages/cli/src/lib/npm-release.ts b/packages/cli/src/lib/npm-release.ts new file mode 100644 index 00000000..39ef94cb --- /dev/null +++ b/packages/cli/src/lib/npm-release.ts @@ -0,0 +1,49 @@ +import { discoverWorkspace } from './discovery.ts'; +import { + type GithubReleaseDeps, + createGithubRelease, + releaseExists, + releaseNotes, + releaseTag, + resolveHeadSha, +} from './github-release.ts'; +import { readParsedManifest } from './workspace.ts'; + +/** + * Creates a GitHub release for every published npm package's current version, + * idempotently: a tag that already has a release is skipped, so re-running on + * an unchanged version is a no-op. `changeset publish` creates its release + * tags only in the local clone and changesets/action (which would push them) + * is not part of this pipeline — creating the release through the API lands + * the tag on GitHub without a git push, mirroring the Pkl channel and keeping + * the whole release reproducible from a local `gtb publish`. No assets: the + * npm registry hosts the artifact; the release is the tag + notes. + */ +export const executePublishNpmReleases = async (deps: GithubReleaseDeps): Promise => { + const discovery = discoverWorkspace({ cwd: deps.cwd }); + const packages = discovery.packages.filter(pkg => pkg.isPublished); + if (packages.length === 0) { + deps.logger.info('no published npm packages to release'); + + return; + } + + const target = await resolveHeadSha(deps); + for (const pkg of packages) { + const { name, version } = readParsedManifest(pkg.dir); + if (name === undefined || version === undefined) { + throw new Error(`${pkg.dir}: package.json is missing name or version`); + } + const tag = releaseTag(name, version, discovery.isMonorepo); + if (await releaseExists(deps, tag)) { + deps.logger.info(`release ${tag} already exists — skipping`); + continue; + } + await createGithubRelease(deps, { + notes: releaseNotes(pkg.dir, version) ?? tag, + tag, + target, + }); + deps.logger.info(`created release ${tag}`); + } +}; diff --git a/packages/cli/src/lib/pkl-release.ts b/packages/cli/src/lib/pkl-release.ts index 16abdf0f..9e90e669 100644 --- a/packages/cli/src/lib/pkl-release.ts +++ b/packages/cli/src/lib/pkl-release.ts @@ -1,10 +1,15 @@ import { readFileSync } from 'node:fs'; import path from 'node:path'; import { discoverWorkspace } from './discovery.ts'; -import type { Logger } from './logger.ts'; -import { pklReleaseTag } from './manifest-sync.ts'; +import { + type GithubReleaseDeps, + createGithubRelease, + releaseExists, + releaseNotes, + releaseTag, + resolveHeadSha, +} from './github-release.ts'; import { readPackageName, readPackageVersion } from './pkl-project.ts'; -import type { RunOptions } from './process.ts'; /** Output directory for the packaged Pkl artifacts (mirrors pack:npm). */ export const pklPackDestination = path.join('dist', 'packages', 'pkl'); @@ -43,74 +48,17 @@ export const planPklRelease = ( assets: [base, `${base}.sha256`, `${base}.zip`, `${base}.zip.sha256`].map(file => path.join(assetDir, file), ), - tag: pklReleaseTag(name, version, isMonorepo), + tag: releaseTag(name, version, isMonorepo), }; }; -/** - * Extracts a version's section from CHANGELOG.md content — the `## ` - * heading's body up to the next `## ` (or EOF) — so the GitHub release notes - * match what changesets writes for npm packages. Returns undefined when the - * section is absent or empty. - */ -export const extractChangelogNotes = ( - changelog: string, - version: string, -): string | undefined => { - const lines = changelog.split('\n'); - const start = lines.findIndex(line => line.trim() === `## ${version}`); - if (start === -1) { - return undefined; - } - const after = lines.slice(start + 1); - const end = after.findIndex(line => line.startsWith('## ')); - const section = (end === -1 ? after : after.slice(0, end)).join('\n').trim(); - - return section === '' ? undefined : section; -}; - -/** Reads the CHANGELOG.md section for the version, if any. */ -const releaseNotes = (pkgDir: string, version: string | undefined): string | undefined => { - if (version === undefined) { - return undefined; - } - try { - return extractChangelogNotes(readFileSync(path.join(pkgDir, 'CHANGELOG.md'), 'utf8'), version); - } catch { - return undefined; - } -}; - -/** - * Side-effecting I/O the publisher depends on. Injected so the orchestration - * (discover packages, skip-if-exists, create) is unit-testable without - * spawning gh; the citty wrapper wires the real implementations. - */ -export interface PklReleaseDeps { - readonly capture: (command: string, args: readonly string[]) => Promise; - readonly cwd: string; - readonly logger: Logger; - readonly run: (command: string, options?: RunOptions) => Promise; -} - -/** True when a release already exists for the tag (`gh release view` exits 0). */ -const releaseExists = async (deps: PklReleaseDeps, tag: string): Promise => { - try { - await deps.capture('gh', ['release', 'view', tag]); - - return true; - } catch { - return false; - } -}; - /** * Publishes every Pkl package in the workspace to a GitHub release, * idempotently: a tag that already has a release is skipped, so re-running on * an unchanged version is a no-op. Designed to run in CD after `pack` has * produced the assets. */ -export const executePublishPkl = async (deps: PklReleaseDeps): Promise => { +export const executePublishPkl = async (deps: GithubReleaseDeps): Promise => { const discovery = discoverWorkspace({ cwd: deps.cwd }); const packages = discovery.packages.filter(pkg => pkg.hasPklPackage); if (packages.length === 0) { @@ -119,6 +67,7 @@ export const executePublishPkl = async (deps: PklReleaseDeps): Promise => return; } + const target = await resolveHeadSha(deps); for (const pkg of packages) { const source = readFileSync(path.join(pkg.dir, 'PklProject'), 'utf8'); const name = readPackageName(source); @@ -131,11 +80,11 @@ export const executePublishPkl = async (deps: PklReleaseDeps): Promise => deps.logger.info(`release ${tag} already exists — skipping`); continue; } - // Mirror changesets' release shape: title = tag, body = the changelog - // section (falling back to the tag when there's no entry). - const notes = releaseNotes(pkg.dir, version) ?? tag; - await deps.run('gh', { - args: ['release', 'create', tag, '--title', tag, '--notes', notes, ...assets], + await createGithubRelease(deps, { + assets, + notes: releaseNotes(pkg.dir, version) ?? tag, + tag, + target, }); deps.logger.info(`created release ${tag}`); } diff --git a/packages/cli/test/github-release.test.ts b/packages/cli/test/github-release.test.ts new file mode 100644 index 00000000..98dde995 --- /dev/null +++ b/packages/cli/test/github-release.test.ts @@ -0,0 +1,32 @@ +import { describe, it } from 'vitest'; +import { extractChangelogNotes, releaseTag } from '#src/lib/github-release.js'; + +describe.concurrent(releaseTag, () => { + it('uses @ for a monorepo member', ({ expect }) => { + expect(releaseTag('@scope/pkg', '1.2.3', true)).toBe('@scope/pkg@1.2.3'); + }); + + it('uses a plain v for a single-package repo', ({ expect }) => { + expect(releaseTag('@scope/pkg', '1.2.3', false)).toBe('v1.2.3'); + }); +}); + +describe.concurrent(extractChangelogNotes, () => { + const changelog = '# pkg\n\n## 1.2.0\n\n### Minor Changes\n\n- A change\n\n## 1.1.0\n\n- Older\n'; + + it('extracts the section body up to the next heading', ({ expect }) => { + expect(extractChangelogNotes(changelog, '1.2.0')).toBe('### Minor Changes\n\n- A change'); + }); + + it('extracts the final section through end of file', ({ expect }) => { + expect(extractChangelogNotes(changelog, '1.1.0')).toBe('- Older'); + }); + + it('returns undefined when the version is absent', ({ expect }) => { + expect(extractChangelogNotes(changelog, '9.9.9')).toBeUndefined(); + }); + + it('returns undefined for an empty section', ({ expect }) => { + expect(extractChangelogNotes('## 1.0.0\n\n## 0.9.0\n\n- x\n', '1.0.0')).toBeUndefined(); + }); +}); diff --git a/packages/cli/test/helpers.ts b/packages/cli/test/helpers.ts index 5b029cf2..e0102e48 100644 --- a/packages/cli/test/helpers.ts +++ b/packages/cli/test/helpers.ts @@ -104,6 +104,37 @@ export const pklProjectSource = ( ].join('\n'); }; +/** A scaffolded temp monorepo containing one published npm package. */ +export interface NpmWorkspace { + /** Scoped package name (`@scope/pkg`). */ + readonly name: string; + /** Absolute path of the published package directory. */ + readonly pkgDir: string; + /** Workspace root directory. */ + readonly root: string; + /** Package version. */ + readonly version: string; +} + +/** Scaffolds a temp monorepo with a single published npm package. */ +export const createNpmWorkspace = (): NpmWorkspace => { + const root = createTempDir(); + writeFileSync(path.join(root, 'pnpm-workspace.yaml'), "packages:\n - 'packages/*'\n"); + writeJson(root, 'package.json', { name: build.packageName(), private: true }); + + const name = build.scopedPackageName(); + const version = build.semverVersion(); + const pkgDir = path.join(root, 'packages', build.packageName()); + mkdirSync(pkgDir, { recursive: true }); + writeJson(pkgDir, 'package.json', { + name, + publishConfig: { directory: build.publishDirectory() }, + version, + }); + + return { name, pkgDir, root, version }; +}; + /** Options for {@link createPklWorkspace}. */ export interface PklWorkspaceOptions { /** diff --git a/packages/cli/test/npm-release.test.ts b/packages/cli/test/npm-release.test.ts new file mode 100644 index 00000000..4f8015b8 --- /dev/null +++ b/packages/cli/test/npm-release.test.ts @@ -0,0 +1,138 @@ +import { writeFileSync } from 'node:fs'; +import path from 'node:path'; +import * as build from '@gtbuchanan/test-utils/builders'; +import { describe, it } from 'vitest'; +import type { GithubReleaseDeps } from '#src/lib/github-release.js'; +import { createLogger } from '#src/lib/logger.js'; +import { executePublishNpmReleases } from '#src/lib/npm-release.js'; +import { captureLogger, createNpmWorkspace, createTempDir, writeJson } from './helpers.ts'; + +interface RunCall { + readonly args: readonly string[]; + readonly command: string; +} + +const silentLogger = createLogger( + { write: () => true } as unknown as NodeJS.WritableStream, + { write: () => true } as unknown as NodeJS.WritableStream, +); + +interface Stub { + readonly deps: GithubReleaseDeps; + readonly runCalls: readonly RunCall[]; + readonly sha: string; +} + +/** Builds injected deps; `exists` decides whether `gh release view` succeeds. */ +const stubDeps = ( + cwd: string, + options: { exists: boolean; logger?: GithubReleaseDeps['logger'] }, +): Stub => { + const runCalls: RunCall[] = []; + const sha = build.commitSha(); + + return { + deps: { + capture: (command) => { + if (command === 'git') { + return Promise.resolve(`${sha}\n`); + } + + return options.exists + ? Promise.resolve('') + : Promise.reject(new Error('release not found')); + }, + cwd, + logger: options.logger ?? silentLogger, + run: (command, runOptions) => { + runCalls.push({ args: runOptions?.args ?? [], command }); + + return Promise.resolve(); + }, + }, + runCalls, + sha, + }; +}; + +describe.concurrent(executePublishNpmReleases, () => { + it('creates a release tagging HEAD when none exists', async ({ expect }) => { + const ws = createNpmWorkspace(); + const { deps, runCalls, sha } = stubDeps(ws.root, { exists: false }); + + await executePublishNpmReleases(deps); + + const tag = `${ws.name}@${ws.version}`; + + expect(runCalls).toStrictEqual([{ + args: [ + 'release', 'create', tag, '--target', sha, '--title', tag, '--notes', tag, + ], + command: 'gh', + }]); + }); + + it('releases a single-package repo with a plain v tag', async ({ expect }) => { + const root = createTempDir(); + const version = build.semverVersion(); + writeJson(root, 'package.json', { + name: build.scopedPackageName(), + publishConfig: { directory: build.publishDirectory() }, + version, + }); + const { deps, runCalls } = stubDeps(root, { exists: false }); + + await executePublishNpmReleases(deps); + + expect(runCalls).toHaveLength(1); + expect(runCalls[0]?.args.slice(0, 3)).toStrictEqual(['release', 'create', `v${version}`]); + }); + + it('uses the matching CHANGELOG.md section as the release notes', async ({ expect }) => { + const ws = createNpmWorkspace(); + writeFileSync( + path.join(ws.pkgDir, 'CHANGELOG.md'), + `# pkg\n\n## ${ws.version}\n\n### Minor Changes\n\n- Added a thing\n\n## 0.0.1\n\n- old\n`, + ); + const { deps, runCalls } = stubDeps(ws.root, { exists: false }); + + await executePublishNpmReleases(deps); + + const args = runCalls[0]?.args ?? []; + + expect(args[args.indexOf('--notes') + 1]).toBe('### Minor Changes\n\n- Added a thing'); + }); + + it('skips when the release already exists', async ({ expect }) => { + const ws = createNpmWorkspace(); + const { deps, runCalls } = stubDeps(ws.root, { exists: true }); + + await executePublishNpmReleases(deps); + + expect(runCalls).toHaveLength(0); + }); + + it('throws when the manifest is missing its package identity', async ({ expect }) => { + const ws = createNpmWorkspace(); + writeJson(ws.pkgDir, 'package.json', { + publishConfig: { directory: build.publishDirectory() }, + version: build.semverVersion(), + }); + const { deps } = stubDeps(ws.root, { exists: false }); + + await expect(executePublishNpmReleases(deps)).rejects.toThrow(/name or version/v); + }); + + it('no-ops when no package is published', async ({ expect }) => { + const root = createTempDir(); + writeFileSync(path.join(root, 'pnpm-workspace.yaml'), "packages:\n - 'packages/*'\n"); + writeJson(root, 'package.json', { name: build.packageName(), private: true }); + const captured = captureLogger(); + const { deps, runCalls } = stubDeps(root, { exists: false, logger: captured.logger }); + + await executePublishNpmReleases(deps); + + expect(runCalls).toHaveLength(0); + expect(captured.out()).toContain('no published npm packages'); + }); +}); diff --git a/packages/cli/test/pkl-release.test.ts b/packages/cli/test/pkl-release.test.ts index 97e1aedb..3336a692 100644 --- a/packages/cli/test/pkl-release.test.ts +++ b/packages/cli/test/pkl-release.test.ts @@ -2,13 +2,9 @@ import { writeFileSync } from 'node:fs'; import path from 'node:path'; import * as build from '@gtbuchanan/test-utils/builders'; import { describe, it } from 'vitest'; +import type { GithubReleaseDeps } from '#src/lib/github-release.js'; import { createLogger } from '#src/lib/logger.js'; -import { - type PklReleaseDeps, - executePublishPkl, - extractChangelogNotes, - planPklRelease, -} from '#src/lib/pkl-release.js'; +import { executePublishPkl, planPklRelease } from '#src/lib/pkl-release.js'; import { captureLogger, createPklWorkspace, createTempDir, writeJson } from './helpers.ts'; interface RunCall { @@ -22,21 +18,30 @@ const silentLogger = createLogger( ); interface Stub { - readonly deps: PklReleaseDeps; + readonly deps: GithubReleaseDeps; readonly runCalls: readonly RunCall[]; + readonly sha: string; } /** Builds injected deps; `exists` decides whether `gh release view` succeeds. */ const stubDeps = ( cwd: string, - options: { exists: boolean; logger?: PklReleaseDeps['logger'] }, + options: { exists: boolean; logger?: GithubReleaseDeps['logger'] }, ): Stub => { const runCalls: RunCall[] = []; + const sha = build.commitSha(); return { deps: { - capture: () => - options.exists ? Promise.resolve('') : Promise.reject(new Error('release not found')), + capture: (command) => { + if (command === 'git') { + return Promise.resolve(`${sha}\n`); + } + + return options.exists + ? Promise.resolve('') + : Promise.reject(new Error('release not found')); + }, cwd, logger: options.logger ?? silentLogger, run: (command, runOptions) => { @@ -46,6 +51,7 @@ const stubDeps = ( }, }, runCalls, + sha, }; }; @@ -73,9 +79,9 @@ describe.concurrent(planPklRelease, () => { }); describe.concurrent(executePublishPkl, () => { - it('creates a release with the assets when none exists', async ({ expect }) => { + it('creates a release tagging HEAD with the assets when none exists', async ({ expect }) => { const ws = createPklWorkspace(); - const { deps, runCalls } = stubDeps(ws.root, { exists: false }); + const { deps, runCalls, sha } = stubDeps(ws.root, { exists: false }); await executePublishPkl(deps); @@ -83,8 +89,8 @@ describe.concurrent(executePublishPkl, () => { expect(runCalls).toHaveLength(1); expect(runCalls[0]?.command).toBe('gh'); - expect(runCalls[0]?.args.slice(0, 7)).toStrictEqual([ - 'release', 'create', tag, '--title', tag, '--notes', tag, + expect(runCalls[0]?.args.slice(0, 9)).toStrictEqual([ + 'release', 'create', tag, '--target', sha, '--title', tag, '--notes', tag, ]); expect(runCalls[0]?.args).toContain( path.join(ws.pkgDir, 'dist', 'packages', 'pkl', `${tag}.zip`), @@ -157,23 +163,3 @@ describe.concurrent(executePublishPkl, () => { expect(captured.out()).toContain('no Pkl packages to publish'); }); }); - -describe.concurrent(extractChangelogNotes, () => { - const changelog = '# pkg\n\n## 1.2.0\n\n### Minor Changes\n\n- A change\n\n## 1.1.0\n\n- Older\n'; - - it('extracts the section body up to the next heading', ({ expect }) => { - expect(extractChangelogNotes(changelog, '1.2.0')).toBe('### Minor Changes\n\n- A change'); - }); - - it('extracts the final section through end of file', ({ expect }) => { - expect(extractChangelogNotes(changelog, '1.1.0')).toBe('- Older'); - }); - - it('returns undefined when the version is absent', ({ expect }) => { - expect(extractChangelogNotes(changelog, '9.9.9')).toBeUndefined(); - }); - - it('returns undefined for an empty section', ({ expect }) => { - expect(extractChangelogNotes('## 1.0.0\n\n## 0.9.0\n\n- x\n', '1.0.0')).toBeUndefined(); - }); -}); diff --git a/packages/cli/test/publish.test.ts b/packages/cli/test/publish.test.ts index 2938643c..49f6544d 100644 --- a/packages/cli/test/publish.test.ts +++ b/packages/cli/test/publish.test.ts @@ -7,20 +7,28 @@ interface RunCall { } describe.concurrent(executePublish, () => { - it('awaits npm publish before starting the non-npm channels', async ({ expect }) => { + it('runs npm publish, then the npm releases, then the non-npm channels', async ({ expect }) => { const order: string[] = []; const runCalls: RunCall[] = []; let hasNpmFinished = false; await executePublish({ publishNonNpm: async () => { - // npm publish must have fully settled before this starts. - expect(hasNpmFinished).toBe(true); + // The npm channel (publish + releases) must have settled before this. + expect(order).toContain('releases:end'); order.push('non-npm:start'); await Promise.resolve(); order.push('non-npm:end'); }, + releaseNpm: async () => { + // npm publish must have fully settled before its releases are cut. + expect(hasNpmFinished).toBe(true); + + order.push('releases:start'); + await Promise.resolve(); + order.push('releases:end'); + }, run: async (command, options) => { order.push('npm:start'); runCalls.push({ args: options?.args ?? [], command }); @@ -34,7 +42,7 @@ describe.concurrent(executePublish, () => { { args: ['exec', 'changeset', 'publish'], command: 'pnpm' }, ]); expect(order).toStrictEqual([ - 'npm:start', 'npm:end', 'non-npm:start', 'non-npm:end', + 'npm:start', 'npm:end', 'releases:start', 'releases:end', 'non-npm:start', 'non-npm:end', ]); }); }); diff --git a/packages/test-utils/src/builders.ts b/packages/test-utils/src/builders.ts index 41191a4b..5a3591a4 100644 --- a/packages/test-utils/src/builders.ts +++ b/packages/test-utils/src/builders.ts @@ -18,6 +18,9 @@ const importSubpath = (): string => `#${faker.lorem.slug()}/*`; export const binMap = (): Record => arbitraryRecord(packageName, relativePath); +/** Arbitrary full-length git commit SHA. */ +export const commitSha = (): string => faker.git.commitSha(); + /** Arbitrary `dependencies` / `devDependencies` map (package name → semver range). */ export const dependencyMap = (): Record => arbitraryRecord(scopedPackageName, semverRange); From 66fb7ae15144c2d7be15b42aea3df6d6118a8b5a Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Fri, 31 Jul 2026 21:30:46 -0500 Subject: [PATCH 2/2] Extract shared GitHub release loop from channels CodeRabbit review feedback on the npm release channel: - Move the per-package release iteration (tag, skip-if-exists, changelog notes, HEAD targeting) into a publishReleases helper in lib/github-release; the npm and Pkl channels now just resolve each package's identity and assets, so the channels can't drift apart. Identity validation moves ahead of any side effects as a result. - Generate the releaseTag test inputs via builders instead of hard-coded literals, per the test-data convention. - Drop the commitSha builder: it wrapped faker.git.commitSha() without adding any domain shape; call faker directly at the use sites. Co-Authored-By: Claude Fable 5 --- packages/cli/src/lib/github-release.ts | 40 ++++++++++++++++++++++++ packages/cli/src/lib/npm-release.ts | 28 ++++------------- packages/cli/src/lib/pkl-release.ts | 34 +++++++------------- packages/cli/test/github-release.test.ts | 10 ++++-- packages/cli/test/npm-release.test.ts | 3 +- packages/cli/test/pkl-release.test.ts | 3 +- packages/test-utils/src/builders.ts | 3 -- 7 files changed, 69 insertions(+), 52 deletions(-) diff --git a/packages/cli/src/lib/github-release.ts b/packages/cli/src/lib/github-release.ts index 30ea179b..c61dc04c 100644 --- a/packages/cli/src/lib/github-release.ts +++ b/packages/cli/src/lib/github-release.ts @@ -117,3 +117,43 @@ export const createGithubRelease = ( ...(plan.assets ?? []), ], }); + +/** + * A package pending release: its resolved identity (validated by the channel, + * which knows its own manifest format) plus any uploadable assets. + */ +export interface PendingRelease { + readonly assets?: readonly string[]; + readonly dir: string; + readonly name: string; + readonly version: string; +} + +/** + * Creates a GitHub release for each pending package, idempotently: a tag that + * already has a release is skipped, so re-running on an unchanged version is + * a no-op (and a run that died between channels backfills on the next). The + * shared loop for every release channel — tag convention, changelog notes, + * skip-if-exists, HEAD targeting — so channels can't drift apart. + */ +export const publishReleases = async ( + deps: GithubReleaseDeps, + packages: readonly PendingRelease[], + isMonorepo: boolean, +): Promise => { + const target = await resolveHeadSha(deps); + for (const { assets, dir, name, version } of packages) { + const tag = releaseTag(name, version, isMonorepo); + if (await releaseExists(deps, tag)) { + deps.logger.info(`release ${tag} already exists — skipping`); + continue; + } + await createGithubRelease(deps, { + ...(assets !== undefined && { assets }), + notes: releaseNotes(dir, version) ?? tag, + tag, + target, + }); + deps.logger.info(`created release ${tag}`); + } +}; diff --git a/packages/cli/src/lib/npm-release.ts b/packages/cli/src/lib/npm-release.ts index 39ef94cb..e08f07c7 100644 --- a/packages/cli/src/lib/npm-release.ts +++ b/packages/cli/src/lib/npm-release.ts @@ -1,12 +1,5 @@ import { discoverWorkspace } from './discovery.ts'; -import { - type GithubReleaseDeps, - createGithubRelease, - releaseExists, - releaseNotes, - releaseTag, - resolveHeadSha, -} from './github-release.ts'; +import { type GithubReleaseDeps, publishReleases } from './github-release.ts'; import { readParsedManifest } from './workspace.ts'; /** @@ -28,22 +21,13 @@ export const executePublishNpmReleases = async (deps: GithubReleaseDeps): Promis return; } - const target = await resolveHeadSha(deps); - for (const pkg of packages) { + const pending = packages.map((pkg) => { const { name, version } = readParsedManifest(pkg.dir); if (name === undefined || version === undefined) { throw new Error(`${pkg.dir}: package.json is missing name or version`); } - const tag = releaseTag(name, version, discovery.isMonorepo); - if (await releaseExists(deps, tag)) { - deps.logger.info(`release ${tag} already exists — skipping`); - continue; - } - await createGithubRelease(deps, { - notes: releaseNotes(pkg.dir, version) ?? tag, - tag, - target, - }); - deps.logger.info(`created release ${tag}`); - } + + return { dir: pkg.dir, name, version }; + }); + await publishReleases(deps, pending, discovery.isMonorepo); }; diff --git a/packages/cli/src/lib/pkl-release.ts b/packages/cli/src/lib/pkl-release.ts index 9e90e669..601cd52d 100644 --- a/packages/cli/src/lib/pkl-release.ts +++ b/packages/cli/src/lib/pkl-release.ts @@ -1,14 +1,7 @@ import { readFileSync } from 'node:fs'; import path from 'node:path'; import { discoverWorkspace } from './discovery.ts'; -import { - type GithubReleaseDeps, - createGithubRelease, - releaseExists, - releaseNotes, - releaseTag, - resolveHeadSha, -} from './github-release.ts'; +import { type GithubReleaseDeps, publishReleases, releaseTag } from './github-release.ts'; import { readPackageName, readPackageVersion } from './pkl-project.ts'; /** Output directory for the packaged Pkl artifacts (mirrors pack:npm). */ @@ -67,25 +60,20 @@ export const executePublishPkl = async (deps: GithubReleaseDeps): Promise return; } - const target = await resolveHeadSha(deps); - for (const pkg of packages) { + const pending = packages.map((pkg) => { const source = readFileSync(path.join(pkg.dir, 'PklProject'), 'utf8'); const name = readPackageName(source); const version = readPackageVersion(source); if (name === undefined || version === undefined) { throw new Error(`${pkg.dir}: PklProject is missing package.name or package.version`); } - const { assets, tag } = planPklRelease(pkg.dir, name, version, discovery.isMonorepo); - if (await releaseExists(deps, tag)) { - deps.logger.info(`release ${tag} already exists — skipping`); - continue; - } - await createGithubRelease(deps, { - assets, - notes: releaseNotes(pkg.dir, version) ?? tag, - tag, - target, - }); - deps.logger.info(`created release ${tag}`); - } + + return { + assets: planPklRelease(pkg.dir, name, version, discovery.isMonorepo).assets, + dir: pkg.dir, + name, + version, + }; + }); + await publishReleases(deps, pending, discovery.isMonorepo); }; diff --git a/packages/cli/test/github-release.test.ts b/packages/cli/test/github-release.test.ts index 98dde995..ee154dec 100644 --- a/packages/cli/test/github-release.test.ts +++ b/packages/cli/test/github-release.test.ts @@ -1,13 +1,19 @@ +import * as build from '@gtbuchanan/test-utils/builders'; import { describe, it } from 'vitest'; import { extractChangelogNotes, releaseTag } from '#src/lib/github-release.js'; describe.concurrent(releaseTag, () => { it('uses @ for a monorepo member', ({ expect }) => { - expect(releaseTag('@scope/pkg', '1.2.3', true)).toBe('@scope/pkg@1.2.3'); + const name = build.scopedPackageName(); + const version = build.semverVersion(); + + expect(releaseTag(name, version, true)).toBe(`${name}@${version}`); }); it('uses a plain v for a single-package repo', ({ expect }) => { - expect(releaseTag('@scope/pkg', '1.2.3', false)).toBe('v1.2.3'); + const version = build.semverVersion(); + + expect(releaseTag(build.scopedPackageName(), version, false)).toBe(`v${version}`); }); }); diff --git a/packages/cli/test/npm-release.test.ts b/packages/cli/test/npm-release.test.ts index 4f8015b8..a69797c4 100644 --- a/packages/cli/test/npm-release.test.ts +++ b/packages/cli/test/npm-release.test.ts @@ -1,5 +1,6 @@ import { writeFileSync } from 'node:fs'; import path from 'node:path'; +import { faker } from '@faker-js/faker'; import * as build from '@gtbuchanan/test-utils/builders'; import { describe, it } from 'vitest'; import type { GithubReleaseDeps } from '#src/lib/github-release.js'; @@ -29,7 +30,7 @@ const stubDeps = ( options: { exists: boolean; logger?: GithubReleaseDeps['logger'] }, ): Stub => { const runCalls: RunCall[] = []; - const sha = build.commitSha(); + const sha = faker.git.commitSha(); return { deps: { diff --git a/packages/cli/test/pkl-release.test.ts b/packages/cli/test/pkl-release.test.ts index 3336a692..69a7e0f0 100644 --- a/packages/cli/test/pkl-release.test.ts +++ b/packages/cli/test/pkl-release.test.ts @@ -1,5 +1,6 @@ import { writeFileSync } from 'node:fs'; import path from 'node:path'; +import { faker } from '@faker-js/faker'; import * as build from '@gtbuchanan/test-utils/builders'; import { describe, it } from 'vitest'; import type { GithubReleaseDeps } from '#src/lib/github-release.js'; @@ -29,7 +30,7 @@ const stubDeps = ( options: { exists: boolean; logger?: GithubReleaseDeps['logger'] }, ): Stub => { const runCalls: RunCall[] = []; - const sha = build.commitSha(); + const sha = faker.git.commitSha(); return { deps: { diff --git a/packages/test-utils/src/builders.ts b/packages/test-utils/src/builders.ts index 5a3591a4..41191a4b 100644 --- a/packages/test-utils/src/builders.ts +++ b/packages/test-utils/src/builders.ts @@ -18,9 +18,6 @@ const importSubpath = (): string => `#${faker.lorem.slug()}/*`; export const binMap = (): Record => arbitraryRecord(packageName, relativePath); -/** Arbitrary full-length git commit SHA. */ -export const commitSha = (): string => faker.git.commitSha(); - /** Arbitrary `dependencies` / `devDependencies` map (package name → semver range). */ export const dependencyMap = (): Record => arbitraryRecord(scopedPackageName, semverRange);