Repository navigation
chore(release): scripts/release.sh - #171
Conversation
…ish, verify and tag Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @krassx's task in 1m 5s —— View job Review of PR #171:
|
… pinned dispatch commit Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Claude finished @krassx's task in 1m 8s —— View job Re-review of PR #171 after
|
| # | Finding | Status |
|---|---|---|
| 1 | P1: bootstrap publish could not take 2FA | Fixed. --interactive inherits stdio and the prompt text is accurate. The cost is the new P3 below. |
| 2 | P2: resume skipped the trust attach | Fixed. The release-trust-pending marker in the git dir is kept until npm-trust-all.mjs --only succeeds. |
| 3 | P2: dispatched commit could differ from the tagged commit | Mostly fixed. release.sh:49-50 checks origin/main == $sha right before dispatch, and the run lookup filters on event and --commit. A window of a few milliseconds remains because --ref main is still a branch name. That is not worth more code. |
| 4 | P3: tail steps not resumable | Fixed. The tag, push and GitHub release are each skipped when already done. Still open: nothing stops a prerelease version like 0.1.0-beta.5 from going out under the default --tag latest. Given beta.4 was the motivation, a guard that dies when $version contains - and TAG=latest would be cheap. |
| 5 | P3: --help range and npm whoami |
Fixed. The help range is 2,17p, and npm whoami now only runs when a bootstrap is needed. |
New finding (1 inline comment):
- P3,
--interactiveloses the "accepted but not readable yet" skip (scripts/publish-all.mjs:105). Withstdio: 'inherit'the thrown error has nostderr, so the "cannot publish over the previously published versions" branch can never match. A re-run shortly after an interrupted bootstrap hits E403 on the already-accepted package and aborts until the registry catches up.
Overall risk: Low. This is release tooling, and the earlier P1 and P2 issues are resolved.
Merge recommendation: Merge. Optionally add the prerelease-under-latest guard and handle the P3 above in this PR or a follow-up.
Most important remaining items:
- The prerelease-under-
latestguard inrelease.sh. - The interactive publish error handling in
publish-all.mjs.
Positives:
- The marker-file approach for resuming the trust step is simple and survives crashes.
- The registry wait loops tolerate the 202-then-readable delay.
- Every tail step is idempotent.
--onlyon both helper scripts keeps the bootstrap narrow.
…-but-unreadable package Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Claude finished @krassx's task in 1m 14s —— View job Re-review of PR #171 after
|
| Finding | Status |
|---|---|
| Bootstrap publish couldn't take 2FA | Fixed. --interactive inherits stdio. |
| Resume skipped the trust attach | Fixed. The release-trust-pending marker is removed only after npm-trust-all.mjs --only succeeds. |
| Dispatched commit could differ from the tagged commit | Fixed. origin/main is re-checked before each dispatch, and the run is matched by event and --commit. |
| Tail steps not resumable | Fixed. The tag, push and GitHub release are each skipped when already done. |
--help range and npm whoami |
Fixed. |
--interactive loses the "accepted but not readable" skip |
Acknowledged. It can't be captured with an inherited terminal. The new message in b63a55e1 tells the user to wait and re-run, which is enough. |
Prerelease version under --tag latest |
Still open, P3. Nothing stops 0.1.0-beta.5 from going out under the default latest. A guard that dies when $version contains - and TAG=latest would be cheap. |
New finding (1 inline comment)
- P3, lost failure message (
scripts/release.sh:62). Ifgh run view --log-faileditself exits non-zero,set -eandpipefailstop the script beforedieruns, so "Release workflow run … failed (dry_run=…)" is never printed. Appending|| trueto the log dump fixes it.
Overall risk: Low. This is release tooling. I found no new correctness or security issues beyond the P3 above.
Merge recommendation: Merge. Optionally add the prerelease-under-latest guard and the || true before merging.
Most important remaining items
- The prerelease-under-
latestguard. || trueon the--log-faileddump.
Positives
- The dispatch is pinned to the preflight commit and refused if
mainmoves. - The run is matched by event and commit, so another person's run is never watched.
- The marker file keeps the trust step resumable, and every tail step is idempotent.
- Registry waits tolerate the 202-then-readable delay.
--onlyon both helper scripts keeps the bootstrap narrow.
… prints Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Claude finished @krassx's task in 25s —— View job Re-review of PR #171 after
|
| Finding | Status |
|---|---|
| Bootstrap publish couldn't take 2FA | Fixed. --interactive inherits stdio. |
| Resume skipped the trust attach | Fixed. The release-trust-pending marker is removed only after npm-trust-all.mjs --only succeeds. |
| Dispatched commit could differ from the tagged commit | Fixed. origin/main is re-checked before each dispatch, and the run is matched by event and --commit. |
| Tail steps not resumable | Fixed. The tag, push and GitHub release are each skipped when already done. |
--help range and npm whoami |
Fixed. |
| Interactive publish loses the "accepted but not readable" skip | Acknowledged. The new message tells the user to wait and re-run. |
Lost failure message from --log-failed |
Fixed in 5fecae6c (2>&1 | tail -30 || true, release.sh:62). die now always runs. |
Prerelease version under --tag latest |
Open, P3. It is optional, and I am not re-posting it inline. release.yml also defaults tag to latest, so publishing betas under latest may be intentional while no stable release exists. If it isn't, a guard that dies when $version contains - and TAG=latest (unless --yes) is a few lines. |
New findings: none that I can support with a concrete failure scenario.
Overall risk: Low. This is release tooling only, and every earlier correctness issue is resolved.
Merge recommendation: Merge. The prerelease-under-latest guard is optional.
Most important remaining item
- The optional prerelease-under-
latestguard inrelease.sh.
Positives
- Preflight is thorough: branch, clean tree,
origin/mainmatch, CI on the exact commit, and the publishable gate. - The dispatch is pinned to the preflight commit and refused if
mainmoves. - The marker file keeps the trust step resumable.
- Every tail step is idempotent.
- Registry waits tolerate the 202-then-readable delay.
--onlyon both helper scripts keeps the bootstrap narrow.
One script for the release steps: preflight (main, clean, CI green), first-publish of packages never on npm (the only step needing npm auth/2FA), Release workflow dry run then real, registry verification, git tag and GitHub release. Resumable. Motivated by beta.4, where the new
@bugsee/trackerscould not be first-published through OIDC.🤖 Generated with Claude Code