diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 6d7c73991..4df046667 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -31,8 +31,8 @@ jobs: # The same check `npm version` runs first (see CONTRIBUTING.md): an # open issue labelled release-hold stops the release. Both jobs that - # release need this one and check again themselves, so a held tag - # reaches neither npm nor the GitHub release notes. A dispatch on a + # release need this one and check again themselves, each when it + # runs, so a hold open at this check stops both. A dispatch on a # branch releases nothing, so it isn't held. - name: Release holds if: startsWith(github.ref, 'refs/tags/v') @@ -108,7 +108,10 @@ jobs: needs: build-and-test # Deliberately independent of the npm publish job: some consumers run # github-clone checkouts, and release notes must exist even when npm - # publish fails — the two jobs succeed or fail on their own. + # publish fails — the two jobs succeed or fail on their own. Both need + # the build job, so a failed build stops both; past it, so do a tag with + # no changelog section, which neither can release, and an open release + # hold, which each checks. if: startsWith(github.ref, 'refs/tags/v') # issues: read lets the release-hold check list the holds. @@ -121,9 +124,15 @@ jobs: with: persist-credentials: false + - name: Setup Node.js + uses: actions/setup-node@249970729cb0ef3589644e2896645e5dc5ba9c38 # v6.5.0 + with: + node-version: 24 + # Checked again here, as prepublishOnly checks it again in the - # publish job: a hold filed after the build job's check stops these - # notes as it stops the npm publish. + # publish job, each when its job reaches it: a hold filed after the + # build job's check stops whichever of the two checks after it. A + # hold filed between the two checks can stop one and not the other. - name: Release holds run: node scripts/release-holds.mjs env: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ed34dc1f6..eb6012a45 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -133,22 +133,30 @@ never do. (subsections emitted in `### Breaking` / `### Added` / `### Changed` / `### Fixed` order), deletes the consumed fragments, keeps a fresh empty `Unreleased` above, and refuses to release when there is nothing to - release; npm then commits and tags. `git push --follow-tags` triggers CI, - which refuses a tag with no matching changelog section, publishes + release; npm then commits and tags. `git push --follow-tags` triggers + CI, which refuses a tag with no matching changelog section, publishes `@animalabs/agent-framework` to npm, and creates the GitHub release with that section as its notes. The two release jobs are independent: some consumers run github-clone checkouts, so release notes must exist even - when npm publish fails. Version bumps are a maintainer release-time - action, not part of feature PRs. + when npm publish fails. Both need the build job, so a failed build stops + both; past it, so do a tag with no changelog section, which neither can + release, and an open release hold (below), which each checks. Version + bumps are a maintainer release-time action, not part of feature PRs. - **Release holds.** When releasing `main` as it stands would harm someone running it, open an issue that says what has to happen first and label it `release-hold`, or ask a maintainer to label it. Close it - once that's happened, or if it no longer applies. While one is open, - `npm version` stops before it bumps anything, `npm publish` refuses, - and the publish workflow refuses both its npm publish and its GitHub - release notes. The check also stops a release when it can't read the - holds from GitHub, and says why. npm's `--ignore-scripts` skips it, as - it skips every lifecycle script. + once that's happened, or if it no longer applies. When what has to + happen is a fix that this package itself must release, close the hold + once the fix is merged, then release. While one is open, `npm version` + stops before it bumps anything, `npm publish` refuses, and the publish + workflow refuses both its npm publish and its GitHub release notes, + each checking when it runs. The check also stops a release when it + can't read the holds from GitHub, and says why. Closing the hold is + the way past it. npm's `--ignore-scripts` skips the check, but on + `npm version` it skips the changelog fold too, so the tag lands + without its changelog section and the publish workflow's changelog + check fails it. Publishing a tarball made earlier with `npm pack` runs + no lifecycle script, so no check sees it. ## Building and testing diff --git a/scripts/release-holds.mjs b/scripts/release-holds.mjs index 7a4fd2530..892b17029 100644 --- a/scripts/release-holds.mjs +++ b/scripts/release-holds.mjs @@ -1,6 +1,7 @@ // Runs as npm's `preversion` lifecycle hook (see package.json), before // `npm version` bumps anything; as `prepublishOnly`, before `npm publish`; -// and in both of the publish workflow's releasing jobs for the pushed tag. +// and in the publish workflow for a pushed tag: in its build job, and in +// each of its two releasing jobs. // Stops the release while the repository has an open issue labelled // `release-hold`: someone found that releasing `main` as it stands would harm // whoever runs it, and the issue says what has to happen first. @@ -8,7 +9,7 @@ // The repository is the GitHub repository package.json names, as a URL or in // npm's shorthand, so a fork or a copy asks the package's home; // GITHUB_REPOSITORY (as in GitHub Actions) is used only when package.json -// names none. A token in GH_TOKEN or GITHUB_TOKEN is sent when present; the +// names none (no repository field, or a blank one). A token in GH_TOKEN or GITHUB_TOKEN is sent when present; the // issues of a public repository read without one. If the holds can't be read, // the release stops too, since an open hold can't be ruled out. import { readFileSync, writeSync } from "node:fs"; @@ -25,14 +26,16 @@ function fail(message) { } /** owner/repo from package.json's repository field: a github.com URL, or - * npm's shorthand (github:owner/repo, or owner/repo alone). */ + * npm's shorthand (github:owner/repo, or owner/repo alone). A URL can go on + * past the repository (a path, such as a monorepo's /tree/main/pkg, a query + * or a fragment), and the shorthand can name a ref after `#`. */ function githubRepository(field) { const value = typeof field === "string" ? field : field?.url; if (typeof value !== "string" || value.trim() === "") return {}; - const url = /github\.com[/:]([^/]+\/[^/]+?)(?:\.git)?\/?$/.exec(value); - if (url) return { repo: url[1] }; - const shorthand = /^(?:github:)?([\w.-]+\/[\w.-]+?)(?:\.git)?$/.exec(value); - if (shorthand) return { repo: shorthand[1] }; + const url = /github\.com[/:]([^/#?\s]+)\/([^/#?\s]+?)(?:\.git)?(?:[/#?].*)?$/.exec(value); + if (url) return { repo: `${url[1]}/${url[2]}` }; + const shorthand = /^(?:github:)?([\w.-]+)\/([\w.-]+?)(?:\.git)?(?:#.*)?$/.exec(value); + if (shorthand) return { repo: `${shorthand[1]}/${shorthand[2]}` }; return { unreadable: value }; } @@ -60,6 +63,16 @@ const { repo, namedBy } = repository(); const api = (process.env.GITHUB_API_URL ?? "https://api.github.com").replace(/\/+$/, ""); const token = process.env.GH_TOKEN || process.env.GITHUB_TOKEN; const where = `https://github.com/${repo}/issues?q=is%3Aissue+is%3Aopen+label%3A${LABEL}`; +// How to go on after a stop, for where it ran: npm version's hook, wherever +// it runs, comes before any tag; in the workflow, the tag is there to re-run. +const lifecycle = process.env.npm_lifecycle_event; +const rerun = lifecycle === "preversion" + ? "run npm version again" + : process.env.GITHUB_ACTIONS === "true" + ? "re-run this workflow for the tag" + : lifecycle === "prepublishOnly" + ? "run npm publish again" + : "run this again"; /** What a maintainer can do about an answer other than the list. */ function refusal(status) { @@ -76,7 +89,10 @@ function refusal(status) { ? `no repository ${repo} is visible to the token in GH_TOKEN or GITHUB_TOKEN; check ${namedBy} and the token's access` : `no repository ${repo} is visible without a token; check ${namedBy}, or set GH_TOKEN if the repository is private`; } - if (status >= 500) return "GitHub had an error; run this again once it answers normally"; + if (status === 410) { + return `issues are turned off for ${repo}, so no hold can be filed or read there; turn them on, or point ${namedBy} at the repository that tracks them`; + } + if (status >= 500) return `GitHub had an error; ${rerun} once it answers normally`; return "this check doesn't know that answer; see what GitHub shows"; } @@ -98,7 +114,7 @@ async function page(number) { const why = error.name === "TimeoutError" ? "no answer within 30 s" : error.cause?.code ?? error.cause?.message ?? error.message; - fail(`release-holds: couldn't reach GitHub to read the release holds (${why}). Check ${where}, and run this again once GitHub can be reached.`); + fail(`release-holds: couldn't reach GitHub to read the release holds (${why}). Check ${where}, and ${rerun} once GitHub can be reached.`); } if (!response.ok) { fail(`release-holds: GitHub answered ${`${response.status} ${response.statusText}`.trim()} when asked for the release holds: ${refusal(response.status)}. ${where}`); @@ -107,39 +123,34 @@ async function page(number) { try { items = await response.json(); } catch (error) { - fail(`release-holds: couldn't read GitHub's answer (${error.name === "TimeoutError" ? "no answer within 30 s" : error.message}). Check ${where}, and run this again.`); + fail(`release-holds: couldn't read GitHub's answer (${error.name === "TimeoutError" ? "no answer within 30 s" : error.message}). Check ${where}, and ${rerun}.`); } if (!Array.isArray(items)) { - fail(`release-holds: GitHub's answer wasn't a list of issues. Check ${where}, and run this again.`); + fail(`release-holds: GitHub's answer wasn't a list of issues. Check ${where}, and ${rerun}.`); } return items; } // The issues endpoint lists pull requests too. A hold is an issue: a pull // request isn't on main until it merges. +// Only a page after the last it reads that holds something is past the +// limit: a full last page leads to one more read, which may be empty. const holds = []; for (let number = 1; ; number++) { - if (number > MAX_PAGES) { + const items = await page(number); + if (number > MAX_PAGES && items.length > 0) { fail(`release-holds: more than ${MAX_PAGES * PAGE} open items are labelled ${LABEL} in ${repo}, more than this check reads. ${where}`); } - const items = await page(number); holds.push(...items.filter((item) => !item.pull_request)); if (items.length < PAGE) break; } if (holds.length > 0) { - const again = process.env.GITHUB_ACTIONS === "true" - ? "then re-run this workflow for the tag" - : process.env.npm_lifecycle_event === "preversion" - ? "then run npm version again" - : process.env.npm_lifecycle_event === "prepublishOnly" - ? "then run npm publish again" - : "then run this again"; fail( [ - `Release held: ${holds.length === 1 ? "an open issue is" : `${holds.length} open issues are`} labelled ${LABEL} in ${repo}.`, + `Release held: ${holds.length === 1 ? "an open issue is" : `${holds.length} open issues are`} labelled ${LABEL} in ${repo} (named by ${namedBy}).`, ...holds.map((hold) => ` #${hold.number} ${hold.title}`), - `Resolve each one, or close it if it no longer applies, ${again}. ${where}`, + `Resolve each one, or close it if it no longer applies, then ${rerun}. ${where}`, ].join("\n"), ); } diff --git a/test/release-holds.test.ts b/test/release-holds.test.ts index 04bdf428c..9ecbb5356 100644 --- a/test/release-holds.test.ts +++ b/test/release-holds.test.ts @@ -24,17 +24,20 @@ interface Seen { } /** Answers every request with `status` and `body`, or with what `body` - * gives for the request's URL, recording each request. */ + * gives for the request's URL, recording each request. A `raw` body is + * sent as it is, not as JSON. */ async function github( status: number, body: unknown | ((url: string) => unknown), + raw = false, ): Promise<{ url: string; seen: Seen[]; close: () => Promise }> { const seen: Seen[] = []; const server = createServer((req, res) => { const url = req.url ?? ""; seen.push({ url, headers: req.headers }); res.writeHead(status, { "content-type": "application/json" }); - res.end(JSON.stringify(typeof body === "function" ? (body as (url: string) => unknown)(url) : body)); + const answer = typeof body === "function" ? (body as (url: string) => unknown)(url) : body; + res.end(raw ? String(answer) : JSON.stringify(answer)); }); await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); const { port } = server.address() as AddressInfo; @@ -105,7 +108,7 @@ test("an open hold stops the release and names it; a labelled pull request isn't try { const result = await run(project(PKG), { GITHUB_API_URL: gh.url }); assert.equal(result.code, 1); - assert.match(result.stderr, /^Release held: an open issue is labelled release-hold in owner\/repo\./); + assert.match(result.stderr, /^Release held: an open issue is labelled release-hold in owner\/repo \(named by package\.json's repository field\)\./); assert.match(result.stderr, /\n #300 DefaultErrorPolicy sleeps the provider's retryAfterMs uncapped\n/); assert.doesNotMatch(result.stderr, /#301/); assert.match(result.stderr, /then run this again\. https:\/\/github\.com\/owner\/repo\/issues\?q=/); @@ -169,6 +172,17 @@ test("package.json names the repository, though GITHUB_REPOSITORY names another, } }); +test("GH_TOKEN is sent when both tokens are set", async () => { + const gh = await github(200, []); + try { + const result = await run(project(PKG), { GITHUB_API_URL: gh.url, GH_TOKEN: "f1rst", GITHUB_TOKEN: "act1ons" }); + assert.equal(result.code, 0, result.stderr); + assert.equal(gh.seen[0]!.headers.authorization, "Bearer f1rst"); + } finally { + await gh.close(); + } +}); + test("GITHUB_TOKEN alone is sent too", async () => { const gh = await github(200, []); try { @@ -183,10 +197,15 @@ test("GITHUB_TOKEN alone is sent too", async () => { test("each form npm reads in the repository field names the same repository", async () => { const forms: unknown[] = [ "github:owner/repo", + "github:owner/repo#v1", "owner/repo", "https://github.com/owner/repo", + "https://github.com/owner/repo#readme", + "https://github.com/owner/repo?tab=readme", + "https://github.com/owner/repo/tree/main/packages/fixture", "git@github.com:owner/repo.git", { type: "git", url: "git+ssh://git@github.com/owner/repo.git" }, + { type: "git", url: "git+https://github.com/owner/repo.git#main" }, ]; const gh = await github(200, []); try { @@ -212,15 +231,18 @@ test("a repository field that names no GitHub repository stops the release, what assert.match(result.stderr, /package\.json's repository field \(gitlab:owner\/repo\) names no GitHub repository this check can ask/); }); -test("GITHUB_REPOSITORY names the repository when package.json names none", async () => { +test("GITHUB_REPOSITORY names the repository when package.json names none, or can't be read", async () => { const gh = await github(200, []); try { - const result = await run(project({ name: "fixture" }), { - GITHUB_API_URL: gh.url, - GITHUB_REPOSITORY: "other/place", - }); - assert.equal(result.code, 0, result.stderr); - assert.match(gh.seen[0]!.url, /^\/repos\/other\/place\/issues\?/); + // No field, a blank one, and a package.json that isn't JSON. + for (const pkg of [{ name: "fixture" }, { name: "fixture", repository: "" }, { name: "fixture", repository: { url: " " } }, "{ not json"]) { + const result = await run(project(pkg), { + GITHUB_API_URL: gh.url, + GITHUB_REPOSITORY: "other/place", + }); + assert.equal(result.code, 0, `${JSON.stringify(pkg)}: ${result.stderr}`); + assert.match(gh.seen.at(-1)!.url, /^\/repos\/other\/place\/issues\?/, JSON.stringify(pkg)); + } } finally { await gh.close(); } @@ -267,6 +289,9 @@ test("each refusal says what to do, with and without a token", async () => { { status: 403, env: { GH_TOKEN: "narrow" }, says: /answered 403 Forbidden .*the token in GH_TOKEN or GITHUB_TOKEN can't read this repository's issues/ }, { status: 404, env: {}, says: /answered 404 Not Found .*no repository owner\/repo is visible without a token; check package\.json's repository field, or set GH_TOKEN/ }, { status: 404, env: { GH_TOKEN: "narrow" }, says: /answered 404 Not Found .*no repository owner\/repo is visible to the token in GH_TOKEN or GITHUB_TOKEN/ }, + { status: 410, env: {}, says: /answered 410 Gone .*issues are turned off for owner\/repo, so no hold can be filed or read there; turn them on, or point package\.json's repository field at the repository that tracks them/ }, + { status: 429, env: {}, says: /answered 429 Too Many Requests .*rate limit for requests without a token; set GH_TOKEN/ }, + { status: 429, env: { GH_TOKEN: "busy" }, says: /answered 429 Too Many Requests .*or its rate limit is spent/ }, { status: 418, env: {}, says: /answered 418 I'm a Teapot .*this check doesn't know that answer/ }, ]; for (const { status, env, says } of cases) { @@ -281,6 +306,39 @@ test("each refusal says what to do, with and without a token", async () => { } }); +test("an answer that isn't JSON stops the release, saying how to go on", async () => { + const gh = await github(200, "not json", true); + try { + const result = await run(project(PKG), { GITHUB_API_URL: gh.url, npm_lifecycle_event: "preversion" }); + assert.equal(result.code, 1); + assert.match(result.stderr, /^release-holds: couldn't read GitHub's answer \(.+\)\. Check https:\/\/github\.com\/owner\/repo\/issues\?q=\S+, and run npm version again\./); + } finally { + await gh.close(); + } +}); + +test("exactly the most it reads, five thousand labelled items, is read whole; one more stops the release", async () => { + const full = (from: number) => Array.from({ length: 100 }, (_, i) => ({ number: from + i, title: "pr", pull_request: {} })); + for (const [last, stops] of [[[], false], [[{ number: 9999, title: "pr", pull_request: {} }], true]] as const) { + const gh = await github(200, (url: string) => { + const n = Number(url.replace(/.*&page=/, "")); + return n <= 50 ? full(n * 100) : last; + }); + try { + const result = await run(project(PKG), { GITHUB_API_URL: gh.url }); + assert.equal(gh.seen.length, 51); + if (stops) { + assert.equal(result.code, 1); + assert.match(result.stderr, /^release-holds: more than 5000 open items are labelled release-hold in owner\/repo, more than this check reads\./); + } else { + assert.equal(result.code, 0, result.stderr); + } + } finally { + await gh.close(); + } + } +}); + test("an answer that isn't a list stops the release", async () => { const gh = await github(200, { message: "not a list" }); try { @@ -299,7 +357,7 @@ test("GitHub out of reach stops the release, naming why", async () => { await gh.close(); const result = await run(project(PKG), { GITHUB_API_URL: gh.url }); assert.equal(result.code, 1); - assert.match(result.stderr, /^release-holds: couldn't reach GitHub to read the release holds \(E[A-Z]+\)\./); + assert.match(result.stderr, /^release-holds: couldn't reach GitHub to read the release holds \(E[A-Z]+\)\. Check \S+, and run this again once GitHub can be reached\./); }); test("an API URL with a trailing slash asks the same path", async () => { @@ -318,6 +376,9 @@ test("the stop message says how to go on where it ran", async () => { try { const workflow = await run(project(PKG), { GITHUB_API_URL: gh.url, GITHUB_ACTIONS: "true" }); assert.match(workflow.stderr, /then re-run this workflow for the tag\./); + // npm version comes before any tag, wherever it runs. + const versionInActions = await run(project(PKG), { GITHUB_API_URL: gh.url, GITHUB_ACTIONS: "true", npm_lifecycle_event: "preversion" }); + assert.match(versionInActions.stderr, /then run npm version again\./); const version = await run(project(PKG), { GITHUB_API_URL: gh.url, npm_lifecycle_event: "preversion" }); assert.match(version.stderr, /then run npm version again\./); const publish = await run(project(PKG), { GITHUB_API_URL: gh.url, npm_lifecycle_event: "prepublishOnly" }); @@ -334,7 +395,8 @@ test("no repository to ask stops the release", async () => { }); /** A throwaway package whose preversion and prepublishOnly are this - * repository's own, with the script beside it and a stand-in build. */ + * repository's own, with the script beside it and a stand-in for the build + * that prepublishOnly runs after the check. */ function wiredProject(): string { const scripts = (JSON.parse(readFileSync(PACKAGE_JSON, "utf8")) as { scripts: Record }).scripts; const dir = project({ @@ -350,7 +412,14 @@ function wiredProject(): string { /** npm, kept to the throwaway directory: its own cache and user config, * and the stand-in for its registry and GitHub's API. */ function npm(dir: string, args: string[], api: string) { - return exec("npm", args, dir, { + // Under `npm test`, npm names its own CLI script in npm_execpath, and this + // Node runs it: a bare `npm` is npm.cmd on Windows, which execFile can't + // start without a shell. Another package manager names its own CLI there + // (bun its binary), so anything but npm's script falls back to the `npm` + // on PATH. + const cli = process.env.npm_execpath; + const [file, argv] = cli && /npm-cli\.c?js$/.test(cli) ? [process.execPath, [cli, ...args]] : ["npm", args]; + return exec(file, argv, dir, { HOME: dir, npm_config_cache: join(dir, ".npm-cache"), npm_config_userconfig: join(dir, ".npmrc"), @@ -401,3 +470,15 @@ test("this repository's prepublishOnly stops npm publish while a hold is open", await gh.close(); } }); + +test("with no hold open, this repository's prepublishOnly lets npm publish go ahead", async () => { + const gh = await github(200, []); + try { + const dir = wiredProject(); + const result = await npm(dir, ["publish", "--dry-run"], gh.url); + assert.equal(result.code, 0, result.stderr); + assert.ok(gh.seen.some((s) => s.url.startsWith("/repos/owner/repo/issues?"))); + } finally { + await gh.close(); + } +});