feat(ci): automate the vendored toolchain upgrades, with changelogs - #332
Conversation
|
@claude /review - make sure we don't break OIDC and check that we have cleaned up these workflows so vale/sg upgrades are similar and separated from our vale binary releases (which are important, but are a different problem) |
|
Claude finished @thecodedrift's task in 4m 32s —— View job Review mode: incremental — read 0 prior review thread(s) before reviewing.
Your two specific asksOIDC is not broken. Verified directly against The separation looks clean. [New] Confirmed bug: changeset filename uses the stamped version, not the base version
changeset=".changeset/vale-${VALE_VERSION//./-}.md"The adjacent comment claims "The filename is stable per version, so a rolling branch rewrites it rather than accumulating a file per release." But [New]
|
Verified all five findings against the code rather than taking them as read. Two were real and are fixed in 92d5755: Force-push race — fixed, and it was the important one. The guard read the tip author and the push happened afterwards, so it was a check with a gap after it. The push now leases against the exact commit the guards inspected ( Changeset filename — fixed. Now Missing Two left open, deliberately:
Both are cleanup with no correctness risk today, and I have left them for the repo owner to call rather than growing a PR already past the size guideline. — AI Coding Agent |
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.
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-<version> 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.
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/<anything> impossible to create ever after.
Vendoring Vale has two halves and only one was automated. release-vale.yml watches upstream and republishes the six @taskless/vale-* packages at <valeVersion>-<stamp>; 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.
…e version 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.
…ller 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.
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.
fd43e93 to
a0fbd38
Compare
|
@claude /review Second pass requested. Everything below landed after the first review, and the branch was rebased onto Fixed from the first review:
Cleanup, two of the three raised:
Post-rebase reconcile:
Worth a second look specifically at the lease semantics in — AI Coding Agent |
|
Claude finished @thecodedrift's task in 4m 23s —— View job Review mode: incremental — read 0 prior review thread(s) before reviewing.No Treated as already addressed (not re-raised):
Specifically requested: lease semantics in
|
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.
Thanks — both re-checks confirmed, and the lease trace matches what I intended: Cosmetic point 1 — fixed in 9fce3b0. You are right that the comment's justification was off, and it is the same falsehood I corrected on the Vale side without carrying the fix across. The conclusion stands (a version-carrying filename keeps two different bumps from sharing a release note); the stated reason did not, since Cosmetic point 2 — accurate, and deliberately worded that way. The real-git lifecycle run is manual pre-merge verification, listed under "Verification" in the PR body rather than claimed as suite coverage. That said, the manual run earned its keep: it is what caught the unchanged-check comparing whole trees, where an unrelated commit on — AI Coding Agent |
1 participant
Two vendored toolchains, three stages, one shared lifecycle — and a changelog on every proposal.
What was missing
vendor/vale/republish, changelog in the bodyvendor/vale/upgrade, automatedvendor/ast-grep/upgrade, automatedsg-detect.cjscould 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. ast-grep 0.45.3 duly sat unpinned with nothing reporting it.The Vale gap is subtler.
release-vale.ymlpublishes@taskless/vale-*at<valeVersion>-<stamp>, which changes nothing for a consumer — the CLI pins each package exactly. Its own header says a release reaches a user "only when someone reviews a bump to that pin," andgit logshows somebody always did, by hand, three times. So this automates a step that was working. What it removes is the dependency on remembering.The publish path is untouched
Checked rather than assumed, because npm trusted publishing binds to specific values:
release-vale.ymlis modified, not renamed.gate,prepare, andpublishjobs are byte-identical tomain. Every edit is indetect.name:,environment: npm-autopublish, and the singleid-token: writeare unchanged.id-token, anenvironment, a token, ornpm publish. They holdcontents: write+pull-requests: write.The detect branch name changed (
vale/update-<version>→vendor/vale/republish). That is not part of the binding: publish fires on a push tomainfiltered by path, so it triggers on the merge regardless.Rolling branches, and the two guards that make them safe
One branch per stage, rebuilt from
mainand retitled as upstream moves, rather than a branch per version. Previously a second upstream release opened a second PR proposing a conflicting edit to the same lines.Rebuilding a branch someone may be reviewing is the hazard, so
vendor-pr.cjsforce-pushes only when:Both are mutation-tested. I broke each and confirmed a test fails.
Two bugs my own tests caught
Truncation was budgeted against the wrong length. The 40k limit measured the source, but quoting adds two characters per line — ast-grep's changelog is a long list of short lines, so a 40,000-character body rendered as 80,202 and would have failed the API call outright rather than truncating. Now measured against rendered output, cut on a line boundary.
The unchanged-guard compared whole trees. An unrelated commit on
mainmade the trees differ, so an upstream that had not moved still force-pushed and rewrote the PR over somebody else's edit to a different file. Now scoped to the paths being proposed.Notable decisions
Release notes travel as a file, never a step output. They are third-party Markdown, and a
$GITHUB_OUTPUTline is delimited text that a body containing the delimiter can break out of. They reachghvia--body-fileand never meet a shell or a${{ }}expression. Every line is blockquoted so an unterminated upstream code fence cannot swallow the body around it.Pins are rewritten in the source text, not via JSON round-trip. Re-serializing reformats a prettier-formatted file in CI, where no
lint-stagedruns to normalize it back, burying the versions in a whole-file diff. The rewrite returns its count and the caller fails on a mismatch: these platform packages are selected by optional dependency, so a straggler at the old version is a different binary on one platform than the others.The Vale upgrade reads all six packages from npm, not one. The publish loop can leave the set split across two versions, and that is precisely the state exact pins cannot tolerate. A split set fails the run.
vendor/ast-grep/upgrade, notvendor/ast-grep. Git refs are paths: a branch at the shorter name is a file there and makesvendor/ast-grep/<anything>impossible to create ever after.Size
~2300 lines across 14 files, past the ~1200 guideline. It splits into four independently-safe units and I offered to stack it; the call was one PR. The four commits map to those units and are the intended reading order:
release-notes,pin-bump,vendor-pr)Roughly half is tests (425 passing) and about a third is comments.
No changeset
CI-only; nothing consumer-visible ships. The generated PRs each carry their own.
Verification
pnpm lint,pnpm typecheckgreen.vendor-pr.cjsdriven through its full lifecycle against a real git repo with a bare origin: first proposal → same version again →mainmoves → upstream moves → reviewer pushes. All five behave; the reviewer's commit survives.--writetrialled on the realpackages/cli/package.json: 8 pins, 8 insertions / 8 deletions,prettier --checkclean, lockfile resolved. Reverted — this PR does not contain the 0.45.3 bump; the workflow proposes it once merged, which doubles as the end-to-end proof.