Skip to content

feat(release): commit-driven auto-generated changesets - #1753

Merged
james-elicx merged 13 commits into
mainfrom
claude/distracted-taussig-5766a2
Jun 5, 2026
Merged

feat(release): commit-driven auto-generated changesets#1753
james-elicx merged 13 commits into
mainfrom
claude/distracted-taussig-5766a2

Conversation

@james-elicx

@james-elicx james-elicx commented Jun 5, 2026

Copy link
Copy Markdown
Member

What & why

Replaces the manual workflow_dispatch (human-picks-the-bump) release with a commit-driven Changesets flow in one workflow. Changesets handles as much as possible; the only custom code is a script that auto-creates changesets from Conventional Commits (plus a small contributors-list step).

Auto-generated changeset files never live on main — written to the CI working tree, consumed by changesets/action, discarded. Manually authored changesets still work.

One workflow: release.yml

On push to main:

  1. create-changeset.mjs writes .changeset/auto-*.md to the working tree.
  2. changesets/action does everything else — maintains the rolling "Version Packages" PR while changesets exist, and when none remain (the PR just merged) it publishes via changeset publish with OIDC trusted publishing + provenance (id-token: write, NPM_CONFIG_PROVENANCE), and creates the git tag + GitHub Release.

The guard (why one workflow is enough)

create-changeset.mjs regenerates the unreleased set from <lastTag>..HEAD each run, but skips when package.json version > the latest tag (release merged, awaiting publish). That empty working tree is exactly what makes changesets/action switch from "maintain PR" to "publish" — so no separate publish workflow or guard job is needed.

Custom surface (2 scripts)

  • scripts/create-changeset.mjs — Conventional Commit → bump (feat→minor, fix/perf/revert→patch, !/BREAKING CHANGE→major, else skip), changed paths → affected package(s), aggregate to highest bump. Multi-package ready (independent versions).
  • scripts/version.mjschangeset version, then appends a deduped ## Contributors list to each bumped package's CHANGELOG.md (idempotent; same content flows to the GitHub Release).

Removed

  • publish.yml entirely (guard job, manual bump, manual OIDC/tag/GitHub-release/Google-Chat steps, AI-generated notes) — changesets/action now owns publish + tags + releases.
  • The two-workflow split and the duplicate guard job.

Migration note

packages/vinext/package.json version 0.0.5 → 0.0.55 (was stale; versions were driven by npm view). Changesets makes package.json the source of truth.

Tag format changes from v0.0.x (old manual scheme) to vinext@0.0.x (changeset publish default). The guard reads both, so old and new tags coexist.

Testing

  • ✅ Unit tests for both scripts: 32/32.
  • vp check (format + lint + types) clean.

Needs validation in a live CI run (cannot be tested locally)

  • OIDC + changeset publish: trusted publishing + provenance work through changeset publish/pnpm publish without a token (highest-risk item — the old flow used vp pm publish --provenance directly).
  • The auto-*.md changeset does not leak into the Version PR or main (created → consumed → gone within the action step).
  • changeset version's CHANGELOG format matches what insertContributors expects, so the ## Contributors block lands correctly.
  • gh api contributor resolution authenticates with the passed GITHUB_TOKEN (degrades gracefully otherwise).

🤖 Generated with Claude Code

Adopt stock Changesets for versioning/changelog/publishing, with the only
bespoke surface being a script that creates changesets automatically from
Conventional Commits. Auto-generated changeset files never live on `main` —
they are written to the CI working tree, consumed by changesets/action into a
rolling Version PR, and discarded. Manually authored changesets still work.

- scripts/create-changeset.mjs: derive per-package bumps from Conventional
  Commits (paths -> package, type -> bump). Includes the version-vs-tag guard
  so a merged Version PR publishes instead of re-opening a PR.
- scripts/version.mjs: `changeset version` + append a `## Contributors` list to
  each bumped package's CHANGELOG.md (idempotent, pure rewrite unit-tested).
- .github/workflows/release-pr.yml: version-only changesets/action (no publish).
- .github/workflows/publish.yml: guarded OIDC publish (preserves
  `vp pm publish --provenance`); version now comes from `changeset version`.
- Reconcile packages/vinext version (0.0.5 -> 0.0.55) so the guard is coherent.

Unit tests for both scripts pass (35). The release orchestration (Version PR
creation, OIDC publish, gh contributor resolution) can only be validated in a
live CI run.
Comment thread .github/workflows/release-pr.yml Fixed
@pkg-pr-new

pkg-pr-new Bot commented Jun 5, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1753

commit: 56ac310

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

Let changesets/action own as much as possible. Delete the separate publish.yml
(guard job, manual bump, manual OIDC/tag/release/notify steps) and the
release-pr.yml split. One workflow now:

- create-changeset.mjs writes auto changesets to the working tree (its
  version-vs-tag guard yields nothing right after a Version PR merges).
- changesets/action maintains the Version PR and, when no changesets remain,
  publishes via `changeset publish` with OIDC trusted publishing + provenance,
  and creates the git tag + GitHub Release.

Removes the unused release:version script. Contributors list still handled by
scripts/version.mjs as the action's version command.
Comment on lines +37 to +39
- uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
with:
fetch-depth: 0 # full history + tags for the guard and contributor list

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changesets/action pushes the Version PR branch, tags, and GitHub Releases using credentials persisted by actions/checkout, so persist-credentials: false would break it. The # zizmor: ignore[artipacked] above the step suppresses this; the job uploads no artifacts, so the credential-in-artifact leak vector does not apply.

Replace the .mjs + hand-written .d.mts declaration pairs with real TypeScript
(.mts) source. Node >=24 (the setup default) runs .mts directly via native type
stripping; .mts is unambiguously ESM so it needs no "type": "module" and emits
no MODULE_TYPELESS warning. The cross-import uses an explicit .mts specifier
(Node requires it), permitted in tsc via allowImportingTsExtensions (safe: the
project is noEmit).

- scripts/create-changeset.{mjs,d.mts} -> scripts/create-changeset.mts
- scripts/version.{mjs,d.mts}          -> scripts/version.mts
- tsconfig: allowImportingTsExtensions
- release.yml: run node scripts/*.mts

vp check (format + lint + types) clean; 35/35 unit tests pass.
Cut comment bloat, remove dead code, tighten without dropping behavior:
- Condense verbose JSDoc/@PARAM blocks to one-line purpose comments; keep the
  load-bearing "why" (correctness-rule header, insertContributors idempotency).
- Delete unused `newestChangelogVersion` (+ its tests) — exported/tested but
  never called.
- Tighten run()/insertContributors/latestTagVersion without behavior change.

Multi-package machinery and the bottom `## Contributors` list are retained per
requirements. vp check clean; 33 unit tests pass.
Comment thread scripts/create-changeset.mts Outdated
Comment thread scripts/create-changeset.mts Outdated
Comment thread scripts/create-changeset.mts Outdated
The default changesets changelog groups by bump level (### Minor Changes) and
renders our changeset summary as a nested bullet dump. Replace it with a real
conventional-commits changelog: version.mts now regroups each release's commits
into ### Features / ### Bug Fixes / ### Performance sections (scope bolded, type
prefix dropped) and rewrites the newest CHANGELOG section, then appends the
## Contributors list with [bot] accounts filtered out.

- create-changeset.mts: extract conventionalParts + collectReleaseCommits +
  releaseRangeStart and export the commit-walk so version.mts reuses it (no
  duplicate git logic).
- version.mts: groupedChangelogBody + rewriteReleaseSection (idempotent: only
  `## <digit>` is a section boundary); dedupeSortLogins drops `[bot]` logins.
- Tests updated for the new pure builders.

vp check clean; 33 unit tests pass. Verified end-to-end with a local dry run.
- discoverPublishablePackages: scan only packages/* (publishable packages don't
  live elsewhere), drop the apps/examples/benchmarks scan.
- Delete isReleaseCommit: the "chore: version packages" release commit is a
  `chore`, already excluded by parseBumpFromSubject returning null. No need to
  hardcode release-message formats.

vp check clean; 31 unit tests pass.
Per PR review: the guard was confusing. Document inline that it only suppresses
the post-merge/pre-publish window (version bumped, tag not yet created), which
would otherwise re-open a Version PR instead of publishing. Keeping the
never-on-main design per review decision.
Within each type section (### Features / ### Bug Fixes / …), areas (commit
scopes) with more than 3 items now get their own `#### <Area>` sub-group with
the scope prefix dropped; smaller areas and scopeless commits fall under
`#### Other`. When no area qualifies the list stays flat. Area names are
humanized (app-router → "App Router", css → "CSS", ppr → "PPR", i18n → "i18n")
in headings and in the bold prefixes.

Also: groupedChangelogBody now renders only known release types (GROUPS) — it
no longer leaks non-release types into an "Other Changes" bucket.

vp check clean; 32 unit tests pass. Verified with a local dry run.
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/bigbonk review — PR #1753

Solid, well-tested refactor. The two scripts are clean, the pure logic is properly unit-tested, and the single-workflow design is a nice simplification over the old publish.yml. vp check is clean and the script unit tests pass locally.

A few things worth addressing before merge — one of them (the no-tag bootstrap window) is a real correctness risk for the first release cycle.


High — the version-vs-tag guard fails when no tag exists yet

The guard in decideGeneration is the linchpin of the "never on main" design, but it only works once a release tag exists:

// scripts/create-changeset.mts:135
if (tagVersion == null) return { action: "generate", reason: "no release tag yet" };

Today the repo has zero git tags (git tag -l is empty) but vinext is already published at 0.0.55 on npm (npm view vinext version0.0.55). Walk the first release cycle through:

  1. First push to main → no tags → latestTagVersion returns nullreleaseRangeStart falls back to firstCommit() and scans all history, generating a changeset listing the entire backlog. (I reproduced this: node scripts/create-changeset.mts on this branch generated a changeset with every conventional commit since the first commit.) That gives a minor bump → 0.0.56. Probably tolerable as a one-time backfill, but worth being aware of.
  2. Version PR merges → package.json is now 0.0.56, but changeset publish has not run yet on that same push (it runs in the next changesets/action invocation, which is the very push being processed).
  3. On that post-merge push, the guard checks 0.0.56 vs latestTagVersion. Because no vinext@* or v* tag exists yet, tagVersion == nullgenerate, not skip. It then regenerates from firstCommit() again and re-opens a Version PR instead of letting publish run — exactly the failure the guard exists to prevent.

The guard's whole premise ("right after a Version PR merges, package.json is bumped but the tag isn't created yet") describes a window where tagVersion can legitimately be null (the first publish hasn't tagged anything). Treating null as unconditional generate defeats the guard in precisely that window.

Options:

  • Seed a tag before enabling the workflow (e.g. create vinext@0.0.55 to match the published version), and document that bootstrapping requires an initial tag. The migration note already changes the tag scheme; this should be called out as a required setup step.
  • Or make the guard fall back to the published npm version (or the package.json of the last release commit) when no tag exists, so the post-merge/pre-first-publish window is still suppressed.

At minimum, the PR's "first run" behavior should be validated against the no-tag state the repo is actually in, since the guard's unit tests only cover the tagVersion != null cases.


Medium — stale release-pr.yml / .mjs references in docs and comments

Several references point at files that don't exist on this branch (the workflow is release.yml and the scripts are .mts):

  • .changeset/README.md:16scripts/create-changeset.mjs
  • .changeset/README.md:17.github/workflows/release-pr.yml
  • .github/workflows/release.yml:4create-changeset.mjs
  • .github/workflows/release.yml:12scripts/version.mjs

These will mislead anyone debugging the release flow. Suggest a sweep to .mts / release.yml.


Low — version.mts git() leaks fatal: noise to CI logs

create-changeset.mts deliberately pipes stderr so expected probe failures (missing tag) don't spam logs:

// scripts/create-changeset.mts:165
stdio: ["ignore", "pipe", "pipe"],

But version.mts:166-168 uses the default stdio, so the same probe-style calls (e.g. tagRefForrev-parse --verify on a missing scoped tag, reached via collectReleaseCommits/releaseRangeStart) will print fatal: lines. Worth aligning the two git() helpers, or extracting a shared one in create-changeset.mts and importing it.


Low — resolveContributors makes one gh api call per commit

scripts/version.mts:192-208 loops over every SHA in the release range and issues a separate gh api repos/.../commits/<sha> call. For a release with many commits that's N serial round-trips and N units against the API rate limit. The GitHub "list commits" / "compare" / associated-PR APIs can return author logins in far fewer calls. Not blocking (it degrades gracefully on failure), but worth a follow-up if release ranges get large.


Nit — test count

The PR body says "Unit tests for both scripts: 35/35", but vp test run scripts/create-changeset.test.ts scripts/version.test.ts reports 32 passing here. Minor, just update the description (or the count drifted as tests were trimmed during review).


Nothing here blocks the design, which is good. The one I'd really want resolved before this goes live is the no-tag bootstrap behavior, since the repo is currently in exactly that state and the first release cycle is the one most likely to misfire.

@ask-bonk

ask-bonk Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #1753. Summary of what I found:

High — no-tag bootstrap window breaks the guard. The repo currently has zero git tags but vinext is published at 0.0.55 on npm. The guard's decideGeneration treats tagVersion == null as unconditional generate, which defeats the guard in exactly the post-merge/pre-first-publish window it's meant to protect. I reproduced the "scan all history" behavior by running the script on the branch. The first release cycle is the most likely to misfire.

Medium — stale references. .changeset/README.md and release.yml comments still point to release-pr.yml and .mjs files that don't exist (workflow is release.yml, scripts are .mts).

Low — version.mts git() doesn't pipe stderr like create-changeset.mts does, so missing-tag probes will leak fatal: noise into CI logs.

Low — resolveContributors issues one gh api call per commit (N serial round-trips / rate-limit units).

Nit — PR says 35 tests, actual is 32.

The design itself is sound and well-tested; the no-tag bootstrap is the one I'd want resolved before going live.

github run

- Fix stale references to deleted files: .mjs → .mts and release-pr.yml →
  release.yml in .changeset/README.md and the release.yml header comment.
- resolveContributors: one paginated `gh api compare` call instead of one
  `gh api commits/<sha>` per commit (N round-trips → 1). This also removes
  version.mts's local git() helper, so no more `fatal:` probe noise from it.

No behavior change to the changelog/contributor output (verified via dry run:
same 4 contributors resolved). vp check clean; 32 unit tests pass.

The bot's "no git tags" bootstrap concern does not apply: tags v0.0.10..v0.0.55
exist and CI checks out with fetch-depth: 0, so the guard resolves 0.0.55.
@james-elicx

Copy link
Copy Markdown
Member Author

Thanks for the review — addressed:

High — no-tag bootstrap: This does not apply to the real repo. The premise ("git tag -l is empty") is false here — tags v0.0.10v0.0.55 exist, and release.yml checks out with fetch-depth: 0, which fetches tags. So latestTagVersion("vinext") resolves 0.0.55, and on the post-merge push the guard sees 0.0.56 > 0.0.55 → skip → publish. No deadlock and no all-history backfill. The bot appears to have run against a shallow/tagless clone. The tagVersion == null branch is only reachable for a genuinely never-released package. No change made; happy to add an npm-version fallback as defense-in-depth if preferred.

Medium — stale .mjs / release-pr.yml refs: Fixed in .changeset/README.md and the release.yml header comment (→ .mts / release.yml).

Low — git() fatal: noise: Resolved. The noisy tagRefFor probe already lived in create-changeset.mts (which pipes stderr) since version.mts imports it; and switching contributor resolution off git log (below) removed the local git() helper in version.mts entirely.

Low — one gh api call per commit: resolveContributors now makes a single paginated gh api repos/…/compare/<from>...HEAD call instead of N per-SHA calls. Verified via dry run — same contributors resolved.

Nit — test count: PR body updated 35 → 32 (count drifted as tests were trimmed during review).

vp check clean; 32 unit tests pass. Pushed.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/bigbonk review — PR #1753

I re-reviewed independently, focusing on the disputed "no-tag bootstrap" finding plus a fresh pass over what the workflow consolidation actually removed. Net: the design is sound and well-tested, the prior High finding is not a real-repo issue, but the workflow collapse dropped a pre-publish CI gate that's worth a deliberate decision, and there are two reproducibility nits.


Resolved — the "no-tag bootstrap" High finding does not apply to this repo

I verified the author's rebuttal directly rather than taking it on faith:

  • git ls-remote --tags origin shows v0.0.6 … v0.0.55 (50 tags); npm view vinext version0.0.55.
  • release.yml checks out with fetch-depth: 0, which fetches tags.
  • With tags present, releaseRangeStart("vinext") returns v0.0.55, not firstCommit(). Running node scripts/create-changeset.mts then diffs v0.0.55..HEAD and emits only the unreleased commits — no all-history backfill.
  • The guard transitions correctly through the publish window: post-merge package.json 0.0.56 > tag 0.0.55 → skip → publish; after vinext@0.0.56 is tagged, 0.0.56 == 0.0.56 → generate. I checked the mixed legacy-v / scoped-vinext@ sort and tagRefFor fallback (vinext@0.0.55 absent → falls back to v0.0.55) — both correct.

The earlier reproduction ("scans all history") only happens in a tagless/shallow checkout, which is exactly the state of the GitHub Action runner, not the real release workflow. The previous reviewer ran against that artifact. No change needed for correctness here. (A latestTagVersion → npm version fallback would still be cheap defense-in-depth if a future runner ever fetches shallowly, but it's optional.)

The stale-ref, git() stderr, and N-call contributor fixes from the prior round all check out in the current tree.


Medium — the consolidated release.yml dropped the pre-publish CI gate

The deleted publish.yml gated publishing behind a real CI run:

ci:
  needs: guard
  uses: ./.github/workflows/ci.yml   # Check + Vitest + Playwright
publish:
  needs: [guard, ci]

The new release.yml has no Check/Vitest/Playwright gate and no explicit vp run build step before changeset publish. The package ships files: ["dist"] with no committed dist, so the build only happens via the prepack script (cp README + vp run build) that pnpm publish fires.

Two things to weigh:

  1. No test gate before publish. Branch protection means code on main already passed CI at merge time, but publish runs on a separate push (the Version PR merge) and never re-runs tests. If main is ever red at the moment a Version PR merges, this publishes a broken build. The old flow explicitly guarded against that. If that's an acceptable tradeoff (rely on merge-time CI + provenance), fine — but it should be a conscious decision, not a silent drop.
  2. Build relies entirely on prepack firing under changeset publish. This is already on your "needs live-CI validation" list; calling it out as load-bearing: if prepack doesn't run (wrong cwd, PM quirk), you publish an empty dist. Worth a post-publish smoke check (e.g. npm pack --dry-run / install the tarball) the first time.

Low — @changesets/cli is pinned as a devDependency but invoked via vp dlx (unpinned)

package.json adds "@changesets/cli": "2.31.0" and a "changeset": "changeset" script, but the CI paths bypass it:

  • release.yml:50publish: vp dlx @changesets/cli publish
  • version.mts:206execFileSync("vp", ["dlx", "@changesets/cli", "version"])

vp dlx resolves the latest published @changesets/cli at run time, so CI runs a different (floating) version than the one pinned in the lockfile and than .changeset/config.json's @changesets/config@3.1.1 schema. That's an avoidable reproducibility gap and a wasted download when the dep is already installed. Prefer the installed binary (vp exec changeset version / pnpm exec changeset publish, or pnpm changeset …) so the pinned 2.31.0 is what actually runs in both local and CI.


Nit — dead branch in affectedPackages

discoverPublishablePackages only ever returns packages/* keys, so the dir === "." / prefix === "" branch (create-changeset.mts:97-98) is unreachable for the real caller. Harmless (it's an exported pure util with tests), just noting it as no-longer-needed since the apps/examples scan was removed.


Confirmed locally: 32/32 script tests pass; vp check clean on the four script files. The publish-path items (CI gate, prepack build, OIDC) are the ones that can only be truly proven in the first live run — the Medium is the one I'd want an explicit decision on before merge.

github run

@ask-bonk

ask-bonk Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #1753.

Summary

I verified the disputed finding directly and did a fresh pass over the workflow consolidation.

Resolved — the "no-tag bootstrap" High finding is NOT a real-repo issue. The author's rebuttal is correct, confirmed by direct testing:

  • git ls-remote --tags origin shows 50 tags (v0.0.6v0.0.55); release.yml uses fetch-depth: 0 which fetches them.
  • With tags present, releaseRangeStart("vinext") returns v0.0.55 (not firstCommit()), so it diffs only unreleased commits — no all-history backfill.
  • The guard transitions correctly through the publish window, including the mixed legacy-v/scoped-vinext@ tag sort and tagRefFor fallback.

The earlier "scans all history" reproduction only happens in a tagless/shallow checkout (the CI runner's state), not the real release workflow. Prior stale-ref/stderr/contributor fixes also check out.

New findings from my own pass:

  • Medium — The consolidated release.yml dropped the pre-publish CI gate that the deleted publish.yml had (needs: [guard, ci] ran Check + Vitest + Playwright before publish). Publish now runs on the Version PR merge push with no test gate and relies entirely on prepack for the build. Worth a deliberate decision, not a silent drop.
  • Low@changesets/cli@2.31.0 is pinned as a devDependency, but CI invokes it via vp dlx (unpinned, floating latest), creating a reproducibility gap vs the lockfile and the config schema. Should use the installed binary.
  • Nit — dead dir === "." branch in affectedPackages now that the apps/examples scan was removed.

Verified locally: 32/32 script tests pass, vp check clean.

github run

@james-elicx
james-elicx marked this pull request as ready for review June 5, 2026 13:18
@james-elicx james-elicx changed the title feat(release): commit-driven changesets with auto-generated changesets feat(release): commit-driven auto-generated changesets Jun 5, 2026
…anch

Per second bonk review:
- Invoke the pinned, installed @changesets/cli via `vp exec changeset` instead
  of `vp dlx @changesets/cli` (which fetches a floating latest at run time) — in
  release.yml publish and version.mts. Removes the reproducibility gap vs the
  lockfile-pinned 2.31.0.
- affectedPackages: remove the unreachable `dir === "."` / empty-prefix branch
  now that discovery only returns `packages/*` keys.

vp check clean; 32 unit tests pass.
@james-elicx

Copy link
Copy Markdown
Member Author

Thanks for the independent re-review and confirming the bootstrap finding was an artifact of the tagless runner. Addressed the new pass:

Medium — dropped pre-publish CI gate (deliberate decision): Keeping the single-workflow design and relying on the Version PR's required checks rather than re-adding an explicit gate. Rationale: publish runs on the merged Version PR, whose only diff is package.json + CHANGELOG.md (no source), and the repo's active main ruleset requires the CI check before merge — so the code that publishes is exactly the code that passed CI on that PR. Re-adding needs: ci to release.yml would re-run the full suite (incl. Playwright) on every push to main, duplicating ci.yml's own push-to-main run. This is now a conscious tradeoff, not a silent drop. (The prepack-driven build remains on the first-live-run validation list — will sanity-check the published tarball's dist on the first real release.)

Low — floating @changesets/cli: Fixed. Both call sites now use the pinned, installed binary via vp exec changeset … (verified → 2.31.0) instead of vp dlx @changesets/cli (floating latest), in release.yml publish and version.mts.

Nit — dead affectedPackages branch: Removed the unreachable dir === "." / empty-prefix branch now that discovery only returns packages/* keys.

vp check clean; 32 unit tests pass. Pushed.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk re-review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/bigbonk re-review — PR #1753

Re-reviewed the current tree (f751d93) after the last round of fixes. Verified prior findings are resolved and did a fresh pass. The design is sound and well-tested; nothing here blocks merge.

Verified resolved

  • @changesets/cli now runs pinned, not floating. Both call sites use vp exec changeset (release.yml:51, version.mts:207). I confirmed vp exec changeset --version2.31.0 after a full vp install. (The binary is absent in a partial checkout, but .github/actions/setup runs run-install: true, so CI has it.)
  • Stale .mjs / release-pr.yml refs fixed in .changeset/README.md and the release.yml header.
  • git() stderr noise goneversion.mts no longer defines its own git(); it reuses the stderr-piping helpers from create-changeset.mts.
  • Contributor resolution is one paginated gh api compare call (version.mts:181-198), not N per-SHA.
  • Dead affectedPackages / dir === "." branch removed.
  • No-tag bootstrap is a non-issue for the real repo. git ls-remote --tags origin shows 50 legacy v0.0.6…v0.0.55 tags; release.yml checks out fetch-depth: 0, so latestTagVersion("vinext") resolves 0.0.55 and the guard works. The tagless reproduction is only the CI-runner artifact.
  • Test count: 32/32 confirmed locally; PR body now matches. vp check clean on all four script files.

New (non-blocking) observations

Low — contributor name fallback produces malformed mentions. version.mts:190 uses .commits[].author.login // .commit.author.name. When a commit's GitHub author is null (unlinked email), the fallback is the raw git display name (with spaces), which dedupeSortLogins then renders as - @Full Name in the Contributors list — a broken @-mention. Degrades gracefully (no crash), but consider dropping non-login fallbacks (or only keeping entries matching /^[a-zA-Z0-9-]+$/) so the list contains valid handles only.

Low — vp dlx @changesets/cli still suggested in docs. .changeset/README.md:25 recommends vp dlx @changesets/cli for manually authoring changesets. This is the same floating-version pattern that was fixed in CI; since the dep is now pinned and installed, prefer suggesting pnpm changeset / vp exec changeset for consistency. Docs-only, harmless.

Note — publish runs without an explicit pre-publish test gate (acknowledged tradeoff). release.yml publishes on the Version-PR-merge push and relies on (a) the merge-time required checks on that PR and (b) ci.yml's own push-to-main run, which fires concurrently in a separate concurrency group. There's no ordering guarantee between that CI run and publish, but since the Version PR's only diff is package.json + CHANGELOG.md (no source), the published code is byte-identical to what passed CI at merge. Reasonable and documented; flagging only so it stays a conscious decision. The prepack-driven dist build remains the right thing to smoke-check on the first live release (already on the author's validation list).

Nothing here needs to block. The items that can only be proven in the first live run (OIDC publish, prepack build producing a non-empty dist, auto-*.md consumed-then-discarded) are correctly called out in the PR body.

@ask-bonk

ask-bonk Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #1753.

Summary

I re-reviewed the current tree (f751d93) and confirmed all prior-round findings are resolved:

  • @changesets/cli pinned — both release.yml:51 and version.mts:207 use vp exec changeset; verified it resolves to 2.31.0 after a full install (CI's setup action runs run-install: true).
  • Stale .mjs/release-pr.yml refs fixed, git() stderr noise gone (shared helper), contributors now one paginated gh api compare call, dead affectedPackages branch removed.
  • No-tag bootstrap is confirmed a non-issuegit ls-remote --tags origin shows 50 v* tags and release.yml fetches them with fetch-depth: 0.
  • 32/32 tests pass, vp check clean, PR body count now matches.

New non-blocking notes:

  • Lowversion.mts:190 .commit.author.name fallback can render - @Full Name (broken mention) for unlinked commit authors; suggest filtering to valid-login shape.
  • Low.changeset/README.md:25 still suggests floating vp dlx @changesets/cli for manual authoring.
  • Note — publish has no explicit pre-publish test gate (relies on merge-time checks + concurrent ci.yml push run); reasonable since the Version PR diff is metadata-only, flagged to keep it a conscious decision.

Nothing blocks merge. The OIDC publish, prepack build, and auto-*.md lifecycle are correctly listed as first-live-run validation items.

github run

Per third bonk review:
- resolveContributors: `.author.login // empty` (was `// .commit.author.name`),
  so commits with an unlinked email contribute no entry instead of a raw git
  display name. dedupeSortLogins now keeps only `[a-zA-Z0-9-]+` shapes, which
  also subsumes the previous `[bot]` filter — no more broken `- @Full Name`
  mentions.
- .changeset/README.md: suggest `vp exec changeset` instead of the floating
  `vp dlx @changesets/cli` for manual changeset authoring.

vp check clean; 32 unit tests pass; dry run resolves the same valid handles.
@james-elicx

Copy link
Copy Markdown
Member Author

Thanks — addressed both new Low items:

Malformed @-mentions from the name fallback: resolveContributors now uses .commits[] | .author.login // empty (dropping the .commit.author.name fallback), so a commit with an unlinked email contributes no entry rather than a spaced git display name. As defense-in-depth, dedupeSortLogins now keeps only ^[a-zA-Z0-9-]+$ shapes — which also subsumes the previous [bot] filter. No more - @Full Name. Dry run still resolves the same valid handles.

Floating vp dlx in docs: .changeset/README.md now suggests vp exec changeset (pinned/installed) instead of vp dlx @changesets/cli for manual authoring.

Pre-publish gate: acknowledged and kept as a conscious decision — relying on the Version PR's merge-time required checks (its diff is metadata-only, so the published code is identical to what passed CI). The prepack dist build stays on the first-live-run validation list.

vp check clean; 32 unit tests pass. Pushed.

@james-elicx
james-elicx enabled auto-merge (squash) June 5, 2026 13:29
@james-elicx
james-elicx merged commit 61d5fe0 into main Jun 5, 2026
39 checks passed
@james-elicx
james-elicx deleted the claude/distracted-taussig-5766a2 branch June 5, 2026 13:34
@github-actions github-actions Bot mentioned this pull request Jun 5, 2026
james-elicx added a commit that referenced this pull request Jun 5, 2026
#1760)

changesets/action builds the GitHub Release body via getChangelogEntry(), which
slices from the `## <version>` heading to the next *same-depth* (`##`) heading.
A `## Contributors` (h2) was acting as that boundary and getting excluded from
the release notes — present in CHANGELOG.md but dropped from the GitHub Release.
Demote to `### Contributors` (h3) so it stays inside the extracted body. The
in-file boundary logic (/^##\s+\d/) is unaffected.

Follow-up to #1753 (merged before this fix landed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants