Skip to content

fix: harden release script with clean build and verification - #37

Merged
adnaan merged 3 commits into
mainfrom
fix/harden-release-script
Mar 29, 2026
Merged

fix: harden release script with clean build and verification#37
adnaan merged 3 commits into
mainfrom
fix/harden-release-script

Conversation

@adnaan

@adnaan adnaan commented Mar 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Problem: v0.8.8 was published to npm with stale build artifacts — the shouldSkipUpdate fix from PR fix: skip patching focused form elements during DOM update #32 was missing from all dist/ files in the published package
  • Root cause: build_and_test() ran npm run build on top of existing dist/ without cleaning first, and no verification caught the stale artifacts
  • Fix: Adds clean-before-build and post-build verification to scripts/release.sh

Changes to scripts/release.sh:

  • build_and_test() now runs npm run clean before npm run build
  • New verify_build() function: checks dist files exist and are non-empty, smoke tests the module export, validates version consistency
  • New verify_package_contents() function: runs npm pack --dry-run to verify the tarball includes required files
  • Both verification functions wired into the main release flow between build and commit

Test plan

  • bash -n scripts/release.sh — syntax check passes
  • npm test — all 278 tests pass
  • npm run clean && npm run build — verified shouldSkipUpdate present in all dist/ files
  • After merge: run ./scripts/release.sh to publish v0.8.9 with correct artifacts

🤖 Generated with Claude Code

v0.8.8 was published with stale build artifacts (missing shouldSkipUpdate
from PR #32). The release script built on top of old dist/ without cleaning
first, and had no verification that artifacts reflected current source.

Changes:
- Clean dist/ before building to prevent stale artifacts
- Add verify_build(): checks dist files exist, smoke tests module export,
  validates version consistency
- Add verify_package_contents(): runs npm pack --dry-run to verify tarball
  includes required files

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 29, 2026 18:20
@claude

claude Bot commented Mar 29, 2026

Copy link
Copy Markdown

Good fix for the stale artifact problem. One bug to address before merging:

Missing error handling on npm run clean — In build_and_test(), the new npm run clean call has no || { ...; exit 1 } guard, unlike npm run build immediately below it. If clean fails (e.g. the script is missing from package.json), the release continues on stale artifacts — exactly the problem this PR is trying to prevent.

# Should be:
npm run clean || {
    log_error "Clean failed, aborting release"
    exit 1
}

Minor: verify_package_contents uses grep -q "$f" without anchoring, so dist/livetemplate-client.js.map would satisfy the check for dist/livetemplate-client.js. Not a real risk with these specific filenames, but worth noting.

The verify_build and verify_package_contents additions are solid — good defense against a repeat of the v0.8.8 incident.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens scripts/release.sh to prevent publishing stale dist/ artifacts to npm by enforcing a clean build and adding post-build/package verification steps.

Changes:

  • Run npm run clean before npm run build in build_and_test().
  • Add verify_build() to validate required dist/ outputs, smoke-test the built module export, and confirm version consistency.
  • Add verify_package_contents() to ensure npm pack --dry-run includes required build artifacts before tagging/publishing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread scripts/release.sh
Comment on lines +281 to +284
for f in "${required_in_pack[@]}"; do
if ! echo "$pack_output" | grep -q "$f"; then
log_error "npm pack missing required file: $f"
exit 1

Copilot AI Mar 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The grep -q "$f" check can produce false positives (e.g., dist/livetemplate-client.js will also match dist/livetemplate-client.js.map). This undermines the verification goal—consider matching the exact filename at end-of-line, using fixed-string matching, or parsing npm pack --dry-run --json output to reliably assert required files are included.

Copilot uses AI. Check for mistakes.
Comment thread scripts/release.sh Outdated
log_info "Tests passed"

log_step "Cleaning previous build artifacts..."
npm run clean

Copilot AI Mar 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm run clean is not wrapped with the same error handling as tests/build. With set -e, a clean failure will abort the script without a clear log_error message; consider adding an explicit failure handler (similar to the npm test / npm run build blocks) so the release output is actionable.

Suggested change
npm run clean
npm run clean || {
log_error "Clean failed, aborting release"
exit 1
}

Copilot uses AI. Check for mistakes.
- Add error handling for `npm run clean` with log_error on failure
- Use `grep -qF "/$f"` for exact path matching in verify_package_contents
  to prevent false positives from substring matches (e.g., .js.map matching .js)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Mar 29, 2026

Copy link
Copy Markdown

Review: looks good overall, one potential bug to check

The clean-before-build and post-build verification approach is the right fix for the stale artifact problem.

Potential bug in verify_package_contents:

if ! echo "$pack_output" | grep -qF "/"; then

The grep pattern has a leading / (e.g. /dist/livetemplate-client.js), but npm pack --dry-run output typically lists paths without a leading slash:

npm notice 15.2kB  dist/livetemplate-client.js

This means the check will never match and will always fail, making verify_package_contents a hard blocker on every release run. Drop the leading /:

if ! echo "$pack_output" | grep -qF "$f"; then

Everything else looks solid:

  • Clean step placement (before build, abort on failure) is correct
  • verify_build artifact checks (-s for non-empty), smoke test, and version consistency check are all sensible
  • jq -r '.version' package.json for version check is more reliable than grep

Suggest fixing the grep pattern before merging.

npm pack --dry-run lists paths like "dist/foo.js" without a leading slash,
so grep -qF "/dist/foo.js" would never match, blocking every release.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Mar 29, 2026

Copy link
Copy Markdown

Review

Good fix for the stale artifact problem. A few actionable items:

Bug: Smoke test may fail for ESM output

If package.json has "type": "module", the require() call in verify_build() will throw. Worth verifying the dist output is CJS before the v0.8.9 release run, or switching to --input-type=commonjs.

Minor: npm run clean is a hard failure

If clean is not defined in package.json scripts, the release aborts before building. Worth confirming this script is always present, or softening to a warning.

npm pack --dry-run output parsing is fragile

npm v6 and v7+ format this output differently. Since verify_build() already checks files exist on disk, verify_package_contents() only adds value for catching .npmignore/files misconfig. If that is the intent, inspecting an actual tarball (npm pack without --dry-run) is more reliable.

@adnaan
adnaan merged commit b2b59e4 into main Mar 29, 2026
6 checks passed
@adnaan
adnaan deleted the fix/harden-release-script branch March 29, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants