Address review feedback on the SDK bump automation - #103
Conversation
Three follow-ups from the review of #101. The stale-CDN check dropped whole lines, so a line carrying both a bumped and a stale reference hid the stale one; it now matches with -o so each reference is judged on its own, and excludes the current version with -F so the dots in the version stay literal. A dispatched version was taken at face value, surfacing a typo only as an npm 404 well into the bump. It is now confirmed against the registry in the check step. An older version stays allowed on purpose, as that is how a bad release gets rolled back. A draft opened by a failing e2e run told nobody what to do with it, while every later scheduled run skipped that version for good. The draft body now spells out the three recovery paths. Claude-Session: https://claude.ai/code/session_0117hJP3TxNWJxrg4YPMNNiS
sashamilenkovic
left a comment
sashamilenkovic
left a comment
There was a problem hiding this comment.
The grep fix and the registry check are both correct; I verified them against the real tree and npm. Approving, with a few things worth a look, most important first.
Things to consider
1. "Close, delete the branch, re-dispatch" does not work (update-nutrient-sdk.yml:184). check-nutrient-update.sh lists PRs with --state all, so the closed draft still counts as "already exists" and the re-run exits with should_update=false. Confirmed: gh pr list --state all --head update-examples-1.20.0 still returns merged #100. Options two and three therefore have the same outcome.
Suggested wording: close and reopen this PR. A human reopening fires pull_request: reopened as that human, so Biome and Playwright run on the PR itself. Marking a draft ready does not trigger workflows, so this is also the way to get CI attached in the first option. If you'd rather make dispatch itself retry, list only open PRs when a version was passed explicitly. Caveat: all fourteen old update-examples-* branches still exist on origin, so a re-dispatch would then hit the "branch exists with no PR" check instead.
2. Rollback rationale doesn't hold (check-nutrient-update.sh:31). Every prior version has a merged update-examples-X PR, so the existing-PR check refuses all of them. ./scripts/check-nutrient-update.sh 1.17.0 prints "a pull request already exists", not "accepted" as the description says. Soften the comment or drop the rollback claim.
3. Version exclusion is a substring match (update-nutrient-sdk.yml:75). 1.20.1 would hide a stale 1.20.10. No SDK release has ever had a patch above 1, so purely theoretical; anchoring with grep -vE "pspdfkit-web@${VERSION//./\\.}\$" closes it if you want.
4. Registry error conflates an outage with a 404 (check-nutrient-update.sh:37). curl's stderr shows the real cause, so cosmetic. -w '%{http_code}' would make it precise.
Verified
- Registry: nonexistent / older / current version returns 404 / 200 / 200; script exits 1 only on the first.
- CDN grep at
1.20.0flagsnuxtjs1.3.0 andgatsbyjs1.8.0, exemptssalesforce/README.md; both are in the updater map. - PR body renders correctly for success and failure with the YAML indent stripped; all variables substituted.
Outside this PR: the workflow has never run in CI, so a workflow_dispatch after merge would be the first real test.
The recovery section told a human to close the draft, delete the branch and re-dispatch. That does not work: the existing-pull-request check lists with --state all, so the closed draft still reads as "already handled" and the re-run exits with should_update=false. It now says to close and reopen the pull request instead, which fires pull_request: reopened as that human and so does run Biome and Playwright against the bump. Marking a draft ready for review is not an activity type either workflow acts on, so it attaches no checks; the section now says so rather than implying otherwise. For the same reason, the comment claiming a dispatched older version is the rollback path was wrong: every released version already has an update-examples-X pull request, so the check refuses all of them. The comment now records why no ordering check is needed at all. Two smaller ones: the stale-CDN exclusion matched the version as a substring, so 1.20.1 would have hidden a stale 1.20.10; it now escapes the dots and anchors on $. And the registry check dropped curl's -f so that a 404 and an unreachable registry no longer report identically. Claude-Session: https://claude.ai/code/session_0117hJP3TxNWJxrg4YPMNNiS
|
All four are in 7ebd6b3. 1. You're right that the closed draft still blocks the re-dispatch, and that reopening is the only way to get checks onto it. The section now offers close-and-reopen, push-a-fix-to-the-branch, or close-and-leave-it, and says explicitly that marking a draft ready for review triggers nothing and that re-dispatching for the same version is refused. I left 2. Confirmed — #96 is merged for 3. Anchored. Reproduced it first: at 4. Dropped |
2 participants
⚡ TL;DR
Follow-ups from the review of #101, then from the review of this PR. The stale-CDN guard had two blind spots, a hand-typed version was never checked against the registry, and a draft opened by a failing e2e run first told nobody what to do with it, then told them something that does not work.
🎯 What this improves
1.20.1no longer hides a stale1.20.10.workflow_dispatchversion fails in the first step, naming the version and the registry's status code, instead of as an npm 404 twenty minutes into the bump. A registry outage now reads differently from a version that does not exist.🔧 What changed
update-nutrient-sdk.yml, stale-CDN check.grep -rEn ... | grep -v "pspdfkit-web@${VERSION}"printed one line per matching line, and the exclusion then discarded that whole line, so a line holding bothpspdfkit-web@1.21.0andpspdfkit-web@1.8.0was excluded on the strength of the fresh reference.-omakes each reference its own output line. The exclusion also matched the version as a substring, so1.20.1would have hidden a stale1.20.10; it now escapes the dots in${VERSION}and anchors on$.update-nutrient-sdk.yml, draft PR body. When the e2e suite fails the body gains a "This draft needs a human" section, because no checks are attached to the draft and every later scheduled run skips the version as "already handled". Its instructions now match what GitHub actually does: close and reopen the pull request to get Biome and Playwright to run against the bump, push a fix to the branch if one is needed, or close it and walk away. It also states that marking a draft ready for review triggers nothing, and that re-dispatching the workflow for the same version is refused.check-nutrient-update.sh. A dispatched version is now confirmed to exist on the registry before anything else runs.-fis deliberately absent so a 404 and an unreachable registry stay distinguishable, and the message carries the status code.🤔 Why
check-nutrient-update.shlists pull requests with--state all, so a closed draft still reads as "already handled" and a re-run exits withshould_update=false. Reopening firespull_request: reopenedas the human who did it, and that is a default activity type for bothbiome.ymlandplaywright.yml.ready_for_reviewis not, so marking the draft ready attaches nothing.update-examples-Xpull request, and the existing-PR check refuses those whatever state they are in. The comment now records the real reason no ordering check is needed — that check already covers it.pnpm audit fix. Fix remaining pnpm 11 audit handling #102 already fixes the invalid invocation on that line, so touching it here would only conflict.🧪 How to test
The registry check, and what an old version really does:
Both grep blind spots, reproduced directly:
✅ Verification
bash -non the script and on all ten workflowrunblockscheck-nutrient-update.shwith1.17.99/1.17.0/ no version(HTTP 404)/ refused, a pull request for that branch exists / dist-tag path unchangedregistry.npmjs.invalid)curlexits 6, reported as "could not reach", not as unpublishedVERSION=1.20.0nuxtjs1.3.0 andgatsbyjs1.8.0, exemptssalesforce/README.mdVERSION=1.20.1over a fixture tree1.20.10and a stale1.9.0sharing a line with1.20.1; keeps1.20.1outbiome ci .workflow_dispatchafter merge is its first real test, as noted in review.update-nutrient-in-examples.sh, now tracked as Derive the bumped example list instead of hard-coding it #104. It coversupdate-nutrient-in-cdn.jstoo, since the hand-maintained map there has the identical failure mode.https://claude.ai/code/session_0117hJP3TxNWJxrg4YPMNNiS