fix(cli): sync publish version flow#416
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the CLI publishing process by introducing version synchronization checks and registry verification. Key changes include new functions in publish-cli.sh to calculate next versions, verify registry status, and ensure consistency between the package version and the built binary. Corresponding tests were added to handle version conflicts and registry errors. Feedback was provided regarding a potential file leak when using temporary files for error capturing, suggesting a variable-based approach instead.
| view_error="$(mktemp)" | ||
| if npm view "${package_name}@${version}" version --registry "$NPM_REGISTRY" >/dev/null 2>"$view_error"; then |
There was a problem hiding this comment.
The use of a temporary file to capture npm view errors can lead to file leaks if the script is interrupted (e.g., via SIGINT) or if it exits unexpectedly before the rm commands are reached. Since assert_version_not_published is called before the main trap is established at line 231, these files won't be cleaned up automatically.
Consider capturing the output directly into a variable to avoid disk I/O and potential leaks.
| view_error="$(mktemp)" | |
| if npm view "${package_name}@${version}" version --registry "$NPM_REGISTRY" >/dev/null 2>"$view_error"; then | |
| local view_output | |
| view_output=$(npm view "${package_name}@${version}" version --registry "$NPM_REGISTRY" 2>&1) | |
| local status=$? | |
| if [[ $status -eq 0 ]]; then | |
| echo "${package_name}@${version} already exists on $NPM_REGISTRY" >&2 | |
| echo "Update cli/package.json to the latest published version before running this release." >&2 | |
| exit 1 | |
| fi | |
| if echo "$view_output" | grep -Eiq '(E404|404 Not Found|is not in this registry|Not found)'; then | |
| return 0 | |
| fi | |
| echo "failed to verify whether ${package_name}@${version} exists on $NPM_REGISTRY" >&2 | |
| echo "$view_output" >&2 | |
| exit 1 |
There was a problem hiding this comment.
Fixed in 299659b. I removed the temporary file and now capture npm view output directly into a shell variable inside the if assignment, so set -e still allows us to inspect the non-zero path without leaking a file. The fail-closed behavior is unchanged: existing versions stop the release, explicit 404/not found continues, and other registry errors abort with the captured output.
Validated with:
bash scripts/tests/publish-cli-test.shbash -n scripts/publish-cli.sh && bash -n scripts/tests/publish-cli-test.shgit diff --checkcd cli && bun run buildcd cli && bun run typecheckcd cli && bun run lintcd cli && bun test
Summary
Tests