fix(npm): pass absolute tarball paths to npm publish - #4
Conversation
The bootstrap script passed the tarball paths exactly as globbed from the directory argument, so `bash scripts/bootstrap-npm.sh dist-npm` produced `npm publish dist-npm/planmonster-olk-darwin-arm64-1.10.0-pm.1.tgz`. npm parses a bare relative path as the GitHub shorthand <owner>/<repo> and tries to clone ssh://git@github.com/dist-npm/planmonster-olk-darwin-arm64-1.10.0-pm.1.tgz.git, which fails: npm error code 128 npm error An unknown git error occurred npm error git@github.com: Permission denied (publickey). Reported against a real bootstrap attempt. The dry run did not catch it, because it only echoes the command. Resolve the directory argument to an absolute path once, before building any tarball argument. An absolute path (like a "./" prefix) is unambiguously a file. Adds scripts/test-bootstrap-npm.sh, wired into the ci.yml package job. It packs the seven wrapper tarballs (no Go binaries needed), runs the bootstrap script in dry-run mode against a *relative* directory, and asserts that every publish argument is absolute. Verified that the test fails against the unfixed script and passes against the fixed one. It also covers the publish order and the input validation from 89ac14f: an extra tarball, a missing platform package, an empty directory, and `--tag` swallowing a following flag. Corrects the version examples in docs/npm-publishing.md from 0.9.5-pm.N to 1.10.0-pm.N: git describe puts this fork at v1.10.0-16, so 1.10.0 is the upstream base. The 0.9.5 in server.json is stale and is stamped by CI anyway.
📝 WalkthroughWalkthroughThe npm bootstrap script now resolves relative tarball directories to absolute paths. A new regression test validates packaging, publish ordering, malformed inputs, and option handling. CI runs the test, while npm publishing documentation and the packaged artifact are updated for version 1.10.0-pm.1. ChangesNPM bootstrap validation and release flow
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1e25881-821c-4ab0-b90a-f388e3604404
📒 Files selected for processing (11)
.github/workflows/ci.ymldist-npm/planmonster-olk-darwin-arm64-1.10.0-pm.1.tgzdist-npm/planmonster-olk-darwin-x64-1.10.0-pm.1.tgzdist-npm/planmonster-olk-linux-arm64-1.10.0-pm.1.tgzdist-npm/planmonster-olk-linux-x64-1.10.0-pm.1.tgzdist-npm/planmonster-olk-win32-arm64-1.10.0-pm.1.tgzdist-npm/planmonster-olk-win32-x64-1.10.0-pm.1.tgzdist-npm/planmonster-olkcli-1.10.0-pm.1.tgzdocs/npm-publishing.mdscripts/bootstrap-npm.shscripts/test-bootstrap-npm.sh
| cleanup() { | ||
| git checkout -- npm/olk/package.json npm/olk-*/package.json 2>/dev/null || true | ||
| rm -rf "$tmp" | ||
| } | ||
| trap cleanup EXIT |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not discard the caller’s working-tree changes.
Both cleanup paths use git checkout, which can erase a developer’s unstaged edits to the package manifests when this regression test is run locally. Snapshot the files and restore those exact bytes, or run the build in an isolated temporary worktree instead. This also prevents the CI working-tree check from masking manifest mutations.
Also applies to: 38-38
| echo "==> assert publish order: 6 platform packages, launcher last" | ||
| order="$(printf '%s\n' "$out" | grep -c 'DRY RUN: npm publish')" | ||
| [ "$order" -eq 7 ] || { | ||
| echo "expected 7 publish commands, saw $order" >&2 | ||
| exit 1 | ||
| } | ||
| last="$(printf '%s\n' "$out" | grep 'DRY RUN: npm publish' | tail -1)" | ||
| case "$last" in | ||
| *olkcli-*) ;; | ||
| *) | ||
| echo "launcher must be published last, saw: $last" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete publish sequence, not just its length.
The current check only requires seven publish commands and a launcher-like final argument. It would still pass if a platform package were duplicated, omitted, or published in the wrong position. Capture the seven basenames and compare them with an explicit expected sequence containing each platform package exactly once and the launcher last.
| echo "==> assert input validation" | ||
| cp -r "$tmp/dist-npm" "$tmp/extra" | ||
| cp "$tmp/extra"/*olkcli-*.tgz "$tmp/extra/unrelated-9.9.9.tgz" | ||
| expect_failure "an extra/unexpected tarball" extra | ||
|
|
||
| cp -r "$tmp/dist-npm" "$tmp/missing" | ||
| rm -f "$tmp/missing"/*olk-win32-arm64*.tgz | ||
| expect_failure "a missing platform package" missing | ||
|
|
||
| cp -r "$tmp/dist-npm" "$tmp/empty-ish" | ||
| rm -f "$tmp/empty-ish"/*.tgz | ||
| expect_failure "no tarballs at all" empty-ish |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add an isolated mixed-version fixture.
The “extra/unexpected tarball” case adds an eighth file named 9.9.9; it does not test a valid seven-tarball set containing one package from a different version. Add that scenario separately so version-consistency validation cannot regress without this test failing.
1 participant
Found during a real bootstrap attempt.
Symptom
Cause
bootstrap-npm.shpassed tarball paths exactly as globbed from the directory argument, so a relative argument producednpm publish dist-npm/pkg.tgz. npm parses a bare relative path as the GitHub shorthand<owner>/<repo>and tries to clone it. Nothing to do with the user's keys or npm auth.Reproduced and verified locally:
npm publish dist-npm/pkg.tgz --dry-runcode 128, tries to clonegithub.com/dist-npm/...npm publish ./dist-npm/pkg.tgz --dry-runname: @planmonster/olk-darwin-arm64The existing
--dry-rundid not catch this because it only echoes the command.Fix
Resolve the directory argument to an absolute path once, before any tarball argument is built.
Regression test
New
scripts/test-bootstrap-npm.sh, wired into thepackagejob. It packs all seven wrapper tarballs (no Go binaries needed — the placeholder layout packs fine), runs the bootstrap script in dry-run mode against a relative directory, and asserts every publish argument is absolute.Proof it works:
It also locks in the validation added in 89ac14f: publish order (6 platform packages, launcher last), an extra tarball, a missing platform package, an empty directory, and
--tagswallowing a following flag.Also
Corrects version examples in
docs/npm-publishing.mdfrom0.9.5-pm.Nto1.10.0-pm.N.git describeputs this fork atv1.10.0-16-g764c720, so 1.10.0 is the upstream base; the0.9.5inserver.jsonis stale and CI stamps it from the tag regardless.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.