Conversation
Make CI the sole publisher of @livetemplate/client, triggered only by the explicit GitHub release event so ordinary pushes never publish. Removes the need for a local npm login (or pasted NPM_TOKEN) when running release.sh. publish.yml now uses npm OIDC trusted publishing (id-token: write) and npm publish --provenance, so no NPM_TOKEN secret is required and the package gets a verified provenance attestation on npmjs.com. The checkout step pins to the release tag to guarantee the published artifact matches what was tagged, and a version-match guard catches releases pointing at a mismatched package.json. release.sh no longer runs npm publish locally — it only bumps the version, runs tests/build as a pre-flight gate, commits, tags, pushes, and creates the GitHub release. The release event then triggers publish.yml. Output text and dry-run summary updated to reflect that npm publish is async. Requires (one-time, already done): trusted publisher configured for @livetemplate/client on npmjs.com pointing at this repo + publish.yml. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Review: ci: publish to npm via OIDC trusted publishing Overall this is a solid improvement — moving from a long-lived Looks good:
One thing to confirm: The version-match guard strips a leading Potential gap in the verify step:
for i in 1 2 3; do
npm view "@livetemplate/client@$PKG_VERSION" && break
echo "Attempt $i failed, retrying in 30s..."
sleep 30
doneThis is non-blocking for merging, but worth a follow-up issue to avoid spurious failures on an otherwise successful publish. No blockers. The core security goal is met and the implementation is clean. |
There was a problem hiding this comment.
Pull request overview
Updates the release process so @livetemplate/client is published to npm exclusively from GitHub Actions using npm OIDC trusted publishing (no stored npm token), triggered by release: published.
Changes:
- Remove local
npm publishfromscripts/release.shand update dry-run/final output to point to CI publishing. - Update
.github/workflows/publish.ymlto publish via OIDC (id-token: write) withnpm publish --provenance, and pin builds to the release tag. - Add a guard in CI to ensure the release tag version matches
package.jsonbefore publishing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
scripts/release.sh |
Stops local npm publishing; clarifies that GitHub release triggers CI publishing. |
.github/workflows/publish.yml |
Switches npm auth to OIDC + provenance; checks out the release tag and validates version match. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| npm view @livetemplate/client@$PACKAGE_VERSION | ||
| PKG_VERSION=$(node -p "require('./package.json').version") | ||
| echo "Published version: $PKG_VERSION" | ||
| npm view "@livetemplate/client@$PKG_VERSION" |
There was a problem hiding this comment.
The post-publish verification step runs a single npm view, but npm read-after-write can lag (often 10–60s). This can cause the workflow to fail even though npm publish succeeded. Consider adding a small retry loop with backoff (or making this check non-blocking) so transient registry propagation doesn’t break releases.
| npm view "@livetemplate/client@$PKG_VERSION" | |
| for attempt in 1 2 3 4 5; do | |
| if npm view "@livetemplate/client@$PKG_VERSION"; then | |
| echo "Package is visible on npm." | |
| exit 0 | |
| fi | |
| if [ "$attempt" -lt 5 ]; then | |
| delay=$((attempt * 10)) | |
| echo "Package not visible yet on npm (attempt $attempt/5). Retrying in ${delay}s..." | |
| sleep "$delay" | |
| fi | |
| done | |
| echo "Package @livetemplate/client@$PKG_VERSION was not visible on npm after multiple attempts." | |
| exit 1 |
Both Claude and Copilot review bots flagged that the post-publish 'npm view' verification step runs a single query with no retry logic. npm's read-after-write can lag 10–60s due to registry replication, so a transient propagation delay would fail the workflow even though 'npm publish' succeeded — forcing a manual re-run on an otherwise successful release. Wrap the check in a bounded 5-attempt loop with progressive backoff (10s, 20s, 30s, 40s = up to 100s total wait), exiting on first success. If the package still isn't visible after 5 attempts, exit 1 so genuine publish failures are still surfaced. Addresses review comments from claude[bot] and Copilot on #71. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ReviewThis is a solid improvement. OIDC trusted publishing eliminates the NPM_TOKEN secret, provenance attestation improves supply chain transparency, and pinning the checkout to One thing to verify: Minor note: If the verify step exhausts all 5 retries and exits 1, the package is already live on npm — the CI failure would be a false alarm. The rollback section in the PR description covers this correctly, but it's worth adding an inline comment in the workflow (above Otherwise: clean, well-scoped change. No bugs or security issues found. |
Claude review on dc8dd26 pointed out that if the retry loop exhausts all 5 attempts and exits 1, the package has already been published — the failure is a registry propagation lag, not a publish failure. Without context, whoever investigates the red CI run might start an unwarranted 'npm deprecate' rollback on a perfectly good release. Add an inline comment at the exit path making this operationally obvious, plus a second echo clarifying that the publish step itself succeeded. Verify step logic is unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Good change overall — moving to OIDC trusted publishing eliminates the NPM_TOKEN secret and adds provenance attestation without much added complexity. One thing to verify: The Minor concern on the verify step: The final Everything else looks solid:
|
Claude review on 61a5dc8 pointed out that exiting non-zero when the 5-attempt verify loop exhausts is semantically wrong: by definition the publish step already succeeded to reach the verify step, so a red CI run on a successful release just invites a panicked rollback on what is actually registry replication lag. Switch the exhausted-retries path from 'exit 1' to a GitHub Actions '::warning::' annotation that shows up in the run summary. The workflow still completes green (matching reality: the package IS published), but the anomaly is surfaced clearly in the UI with actionable guidance ('manually confirm with npm view ... before taking any action'). A genuine publish failure would have aborted the workflow at the 'npm publish' step itself, so no signal is lost by this change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Review: ci: publish to npm via OIDC trusted publishing The approach is sound and well-executed. A few notes: Looks good:
One note on the verify step: After 5 failed attempts the step emits a warning annotation and exits 0, so the overall workflow run stays green. This is the stated intent and the reasoning is sound (npm publish success is the authoritative signal). Just make sure your team is aware that a warning annotation can be easy to miss in the GitHub Actions summary — worth noting in a runbook or RELEASING.md if one exists. Minor: No blockers. The security improvement is real and the implementation is clean. |
Step 6 of the Release Process in CONTRIBUTING.md still said 'Publish to npm' as something scripts/release.sh does locally, which has not been true since the OIDC workflow landed. Update the section to describe the actual two-stage flow: 1. release.sh: validate, version-bump, tests, build, tag, push, GitHub release (no local npm publish) 2. publish.yml workflow: triggered by release:published event, runs OIDC trusted publishing with --provenance Also add a 'Watching a release' subsection covering the three outcomes an operator might see on the Actions tab (green / green-with-warning / failed-publish), with specific guidance on the ::warning:: annotation added in the previous commit so nobody rolls back a successfully published package on a propagation lag. Claude review on 51c6da8 suggested capturing this operator-facing context in a runbook. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Review: ci: publish to npm via OIDC trusted publishing Overall this is a solid improvement — removing the NPM_TOKEN secret, adding provenance attestation, and pinning checkout to the release tag are all good practices. One notable gap: no Minor: Everything else looks good:
|
|
Thanks for the review — a quick clarification on the
- name: Install dependencies
run: npm ci
- name: Run tests
run: npm test
- name: Build
run: npm run buildIt was present in the very first commit of this PR and I haven't touched it since, so it doesn't appear in any of the later diffs the review workflow sees. The environment-specific-failure concern is already covered — tests run in CI before The No code change for this round. |
Summary
@livetemplate/client, triggered only by the explicitrelease: publishedevent — no more pasting an npm token (ornpm login) when runningrelease.sh.publish.ymlnow uses npm OIDC trusted publishing (id-token: write) plusnpm publish --provenance, so the repo holds zero npm secrets and the package gets a verified provenance badge on npmjs.com. Checkout is pinned togithub.event.release.tag_nameand a version-match guard rejects releases that point at a mismatchedpackage.json.release.shkeeps its local pre-flight (test, build, verify, package contents) and still tags + pushes + creates the GitHub release — but no longer runsnpm publishlocally. Thepublish_npm()function andnpm whoamicheck are gone. Dry-run text and closing summary updated to point at the Actions workflow.Trusted publisher (already configured)
The npmjs.com Trusted Publisher for
@livetemplate/clienthas been pointed at this repo +publish.yml, so the new workflow can authenticate via OIDC withoutNPM_TOKEN.Test plan
bash -n scripts/release.sh— syntax check passesdry_runoutput inspected — no longer mentions a local npm publish stepnpm test— 340 passed across 20 suitesv0.8.24) via./scripts/release.shand watch thePublishworkflow on the Actions tabnpm view @livetemplate/client@<new>showsdist.attestations(provenance uploaded)Rollback
npm publish: nothing on npm. Re-run the failed workflow from the Actions UI; no need to recreate the tag/release.npm publishbut in the verify step: the package is published and fine — npm read-after-write can lag 10–60s. Re-running the verify step (or just runningnpm viewmanually) will succeed.npm deprecate @livetemplate/client@X.Y.Z "reason". Don't unpublish — npm forbids it after 72h and breaks downstream installs.🤖 Generated with Claude Code