From 38f522880fe718e539716710644b6b17207cb045 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 14 Sep 2026 18:20:40 -0700 Subject: [PATCH 1/8] feat(ci): add the shared pieces every vendor bump needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small modules, each doing one thing the two vendored toolchains (Vale and ast-grep) both need, so the behaviour is written and tested once rather than described twice in shell. release-notes.cjs fetches a GitHub release and renders it as a quoted Markdown section. The quoting is load-bearing: an upstream body with an unterminated code fence would otherwise swallow whatever the workflow appends after it. The truncation limit is measured against the RENDERED output, not the source — every line gains two characters of blockquote marker, and budgeting against the source let a 40000-character body render as 80202, over the 65536 cap that fails the API call outright rather than truncating. pin-bump.cjs moves a family of exact pins in the package.json SOURCE TEXT. Parsing and re-serializing would reformat a prettier-formatted file in CI, where no lint-staged runs to normalise it back, burying the version strings in a whole-file diff. It returns the count it rewrote so a caller can refuse a half-applied bump: these platform packages are selected by optional dependency, so a straggler left at the old version is a different binary on one platform than on the others. vendor-pr.cjs owns the branch and pull request lifecycle — one rolling branch per engine, rebuilt from main and retitled as upstream moves. Rebuilding a branch someone may be reviewing is the hazard, so it force-pushes only when the proposed paths actually changed, and only when the branch tip was written by this workflow. Both guards are mutation-tested. --- .github/scripts/pin-bump.cjs | 52 +++++ .github/scripts/release-notes.cjs | 161 +++++++++++++++ .github/scripts/release-notes.test.cjs | 175 +++++++++++++++++ .github/scripts/vendor-pr.cjs | 260 +++++++++++++++++++++++++ .github/scripts/vendor-pr.test.cjs | 257 ++++++++++++++++++++++++ 5 files changed, 905 insertions(+) create mode 100644 .github/scripts/pin-bump.cjs create mode 100644 .github/scripts/release-notes.cjs create mode 100644 .github/scripts/release-notes.test.cjs create mode 100644 .github/scripts/vendor-pr.cjs create mode 100644 .github/scripts/vendor-pr.test.cjs diff --git a/.github/scripts/pin-bump.cjs b/.github/scripts/pin-bump.cjs new file mode 100644 index 00000000..7abde8fc --- /dev/null +++ b/.github/scripts/pin-bump.cjs @@ -0,0 +1,52 @@ +#!/usr/bin/env node +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Move a family of exact dependency pins from one version to another, in the + * package.json SOURCE TEXT rather than in a parsed object. + * + * Shared by the two upgrade detectors — ast-grep's and Vale's — because the + * rewrite is the same operation on a different prefix, and because the count + * check below is the kind of guard that is worth having exactly one of. + * + * WHY TEXT AND NOT JSON. Parsing and re-serializing would reformat a file this + * repository formats with prettier, and it would do so in CI, where no + * `lint-staged` runs to normalize it back. The bump would then arrive as a + * whole-file diff with six version strings buried in it. A targeted replacement + * leaves every byte it did not have to touch, so the review is the versions. + * + * WHY THE COUNT IS RETURNED RATHER THAN ASSUMED. A pin the pattern fails to + * match is the failure that matters: it leaves a straggler at the old version, + * and because these platform packages are selected by optional dependency, a + * straggler is a DIFFERENT BINARY on one platform than on the others. Callers + * compare this count against the pins they independently enumerated and abort + * on a mismatch, so the two disagreeing is a failed run rather than a + * half-applied upgrade that looks fine in review. + */ + +/** Escape a version for literal use in a regular expression. */ +function escapeLiteral(text) { + return String(text).replaceAll(/[.*+?^${}()|[\]\\]/g, String.raw`\$&`); +} + +/** + * @param source the package.json text + * @param prefix package-name prefix, e.g. `@taskless/vale-` or `@ast-grep/cli` + * @param from the exact version every matching pin currently holds + * @param to the exact version to write + */ +function bumpPins(source, { prefix, from, to }) { + const pattern = new RegExp( + `("${escapeLiteral(prefix)}[^"]*"\\s*:\\s*")${escapeLiteral(from)}(")`, + "g" + ); + let count = 0; + const bumped = source.replaceAll(pattern, (_match, head, tail) => { + count += 1; + return `${head}${to}${tail}`; + }); + return { source: bumped, count }; +} + +module.exports = { bumpPins, escapeLiteral }; diff --git a/.github/scripts/release-notes.cjs b/.github/scripts/release-notes.cjs new file mode 100644 index 00000000..50034f83 --- /dev/null +++ b/.github/scripts/release-notes.cjs @@ -0,0 +1,161 @@ +#!/usr/bin/env node +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Upstream release notes, fetched and rendered for a pull request or issue body. + * + * WHY THIS EXISTS. Both detect scripts answer "is upstream ahead?" with a + * version number, and a version number is not enough to review a bump. A + * reviewer looking at `3.20.0 -> 3.21.0` and six changed digests has no way to + * tell a security fix from a docs release without leaving the pull request and + * going to find the changelog by hand. Carrying the notes into the body is the + * difference between reviewing a bump and approving one. + * + * THE NOTES ARE UNTRUSTED TEXT. They are Markdown written by a third party and + * they reach a body that a workflow composes. Nothing here interpolates them + * into a shell command, a `${{ }}` expression, or a $GITHUB_OUTPUT line — a + * release body is free to contain a heredoc terminator or an output delimiter, + * and either one is an injection if it meets a shell. `writeNotesFile` puts + * them on disk and the workflow passes that path to `--body-file`, so the bytes + * never pass through an interpreter. + * + * They are also FENCED when rendered. An upstream body that opens a fence and + * never closes it, or that ends mid-table, would otherwise swallow whatever the + * workflow appends after it. Quoting the whole block as Markdown blockquote + * lines keeps the surrounding body's structure independent of what upstream + * wrote, at the cost of one level of indentation in the rendering. + */ + +const { writeFileSync } = require("node:fs"); + +/** + * How much of an upstream body to carry, measured AFTER quoting. + * + * A GitHub pull request or issue body caps at 65536 characters, and a body that + * hits the cap fails the API call rather than truncating — so the whole + * proposal is lost to a talkative release. The limit is well under the cap + * because the workflow's own preamble and the truncation footer have to fit + * alongside it. + * + * Measured after quoting because the two differ by more than a rounding error: + * every line gains two characters of blockquote marker, so a release whose + * notes are a long list of short lines — which is exactly what ast-grep's + * generated changelog is — nearly doubles. Budgeting against the source length + * let a 40000-character body render as 80202, over the cap the limit exists to + * stay under. + */ +const NOTES_LIMIT = 40_000; + +/** + * Fetch a release from the GitHub API. + * + * `reference` is either `latest` or `tags/`, matching the two endpoint + * shapes. GITHUB_TOKEN, when present, is only for the rate limit; both + * endpoints are public. + * + * A missing release is NOT an error. Upstream may tag without publishing a + * release, or name its tags differently from its npm versions, and neither is a + * reason to fail a detect run whose actual job — comparing versions — already + * succeeded. The caller gets `undefined` and says so in the body. + */ +async function fetchRelease(repository, reference) { + const headers = { + accept: "application/vnd.github+json", + "user-agent": "taskless-release-notes", + }; + if (process.env.GITHUB_TOKEN) { + headers.authorization = `Bearer ${process.env.GITHUB_TOKEN}`; + } + const url = `https://api.github.com/repos/${repository}/releases/${reference}`; + const response = await fetch(url, { headers }); + if (response.status === 404) { + return undefined; + } + if (!response.ok) { + throw new Error(`GET ${url} responded ${response.status}`); + } + const release = await response.json(); + if (typeof release.tag_name !== "string") { + throw new TypeError(`${url} returned no tag_name`); + } + return { + tag: release.tag_name, + notes: typeof release.body === "string" ? release.body : "", + url: typeof release.html_url === "string" ? release.html_url : undefined, + }; +} + +/** The latest non-prerelease, non-draft release. */ +const fetchLatestRelease = (repository) => fetchRelease(repository, "latest"); + +/** A specific release by tag, for upstreams whose version we learn elsewhere. */ +const fetchReleaseByTag = (repository, tag) => + fetchRelease(repository, `tags/${encodeURIComponent(tag)}`); + +/** + * Render a release as a Markdown section, quoted so it cannot restructure the + * body around it. + * + * `release` may be undefined (no release found) and its `notes` may be empty (a + * release published with no body). Both render as a line saying so plus a link, + * rather than as an absent section — "upstream wrote no notes" and "we forgot + * to fetch them" look identical otherwise, and only one of them is fine. + */ +function formatReleaseNotes({ + repository, + version, + release, + limit = NOTES_LIMIT, +}) { + const link = release?.url ?? `https://github.com/${repository}/releases`; + const heading = `## Upstream release notes — ${version}\n\n${link}\n`; + + const body = release?.notes?.trim() ?? ""; + if (body.length === 0) { + return `${heading}\nUpstream published no release notes for this version.\n`; + } + + // Truncation happens on a line boundary, so the rendering cannot end + // mid-marker and produce a line that is not part of the quoted block. + const lines = body + .split("\n") + .map((line) => (line.length > 0 ? `> ${line}` : ">")); + const kept = []; + let length = 0; + for (const line of lines) { + if (length + line.length + 1 > limit) { + break; + } + kept.push(line); + length += line.length + 1; + } + let truncated = kept.length < lines.length; + // A first line longer than the whole budget keeps nothing, which would render + // a truncation footer under an empty quote. Cut inside that line instead: a + // release whose notes are one enormous paragraph still says something. This + // is also truncation, even when it drops no whole line. + if (kept.length === 0) { + kept.push(lines[0].slice(0, limit)); + truncated = true; + } + const quoted = kept.join("\n"); + + const footer = truncated + ? `\n\n_Truncated at ${limit} characters. Read the rest at ${link}._\n` + : "\n"; + return `${heading}\n${quoted}\n${footer}`; +} + +/** Write a rendered section to disk for a workflow to pass to `--body-file`. */ +function writeNotesFile(path, section) { + writeFileSync(path, section.endsWith("\n") ? section : `${section}\n`); +} + +module.exports = { + NOTES_LIMIT, + fetchLatestRelease, + fetchReleaseByTag, + formatReleaseNotes, + writeNotesFile, +}; diff --git a/.github/scripts/release-notes.test.cjs b/.github/scripts/release-notes.test.cjs new file mode 100644 index 00000000..3b7b5d66 --- /dev/null +++ b/.github/scripts/release-notes.test.cjs @@ -0,0 +1,175 @@ +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Tests for release-notes.cjs. + * + * The rendering is what is worth pinning down. These notes are third-party + * Markdown pasted into a body that a workflow composes, so the two failures + * that matter are structural: upstream restructuring the body around it, and + * upstream being long enough to push the body past GitHub's 65536-character + * cap, which fails the API call outright rather than truncating. + */ + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { mkdtempSync, readFileSync, rmSync } = require("node:fs"); +const { tmpdir } = require("node:os"); +const { join } = require("node:path"); + +const { + NOTES_LIMIT, + fetchLatestRelease, + formatReleaseNotes, + writeNotesFile, +} = require("./release-notes.cjs"); + +const release = (notes) => ({ + tag: "v3.21.0", + notes, + url: "https://github.com/errata-ai/vale/releases/tag/v3.21.0", +}); + +test("notes: the section names the version and links the release", () => { + const section = formatReleaseNotes({ + repository: "errata-ai/vale", + version: "3.21.0", + release: release("Some change."), + }); + assert.match(section, /^## Upstream release notes — 3\.21\.0$/m); + assert.match( + section, + /^https:\/\/github\.com\/errata-ai\/vale\/releases\/tag\/v3\.21\.0$/m + ); + assert.match(section, /^> Some change\.$/m); +}); + +/** + * The reason every line is quoted. An upstream body that opens a fence and + * never closes it would otherwise swallow whatever the workflow appends after + * it — in the Vale case, nothing, but in the ast-grep case the body is composed + * the other way round and a runaway fence eats the preamble. + */ +test("notes: an unterminated fence cannot escape the quoted block", () => { + const section = formatReleaseNotes({ + repository: "errata-ai/vale", + version: "3.21.0", + release: release("```yaml\nscope: sentence"), + }); + for (const line of section.split("\n").slice(4)) { + if (line.length > 0) { + assert.match(line, /^>/, `unquoted line escaped the block: ${line}`); + } + } +}); + +test("notes: blank lines stay blank rather than becoming trailing spaces", () => { + const section = formatReleaseNotes({ + repository: "errata-ai/vale", + version: "3.21.0", + release: release("First.\n\nSecond."), + }); + assert.match(section, /^>$/m); + assert.doesNotMatch(section, /> $/m); +}); + +test("notes: a long body is truncated with a pointer to the rest", () => { + const section = formatReleaseNotes({ + repository: "errata-ai/vale", + version: "3.21.0", + release: release(`${"line\n".repeat(200)}`), + limit: 100, + }); + assert.match(section, /Truncated at 100 characters/); + assert.match(section, /Read the rest at https:\/\/github\.com/); + assert.ok( + section.length < 400, + `expected the kept text to be bounded, got ${section.length} characters` + ); +}); + +test("notes: a single line over budget is cut rather than dropped", () => { + const section = formatReleaseNotes({ + repository: "errata-ai/vale", + version: "3.21.0", + release: release("x".repeat(500)), + limit: 100, + }); + assert.match(section, /^> x{98}$/m); + assert.match(section, /Truncated at 100 characters/); +}); + +test("notes: the default limit leaves room under GitHub's body cap", () => { + const section = formatReleaseNotes({ + repository: "errata-ai/vale", + version: "3.21.0", + // Worst case for the quoting: every line is one character, so every line + // also carries two characters of blockquote marker. + release: release(`${"x\n".repeat(NOTES_LIMIT)}`), + }); + assert.ok( + section.length < 65_536, + `a body of ${section.length} characters would be rejected by the API` + ); +}); + +/** + * Both cases render a section rather than nothing. "Upstream wrote no notes" + * and "we failed to fetch them" look identical when the section is simply + * absent, and only one of those is fine. + */ +test("notes: a release with an empty body says so", () => { + const section = formatReleaseNotes({ + repository: "errata-ai/vale", + version: "3.21.0", + release: release(" \n "), + }); + assert.match(section, /Upstream published no release notes/); +}); + +test("notes: no release at all falls back to the releases page", () => { + const section = formatReleaseNotes({ + repository: "ast-grep/ast-grep", + version: "0.45.3", + release: undefined, + }); + assert.match(section, /Upstream published no release notes/); + assert.match( + section, + /^https:\/\/github\.com\/ast-grep\/ast-grep\/releases$/m + ); +}); + +test("notes: a 404 is not an error, because the version comparison stands", async () => { + const previous = globalThis.fetch; + globalThis.fetch = async () => new Response("", { status: 404 }); + try { + assert.equal(await fetchLatestRelease("ast-grep/ast-grep"), undefined); + } finally { + globalThis.fetch = previous; + } +}); + +test("notes: any other API failure is an error", async () => { + const previous = globalThis.fetch; + globalThis.fetch = async () => new Response("", { status: 500 }); + try { + await assert.rejects( + fetchLatestRelease("ast-grep/ast-grep"), + /responded 500/ + ); + } finally { + globalThis.fetch = previous; + } +}); + +test("notes: the written file always ends in a newline", () => { + const directory = mkdtempSync(join(tmpdir(), "release-notes-test-")); + try { + const path = join(directory, "notes.md"); + writeNotesFile(path, "no trailing newline"); + assert.equal(readFileSync(path, "utf8"), "no trailing newline\n"); + } finally { + rmSync(directory, { recursive: true, force: true }); + } +}); diff --git a/.github/scripts/vendor-pr.cjs b/.github/scripts/vendor-pr.cjs new file mode 100644 index 00000000..27b978cb --- /dev/null +++ b/.github/scripts/vendor-pr.cjs @@ -0,0 +1,260 @@ +#!/usr/bin/env node +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Maintain the rolling pull request for a vendored toolchain bump. + * + * Both detect workflows — Vale's and ast-grep's — arrive here with the same + * shape: a working tree that already carries the proposed bump, a title, and a + * body. What happens next is identical for both, so it lives in one place + * rather than as two copies of the same shell that drift. + * + * ONE STATIC BRANCH PER ENGINE, `vendor/`. The alternative, a branch + * per upstream version, means a second release while the first is unreviewed + * opens a SECOND pull request proposing a conflicting edit to the same lines, + * and someone has to notice and close the stale one. A rolling branch instead + * carries whatever upstream's current answer is: the proposal is rewritten, the + * title and body are rewritten with it, and there is exactly one thing to + * review. It also means a reviewer who comes back after a week is not looking + * at a bump that upstream has already superseded. + * + * THE BRANCH IS REBUILT FROM `main`, NOT APPENDED TO (`checkout -B`). The + * proposal is "main plus this bump", and it has to stay that as `main` moves. + * Appending would accumulate one commit per upstream release and slowly turn + * the diff into a history of versions nobody pinned. + * + * WHICH IS WHY FORCE-PUSHING NEEDS TWO GUARDS, because rebuilding a branch + * somebody may be reviewing is the obvious way this hurts someone: + * + * - Nothing is pushed when the rebuilt branch matches what is already on the + * remote. A scheduled run that finds the same upstream version must be + * inert, or every run rewrites the branch, and every rewrite dismisses + * approvals and re-triggers CI on an unchanged proposal. + * + * - Nothing is pushed when the remote branch carries a commit this workflow + * did not write. A reviewer who pushes a fixup onto the branch has done the + * most reasonable thing available to them, and a force-push would silently + * delete it. The run warns and stops instead, which costs a stale proposal + * until someone looks — strictly better than costing someone's work. + * + * Usage: + * node .github/scripts/vendor-pr.cjs \ + * --branch vendor/vale \ + * --title "chore(vale): pin Vale 3.21.0" \ + * --body-file /path/to/body.md \ + * --message "chore(vale): pin Vale 3.21.0" \ + * [--label skip-changeset] \ + * -- packages/cli/package.json pnpm-lock.yaml + * + * Everything after `--` is the set of paths to stage. They are passed to git as + * an argv array, never through a shell, so a path is a path even if it contains + * something a shell would find interesting. + */ + +const { execFileSync } = require("node:child_process"); + +/** Commits with this author are ours to overwrite. Anything else is not. */ +const BOT_AUTHOR = "github-actions[bot]"; + +const BOT_EMAIL = "41898282+github-actions[bot]@users.noreply.github.com"; + +function parseArgs(argv) { + const separator = argv.indexOf("--"); + const flags = separator === -1 ? argv : argv.slice(0, separator); + const paths = separator === -1 ? [] : argv.slice(separator + 1); + + const read = (name) => { + const at = flags.indexOf(name); + if (at === -1) { + return undefined; + } + const value = flags[at + 1]; + if (!value || value.startsWith("--")) { + throw new Error(`${name} needs a value`); + } + return value; + }; + + const options = { + branch: read("--branch"), + title: read("--title"), + bodyFile: read("--body-file"), + message: read("--message"), + label: read("--label"), + paths, + }; + + for (const required of ["branch", "title", "bodyFile", "message"]) { + if (!options[required]) { + throw new Error( + `--${required.replace(/[A-Z]/g, "-$&").toLowerCase()} is required` + ); + } + } + if (paths.length === 0) { + throw new Error("no paths to stage were given after `--`"); + } + return options; +} + +/** + * Whether anyone but this workflow wrote what is on the branch. + * + * Takes author lines rather than running git itself, because this is the + * decision worth testing and it is a decision about a list of strings. An empty + * list — no branch yet — is not foreign. + */ +function hasForeignCommits(authorLines) { + return authorLines + .map((line) => line.trim()) + .filter((line) => line.length > 0) + .some((author) => author !== BOT_AUTHOR); +} + +const runner = + (command) => + (args, { allowFailure = false } = {}) => { + try { + return execFileSync(command, args, { encoding: "utf8" }).trim(); + } catch (error) { + if (allowFailure) { + return undefined; + } + const detail = error.stderr?.toString().trim() ?? error.message; + throw new Error(`${command} ${args.join(" ")} failed: ${detail}`); + } + }; + +async function main({ + argv = process.argv.slice(2), + git = runner("git"), + gh = runner("gh"), + log = (line) => console.log(line), +} = {}) { + const { branch, title, bodyFile, message, label, paths } = parseArgs(argv); + + git(["config", "user.name", BOT_AUTHOR]); + git(["config", "user.email", BOT_EMAIL]); + + // -B rather than -b: the branch is rebuilt from wherever HEAD is (the freshly + // checked-out `main`), so the proposal is always "main plus this bump". + git(["checkout", "-B", branch]); + git(["add", "--", ...paths]); + + // `diff --cached --quiet` exits non-zero when something IS staged, so a + // successful run here means the working tree carried no bump after all. + const nothingStaged = + git(["diff", "--cached", "--quiet"], { allowFailure: true }) !== undefined; + if (nothingStaged) { + log("Nothing staged; there is no bump to propose."); + return { action: "none", reason: "nothing-staged" }; + } + git(["commit", "-m", message]); + + const remoteExists = + git(["ls-remote", "--exit-code", "--heads", "origin", branch], { + allowFailure: true, + }) !== undefined; + + if (remoteExists) { + git(["fetch", "--quiet", "origin", branch]); + + // The TIP author, not every commit since `main`. Asking "which commits are + // on the branch and not on main" needs a merge base, and `actions/checkout` + // clones at depth 1, so there is none — the question would answer wrongly + // or not at all depending on clone depth, which is the worst property a + // safety guard can have. The tip is sufficient here because this script is + // the only thing that ever writes the branch, and it does so by force-push: + // there is no path by which a bot commit lands ON TOP of a human's. + const tipAuthor = git(["log", "-1", "--format=%an", "FETCH_HEAD"]) ?? ""; + if (hasForeignCommits([tipAuthor])) { + log( + `::warning::${branch} was last written by ${tipAuthor}, not this workflow; refusing to force-push over that.` + ); + return { action: "none", reason: "foreign-commits" }; + } + + // By content, and only the content this run proposes. + // + // Comparing SHAs would always say "changed", because the rebuild reparents + // onto whatever `main` is now. Comparing whole TREES is subtler and was + // wrong in the same direction: an unrelated commit on `main` makes the + // trees differ, so an upstream that had not moved still force-pushed the + // branch and rewrote the pull request — dismissing approvals and + // restarting CI over somebody else's commit to a different file. + // + // The question that matters is "does the branch already propose exactly + // this bump", so the diff is scoped to the paths being proposed. Falling + // behind `main` is a real thing that happens to this branch, but it is + // branch protection's business and one click to resolve, not a reason to + // rewrite a proposal nobody changed. + const identical = + git(["diff", "--quiet", "FETCH_HEAD", "HEAD", "--", ...paths], { + allowFailure: true, + }) !== undefined; + if (identical) { + log(`${branch} already proposes exactly this; leaving it alone.`); + return { action: "none", reason: "unchanged" }; + } + } + + git(["push", "--force", "origin", branch]); + + const existing = gh([ + "pr", + "list", + "--head", + branch, + "--state", + "open", + "--json", + "number", + "--jq", + ".[].number", + ]); + + if (existing) { + // `gh pr edit` goes through GraphQL, which this repository has had broken + // out from under it by the Projects (classic) deprecation. REST does not + // depend on it. `-F body=@file` reads the file rather than passing its + // contents as an argument, so third-party release notes never become argv. + gh([ + "api", + "-X", + "PATCH", + `repos/{owner}/{repo}/pulls/${existing}`, + "-f", + `title=${title}`, + "-F", + `body=@${bodyFile}`, + ]); + log(`Updated #${existing}: ${title}`); + return { action: "updated", pr: Number(existing) }; + } + + gh([ + "pr", + "create", + "--base", + "main", + "--head", + branch, + "--title", + title, + "--body-file", + bodyFile, + ...(label ? ["--label", label] : []), + ]); + log(`Opened a pull request on ${branch}: ${title}`); + return { action: "created" }; +} + +module.exports = { BOT_AUTHOR, hasForeignCommits, main, parseArgs }; + +if (require.main === module) { + main().catch((error) => { + console.error(`\nvendor-pr failed: ${error.message}`); + process.exitCode = 1; + }); +} diff --git a/.github/scripts/vendor-pr.test.cjs b/.github/scripts/vendor-pr.test.cjs new file mode 100644 index 00000000..55f71a79 --- /dev/null +++ b/.github/scripts/vendor-pr.test.cjs @@ -0,0 +1,257 @@ +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Tests for vendor-pr.cjs. + * + * The argument parsing and the force-push guard are pure and tested directly. + * Everything else is tested by driving main() with `git` and `gh` replaced by + * recorders, which is the only way to reach the ORDER of operations — and the + * order is where the damage lives. Pushing before checking who owns the branch, + * or creating a second pull request instead of updating the open one, are both + * states every individual step would report as healthy. + */ + +const test = require("node:test"); +const assert = require("node:assert/strict"); + +const { + BOT_AUTHOR, + hasForeignCommits, + main, + parseArgs, +} = require("./vendor-pr.cjs"); + +const ARGV = [ + "--branch", + "vendor/vale", + "--title", + "chore(vale): pin Vale 3.21.0", + "--body-file", + "/tmp/body.md", + "--message", + "chore(vale): pin Vale 3.21.0", + "--", + ".github/scripts/vale-manifest.json", +]; + +/** + * Drive main() with both commands recorded. + * + * `git` answers are keyed by the subcommand plus enough of its arguments to be + * unambiguous. `undefined` from the runner means the command failed, which is + * how the real one reports a non-zero exit under `allowFailure`. + */ +function harness({ gitAnswers = {}, ghAnswers = {} } = {}) { + const calls = []; + const answer = (table, args, fallback) => { + for (const [key, value] of Object.entries(table)) { + if (args.join(" ").startsWith(key)) { + return value; + } + } + return fallback; + }; + return { + calls, + git: (args) => { + calls.push(["git", ...args]); + return answer(gitAnswers, args, ""); + }, + gh: (args) => { + calls.push(["gh", ...args]); + return answer(ghAnswers, args, ""); + }, + log: () => {}, + }; +} + +const ran = (calls, command, ...prefix) => + calls.some( + ([name, ...args]) => + name === command && prefix.every((part, index) => args[index] === part) + ); + +test("args: paths come after `--` and flags before it", () => { + const options = parseArgs(ARGV); + assert.equal(options.branch, "vendor/vale"); + assert.deepEqual(options.paths, [".github/scripts/vale-manifest.json"]); + assert.equal(options.label, undefined); +}); + +test("args: a missing required flag aborts", () => { + assert.throws(() => parseArgs(ARGV.slice(2)), /--branch is required/); +}); + +test("args: no paths at all aborts rather than committing nothing", () => { + assert.throws( + () => parseArgs(ARGV.slice(0, ARGV.indexOf("--"))), + /no paths to stage/ + ); +}); + +test("guard: the workflow's own commits are not foreign", () => { + assert.equal(hasForeignCommits([BOT_AUTHOR]), false); + assert.equal(hasForeignCommits([]), false); + assert.equal(hasForeignCommits([""]), false); +}); + +test("guard: anyone else's commit is foreign", () => { + assert.equal(hasForeignCommits(["A Reviewer"]), true); +}); + +test("no remote branch yet: pushes and opens a pull request", async () => { + // `diff --cached --quiet` fails when something is staged, which is the + // healthy path; `ls-remote --exit-code` fails when the branch is absent. + const h = harness({ + gitAnswers: { "diff --cached": undefined, "ls-remote": undefined }, + }); + const result = await main({ argv: ARGV, ...h }); + + assert.deepEqual(result, { action: "created" }); + assert.ok(ran(h.calls, "git", "checkout", "-B", "vendor/vale")); + assert.ok(ran(h.calls, "git", "push", "--force", "origin", "vendor/vale")); + assert.ok(ran(h.calls, "gh", "pr", "create")); + assert.ok( + !ran(h.calls, "git", "fetch"), + "fetched a branch that does not exist" + ); +}); + +test("an open pull request is retitled and rewritten, not duplicated", async () => { + const h = harness({ + gitAnswers: { + "diff --cached": undefined, + "ls-remote": "abc123\trefs/heads/vendor/vale", + "log -1": BOT_AUTHOR, + "diff --quiet FETCH_HEAD": undefined, // content differs + }, + ghAnswers: { "pr list": "73" }, + }); + const result = await main({ argv: ARGV, ...h }); + + assert.deepEqual(result, { action: "updated", pr: 73 }); + assert.ok(!ran(h.calls, "gh", "pr", "create"), "opened a duplicate PR"); + // The title has to move with the body: a rolling branch whose PR still names + // the previous version is worse than no automation, because it reads as + // current. + const patch = h.calls.find( + ([name, ...args]) => name === "gh" && args[0] === "api" + ); + assert.ok(patch, "did not PATCH the open pull request"); + assert.ok(patch.includes("title=chore(vale): pin Vale 3.21.0")); + assert.ok(patch.includes("body=@/tmp/body.md")); +}); + +/** + * The guard that makes a static branch safe. A scheduled run that finds the + * same upstream version must be completely inert — otherwise every run rewrites + * the branch, and every rewrite dismisses approvals and restarts CI on a + * proposal that has not changed. + */ +test("an unchanged proposal is not re-pushed", async () => { + const h = harness({ + gitAnswers: { + "diff --cached": undefined, + "ls-remote": "abc123\trefs/heads/vendor/vale", + "log -1": BOT_AUTHOR, + "diff --quiet FETCH_HEAD": "", // identical content + }, + }); + const result = await main({ argv: ARGV, ...h }); + + assert.deepEqual(result, { action: "none", reason: "unchanged" }); + assert.ok(!ran(h.calls, "git", "push"), "force-pushed an unchanged branch"); + assert.ok(!ran(h.calls, "gh", "pr"), "touched the pull request anyway"); +}); + +/** + * The guard that protects a reviewer. Pushing a fixup onto the branch is the + * most reasonable thing a reviewer can do, and a force-push would delete it + * with nothing reporting the loss. + */ +test("a branch last written by someone else is left alone", async () => { + const h = harness({ + gitAnswers: { + "diff --cached": undefined, + "ls-remote": "abc123\trefs/heads/vendor/vale", + "log -1": "A Reviewer", + }, + }); + const result = await main({ argv: ARGV, ...h }); + + assert.deepEqual(result, { action: "none", reason: "foreign-commits" }); + assert.ok(!ran(h.calls, "git", "push"), "force-pushed over a reviewer"); +}); + +test("the ownership check happens before the push, not after", async () => { + const h = harness({ + gitAnswers: { + "diff --cached": undefined, + "ls-remote": "abc123\trefs/heads/vendor/vale", + "log -1": BOT_AUTHOR, + "diff --quiet FETCH_HEAD": undefined, + }, + ghAnswers: { "pr list": "" }, + }); + await main({ argv: ARGV, ...h }); + + const authorAt = h.calls.findIndex( + ([name, ...args]) => name === "git" && args[0] === "log" + ); + const pushAt = h.calls.findIndex( + ([name, ...args]) => name === "git" && args[0] === "push" + ); + assert.ok(authorAt !== -1 && pushAt !== -1); + assert.ok(authorAt < pushAt, "pushed before checking who owns the branch"); +}); + +test("a working tree with no bump in it proposes nothing", async () => { + const h = harness({ gitAnswers: { "diff --cached": "" } }); + const result = await main({ argv: ARGV, ...h }); + + assert.deepEqual(result, { action: "none", reason: "nothing-staged" }); + assert.ok(!ran(h.calls, "git", "commit"), "committed an empty change"); + assert.ok(!ran(h.calls, "git", "push")); +}); + +test("a label is passed on only when one was asked for", async () => { + const h = harness({ + gitAnswers: { "diff --cached": undefined, "ls-remote": undefined }, + }); + await main({ + argv: [...ARGV.slice(0, 8), "--label", "skip-changeset", ...ARGV.slice(8)], + ...h, + }); + + const create = h.calls.find( + ([name, ...args]) => + name === "gh" && args[0] === "pr" && args[1] === "create" + ); + assert.ok(create.includes("skip-changeset")); +}); + +/** + * The narrower half of the unchanged guard, and the one that was wrong first. + * Comparing whole trees made an unrelated commit on `main` look like a changed + * proposal, so a quiet upstream still force-pushed the branch and dismissed the + * pull request's approvals over somebody else's edit to a different file. + */ +test("the unchanged check looks only at the paths being proposed", async () => { + const h = harness({ + gitAnswers: { + "diff --cached": undefined, + "ls-remote": "abc123\trefs/heads/vendor/vale", + "log -1": BOT_AUTHOR, + "diff --quiet FETCH_HEAD": "", + }, + }); + await main({ argv: ARGV, ...h }); + + const compare = h.calls.find( + ([name, ...args]) => + name === "git" && args[0] === "diff" && args[2] === "FETCH_HEAD" + ); + assert.ok(compare.includes("--"), "compared whole trees, not the proposal"); + assert.ok(compare.includes(".github/scripts/vale-manifest.json")); +}); From 95727bd786430d4769a7de59a985bd736221babc Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 14 Sep 2026 18:21:00 -0700 Subject: [PATCH 2/8] feat(ci): give Vale's republish PR a changelog and a rolling branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detect phase proposed a version and six digests and left a reviewer to go find out, by hand, what the release contained. The release notes were already in the payload: vale-detect.cjs calls releases/latest for tag_name, and that same response carries body. It was being discarded. Now --notes-out writes it to a file the workflow appends to the PR body. A file, not a step output: release notes are third-party Markdown, and a $GITHUB_OUTPUT line is delimited text that a body containing the delimiter can break out of. The file reaches gh through --body-file and never meets a shell or a ${{ }} expression. The branch moves from vale/update- to a rolling vendor/vale/republish, and the branch and PR lifecycle is now vendor-pr.cjs. Previously a second upstream release opened a second pull request proposing a conflicting edit to the same lines; now the open one is retitled and rewritten. That also fixes a quieter bug: the old code exited early when a PR was already open, leaving a body naming a version it no longer proposed. The publish path is untouched. The gate, prepare and publish jobs are byte-identical, and the filename, workflow name, environment and id-token grant — the values npm's trusted publisher binding depends on — are unchanged. The branch name is not part of that binding: publish fires on a push to main filtered by path, so it triggers on the merge regardless of what the branch was called. --- .github/scripts/vale-detect.cjs | 89 ++++++++++++++++++-------- .github/scripts/vale-detect.test.cjs | 81 ++++++++++++++++++++++-- .github/workflows/release-vale.yml | 95 +++++++++++++++++++--------- 3 files changed, 204 insertions(+), 61 deletions(-) diff --git a/.github/scripts/vale-detect.cjs b/.github/scripts/vale-detect.cjs index 60a2ba60..9916c1b2 100644 --- a/.github/scripts/vale-detect.cjs +++ b/.github/scripts/vale-detect.cjs @@ -38,6 +38,18 @@ * pin, and skipping the checksums fetch is not a shortcut but the * point (nothing is being verified here). * + * --notes-out + * write upstream's release notes for the proposed version to , + * rendered as a Markdown section. The detect workflow appends that + * file to the pull request body, so a reviewer can see what the bump + * contains without leaving the pull request. Written only when + * upstream is ahead; there is nothing to describe otherwise. + * + * A path rather than a step output on purpose: release notes are + * third-party Markdown, and a $GITHUB_OUTPUT line is delimited text + * that a body containing the delimiter can break out of. A file + * passed to `--body-file` never meets an interpreter. + * * Outputs (appended to $GITHUB_OUTPUT when set): * update "true" when upstream is ahead * vale_version the upstream version @@ -47,6 +59,11 @@ const { appendFileSync, readFileSync, writeFileSync } = require("node:fs"); const { join } = require("node:path"); +const { + fetchLatestRelease, + formatReleaseNotes, + writeNotesFile, +} = require("./release-notes.cjs"); const { applyTemplate, assertManifest, @@ -65,31 +82,6 @@ function setOutput(key, value) { } } -/** - * GitHub's `releases/latest` deliberately excludes prereleases and drafts, so a - * Vale release candidate never trips detection. `GITHUB_TOKEN`, when present, - * is only for the API rate limit; the endpoint is public. - */ -async function fetchLatestTag(repository) { - const headers = { - accept: "application/vnd.github+json", - "user-agent": "taskless-skills-vale-detect", - }; - if (process.env.GITHUB_TOKEN) { - headers.authorization = `Bearer ${process.env.GITHUB_TOKEN}`; - } - const url = `https://api.github.com/repos/${repository}/releases/latest`; - const response = await fetch(url, { headers }); - if (!response.ok) { - throw new Error(`GET ${url} responded ${response.status}`); - } - const release = await response.json(); - if (typeof release.tag_name !== "string") { - throw new TypeError(`${url} returned no tag_name`); - } - return release.tag_name; -} - async function fetchText(url) { const response = await fetch(url, { redirect: "follow" }); if (!response.ok) { @@ -98,19 +90,39 @@ async function fetchText(url) { return response.text(); } +/** `--notes-out `, or undefined when the flag is absent. */ +function readNotesOut(argv) { + const at = argv.indexOf("--notes-out"); + if (at === -1) { + return undefined; + } + const path = argv[at + 1]; + if (!path || path.startsWith("--")) { + throw new Error("--notes-out needs a path"); + } + return path; +} + async function main({ argv = process.argv.slice(2), - latestTag = fetchLatestTag, + latestRelease = fetchLatestRelease, text = fetchText, } = {}) { const json = argv.includes("--json"); const write = argv.includes("--write"); + const notesOut = readNotesOut(argv); // --json is a reporting mode and --write is a writing one. Refusing the // combination beats silently dropping whichever flag loses, since the caller // that passed both is wrong about what it is asking for. if (json && write) { throw new Error("--json is read-only; it cannot be combined with --write"); } + // Same reasoning as above: --json reports a comparison and writes nothing. + if (json && notesOut) { + throw new Error( + "--json is read-only; it cannot be combined with --notes-out" + ); + } // Everything a human wants to read is noise on stdout when a caller is // reading structured output from it. const log = json ? () => {} : (line) => console.log(line); @@ -118,7 +130,16 @@ async function main({ JSON.parse(readFileSync(MANIFEST_PATH, "utf8")) ); - const upstreamTag = await latestTag(manifest.upstream.repository); + // GitHub's `releases/latest` deliberately excludes prereleases and drafts, so + // a Vale release candidate never trips detection. The same call carries the + // release notes, so describing the bump costs no extra request. + const release = await latestRelease(manifest.upstream.repository); + if (!release) { + throw new Error( + `${manifest.upstream.repository} has no published releases to compare against` + ); + } + const upstreamTag = release.tag; log(`pinned: ${manifest.valeVersion} upstream latest: ${upstreamTag}`); // Decide whether to go on with the two pure predicates directly, rather than @@ -177,6 +198,20 @@ async function main({ console.log("\nPass --write to update the manifest."); } + // What the bump actually contains, for whoever reviews the digests. Written + // only on the ahead path: the other paths propose nothing to describe. + if (notesOut) { + writeNotesFile( + notesOut, + formatReleaseNotes({ + repository: manifest.upstream.repository, + version: plan.upstreamVersion, + release, + }) + ); + console.log(`Wrote the upstream release notes to ${notesOut}.`); + } + setOutput("update", "true"); setOutput("vale_version", plan.upstreamVersion); setOutput("pinned_version", plan.pinnedVersion); diff --git a/.github/scripts/vale-detect.test.cjs b/.github/scripts/vale-detect.test.cjs index d0a6c070..51d1a64f 100644 --- a/.github/scripts/vale-detect.test.cjs +++ b/.github/scripts/vale-detect.test.cjs @@ -49,16 +49,28 @@ function checksumsFor(version) { * Run main() with both fetches stubbed and $GITHUB_OUTPUT pointed at a temp * file, then return the parsed step outputs plus which URLs were fetched. */ -async function runDetect({ upstreamTag, checksums, argv = [] }) { +async function runDetect({ + upstreamTag, + notes = "", + checksums, + argv = [], + wantNotes = false, +}) { const directory = mkdtempSync(join(tmpdir(), "vale-detect-test-")); const outputPath = join(directory, "github-output"); + const notesPath = join(directory, "release-notes.md"); + const fullArgv = wantNotes ? [...argv, "--notes-out", notesPath] : argv; const previous = process.env.GITHUB_OUTPUT; const fetched = []; process.env.GITHUB_OUTPUT = outputPath; try { const comparison = await main({ - argv, - latestTag: async () => upstreamTag, + argv: fullArgv, + latestRelease: async () => ({ + tag: upstreamTag, + notes, + url: `https://github.com/${MANIFEST.upstream.repository}/releases/tag/${upstreamTag}`, + }), text: async (url) => { fetched.push(url); return checksums; @@ -73,7 +85,17 @@ async function runDetect({ upstreamTag, checksums, argv = [] }) { return [line.slice(0, at), line.slice(at + 1)]; }) ); - return { comparison, outputs, fetched }; + // Read before the finally below removes the directory. `undefined` means + // the script wrote nothing, which is a distinct answer from an empty file. + let notesWritten; + if (wantNotes) { + try { + notesWritten = readFileSync(notesPath, "utf8"); + } catch { + notesWritten = undefined; + } + } + return { comparison, outputs, fetched, notesWritten }; } finally { if (previous === undefined) { delete process.env.GITHUB_OUTPUT; @@ -176,3 +198,54 @@ test("detect: a checksums file missing a platform aborts", async () => { /publishes no asset named/ ); }); + +test("detect: --notes-out carries upstream's release notes for the proposal", async () => { + const { notesWritten } = await runDetect({ + upstreamTag: "v3.99.0", + notes: "## Fixed\n\nA thing that was broken.", + checksums: checksumsFor("3.99.0"), + wantNotes: true, + }); + + assert.match(notesWritten, /^## Upstream release notes — 3\.99\.0$/m); + assert.match(notesWritten, /^> A thing that was broken\.$/m); +}); + +/** + * The no-op path proposes nothing, so there is nothing to describe. Writing a + * section anyway would leave the previous run's notes on disk for a workflow + * step that only checks whether the file exists. + */ +test("detect: --notes-out writes nothing when upstream is not ahead", async () => { + const { notesWritten } = await runDetect({ + upstreamTag: `v${MANIFEST.valeVersion}`, + notes: "Should not be written.", + checksums: "", + wantNotes: true, + }); + + assert.equal(notesWritten, undefined); +}); + +test("detect: --json refuses to be combined with --notes-out", async () => { + await assert.rejects( + runDetect({ + upstreamTag: "v3.99.0", + checksums: "", + argv: ["--json"], + wantNotes: true, + }), + /--json is read-only/ + ); +}); + +test('detect: --notes-out without a path aborts rather than writing to "--write"', async () => { + await assert.rejects( + runDetect({ + upstreamTag: "v3.99.0", + checksums: checksumsFor("3.99.0"), + argv: ["--notes-out", "--write"], + }), + /--notes-out needs a path/ + ); +}); diff --git a/.github/workflows/release-vale.yml b/.github/workflows/release-vale.yml index 4b8364f2..aab988b5 100644 --- a/.github/workflows/release-vale.yml +++ b/.github/workflows/release-vale.yml @@ -13,8 +13,21 @@ # compares the latest upstream Vale release against the version # pinned in .github/scripts/vale-manifest.json. When upstream is # ahead it opens a pull request that updates the pinned version and -# all six SHA256 digests, taken from upstream's own checksums file. -# It publishes nothing. +# all six SHA256 digests, taken from upstream's own checksums file, +# and carries upstream's release notes so a reviewer can see what the +# digests are for. It publishes nothing. +# +# That pull request lives on ONE ROLLING BRANCH, +# `vendor/vale/republish`, rebuilt and retitled as upstream moves +# rather than accumulating a branch per release. `vendor-pr.cjs` owns +# that lifecycle and its two force-push guards, and every vendor +# workflow uses it, so they behave identically. +# +# THIS IS THE PRODUCER HALF ONLY. Publishing a platform package +# changes nothing for a consumer (see APPROVAL POLICY below); moving +# the CLI's pins onto the published set is `vale-upgrade.yml`, on +# `vendor/vale/upgrade`. Do not add that here — it needs no npm +# credential and belongs nowhere near one. # # publish Runs on the push to main that merges that pull request — i.e. once # a human has reviewed the digests. Split further into `gate`, @@ -142,7 +155,7 @@ jobs: (github.event_name == 'workflow_dispatch' && inputs.phase == 'detect') runs-on: ubuntu-latest permissions: - contents: write # push the vale/update- branch + contents: write # push the vendor/vale/republish branch pull-requests: write # open the manifest update PR steps: # Credentials persist here because this job pushes a branch. It holds no @@ -154,8 +167,19 @@ jobs: # No install step: the script is zero-dependency CommonJS. GITHUB_TOKEN is # passed only to raise the GitHub API rate limit; the endpoints are public. + # + # `--notes-out` writes upstream's release notes for the proposed version + # to a file, which the next step appends to the pull request body. A + # reviewer's job here is to decide whether these bytes should be + # publishable, and a bare version number with six digests does not say + # whether the release is a security fix or a docs pass. The notes arrive + # as a FILE rather than a step output because they are third-party + # Markdown: a $GITHUB_OUTPUT line is delimited text that a release body + # containing the delimiter can break out of. - id: detect - run: node .github/scripts/vale-detect.cjs --write + run: | + node .github/scripts/vale-detect.cjs \ + --write --notes-out "${RUNNER_TEMP}/vale-release-notes.md" env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -163,37 +187,26 @@ jobs: # `${{ }}` interpolation into the script body. The version has already been # validated as major.minor.patch by the script, but the pattern is the # rule regardless of the value. - - name: Open the manifest update PR + # + # The branch and pull request lifecycle is `vendor-pr.cjs`, shared with + # every vendor workflow: one rolling `vendor/vale/republish` branch, + # rebuilt from `main`, retitled and rewritten as upstream moves, and + # force-pushed only when the proposal actually changed and only when this + # workflow wrote what is already there. + - name: Propose the bump if: steps.detect.outputs.update == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} VALE_VERSION: ${{ steps.detect.outputs.vale_version }} PINNED_VERSION: ${{ steps.detect.outputs.pinned_version }} run: | - branch="vale/update-${VALE_VERSION}" - git config user.name "github-actions[bot]" - git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - - if git diff --quiet -- .github/scripts/vale-manifest.json; then - echo "Manifest already pins ${VALE_VERSION}; nothing to propose." - exit 0 - fi - - git checkout -b "$branch" - git add .github/scripts/vale-manifest.json - git commit -m "chore(vale): pin Vale ${VALE_VERSION}" - git push --force origin "$branch" + set -euo pipefail - if [ -n "$(gh pr list --head "$branch" --state open --json number --jq '.[].number')" ]; then - echo "A pull request for $branch is already open; it now carries the current digests." - exit 0 - fi - - gh pr create \ - --base main \ - --head "$branch" \ - --title "chore(vale): pin Vale ${VALE_VERSION}" \ - --body "$(cat <<'BODY' + # Composed as a FILE, not as an argument. The preamble is ours and the + # notes appended below it are upstream's, and the quoted heredoc plus + # `cat` means neither the shell nor gh ever interprets the latter. + body="${RUNNER_TEMP}/vale-pr-body.md" + cat > "$body" <<'BODY' Upstream Vale is ahead of the version this repository packages. This updates `.github/scripts/vale-manifest.json` to the new upstream @@ -210,9 +223,31 @@ jobs: package `-` and publishes the set. That publish is inert on its own: `@taskless/cli` pins exact versions, so nothing reaches a consumer until that pin is deliberately bumped. + + This pull request rolls: if upstream releases again before it merges, + the branch, title, and body are rewritten to the newer version rather + than a second pull request being opened. Push a commit to the branch + and that stops — the workflow will not force-push over a commit it did + not write. BODY - )" \ - --label skip-changeset + + # Absent only if the detect step reported ahead and then wrote no + # notes, which it cannot — so warn rather than skip silently. + notes="${RUNNER_TEMP}/vale-release-notes.md" + if [ -f "$notes" ]; then + printf '\n---\n\n' >> "$body" + cat "$notes" >> "$body" + else + echo "::warning::no release notes file at ${notes}" + fi + + node .github/scripts/vendor-pr.cjs \ + --branch vendor/vale/republish \ + --title "chore(vale): pin Vale ${VALE_VERSION}" \ + --message "chore(vale): pin Vale ${VALE_VERSION}" \ + --body-file "$body" \ + --label skip-changeset \ + -- .github/scripts/vale-manifest.json # Is there anything to publish? The push trigger fires on ANY edit to # vale-manifest.json — a reworded comment, a reformat, a digest correction — From 25ca91e17b39ac9bbb55adb8ca33aefb945c8f49 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 14 Sep 2026 18:21:00 -0700 Subject: [PATCH 3/8] feat(ci): open ast-grep pin bumps instead of only noticing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sg-detect.cjs could answer "is upstream ahead?" and had nowhere to put the answer: its only consumer was the README badge, which renders a version and cannot render a changelog. So an ast-grep release was discoverable by noticing a number on a badge, and 0.45.3 duly sat unpinned with nothing reporting it. This reverses that script's documented refusal to write, and the header now says so rather than quietly dropping it. The half that was right is unchanged — nobody merges a dependency bump unread, and Validate runs on the PR like any other. The half that was wrong is that refusing to write does not cause anyone to review anything; it only meant nothing was ever proposed. A reviewed pull request is more review than no pull request. The write is mechanical and bounded: pin-bump.cjs replaces a version string in pins the script can already enumerate and fails the run if the count does not match, so it cannot add a dependency or reformat the file. The workflow regenerates the lockfile with pnpm --lockfile-only, which also fails if upstream published the CLI without one of its platform siblings, and writes a patch changeset — patch because the package is pre-1.0, even though this is added functionality reaching a consumer. The branch is vendor/ast-grep/upgrade rather than a bare vendor/ast-grep. Git refs are paths, so a branch at the shorter name is a file there and makes vendor/ast-grep/ impossible to create ever after. --- .github/scripts/sg-detect.cjs | 134 +++++++++++++- .github/scripts/sg-detect.test.cjs | 230 ++++++++++++++++++++++++- .github/workflows/ast-grep-upgrade.yml | 182 +++++++++++++++++++ 3 files changed, 533 insertions(+), 13 deletions(-) create mode 100644 .github/workflows/ast-grep-upgrade.yml diff --git a/.github/scripts/sg-detect.cjs b/.github/scripts/sg-detect.cjs index 2047daba..4b2d4bcc 100644 --- a/.github/scripts/sg-detect.cjs +++ b/.github/scripts/sg-detect.cjs @@ -35,12 +35,45 @@ * edit able to break this badge. * * Usage: - * node .github/scripts/sg-detect.cjs [--json] + * node .github/scripts/sg-detect.cjs [--json] [--write] [--notes-out ] * - * --json print `{ pinned, upstream, ahead }` and nothing else. This script - * never writes anything in either mode; unlike Vale there is no - * manifest to rewrite, and bumping eight dependency pins is a - * lockfile-touching change that belongs to a human. + * --json print `{ pinned, upstream, ahead }` and nothing else, writing + * nothing. This is what update-badges.cjs calls. + * + * --write rewrite every `@ast-grep/cli*` pin in packages/cli/package.json to + * the upstream version. `ast-grep-upgrade.yml` then regenerates the + * lockfile and opens a pull request. + * + * THIS REVERSES AN EARLIER DECISION, deliberately. This script used + * to refuse to write on the grounds that a lockfile-touching bump + * belongs to a human. The part of that which was right — nobody + * should merge a machine's dependency bump unread — is unchanged; + * the pull request is reviewed like any other. The part which was + * wrong is that refusing to WRITE does not make anyone review + * anything. It made the answer "upstream is ahead" reach only a + * README badge, so ast-grep 0.45.3 sat unpinned with nothing + * reporting it. A reviewed pull request is more review than no + * pull request, not less. + * + * What keeps it honest is that the write is mechanical and bounded: + * `bumpPins` replaces a version string in pins it can already + * enumerate, and refuses if the number it rewrote is not the number + * it found. It cannot add a dependency, reorder the file, or reflow + * it, so the diff a reviewer reads is N version strings and a + * lockfile. + * + * --notes-out + * write upstream's release notes for the newer version to , + * rendered as a Markdown section. `ast-grep-upgrade.yml` appends that + * file to the pull request body, so the bump arrives with what it + * actually contains rather than as eight changed version strings a + * reviewer has to go look up. Written only when upstream is ahead. + * + * The version comes from npm and the notes come from GitHub, which + * is the one place those two records can disagree: a tag missing + * from GitHub renders as a link to the releases page rather than + * failing the run, because the comparison this script exists to make + * has already succeeded by then. * * Outputs (appended to $GITHUB_OUTPUT when set): * update "true" when upstream is ahead @@ -48,9 +81,16 @@ * pinned_version the version currently pinned in packages/cli/package.json */ -const { appendFileSync, readFileSync } = require("node:fs"); +const { appendFileSync, readFileSync, writeFileSync } = require("node:fs"); const { join } = require("node:path"); +const { bumpPins } = require("./pin-bump.cjs"); +const { + fetchReleaseByTag, + formatReleaseNotes, + writeNotesFile, +} = require("./release-notes.cjs"); + const PACKAGE_JSON_PATH = join( __dirname, "..", @@ -67,6 +107,14 @@ const VERSION_PATTERN = /^(\d+)\.(\d+)\.(\d+)$/; const REGISTRY = "https://registry.npmjs.org"; +/** + * Where the release notes live. ast-grep tags its releases with the bare npm + * version (`0.45.3`, no `v`), so the version read from the registry is also the + * tag — an assumption this script does not have to be right about, since a tag + * it cannot find degrades to a link rather than to an error. + */ +const NOTES_REPOSITORY = "ast-grep/ast-grep"; + function setOutput(key, value) { const file = process.env.GITHUB_OUTPUT; if (file) { @@ -113,7 +161,7 @@ function isAhead(pinned, upstream) { * different ast-grep on one platform than on the others — and a badge that * smoothed it over would hide exactly the drift it was added to expose. */ -function collectPinnedVersion(packageJson) { +function collectPins(packageJson) { const pins = new Map(); for (const field of [ "dependencies", @@ -126,6 +174,11 @@ function collectPinnedVersion(packageJson) { } } } + return pins; +} + +function collectPinnedVersion(packageJson) { + const pins = collectPins(packageJson); if (pins.size === 0) { throw new Error( @@ -182,12 +235,35 @@ async function fetchLatestVersion(packageName) { return latest; } +/** `--notes-out `, or undefined when the flag is absent. */ +function readNotesOut(argv) { + const at = argv.indexOf("--notes-out"); + if (at === -1) { + return undefined; + } + const path = argv[at + 1]; + if (!path || path.startsWith("--")) { + throw new Error("--notes-out needs a path"); + } + return path; +} + async function main({ argv = process.argv.slice(2), latestVersion = fetchLatestVersion, - packageJson = JSON.parse(readFileSync(PACKAGE_JSON_PATH, "utf8")), + releaseFor = fetchReleaseByTag, + packageJsonPath = PACKAGE_JSON_PATH, + packageJson = JSON.parse(readFileSync(packageJsonPath, "utf8")), } = {}) { const json = argv.includes("--json"); + const write = argv.includes("--write"); + const notesOut = readNotesOut(argv); + // --json is what the badge run calls, and a badge run proposes nothing. + if (json && (notesOut || write)) { + throw new Error( + "--json prints the comparison and nothing else; it cannot be combined with --notes-out or --write" + ); + } const log = json ? () => {} : (line) => console.log(line); const pinned = collectPinnedVersion(packageJson); @@ -206,6 +282,46 @@ async function main({ console.log(JSON.stringify(comparison)); } + // Only the ahead path has anything to write. The pins are rewritten before + // the notes are fetched so that a network failure on the (optional) changelog + // cannot leave a half-done bump: by the time anything can throw below, the + // file on disk is either fully bumped or untouched. + if (write && ahead) { + const pins = collectPins(packageJson); + const source = readFileSync(packageJsonPath, "utf8"); + const { source: bumped, count } = bumpPins(source, { + prefix: "@ast-grep/cli", + from: pinned, + to: upstream, + }); + // A straggler left at the old version is a different ast-grep on one + // platform than on the others, which is the exact state collectPinnedVersion + // refuses to report on. Better to fail here than to open that as a PR. + if (count !== pins.size) { + throw new Error( + `expected to rewrite ${pins.size} @ast-grep/cli* pins, rewrote ${count}` + ); + } + writeFileSync(packageJsonPath, bumped); + log(`Rewrote ${count} pins in ${packageJsonPath} to ${upstream}.`); + } + + // Only the ahead path has a bump to describe. A second request, unlike Vale's + // — that one reads its version out of a GitHub release and gets the notes in + // the same response, while this one learns the version from npm. + if (notesOut && ahead) { + const release = await releaseFor(NOTES_REPOSITORY, upstream); + writeNotesFile( + notesOut, + formatReleaseNotes({ + repository: NOTES_REPOSITORY, + version: upstream, + release, + }) + ); + log(`Wrote the upstream release notes to ${notesOut}.`); + } + setOutput("update", String(ahead)); setOutput("sg_version", upstream); setOutput("pinned_version", pinned); @@ -215,7 +331,7 @@ async function main({ // main() both prints and RETURNS the comparison, so update-badges.cjs can call // it in-process and read the answer as data. Nothing should ever parse the // human line above to recover a version that this return value already holds. -module.exports = { collectPinnedVersion, isAhead, main }; +module.exports = { collectPinnedVersion, collectPins, isAhead, main }; if (require.main === module) { main().catch((error) => { diff --git a/.github/scripts/sg-detect.test.cjs b/.github/scripts/sg-detect.test.cjs index 383d8794..defa03d6 100644 --- a/.github/scripts/sg-detect.test.cjs +++ b/.github/scripts/sg-detect.test.cjs @@ -13,11 +13,12 @@ const test = require("node:test"); const assert = require("node:assert/strict"); -const { mkdtempSync, readFileSync, rmSync } = require("node:fs"); +const { mkdtempSync, readFileSync, rmSync, writeFileSync } = require("node:fs"); const { tmpdir } = require("node:os"); const { join } = require("node:path"); const { collectPinnedVersion, isAhead, main } = require("./sg-detect.cjs"); +const { bumpPins } = require("./pin-bump.cjs"); const CLI_PACKAGE_JSON = JSON.parse( readFileSync( @@ -27,15 +28,35 @@ const CLI_PACKAGE_JSON = JSON.parse( ); /** Run main() with the registry stubbed and $GITHUB_OUTPUT captured. */ -async function runDetect({ upstream, packageJson, argv = [] }) { +async function runDetect({ + upstream, + packageJson, + packageJsonSource, + argv = [], + release, + wantNotes = false, +}) { const directory = mkdtempSync(join(tmpdir(), "sg-detect-test-")); const outputPath = join(directory, "github-output"); + const notesPath = join(directory, "release-notes.md"); + // --write rewrites the file on disk, so a test that exercises it needs a + // package.json of its own. The committed one is never written to here. + const packageJsonPath = join(directory, "package.json"); + if (packageJsonSource !== undefined) { + writeFileSync(packageJsonPath, packageJsonSource); + } const previous = process.env.GITHUB_OUTPUT; + const releasesFetched = []; process.env.GITHUB_OUTPUT = outputPath; try { const comparison = await main({ - argv, + argv: wantNotes ? [...argv, "--notes-out", notesPath] : argv, latestVersion: async () => upstream, + releaseFor: async (repository, tag) => { + releasesFetched.push(`${repository}@${tag}`); + return release; + }, + packageJsonPath, packageJson, }); const outputs = Object.fromEntries( @@ -47,7 +68,27 @@ async function runDetect({ upstream, packageJson, argv = [] }) { return [line.slice(0, at), line.slice(at + 1)]; }) ); - return { comparison, outputs }; + // Read before the finally below removes the directory. `undefined` means + // the script wrote nothing, which is a distinct answer from an empty file. + let notesWritten; + if (wantNotes) { + try { + notesWritten = readFileSync(notesPath, "utf8"); + } catch { + notesWritten = undefined; + } + } + const packageJsonWritten = + packageJsonSource === undefined + ? undefined + : readFileSync(packageJsonPath, "utf8"); + return { + comparison, + outputs, + notesWritten, + releasesFetched, + packageJsonWritten, + }; } finally { if (previous === undefined) { delete process.env.GITHUB_OUTPUT; @@ -169,3 +210,184 @@ test("sg-detect: ordering is numeric, not lexical", () => { assert.equal(isAhead("0.10.0", "0.9.0"), false); assert.equal(isAhead("1.0.0", "0.99.99"), false); }); + +test("sg-detect: --notes-out describes the bump upstream is proposing", async () => { + const { notesWritten, releasesFetched } = await runDetect({ + upstream: "0.45.3", + packageJson: pinnedAt("0.45.2"), + release: { + tag: "0.45.3", + notes: "- feat: add min-severity cli", + url: "https://github.com/ast-grep/ast-grep/releases/tag/0.45.3", + }, + wantNotes: true, + }); + + // The tag is the bare npm version, not a `v`-prefixed one. Getting that wrong + // degrades to a link rather than an error, so nothing else would catch it. + assert.deepEqual(releasesFetched, ["ast-grep/ast-grep@0.45.3"]); + assert.match(notesWritten, /^## Upstream release notes — 0\.45\.3$/m); + assert.match(notesWritten, /^> - feat: add min-severity cli$/m); +}); + +test("sg-detect: --notes-out writes nothing, and asks nothing, when the pin is current", async () => { + const { notesWritten, releasesFetched } = await runDetect({ + upstream: "0.45.2", + packageJson: pinnedAt("0.45.2"), + wantNotes: true, + }); + + assert.equal(notesWritten, undefined); + assert.deepEqual(releasesFetched, []); +}); + +/** + * The version comes from npm and the notes come from GitHub, so a tag npm has + * and GitHub does not is a real state. The comparison has already succeeded by + * then, and failing the run would throw that answer away. + */ +test("sg-detect: a version with no GitHub release still reports the bump", async () => { + const { notesWritten, outputs } = await runDetect({ + upstream: "0.45.3", + packageJson: pinnedAt("0.45.2"), + release: undefined, + wantNotes: true, + }); + + assert.equal(outputs.update, "true"); + assert.match(notesWritten, /Upstream published no release notes/); + assert.match( + notesWritten, + /^https:\/\/github\.com\/ast-grep\/ast-grep\/releases$/m + ); +}); + +test("sg-detect: --json refuses to be combined with --notes-out", async () => { + await assert.rejects( + runDetect({ + upstream: "0.45.3", + packageJson: pinnedAt("0.45.2"), + argv: ["--json"], + wantNotes: true, + }), + /cannot be combined with --notes-out/ + ); +}); + +/** A package.json shaped like the real one: prettier-formatted, mixed fields. */ +const sourcePinnedAt = (version) => + `${JSON.stringify( + { + name: "@taskless/cli", + dependencies: { "@ast-grep/cli": version, zod: "^4.0.0" }, + optionalDependencies: { + "@ast-grep/cli-darwin-arm64": version, + "@ast-grep/cli-linux-x64-gnu": version, + }, + }, + undefined, + 2 + )}\n`; + +test("bump: every pin moves and nothing else does", () => { + const before = sourcePinnedAt("0.45.2"); + const { source, count } = bumpPins(before, { + prefix: "@ast-grep/cli", + from: "0.45.2", + to: "0.45.3", + }); + + assert.equal(count, 3); + assert.equal(source.match(/0\.45\.3/g).length, 3); + assert.doesNotMatch(source, /0\.45\.2/); + // The unrelated dependency is untouched, and so is the formatting: the diff + // a reviewer reads is three version strings. + assert.match(source, /"zod": "\^4\.0\.0"/); + assert.equal( + before.split("\n").length, + source.split("\n").length, + "the rewrite reflowed the file" + ); +}); + +/** + * The dots in a version are regular-expression metacharacters. Unescaped, + * `0.45.2` also matches `0X45Y2` — and, far more plausibly, a pin at `0145.2`. + */ +test("bump: the version is matched literally, not as a pattern", () => { + const source = '{ "@ast-grep/cli": "0145.2" }'; + const { count } = bumpPins(source, { + prefix: "@ast-grep/cli", + from: "0.45.2", + to: "0.45.3", + }); + assert.equal(count, 0); +}); + +test("bump: a version appearing outside an @ast-grep pin is left alone", () => { + const source = '{ "some-other-tool": "0.45.2", "@ast-grep/cli": "0.45.2" }'; + const { source: bumped, count } = bumpPins(source, { + prefix: "@ast-grep/cli", + from: "0.45.2", + to: "0.45.3", + }); + assert.equal(count, 1); + assert.match(bumped, /"some-other-tool": "0\.45\.2"/); +}); + +test("sg-detect: --write bumps every pin in the file on disk", async () => { + const { packageJsonWritten, outputs } = await runDetect({ + upstream: "0.45.3", + packageJson: pinnedAt("0.45.2"), + packageJsonSource: sourcePinnedAt("0.45.2"), + argv: ["--write"], + }); + + assert.equal(outputs.update, "true"); + assert.doesNotMatch(packageJsonWritten, /0\.45\.2/); + assert.equal(packageJsonWritten.match(/0\.45\.3/g).length, 3); +}); + +test("sg-detect: --write leaves the file alone when the pin is current", async () => { + const before = sourcePinnedAt("0.45.2"); + const { packageJsonWritten } = await runDetect({ + upstream: "0.45.2", + packageJson: pinnedAt("0.45.2"), + packageJsonSource: before, + argv: ["--write"], + }); + + assert.equal(packageJsonWritten, before); +}); + +/** + * The half-applied bump this guard exists for. `packageJson` says there are + * three pins; the source text on disk only spells two of them at the old + * version, so a rewrite would leave a straggler behind — a different ast-grep + * on one platform than on the others. + */ +test("sg-detect: a pin the rewrite cannot reach fails the run", async () => { + await assert.rejects( + runDetect({ + upstream: "0.45.3", + packageJson: pinnedAt("0.45.2"), + packageJsonSource: sourcePinnedAt("0.45.2").replace( + '"@ast-grep/cli-linux-x64-gnu": "0.45.2"', + '"@ast-grep/cli-linux-x64-gnu": "0.45.1"' + ), + argv: ["--write"], + }), + /expected to rewrite 3 @ast-grep\/cli\* pins, rewrote 2/ + ); +}); + +test("sg-detect: --json refuses to be combined with --write", async () => { + await assert.rejects( + runDetect({ + upstream: "0.45.3", + packageJson: pinnedAt("0.45.2"), + argv: ["--json", "--write"], + }), + /cannot be combined with/ + ); +}); diff --git a/.github/workflows/ast-grep-upgrade.yml b/.github/workflows/ast-grep-upgrade.yml new file mode 100644 index 00000000..8b43e7b4 --- /dev/null +++ b/.github/workflows/ast-grep-upgrade.yml @@ -0,0 +1,182 @@ +# SPDX-License-Identifier: MIT +# ast-grep — notice upstream releases, and open the pin bump with the changelog in it. +# +# WHY THIS EXISTS. Vale's upstream releases have been watched by a machine since +# the platform packages were built. ast-grep had `sg-detect.cjs` but nowhere for +# its answer to go: the only consumer was the README badge, which renders a +# version and cannot render a changelog. So "ast-grep 0.45.3 is out" was +# discoverable only by someone noticing a number on a badge and then going to +# find the release notes by hand — and 0.45.3 duly sat unpinned with nothing +# reporting it. +# +# WHY IT OPENS A PULL REQUEST, having previously argued for not writing at all. +# `sg-detect.cjs` used to refuse to write on the grounds that a lockfile-touching +# bump belongs to a human. Half of that was right and is unchanged: nobody +# merges this unread, and `Validate` runs on it like any other pull request. +# The other half was wrong. Refusing to write does not cause anyone to review +# anything — it only meant nothing was ever proposed. A reviewed pull request is +# more review than no pull request. +# +# HOW THIS DIFFERS FROM release-vale.yml's detect, which it otherwise mirrors: +# +# - There is no trust boundary here, so there is no second phase. Vale's +# detect proposes DIGESTS that authorize a later job to download and publish +# third-party bytes, which is why reviewing it is load-bearing and why the +# publish is split into a separate credential-free fetch. This proposes a +# version string. npm verifies its own integrity through the lockfile, and +# nothing here publishes anything. +# +# - The lockfile is regenerated rather than hand-edited. `pnpm install +# --lockfile-only` resolves the eight platform packages at the new version +# and rewrites `pnpm-lock.yaml`. It fails if upstream published the CLI +# without one of its platform siblings, which is the failure worth catching +# before a reviewer sees it, not after. +# +# - A changeset is written, where Vale's detect labels `skip-changeset`. The +# Vale packages are stamped by their own pipeline and reach a consumer only +# when a pin is bumped; THIS is that bump, and it changes which ast-grep a +# `@taskless/cli` user runs. `patch`, because the package is pre-1.0 — see +# the bump guidance in CLAUDE.md before reaching for `minor`. +# +# ONE ROLLING BRANCH, `vendor/ast-grep/upgrade`. `vendor-pr.cjs` owns that +# lifecycle for every vendor workflow — rebuild from `main`, retitle and rewrite +# as upstream moves, and force-push only when the proposal changed and only when +# this workflow wrote what is already on the branch. +# +# THE `/upgrade` SUFFIX IS NOT DECORATION, even though ast-grep has only the one +# stage. Git refs are paths, so a branch named `vendor/ast-grep` is a FILE at +# that path and makes `vendor/ast-grep/` impossible to create ever +# after. Vale needed exactly that split — `vendor/vale/republish` for the +# packages we build and `vendor/vale/upgrade` for the pins a user gets — and +# reserving the namespace here costs one word. That Vale has a `republish` and +# ast-grep does not is the real difference between them: nothing in this +# repository repackages ast-grep, so there is no set of our own to publish +# before the pins can move. +# +# Action refs are pinned to commit SHAs; the trailing comment records the tag. + +name: Upgrade ast-grep + +on: + # Same cadence and the same reasoning as the Vale detect: upstream's release + # rhythm, not ours. The dispatch covers "upstream just released, do not wait + # until Monday". + schedule: + - cron: "41 7 * * 1" + + workflow_dispatch: + +permissions: {} + +concurrency: ast-grep-upgrade + +jobs: + detect: + name: Upgrade the pinned ast-grep + runs-on: ubuntu-latest + permissions: + contents: write # push the vendor/ast-grep/upgrade branch + pull-requests: write # open the bump PR + steps: + # Credentials persist because this job pushes a branch. It holds no npm + # identity and no id-token. + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + + - uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + with: + node-version: 24 + + # No dependency install: sg-detect.cjs is zero-dependency CommonJS, and + # the lockfile step below needs pnpm rather than node_modules. + # + # `--notes-out` writes upstream's release notes to a FILE rather than to a + # step output, because they are third-party Markdown and a $GITHUB_OUTPUT + # line is delimited text that a release body containing the delimiter can + # break out of. It reaches the body through `--body-file` and never meets + # a shell or a `${{ }}` expression. + - id: detect + run: | + node .github/scripts/sg-detect.cjs \ + --write --notes-out "${RUNNER_TEMP}/sg-release-notes.md" + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # Resolution, not editing. `--lockfile-only` skips linking, so nothing is + # downloaded into node_modules and no lifecycle script runs; the job's + # only product is a rewritten pnpm-lock.yaml. It fails if the eight + # platform packages are not all published at the new version. + - name: Regenerate the lockfile + if: steps.detect.outputs.update == 'true' + run: pnpm install --lockfile-only --ignore-scripts + + # Every value reaching the shell goes through `env:` rather than `${{ }}` + # interpolation into the script body. The branch and pull request + # lifecycle is `vendor-pr.cjs`, shared with release-vale.yml. + - name: Propose the bump + if: steps.detect.outputs.update == 'true' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + SG_VERSION: ${{ steps.detect.outputs.sg_version }} + PINNED_VERSION: ${{ steps.detect.outputs.pinned_version }} + run: | + set -euo pipefail + + # A changeset, because this bump is what reaches a consumer — unlike + # the Vale manifest, whose packages are stamped by their own pipeline + # and reach nobody until a pin like this one moves. `patch` is correct + # while @taskless/cli is pre-1.0 even though the diff adds no surface + # of our own; see the bump guidance in CLAUDE.md before reaching for + # `minor`. The filename is stable per version, so a rolling branch + # rewrites it rather than accumulating one file per release. + changeset=".changeset/ast-grep-${SG_VERSION//./-}.md" + cat > "$changeset" < "$body" <> "$body" + cat "$notes" >> "$body" + else + echo "::warning::no release notes file at ${notes}" + fi + + node .github/scripts/vendor-pr.cjs \ + --branch vendor/ast-grep/upgrade \ + --title "chore(ast-grep): pin ast-grep ${SG_VERSION}" \ + --message "chore(ast-grep): pin ast-grep ${SG_VERSION}" \ + --body-file "$body" \ + -- packages/cli/package.json pnpm-lock.yaml "$changeset" From a53a266e8cdcfe3a4515d4f45d2573e108c20c38 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 14 Sep 2026 18:21:11 -0700 Subject: [PATCH 4/8] feat(ci): ship the Vale we publish, not just publish it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vendoring Vale has two halves and only one was automated. release-vale.yml watches upstream and republishes the six @taskless/vale-* packages at -; that changes nothing for a consumer, because the CLI pins each package exactly. Moving those pins is what ships a release, and its own header says so: "a new version reaches a user only when someone reviews a bump to that pin." Somebody always did. git log on the pins shows 3.18.0, 3.19.0 and 3.20.0 each moved in its own hand-written commit, and the pins are current as this lands — so this automates a step that was working. What it removes is the dependency on remembering, and what it adds is upstream's changelog next to the diff. The failure it prevents is quiet: a publish lands, nobody notices the pins are behind, and the release does not ship. Upstream here is npm, not the manifest. The manifest records what we intend to publish; npm records what was published, and between them sits a job that can fail. All six packages are read rather than one as a representative, because the publish loop can leave the set split across two versions and that is the one state exact pins cannot tolerate — some platforms resolvable, others not. A split set fails the run. The changelog is upstream's, fetched by the base version inside the stamp. Our stamp records when a package was built, which tells a reviewer nothing about what changed. Daily rather than weekly, unlike the republish detect: that one waits on upstream, this one waits on our own publish job, and a weekly schedule would leave a release packaged-but-unshipped for up to a week after we built it. --- .github/scripts/vale-upgrade-detect.cjs | 284 +++++++++++++++++++ .github/scripts/vale-upgrade-detect.test.cjs | 227 +++++++++++++++ .github/workflows/vale-upgrade.yml | 161 +++++++++++ 3 files changed, 672 insertions(+) create mode 100644 .github/scripts/vale-upgrade-detect.cjs create mode 100644 .github/scripts/vale-upgrade-detect.test.cjs create mode 100644 .github/workflows/vale-upgrade.yml diff --git a/.github/scripts/vale-upgrade-detect.cjs b/.github/scripts/vale-upgrade-detect.cjs new file mode 100644 index 00000000..6afd9035 --- /dev/null +++ b/.github/scripts/vale-upgrade-detect.cjs @@ -0,0 +1,284 @@ +#!/usr/bin/env node +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Vale — the CONSUMER half of the vendoring pipeline. + * + * THE TWO STAGES, and why this one was missing. `vale-detect.cjs` watches + * upstream Vale and proposes a new manifest; merging that republishes the six + * `@taskless/vale-*` platform packages at `-`. That is the + * PRODUCER stage, and release-vale.yml says plainly what it does not do: + * "publishing a platform package changes no consumer, because the CLI pins each + * one exactly and a new version reaches a user only when someone reviews a bump + * to that pin." + * + * That review has happened — by hand, every time. `git log` on the pins shows + * 3.18.0, 3.19.0 and 3.20.0 each moved in a separate manual commit. So the + * stage is not missing so much as unautomated, and what it costs is not a stale + * pin but a dependency on somebody remembering: nothing detects that a publish + * has landed and the pins are now behind, and nothing puts upstream's changelog + * in front of whoever bumps them. + * + * This script is that detection. It asks whether a newer platform set exists on + * npm and, with `--write`, moves every pin to it. + * + * WHY npm AND NOT THE MANIFEST. The manifest records what we intend to publish; + * npm records what was actually published. Between the two sits a publish job + * that can fail, and a pin bumped to a version npm does not serve is a broken + * install rather than a stale one. The honest question for a consumer-side + * upgrade is "what can be installed", so the registry is the source. + * + * WHY ALL SIX ARE READ, not one as a representative. The publish loop attempts + * every package and reports failures at the end precisely because a partial set + * is the one state the CLI's exact pins cannot tolerate — some platforms + * resolvable, others not. Reading one package would upgrade the pins into that + * state without noticing. Reading six costs six cheap requests and turns it + * into a failed run. + * + * WHY THE COMPARISON IS vale-release.cjs's. Unlike sg-detect.cjs — which keeps + * its own comparator so an ast-grep oddity cannot surface as an error message + * about Vale — these versions really are Vale stamped versions, so + * `compareStampedVersions` is both correct and the one place that already knows + * a stamp's ordering rules. + * + * Usage: + * node .github/scripts/vale-upgrade-detect.cjs [--json] [--write] [--notes-out ] + * + * Outputs (appended to $GITHUB_OUTPUT when set): + * update "true" when a newer published set exists + * vale_version the newer stamped version + * pinned_version the stamped version currently pinned + * base_version the plain Vale version inside the newer stamp + */ + +const { appendFileSync, readFileSync, writeFileSync } = require("node:fs"); +const { join } = require("node:path"); + +const { bumpPins } = require("./pin-bump.cjs"); +const { + fetchReleaseByTag, + formatReleaseNotes, + writeNotesFile, +} = require("./release-notes.cjs"); +const { + assertManifest, + assertStampedVersion, + compareStampedVersions, +} = require("./vale-release.cjs"); + +const PACKAGE_JSON_PATH = join( + __dirname, + "..", + "..", + "packages", + "cli", + "package.json" +); + +const MANIFEST_PATH = join(__dirname, "vale-manifest.json"); + +const PIN_PREFIX = "@taskless/vale-"; + +const REGISTRY = "https://registry.npmjs.org"; + +function setOutput(key, value) { + const file = process.env.GITHUB_OUTPUT; + if (file) { + appendFileSync(file, `${key}=${value}\n`); + } +} + +/** `--notes-out `, or undefined when the flag is absent. */ +function readNotesOut(argv) { + const at = argv.indexOf("--notes-out"); + if (at === -1) { + return undefined; + } + const path = argv[at + 1]; + if (!path || path.startsWith("--")) { + throw new Error("--notes-out needs a path"); + } + return path; +} + +/** + * The one stamped version every `@taskless/vale-*` pin holds. + * + * Disagreement is an error for the same reason it is in sg-detect.cjs: the + * platform packages are selected by optional dependency, so pins that disagree + * are a different Vale on one platform than on the others. There is no version + * this script could honestly report for that state, and picking the highest + * would paper over exactly the drift worth surfacing. + */ +function collectPins(packageJson) { + const pins = new Map(); + for (const field of [ + "dependencies", + "devDependencies", + "optionalDependencies", + ]) { + for (const [name, range] of Object.entries(packageJson[field] ?? {})) { + if (name.startsWith(PIN_PREFIX)) { + pins.set(name, range); + } + } + } + if (pins.size === 0) { + throw new Error( + `packages/cli/package.json declares no ${PIN_PREFIX}* dependency` + ); + } + const versions = new Set(pins.values()); + if (versions.size > 1) { + const detail = [...pins] + .map(([name, range]) => `${name}@${range}`) + .sort() + .join(", "); + throw new Error( + `${PIN_PREFIX}* pins disagree, so there is no single version to upgrade from: ${detail}` + ); + } + const [version] = versions; + assertStampedVersion(version); + return { pins, version }; +} + +/** What `npm install ` would resolve to today. */ +async function fetchLatestVersion(packageName) { + const url = `${REGISTRY}/${packageName.replace("/", "%2F")}`; + const response = await fetch(url, { + headers: { + accept: "application/vnd.npm.install-v1+json", + "user-agent": "taskless-vale-upgrade-detect", + }, + }); + if (!response.ok) { + throw new Error(`GET ${url} responded ${response.status}`); + } + const packument = await response.json(); + const latest = packument["dist-tags"]?.latest; + if (typeof latest !== "string") { + throw new TypeError(`${url} returned no dist-tags.latest`); + } + return latest; +} + +/** + * The version the whole set is published at. + * + * A set that does not agree is a half-finished publish, and upgrading into it + * would pin some platforms to a version npm cannot serve. release-vale.yml's + * publish loop is built to make this rare and re-runnable; this refuses to + * build on it while it is true. + */ +async function resolvePublishedVersion(names, latestVersion) { + const published = new Map( + await Promise.all( + names.map(async (name) => [name, await latestVersion(name)]) + ) + ); + const versions = new Set(published.values()); + if (versions.size > 1) { + const detail = [...published] + .map(([name, version]) => `${name}@${version}`) + .sort() + .join(", "); + throw new Error( + `the published ${PIN_PREFIX}* set is not at one version, so it is mid-publish or partially failed: ${detail}` + ); + } + const [version] = versions; + assertStampedVersion(version); + return version; +} + +/** `3.21.0-20260914012345` -> `3.21.0`, the release upstream actually tagged. */ +function baseVersion(stamped) { + return assertStampedVersion(stamped).split("-")[0]; +} + +async function main({ + argv = process.argv.slice(2), + latestVersion = fetchLatestVersion, + releaseFor = fetchReleaseByTag, + packageJsonPath = PACKAGE_JSON_PATH, + packageJson = JSON.parse(readFileSync(packageJsonPath, "utf8")), + manifest = assertManifest(JSON.parse(readFileSync(MANIFEST_PATH, "utf8"))), +} = {}) { + const json = argv.includes("--json"); + const write = argv.includes("--write"); + const notesOut = readNotesOut(argv); + if (json && (write || notesOut)) { + throw new Error( + "--json prints the comparison and nothing else; it cannot be combined with --write or --notes-out" + ); + } + const log = json ? () => {} : (line) => console.log(line); + + const { pins, version: pinned } = collectPins(packageJson); + const upstream = await resolvePublishedVersion( + [...pins.keys()].sort(), + latestVersion + ); + const ahead = compareStampedVersions(upstream, pinned) > 0; + + log(`pinned: ${pinned} published latest: ${upstream}`); + log( + ahead + ? `A newer platform set is published. Move all ${pins.size} pins together.` + : "The pins are current with what is published. Nothing to do." + ); + + const comparison = { pinned, upstream, ahead }; + if (json) { + console.log(JSON.stringify(comparison)); + } + + if (write && ahead) { + const source = readFileSync(packageJsonPath, "utf8"); + const { source: bumped, count } = bumpPins(source, { + prefix: PIN_PREFIX, + from: pinned, + to: upstream, + }); + if (count !== pins.size) { + throw new Error( + `expected to rewrite ${pins.size} ${PIN_PREFIX}* pins, rewrote ${count}` + ); + } + writeFileSync(packageJsonPath, bumped); + log(`Rewrote ${count} pins in ${packageJsonPath} to ${upstream}.`); + } + + // The changelog a reviewer wants is UPSTREAM's, not ours. Our stamp says when + // the package was built; `v` is the release whose behaviour changes. + if (notesOut && ahead) { + const base = baseVersion(upstream); + const release = await releaseFor(manifest.upstream.repository, `v${base}`); + writeNotesFile( + notesOut, + formatReleaseNotes({ + repository: manifest.upstream.repository, + version: base, + release, + }) + ); + log(`Wrote the upstream release notes to ${notesOut}.`); + } + + setOutput("update", String(ahead)); + setOutput("vale_version", upstream); + setOutput("pinned_version", pinned); + setOutput("base_version", baseVersion(upstream)); + return comparison; +} + +module.exports = { baseVersion, collectPins, main, resolvePublishedVersion }; + +if (require.main === module) { + main().catch((error) => { + console.error(`\nvale-upgrade-detect failed: ${error.message}`); + process.exitCode = 1; + }); +} diff --git a/.github/scripts/vale-upgrade-detect.test.cjs b/.github/scripts/vale-upgrade-detect.test.cjs new file mode 100644 index 00000000..94e4b261 --- /dev/null +++ b/.github/scripts/vale-upgrade-detect.test.cjs @@ -0,0 +1,227 @@ +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Tests for vale-upgrade-detect.cjs — the consumer half of the Vale pipeline. + * + * The state worth guarding against here is not "we missed a release". It is + * "we upgraded into a half-published set", which produces pins that resolve on + * some platforms and 404 on others, and which no amount of local testing on one + * machine would reveal. + */ + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { mkdtempSync, readFileSync, rmSync, writeFileSync } = require("node:fs"); +const { tmpdir } = require("node:os"); +const { join } = require("node:path"); + +const { baseVersion, collectPins, main } = require("./vale-upgrade-detect.cjs"); + +const MANIFEST = { upstream: { repository: "errata-ai/vale" } }; + +const PLATFORMS = [ + "@taskless/vale-darwin-arm64", + "@taskless/vale-darwin-x64", + "@taskless/vale-linux-arm64", + "@taskless/vale-linux-x64", + "@taskless/vale-win32-arm64", + "@taskless/vale-win32-x64", +]; + +const pinnedAt = (version) => ({ + optionalDependencies: Object.fromEntries( + PLATFORMS.map((name) => [name, version]) + ), +}); + +const sourcePinnedAt = (version) => + `${JSON.stringify( + { + name: "@taskless/cli", + ...pinnedAt(version), + dependencies: { zod: "^4" }, + }, + undefined, + 2 + )}\n`; + +async function run({ + packageJson, + packageJsonSource, + published, + argv = [], + release, + wantNotes = false, +}) { + const directory = mkdtempSync(join(tmpdir(), "vale-upgrade-test-")); + const outputPath = join(directory, "github-output"); + const notesPath = join(directory, "release-notes.md"); + const packageJsonPath = join(directory, "package.json"); + if (packageJsonSource !== undefined) { + writeFileSync(packageJsonPath, packageJsonSource); + } + const previous = process.env.GITHUB_OUTPUT; + const tagsFetched = []; + process.env.GITHUB_OUTPUT = outputPath; + try { + const comparison = await main({ + argv: wantNotes ? [...argv, "--notes-out", notesPath] : argv, + latestVersion: async (name) => + typeof published === "string" ? published : published[name], + releaseFor: async (repository, tag) => { + tagsFetched.push(`${repository}@${tag}`); + return release; + }, + packageJsonPath, + packageJson, + manifest: MANIFEST, + }); + const outputs = Object.fromEntries( + readFileSync(outputPath, "utf8") + .split("\n") + .filter(Boolean) + .map((line) => { + const at = line.indexOf("="); + return [line.slice(0, at), line.slice(at + 1)]; + }) + ); + let notesWritten; + if (wantNotes) { + try { + notesWritten = readFileSync(notesPath, "utf8"); + } catch { + notesWritten = undefined; + } + } + const written = + packageJsonSource === undefined + ? undefined + : readFileSync(packageJsonPath, "utf8"); + return { comparison, outputs, notesWritten, tagsFetched, written }; + } finally { + if (previous === undefined) { + delete process.env.GITHUB_OUTPUT; + } else { + process.env.GITHUB_OUTPUT = previous; + } + rmSync(directory, { recursive: true, force: true }); + } +} + +test("a newer published set is an upgrade", async () => { + const { outputs } = await run({ + packageJson: pinnedAt("3.20.0-20260907164938"), + published: "3.21.0-20260914010203", + }); + + assert.equal(outputs.update, "true"); + assert.equal(outputs.vale_version, "3.21.0-20260914010203"); + assert.equal(outputs.pinned_version, "3.20.0-20260907164938"); + // The plain version is what the changelog and the release note talk about. + assert.equal(outputs.base_version, "3.21.0"); +}); + +test("the pins already matching what is published is a no-op", async () => { + const { outputs } = await run({ + packageJson: pinnedAt("3.20.0-20260907164938"), + published: "3.20.0-20260907164938", + }); + assert.equal(outputs.update, "false"); +}); + +/** + * Two stamps of the SAME upstream Vale version. Ordering by the base version + * alone would call this equal and never ship a republish — which is a real + * event here, since a repackaging fix keeps the upstream version and moves only + * the stamp. + */ +test("a newer stamp of the same Vale version is still an upgrade", async () => { + const { outputs } = await run({ + packageJson: pinnedAt("3.20.0-20260907164938"), + published: "3.20.0-20260908000000", + }); + assert.equal(outputs.update, "true"); + assert.equal(outputs.base_version, "3.20.0"); +}); + +/** + * The failure this script exists to refuse. release-vale.yml publishes six + * packages in a loop and can leave the set split across two versions if a + * publish fails partway; upgrading into that pins some platforms to something + * npm does not serve. + */ +test("a half-published set aborts rather than upgrading into it", async () => { + await assert.rejects( + run({ + packageJson: pinnedAt("3.20.0-20260907164938"), + published: Object.fromEntries( + PLATFORMS.map((name, index) => [ + name, + index === 0 ? "3.21.0-20260914010203" : "3.20.0-20260907164938", + ]) + ), + }), + /not at one version, so it is mid-publish or partially failed/ + ); +}); + +test("pins that disagree with each other abort", () => { + assert.throws( + () => + collectPins({ + optionalDependencies: { + "@taskless/vale-darwin-arm64": "3.20.0-20260907164938", + "@taskless/vale-linux-x64": "3.19.0-20260901000817", + }, + }), + /pins disagree/ + ); +}); + +test("an unstamped pin aborts, because it could never have been published", () => { + assert.throws( + () => + collectPins({ + optionalDependencies: { "@taskless/vale-linux-x64": "3.20.0" }, + }), + /not a stamped version/ + ); +}); + +test("--write moves every pin and nothing else", async () => { + const { written } = await run({ + packageJson: pinnedAt("3.20.0-20260907164938"), + packageJsonSource: sourcePinnedAt("3.20.0-20260907164938"), + published: "3.21.0-20260914010203", + argv: ["--write"], + }); + + assert.equal(written.match(/3\.21\.0-20260914010203/g).length, 6); + assert.doesNotMatch(written, /3\.20\.0-20260907164938/); + assert.match(written, /"zod": "\^4"/); +}); + +test("the changelog is upstream's, fetched by the BASE version's tag", async () => { + const { notesWritten, tagsFetched } = await run({ + packageJson: pinnedAt("3.20.0-20260907164938"), + published: "3.21.0-20260914010203", + release: { + tag: "v3.21.0", + notes: "## `doc(...)` selections", + url: "https://github.com/errata-ai/vale/releases/tag/v3.21.0", + }, + wantNotes: true, + }); + + // Not the stamped version, which upstream has never heard of, and not a bare + // `3.21.0`, which is not how Vale tags. + assert.deepEqual(tagsFetched, ["errata-ai/vale@v3.21.0"]); + assert.match(notesWritten, /^## Upstream release notes — 3\.21\.0$/m); + assert.match(notesWritten, /> ## `doc\(\.\.\.\)` selections/); +}); + +test("base version strips the stamp", () => { + assert.equal(baseVersion("3.21.0-20260914010203"), "3.21.0"); + assert.throws(() => baseVersion("3.21.0"), /not a stamped version/); +}); diff --git a/.github/workflows/vale-upgrade.yml b/.github/workflows/vale-upgrade.yml new file mode 100644 index 00000000..65eb550a --- /dev/null +++ b/.github/workflows/vale-upgrade.yml @@ -0,0 +1,161 @@ +# SPDX-License-Identifier: MIT +# Vale — ship the platform packages we already published. +# +# THE STAGE THAT WAS MISSING. Vendoring Vale has two halves, and until now only +# one of them was automated: +# +# republish release-vale.yml. Watches upstream Vale, proposes a manifest +# with new digests on `vendor/vale/republish`, and on merge +# publishes the six @taskless/vale-* packages at +# -. Changes nothing for a consumer. +# +# upgrade this workflow. Moves the six pins in packages/cli/package.json +# to the published set on `vendor/vale/upgrade`. THIS is what +# reaches a user. +# +# release-vale.yml states the dependency in its own header: "a new version +# reaches a user only when someone reviews a bump to that pin". Somebody always +# did — `git log` on the pins shows 3.18.0, 3.19.0 and 3.20.0 each moved in its +# own hand-written commit, and the pins are current as this is written. So this +# automates a step that was working, which is worth being honest about: what it +# removes is the dependency on remembering, and what it adds is the changelog +# next to the diff. The failure it prevents is quiet — a publish lands, nobody +# notices the pins are behind, and the release simply does not ship. +# +# ast-grep has only the upgrade half, because nothing here repackages it; there +# is no `vendor/ast-grep/republish` and its absence is the point. Beyond that +# the two upgrades are the same operation, and they share the same scripts: +# `pin-bump.cjs` rewrites the pins, `release-notes.cjs` renders the changelog, +# and `vendor-pr.cjs` owns the rolling branch and its force-push guards. What +# differs is which registry answers "what is published" and which tag carries +# the release notes. +# +# THE CHANGELOG HERE IS UPSTREAM'S, NOT OURS. Our stamp records when a package +# was built, which tells a reviewer nothing about what changed. The base version +# inside the stamp is the release upstream actually tagged, and that is what +# gets fetched. +# +# Action refs are pinned to commit SHAs; the trailing comment records the tag. + +name: Upgrade Vale + +on: + # Deliberately DAILY, where the republish detect is weekly. That one waits on + # upstream; this one waits on our own publish job, which fires whenever a + # manifest pull request merges. A weekly schedule here would mean a Vale + # release sat packaged-but-unshipped for up to a week after we published it, + # which is the exact failure this workflow exists to end. + schedule: + - cron: "52 7 * * *" + + workflow_dispatch: + +permissions: {} + +concurrency: vale-upgrade + +jobs: + upgrade: + name: Upgrade the pinned Vale + runs-on: ubuntu-latest + permissions: + contents: write # push the vendor/vale/upgrade branch + pull-requests: write # open the upgrade PR + steps: + # Credentials persist because this job pushes a branch. It holds no npm + # identity and no id-token. + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + + - uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + with: + node-version: 24 + + # No dependency install: the script is zero-dependency CommonJS, and the + # lockfile step below needs pnpm rather than node_modules. GITHUB_TOKEN is + # only for the rate limit on the release-notes lookup. + # + # `--notes-out` writes third-party Markdown to a FILE rather than a step + # output, because a $GITHUB_OUTPUT line is delimited text that a release + # body containing the delimiter can break out of. + - id: detect + run: | + node .github/scripts/vale-upgrade-detect.cjs \ + --write --notes-out "${RUNNER_TEMP}/vale-release-notes.md" + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # Resolution, not editing. `--lockfile-only` skips linking, so nothing is + # downloaded and no lifecycle script runs. It fails if the six packages + # are not all resolvable at the new version — a second, independent check + # on the half-published set the detect step already refuses. + - name: Regenerate the lockfile + if: steps.detect.outputs.update == 'true' + run: pnpm install --lockfile-only --ignore-scripts + + - name: Propose the upgrade + if: steps.detect.outputs.update == 'true' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + VALE_VERSION: ${{ steps.detect.outputs.vale_version }} + BASE_VERSION: ${{ steps.detect.outputs.base_version }} + PINNED_VERSION: ${{ steps.detect.outputs.pinned_version }} + run: | + set -euo pipefail + + # A changeset, because this is the bump a consumer experiences — the + # republish that produced the package did not change anyone's install. + # `patch` is correct while @taskless/cli is pre-1.0 even for a new Vale + # minor; see the bump guidance in CLAUDE.md before reaching for + # `minor`. The filename is stable per version, so a rolling branch + # rewrites it rather than accumulating a file per release. + changeset=".changeset/vale-${VALE_VERSION//./-}.md" + cat > "$changeset" < "$body" <> "$body" + cat "$notes" >> "$body" + else + echo "::warning::no release notes file at ${notes}" + fi + + node .github/scripts/vendor-pr.cjs \ + --branch vendor/vale/upgrade \ + --title "chore(vale): upgrade to Vale ${BASE_VERSION}" \ + --message "chore(vale): upgrade to Vale ${BASE_VERSION}" \ + --body-file "$body" \ + -- packages/cli/package.json pnpm-lock.yaml "$changeset" From 60b77cd878d57b621140e5285e92566996a78f94 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 14 Sep 2026 18:47:03 -0700 Subject: [PATCH 5/8] fix(ci): close the force-push race and name the changeset for the base version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from review, both real. vendor-pr.cjs read the branch tip's author and then force-pushed, which is a check with a gap after it: a reviewer pushing a fixup inside that gap lost the commit silently — the exact outcome the ownership guard exists to prevent, reached by timing rather than by logic. The push now leases against the commit the guards inspected, so a branch that moved underneath us rejects the push and fails the run loudly instead. The SHA is explicit rather than implied by a remote-tracking ref, since actions/checkout clones at depth 1 and there may be no tracking ref to lease against. A branch that does not exist yet has nothing to lease and takes an ordinary push. vale-upgrade.yml named the changeset after the stamped version while its own comment claimed the filename was stable per version. A republish mints a fresh timestamp for the same upstream Vale, so the name churned on every republish while the release note inside it said the same thing. It now uses the base version, which is what the title, the body, and the note itself already said. Also adds the missing test for vale-detect.cjs's "upstream has no published releases" guard. Both new tests are mutation-checked: reverting each fix makes them fail. --- .github/scripts/vale-detect.test.cjs | 16 ++++++++++++ .github/scripts/vendor-pr.cjs | 31 ++++++++++++++++++++++- .github/scripts/vendor-pr.test.cjs | 38 +++++++++++++++++++++++++++- .github/workflows/vale-upgrade.yml | 9 ++++--- 4 files changed, 89 insertions(+), 5 deletions(-) diff --git a/.github/scripts/vale-detect.test.cjs b/.github/scripts/vale-detect.test.cjs index 51d1a64f..5f34357c 100644 --- a/.github/scripts/vale-detect.test.cjs +++ b/.github/scripts/vale-detect.test.cjs @@ -249,3 +249,19 @@ test('detect: --notes-out without a path aborts rather than writing to "--write" /--notes-out needs a path/ ); }); + +/** + * A repository with no published GitHub release at all. The comparison has + * nothing to compare against, so failing loudly is the only honest answer — + * reporting "not ahead" would read as "we are current" forever. + */ +test("detect: a repository with no releases aborts rather than reporting current", async () => { + await assert.rejects( + main({ + argv: [], + latestRelease: async () => undefined, + text: async () => "", + }), + /has no published releases/ + ); +}); diff --git a/.github/scripts/vendor-pr.cjs b/.github/scripts/vendor-pr.cjs index 27b978cb..a69f115c 100644 --- a/.github/scripts/vendor-pr.cjs +++ b/.github/scripts/vendor-pr.cjs @@ -157,8 +157,13 @@ async function main({ allowFailure: true, }) !== undefined; + // The SHA the guards below are reasoning about, kept so the push can lease + // against it. See the push itself for why that matters. + let fetchedTip; + if (remoteExists) { git(["fetch", "--quiet", "origin", branch]); + fetchedTip = git(["rev-parse", "FETCH_HEAD"]); // The TIP author, not every commit since `main`. Asking "which commits are // on the branch and not on main" needs a merge base, and `actions/checkout` @@ -199,7 +204,31 @@ async function main({ } } - git(["push", "--force", "origin", branch]); + // --force-with-lease, not --force, and the distinction is the whole point of + // the guards above. + // + // Reading the tip author and then force-pushing is a check and an action with + // a gap between them. A reviewer who pushes a fixup inside that gap has their + // commit destroyed silently — precisely the outcome the ownership guard + // exists to prevent, arrived at through timing rather than through logic. A + // guard that a race defeats is not a guard. + // + // The lease closes it by making the push itself assert what the guards + // assumed: the branch is still the commit we inspected. If it is not, the + // push is REJECTED and the run fails loudly, which is a report rather than a + // loss. The SHA is explicit rather than implied by a remote-tracking ref, + // which also sidesteps the `stale info` failure a shallow clone produces + // (see the shallow-clone note in CLAUDE.md) — `actions/checkout` clones at + // depth 1, so there may be no tracking ref to lease against. + // + // A branch that does not exist yet has nothing to lease and nothing to + // overwrite, so it takes an ordinary push. Forcing there would be asserting + // a claim about a ref that is not there. + git( + remoteExists + ? ["push", `--force-with-lease=${branch}:${fetchedTip}`, "origin", branch] + : ["push", "origin", branch] + ); const existing = gh([ "pr", diff --git a/.github/scripts/vendor-pr.test.cjs b/.github/scripts/vendor-pr.test.cjs index 55f71a79..b9215aa5 100644 --- a/.github/scripts/vendor-pr.test.cjs +++ b/.github/scripts/vendor-pr.test.cjs @@ -110,7 +110,9 @@ test("no remote branch yet: pushes and opens a pull request", async () => { assert.deepEqual(result, { action: "created" }); assert.ok(ran(h.calls, "git", "checkout", "-B", "vendor/vale")); - assert.ok(ran(h.calls, "git", "push", "--force", "origin", "vendor/vale")); + // A branch that does not exist has nothing to overwrite and nothing to lease + // against, so it takes an ordinary push. + assert.ok(ran(h.calls, "git", "push", "origin", "vendor/vale")); assert.ok(ran(h.calls, "gh", "pr", "create")); assert.ok( !ran(h.calls, "git", "fetch"), @@ -123,6 +125,7 @@ test("an open pull request is retitled and rewritten, not duplicated", async () gitAnswers: { "diff --cached": undefined, "ls-remote": "abc123\trefs/heads/vendor/vale", + "rev-parse": "abc123", "log -1": BOT_AUTHOR, "diff --quiet FETCH_HEAD": undefined, // content differs }, @@ -255,3 +258,36 @@ test("the unchanged check looks only at the paths being proposed", async () => { assert.ok(compare.includes("--"), "compared whole trees, not the proposal"); assert.ok(compare.includes(".github/scripts/vale-manifest.json")); }); + +/** + * The ownership guard reads the tip author and the push happens afterwards, so + * on its own it is a check with a gap after it: a reviewer pushing a fixup + * inside that gap loses the commit silently, which is exactly the outcome the + * guard exists to prevent — reached by timing rather than by logic. + * + * The lease makes the push assert what the guard assumed. The SHA is explicit + * rather than implied by a remote-tracking ref, because `actions/checkout` + * clones at depth 1 and there may be no tracking ref to lease against. + */ +test("the push leases against the exact commit the guards inspected", async () => { + const h = harness({ + gitAnswers: { + "diff --cached": undefined, + "ls-remote": "abc123\trefs/heads/vendor/vale", + "rev-parse": "deadbeef", + "log -1": BOT_AUTHOR, + "diff --quiet FETCH_HEAD": undefined, + }, + ghAnswers: { "pr list": "" }, + }); + await main({ argv: ARGV, ...h }); + + const push = h.calls.find( + ([name, ...args]) => name === "git" && args[0] === "push" + ); + assert.ok( + push.includes("--force-with-lease=vendor/vale:deadbeef"), + `expected a lease against the fetched tip, got: ${push.join(" ")}` + ); + assert.ok(!push.includes("--force"), "an unconditional force survived"); +}); diff --git a/.github/workflows/vale-upgrade.yml b/.github/workflows/vale-upgrade.yml index 65eb550a..3efa7819 100644 --- a/.github/workflows/vale-upgrade.yml +++ b/.github/workflows/vale-upgrade.yml @@ -107,9 +107,12 @@ jobs: # republish that produced the package did not change anyone's install. # `patch` is correct while @taskless/cli is pre-1.0 even for a new Vale # minor; see the bump guidance in CLAUDE.md before reaching for - # `minor`. The filename is stable per version, so a rolling branch - # rewrites it rather than accumulating a file per release. - changeset=".changeset/vale-${VALE_VERSION//./-}.md" + # `minor`. Named for the BASE version, not the stamped one: a + # republish mints a fresh timestamp for the same upstream Vale, so a + # stamped filename would churn on every republish while the release + # note it contains says the same thing. The base version is also what + # the title, the body, and the note itself say. + changeset=".changeset/vale-${BASE_VERSION//./-}.md" cat > "$changeset" < Date: Mon, 14 Sep 2026 18:58:09 -0700 Subject: [PATCH 6/8] refactor(ci): share readNotesOut, and make bumpPins agree with its caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cleanup findings from review. readNotesOut was byte-identical in all three detect scripts. It now lives in release-notes.cjs beside the other notes helpers. All three parse the same flag for the same reason, and a partial change — one script taught a new form, two not — would leave a workflow silently writing no notes. bumpPins took a bare string prefix and matched any key starting with it, while the collectPins it is paired with used a boundary-aware pattern. Two functions meant to agree on what counts as a pin disagreed, and only the count check downstream kept that from mattering. Relying on a guard to paper over a disagreement is not the same as not having one: the guard turns it into a failed run, which beats a wrong bump and is worse than the two agreeing. bumpPins now takes the pattern, so each caller hands both functions the same constant and they cannot drift. A /g pattern is refused rather than silently accepted. Its lastIndex persists between .test() calls, so matching would alternate down the file and skip every other pin — and the caller uses that same constant for its own enumeration, where the bug would be just as quiet. The bump tests move to pin-bump.test.cjs, beside the script they cover, and gain the boundary and /g cases. Both fixes are mutation-checked: reverting either makes its tests fail. collectPins is deliberately NOT consolidated. Its two versions differ in error text on purpose — sg-detect.cjs documents why it avoids vale-release.cjs's parser, an ast-grep oddity surfacing as an error about Vale — so sharing it means a label parameter to preserve that, which buys less than it costs. --- .github/scripts/pin-bump.cjs | 43 +++++++-- .github/scripts/pin-bump.test.cjs | 116 ++++++++++++++++++++++++ .github/scripts/release-notes.cjs | 26 ++++++ .github/scripts/release-notes.test.cjs | 21 +++++ .github/scripts/sg-detect.cjs | 18 +--- .github/scripts/sg-detect.test.cjs | 47 ---------- .github/scripts/vale-detect.cjs | 14 +-- .github/scripts/vale-upgrade-detect.cjs | 25 ++--- 8 files changed, 213 insertions(+), 97 deletions(-) create mode 100644 .github/scripts/pin-bump.test.cjs diff --git a/.github/scripts/pin-bump.cjs b/.github/scripts/pin-bump.cjs index 7abde8fc..b96385aa 100644 --- a/.github/scripts/pin-bump.cjs +++ b/.github/scripts/pin-bump.cjs @@ -31,18 +31,45 @@ function escapeLiteral(text) { } /** - * @param source the package.json text - * @param prefix package-name prefix, e.g. `@taskless/vale-` or `@ast-grep/cli` - * @param from the exact version every matching pin currently holds - * @param to the exact version to write + * WHY THE CALLER PASSES A PATTERN RATHER THAN A PREFIX. + * + * This used to take a bare string prefix and match any key starting with it, + * while the `collectPins` it is paired with used a boundary-aware pattern + * (`/^@ast-grep\/cli(-|$)/`). Two functions that are supposed to agree on what + * counts as a pin disagreed on it, and only the `count !== pins.size` check + * downstream kept that from mattering. Relying on a guard to paper over a + * disagreement is not the same as not having one: the guard turns the + * disagreement into a failed run, which is better than a wrong bump and worse + * than the two agreeing in the first place. + * + * Taking the pattern means the caller hands BOTH functions the same constant, + * so they cannot drift apart at all. + * + * @param source the package.json text + * @param pattern anchored RegExp matching a package NAME, the same one the + * caller enumerates pins with + * @param from the exact version every matching pin currently holds + * @param to the exact version to write */ -function bumpPins(source, { prefix, from, to }) { - const pattern = new RegExp( - `("${escapeLiteral(prefix)}[^"]*"\\s*:\\s*")${escapeLiteral(from)}(")`, +function bumpPins(source, { pattern, from, to }) { + // A /g regexp carries `lastIndex` between calls, so `.test()` would alternate + // true and false down the file and silently skip every other pin. Refusing it + // beats stripping the flag, because a caller passing /g also uses that same + // constant for its own enumeration, where the bug would be just as quiet. + if (pattern.global) { + throw new Error( + "the pin pattern must not be /g: a stateful lastIndex would skip pins" + ); + } + const matcher = new RegExp( + `("([^"]+)"\\s*:\\s*")${escapeLiteral(from)}(")`, "g" ); let count = 0; - const bumped = source.replaceAll(pattern, (_match, head, tail) => { + const bumped = source.replaceAll(matcher, (match, head, name, tail) => { + if (!pattern.test(name)) { + return match; + } count += 1; return `${head}${to}${tail}`; }); diff --git a/.github/scripts/pin-bump.test.cjs b/.github/scripts/pin-bump.test.cjs new file mode 100644 index 00000000..1302af40 --- /dev/null +++ b/.github/scripts/pin-bump.test.cjs @@ -0,0 +1,116 @@ +// SPDX-License-Identifier: MIT +"use strict"; + +/** + * Tests for pin-bump.cjs. + * + * Two properties matter here and neither is about the happy path. The rewrite + * must not touch a package it was not asked about, and it must agree with the + * caller's own enumeration about what counts as a pin — a disagreement there + * leaves a straggler at the old version, which for optional-dependency platform + * packages means a different binary on one platform than on the others. + */ + +const test = require("node:test"); +const assert = require("node:assert/strict"); + +const { bumpPins } = require("./pin-bump.cjs"); + +/** The boundary-aware pattern sg-detect.cjs enumerates its pins with. */ +const AST_GREP = /^@ast-grep\/cli(-|$)/; + +const sourcePinnedAt = (version) => + `${JSON.stringify( + { + name: "@taskless/cli", + dependencies: { "@ast-grep/cli": version, zod: "^4.0.0" }, + optionalDependencies: { + "@ast-grep/cli-darwin-arm64": version, + "@ast-grep/cli-linux-x64-gnu": version, + }, + }, + undefined, + 2 + )}\n`; + +test("every pin moves and nothing else does", () => { + const before = sourcePinnedAt("0.45.2"); + const { source, count } = bumpPins(before, { + pattern: AST_GREP, + from: "0.45.2", + to: "0.45.3", + }); + + assert.equal(count, 3); + assert.equal(source.match(/0\.45\.3/g).length, 3); + assert.doesNotMatch(source, /0\.45\.2/); + assert.match(source, /"zod": "\^4\.0\.0"/); + // The formatting is untouched, so the diff a reviewer reads is the versions. + assert.equal(before.split("\n").length, source.split("\n").length); +}); + +/** + * The dots in a version are regular-expression metacharacters. Unescaped, + * `0.45.2` also matches `0145.2`. + */ +test("the version is matched literally, not as a pattern", () => { + const { count } = bumpPins('{ "@ast-grep/cli": "0145.2" }', { + pattern: AST_GREP, + from: "0.45.2", + to: "0.45.3", + }); + assert.equal(count, 0); +}); + +test("the same version under an unrelated package is left alone", () => { + const { source, count } = bumpPins( + '{ "some-other-tool": "0.45.2", "@ast-grep/cli": "0.45.2" }', + { pattern: AST_GREP, from: "0.45.2", to: "0.45.3" } + ); + assert.equal(count, 1); + assert.match(source, /"some-other-tool": "0\.45\.2"/); +}); + +/** + * The reason the caller passes a pattern rather than a prefix. A bare prefix + * match treats `@ast-grep/clippy` as a pin because the string starts the same + * way, while the `collectPins` it is paired with — anchored on `(-|$)` — does + * not. The two would then disagree about what a pin is, and only the caller's + * count check would notice. + */ +test("a package that merely starts with the prefix is not a pin", () => { + const { source, count } = bumpPins( + '{ "@ast-grep/clippy": "0.45.2", "@ast-grep/cli-darwin-arm64": "0.45.2" }', + { pattern: AST_GREP, from: "0.45.2", to: "0.45.3" } + ); + assert.equal(count, 1); + assert.match(source, /"@ast-grep\/clippy": "0\.45\.2"/); + assert.match(source, /"@ast-grep\/cli-darwin-arm64": "0\.45\.3"/); +}); + +test("the Vale prefix carries its boundary in the pattern", () => { + const pattern = /^@taskless\/vale-/; + const { count } = bumpPins( + '{ "@taskless/vale-linux-x64": "3.20.0-20260907164938", "@taskless/valet": "3.20.0-20260907164938" }', + { pattern, from: "3.20.0-20260907164938", to: "3.21.0-20260914010203" } + ); + assert.equal(count, 1); +}); + +/** + * A /g regexp carries `lastIndex` between calls, so `.test()` alternates true + * and false down the file and silently skips every other pin. Refusing it beats + * stripping the flag: the caller uses that same constant for its own + * enumeration, where the bug would be just as quiet. + */ +test("a stateful /g pattern is refused rather than silently skipping pins", () => { + assert.throws( + () => + bumpPins(sourcePinnedAt("0.45.2"), { + pattern: /^@ast-grep\/cli(-|$)/g, + from: "0.45.2", + to: "0.45.3", + }), + /must not be \/g/ + ); +}); diff --git a/.github/scripts/release-notes.cjs b/.github/scripts/release-notes.cjs index 50034f83..e7493486 100644 --- a/.github/scripts/release-notes.cjs +++ b/.github/scripts/release-notes.cjs @@ -147,6 +147,31 @@ function formatReleaseNotes({ return `${heading}\n${quoted}\n${footer}`; } +/** + * Read `--notes-out ` from an argv array, or undefined when absent. + * + * Lives here rather than in each detect script because all three parse the same + * flag for the same reason, and a change to how it is parsed — accepting + * `--notes-out=path`, say — should not be a change three files have to make in + * agreement. A partial fix would leave one workflow silently writing nothing. + * + * A following value that looks like another flag is an error rather than a + * path. `--notes-out --write` is a caller that forgot the argument, and taking + * `--write` as a filename would write release notes to a file named `--write` + * and drop the flag that was meant to do the work. + */ +function readNotesOut(argv) { + const at = argv.indexOf("--notes-out"); + if (at === -1) { + return undefined; + } + const path = argv[at + 1]; + if (!path || path.startsWith("--")) { + throw new Error("--notes-out needs a path"); + } + return path; +} + /** Write a rendered section to disk for a workflow to pass to `--body-file`. */ function writeNotesFile(path, section) { writeFileSync(path, section.endsWith("\n") ? section : `${section}\n`); @@ -157,5 +182,6 @@ module.exports = { fetchLatestRelease, fetchReleaseByTag, formatReleaseNotes, + readNotesOut, writeNotesFile, }; diff --git a/.github/scripts/release-notes.test.cjs b/.github/scripts/release-notes.test.cjs index 3b7b5d66..af19af13 100644 --- a/.github/scripts/release-notes.test.cjs +++ b/.github/scripts/release-notes.test.cjs @@ -21,6 +21,7 @@ const { NOTES_LIMIT, fetchLatestRelease, formatReleaseNotes, + readNotesOut, writeNotesFile, } = require("./release-notes.cjs"); @@ -173,3 +174,23 @@ test("notes: the written file always ends in a newline", () => { rmSync(directory, { recursive: true, force: true }); } }); + +/** + * Parsed here rather than in each detect script. All three parse the same flag + * for the same reason, and a partial fix — one script taught a new form, two + * not — would leave a workflow silently writing no notes. + */ +test("notes: --notes-out yields its path, and its absence yields nothing", () => { + assert.equal( + readNotesOut(["--write", "--notes-out", "/tmp/n.md"]), + "/tmp/n.md" + ); + assert.equal(readNotesOut(["--write"]), undefined); +}); + +test("notes: a flag where the path should be is a mistake, not a filename", () => { + // `--notes-out --write` would otherwise write to a file named `--write` and + // drop the flag meant to do the work. + assert.throws(() => readNotesOut(["--notes-out", "--write"]), /needs a path/); + assert.throws(() => readNotesOut(["--notes-out"]), /needs a path/); +}); diff --git a/.github/scripts/sg-detect.cjs b/.github/scripts/sg-detect.cjs index 4b2d4bcc..784914b4 100644 --- a/.github/scripts/sg-detect.cjs +++ b/.github/scripts/sg-detect.cjs @@ -88,6 +88,7 @@ const { bumpPins } = require("./pin-bump.cjs"); const { fetchReleaseByTag, formatReleaseNotes, + readNotesOut, writeNotesFile, } = require("./release-notes.cjs"); @@ -235,19 +236,6 @@ async function fetchLatestVersion(packageName) { return latest; } -/** `--notes-out `, or undefined when the flag is absent. */ -function readNotesOut(argv) { - const at = argv.indexOf("--notes-out"); - if (at === -1) { - return undefined; - } - const path = argv[at + 1]; - if (!path || path.startsWith("--")) { - throw new Error("--notes-out needs a path"); - } - return path; -} - async function main({ argv = process.argv.slice(2), latestVersion = fetchLatestVersion, @@ -289,8 +277,10 @@ async function main({ if (write && ahead) { const pins = collectPins(packageJson); const source = readFileSync(packageJsonPath, "utf8"); + // The same constant collectPins enumerates with, so the two cannot + // disagree about what counts as a pin. const { source: bumped, count } = bumpPins(source, { - prefix: "@ast-grep/cli", + pattern: PIN_PATTERN, from: pinned, to: upstream, }); diff --git a/.github/scripts/sg-detect.test.cjs b/.github/scripts/sg-detect.test.cjs index defa03d6..7f3b4d6c 100644 --- a/.github/scripts/sg-detect.test.cjs +++ b/.github/scripts/sg-detect.test.cjs @@ -18,7 +18,6 @@ const { tmpdir } = require("node:os"); const { join } = require("node:path"); const { collectPinnedVersion, isAhead, main } = require("./sg-detect.cjs"); -const { bumpPins } = require("./pin-bump.cjs"); const CLI_PACKAGE_JSON = JSON.parse( readFileSync( @@ -289,52 +288,6 @@ const sourcePinnedAt = (version) => 2 )}\n`; -test("bump: every pin moves and nothing else does", () => { - const before = sourcePinnedAt("0.45.2"); - const { source, count } = bumpPins(before, { - prefix: "@ast-grep/cli", - from: "0.45.2", - to: "0.45.3", - }); - - assert.equal(count, 3); - assert.equal(source.match(/0\.45\.3/g).length, 3); - assert.doesNotMatch(source, /0\.45\.2/); - // The unrelated dependency is untouched, and so is the formatting: the diff - // a reviewer reads is three version strings. - assert.match(source, /"zod": "\^4\.0\.0"/); - assert.equal( - before.split("\n").length, - source.split("\n").length, - "the rewrite reflowed the file" - ); -}); - -/** - * The dots in a version are regular-expression metacharacters. Unescaped, - * `0.45.2` also matches `0X45Y2` — and, far more plausibly, a pin at `0145.2`. - */ -test("bump: the version is matched literally, not as a pattern", () => { - const source = '{ "@ast-grep/cli": "0145.2" }'; - const { count } = bumpPins(source, { - prefix: "@ast-grep/cli", - from: "0.45.2", - to: "0.45.3", - }); - assert.equal(count, 0); -}); - -test("bump: a version appearing outside an @ast-grep pin is left alone", () => { - const source = '{ "some-other-tool": "0.45.2", "@ast-grep/cli": "0.45.2" }'; - const { source: bumped, count } = bumpPins(source, { - prefix: "@ast-grep/cli", - from: "0.45.2", - to: "0.45.3", - }); - assert.equal(count, 1); - assert.match(bumped, /"some-other-tool": "0\.45\.2"/); -}); - test("sg-detect: --write bumps every pin in the file on disk", async () => { const { packageJsonWritten, outputs } = await runDetect({ upstream: "0.45.3", diff --git a/.github/scripts/vale-detect.cjs b/.github/scripts/vale-detect.cjs index 9916c1b2..72f76f0d 100644 --- a/.github/scripts/vale-detect.cjs +++ b/.github/scripts/vale-detect.cjs @@ -62,6 +62,7 @@ const { join } = require("node:path"); const { fetchLatestRelease, formatReleaseNotes, + readNotesOut, writeNotesFile, } = require("./release-notes.cjs"); const { @@ -90,19 +91,6 @@ async function fetchText(url) { return response.text(); } -/** `--notes-out `, or undefined when the flag is absent. */ -function readNotesOut(argv) { - const at = argv.indexOf("--notes-out"); - if (at === -1) { - return undefined; - } - const path = argv[at + 1]; - if (!path || path.startsWith("--")) { - throw new Error("--notes-out needs a path"); - } - return path; -} - async function main({ argv = process.argv.slice(2), latestRelease = fetchLatestRelease, diff --git a/.github/scripts/vale-upgrade-detect.cjs b/.github/scripts/vale-upgrade-detect.cjs index 6afd9035..b0049ca8 100644 --- a/.github/scripts/vale-upgrade-detect.cjs +++ b/.github/scripts/vale-upgrade-detect.cjs @@ -59,6 +59,7 @@ const { bumpPins } = require("./pin-bump.cjs"); const { fetchReleaseByTag, formatReleaseNotes, + readNotesOut, writeNotesFile, } = require("./release-notes.cjs"); const { @@ -80,6 +81,13 @@ const MANIFEST_PATH = join(__dirname, "vale-manifest.json"); const PIN_PREFIX = "@taskless/vale-"; +/** + * What counts as a platform pin. Used BOTH to enumerate the pins and to rewrite + * them, so the two cannot drift apart — the trailing hyphen is the boundary + * here, since there is no bare `@taskless/vale` package. + */ +const PIN_PATTERN = /^@taskless\/vale-/; + const REGISTRY = "https://registry.npmjs.org"; function setOutput(key, value) { @@ -89,19 +97,6 @@ function setOutput(key, value) { } } -/** `--notes-out `, or undefined when the flag is absent. */ -function readNotesOut(argv) { - const at = argv.indexOf("--notes-out"); - if (at === -1) { - return undefined; - } - const path = argv[at + 1]; - if (!path || path.startsWith("--")) { - throw new Error("--notes-out needs a path"); - } - return path; -} - /** * The one stamped version every `@taskless/vale-*` pin holds. * @@ -119,7 +114,7 @@ function collectPins(packageJson) { "optionalDependencies", ]) { for (const [name, range] of Object.entries(packageJson[field] ?? {})) { - if (name.startsWith(PIN_PREFIX)) { + if (PIN_PATTERN.test(name)) { pins.set(name, range); } } @@ -238,7 +233,7 @@ async function main({ if (write && ahead) { const source = readFileSync(packageJsonPath, "utf8"); const { source: bumped, count } = bumpPins(source, { - prefix: PIN_PREFIX, + pattern: PIN_PATTERN, from: pinned, to: upstream, }); From a0fbd38c82d437849e086088dcddaaeda692306a Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 14 Sep 2026 19:03:26 -0700 Subject: [PATCH 7/8] ci: bring the new vendor workflows onto the node24 pins Reconciles with ecb1075, f3432ed and ea9cef8, which landed on main while this branch was open. The two new workflows were written against the pins that were current when the branch was cut, so after the rebase they were the only two files left on actions/checkout v4, setup-node v4, pnpm/action-setup v6 and a literal node-version: 24. Nothing would have failed. That is the point: a stale pin does not go red, it just quietly runs an older action, so this is exactly the kind of drift a rebase carries in without reporting it. Both jobs check out the repository, so they read the version from .nvmrc like everything else. The one remaining literal is release-vale.yml's publish step, which deliberately has no checkout for the file to be read from and documents that in place. Every workflow now resolves each action to a single SHA. --- .github/workflows/ast-grep-upgrade.yml | 8 ++++---- .github/workflows/vale-upgrade.yml | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ast-grep-upgrade.yml b/.github/workflows/ast-grep-upgrade.yml index 8b43e7b4..d2f02bc0 100644 --- a/.github/workflows/ast-grep-upgrade.yml +++ b/.github/workflows/ast-grep-upgrade.yml @@ -80,12 +80,12 @@ jobs: steps: # Credentials persist because this job pushes a branch. It holds no npm # identity and no id-token. - - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6 - - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + - uses: pnpm/action-setup@ea17c68df8912ef543352723c149a84f56e3d413 # v6.1.0 + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: - node-version: 24 + node-version-file: .nvmrc # No dependency install: sg-detect.cjs is zero-dependency CommonJS, and # the lockfile step below needs pnpm rather than node_modules. diff --git a/.github/workflows/vale-upgrade.yml b/.github/workflows/vale-upgrade.yml index 3efa7819..01bb915e 100644 --- a/.github/workflows/vale-upgrade.yml +++ b/.github/workflows/vale-upgrade.yml @@ -64,12 +64,12 @@ jobs: steps: # Credentials persist because this job pushes a branch. It holds no npm # identity and no id-token. - - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6 - - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + - uses: pnpm/action-setup@ea17c68df8912ef543352723c149a84f56e3d413 # v6.1.0 + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: - node-version: 24 + node-version-file: .nvmrc # No dependency install: the script is zero-dependency CommonJS, and the # lockfile step below needs pnpm rather than node_modules. GITHUB_TOKEN is From 9fce3b0256c4d6e553511bfcf327ce6eb5250b1e Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 14 Sep 2026 19:14:12 -0700 Subject: [PATCH 8/8] docs(ci): correct the ast-grep changeset comment's justification The conclusion was right and the reason was not. A version-carrying filename is there so two different bumps never share a release note; accumulation is prevented by vendor-pr.cjs rebuilding the branch from main every run, which leaves one changeset file on it whatever the name is. The Vale side had the same false justification and was corrected when its filename was fixed; this makes the two agree. --- .github/workflows/ast-grep-upgrade.yml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ast-grep-upgrade.yml b/.github/workflows/ast-grep-upgrade.yml index d2f02bc0..bc661f56 100644 --- a/.github/workflows/ast-grep-upgrade.yml +++ b/.github/workflows/ast-grep-upgrade.yml @@ -127,8 +127,11 @@ jobs: # and reach nobody until a pin like this one moves. `patch` is correct # while @taskless/cli is pre-1.0 even though the diff adds no surface # of our own; see the bump guidance in CLAUDE.md before reaching for - # `minor`. The filename is stable per version, so a rolling branch - # rewrites it rather than accumulating one file per release. + # `minor`. The filename carries the version so that two different + # bumps never share a release note. Accumulation is not what stops + # here — `vendor-pr.cjs` rebuilds the branch from `main` every run, + # so only ever one changeset file exists on it regardless of the + # name. changeset=".changeset/ast-grep-${SG_VERSION//./-}.md" cat > "$changeset" <