chore(release): link each CHANGELOG entry to its compare range - #211
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 8m 41s —— View job Code review — PR #211Review mode: incremental — read 0 prior review thread(s) before reviewing.
Findings posted inline
None of these are hypothetical edge cases invented for the sake of finding something — they're gaps in the specific new logic this PR introduces, and (1)/(2) affect whether the CI gate can be trusted as a signal.
Conventions — checked against root Not investigated: per-file history/blame (unavailable — no per-file |
8250bea to
29f5e3a
Compare
All six findings answered on their threads. Two were real defects and are fixed in #212; four were declined with reasoning recorded rather than silently dropped. Why this review saw the workflow at all is worth stating, since it was not this PR's doing. The branch was cut from #210's branch rather than from Fixed in #212: the posted-review scan had no author filter, so any comment carrying the run id and the marker satisfied it — the exact failure the step exists to catch, masked by someone talking about it. And a present-but-unparseable execution file was no longer diagnosed at all. Declined: the two duplication notes ( Worth noting this was the first review to run under the new guard, and it went green while finding two genuine holes in that same guard. That is roughly the outcome the change was aiming for. — AI Coding Agent |
Two gaps in the guard that landed in #210, both found by review on #211. The comment scan had no author filter, so ANY comment carrying this run's id and the `Review mode:` marker satisfied it: a quote of the job URL, a paste of this workflow, an acknowledgement citing the review it answers. That is exactly the case the step exists to catch — the action posted nothing — being masked by someone talking about it. It now filters on `.user.type == "Bot"` before grepping. Filtering on type rather than on a login means an app rename does not fail every review closed, while the actual threat, a human comment, is excluded. Verified against the record: the dead run on #210 still reports not-posted, the real review on #211 still reports posted, and four human comments on #210 are now excluded from consideration. An execution file that exists but holds no result record was also no longer diagnosed. It used to be a hard error; once `posted` became the verdict it passed through as `turns=None is_error=None cost=None` with nothing naming the corruption, so a partial write or an action version skew would vanish rather than be noticed. It now warns, and deliberately does not fail, since it says nothing about whether a review was posted.
Changesets writes `## <version>` and the notes beneath it, and nothing about which commits a release actually contained. Reviewing "what shipped in 0.11.0" meant finding the version-bump commit by hand, which is harder than it sounds: every one of them is called `chore: version packages`, so the subject line cannot tell them apart and you have to read the version out of `package.json` at each candidate. `scripts/changelog-compare-links.ts` adds a `vPREV...vNEXT` compare link under each heading, and runs from `pnpm bump` — which is what the Version Packages workflow invokes — so the link lands in the same PR as the notes it describes. The tag it points at does not exist yet at that moment; `release-cli.yml` creates it after publishing, so the newest link is dead for the minutes in between and correct forever after. Releases before 0.9.0 were never tagged and will not be, so a link whose left side is `v0.8.1` would be permanently broken rather than briefly so. A documented `FIRST_TAGGED` floor leaves those headings alone; 0.9.0 itself gets no link for the same reason. Backfilled 0.10.0, 0.10.1 and 0.10.2, whose tags were just created. Both `v0.10.1...v0.10.2` and `v0.9.0...v0.10.0` return HTTP 200. Re-running is a no-op, and a simulated 0.11.0 heading takes exactly one new link.
29f5e3a to
9c8f0b3
Compare
Companion to #210, which adds the tagging this depends on. Independent of it: nothing breaks if they land in either order, the links are simply dead until the tags exist, and the four historical tags are already pushed.
Why
Changesets writes
## <version>and the notes beneath it, and nothing about which commits a release actually contained. Reviewing "what shipped in 0.11.0" meant finding the version-bump commit by hand.That is harder than it sounds: every version-bump commit is called
chore: version packages, so the subject cannot distinguish them and you have to readpackages/cli/package.jsonat each candidate to find the one you want. That is exactly how the four historical tags were located for #210.What
scripts/changelog-compare-links.tsinserts avPREV...vNEXTcompare link under each version heading. It runs frompnpm bump, which is whatrelease-cli-changeset.ymlinvokes (version: pnpm bump), so the link lands in the Version Packages PR alongside the notes it describes.The transform is a pure exported function over the markdown plus a repository URL, so it can be exercised without touching the file. The URL comes from
packages/cli/package.json's ownrepository.urlrather than being hardcoded.The newest link is briefly dead, on purpose
At the moment
pnpm bumpruns, the tag does not exist:release-cli.ymlcreates it after the publish. So the newest entry's link is broken for the minutes between the Version PR merging and the release completing, and correct forever after. That is the accepted trade rather than an oversight, and it is written into the script.Old entries get no link at all
Releases before
0.9.0were never tagged and will not be, so a link whose left side isv0.8.1would be permanently broken rather than briefly so. A documentedFIRST_TAGGED = "0.9.0"floor leaves those headings alone.0.9.0itself gets no link for the same reason: its predecessor is the untagged0.8.1.Briefly-dead is fine. Permanently-dead is a different thing, and worse than absent.
Verification
0.10.0,0.10.1,0.10.20.9.0and older get nonev0.10.1...v0.10.2andv0.9.0...v0.10.0both return HTTP 200## 0.11.0heading tookv0.10.2...v0.11.0, exactly one link added, existing links untouchedInsertion runs back-to-front over the heading list so an insertion never shifts an index still to be used, and idempotency is decided by the first non-empty line after a heading rather than by scanning the whole document.
Also
release-cli-changeset.ymlcarries a comment enumerating whatpnpm bumpdoes. Updated, since it would otherwise have gone stale the moment this landed.Notes
packages/cli/CHANGELOG.mdchanges here are the backfilled links, not a release note.