Distribute CLI through npm with checksum-verified postinstall - #139
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideAdds an npm-distributed wrapper package for the dotagents CLI that downloads a platform-specific binary from GitHub Releases during postinstall with checksum verification, wires it into an executable bin shim, tests the wrapper via node:test in CI, and publishes it from the release workflow if not already on npm. Sequence diagram for npm postinstall and CLI executionsequenceDiagram
actor Developer
participant npm as npm
participant install_js as install_js
participant GitHub as GitHub_Releases
participant tar as tar
participant bin_shim as bin_dotagents_js
participant dotagents as dotagents_binary
Developer->>npm: npm install -g dotagents
npm->>install_js: run postinstall
install_js->>GitHub: fetchBuffer(checksums.txt)
install_js->>GitHub: fetchBuffer(dotagents_version_platform.tar.gz)
install_js-->>install_js: sha256(archive)
install_js-->>install_js: expectedChecksum(checksums, filename)
install_js-->>install_js: compare checksum
install_js->>tar: spawnSync(tar -xzf archive -C bin)
tar-->>install_js: extracted dotagents
install_js-->>Developer: binary installed in npm/bin
Developer->>bin_shim: dotagents [args]
bin_shim-->>bin_shim: fs.existsSync(binary)
bin_shim->>dotagents: spawnSync(binary, args)
dotagents-->>Developer: CLI output
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The installer assumes
taris available on PATH; consider checking for its presence up front and emitting a clearer error message if it's missing rather than surfacing a generic spawn or non‑zero exit error. - In
install.js, the error message for unsupported platforms currently uses the rawoptions.platform/archorprocess.platform/arch; it may be helpful to explicitly mention that only darwin/linux on x64/arm64 are supported to avoid confusion for users on other combinations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The installer assumes `tar` is available on PATH; consider checking for its presence up front and emitting a clearer error message if it's missing rather than surfacing a generic spawn or non‑zero exit error.
- In `install.js`, the error message for unsupported platforms currently uses the raw `options.platform`/`arch` or `process.platform`/`arch`; it may be helpful to explicitly mention that only darwin/linux on x64/arm64 are supported to avoid confusion for users on other combinations.
## Individual Comments
### Comment 1
<location path="npm/bin/dotagents.js" line_range="17-22" />
<code_context>
+ process.exit(1);
+}
+
+const result = spawnSync(binary, process.argv.slice(2), { stdio: "inherit" });
+if (result.error) {
+ console.error(result.error.message);
+ process.exit(1);
+}
+process.exit(result.status === null ? 1 : result.status);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Preserve child signal termination instead of treating it as a generic exit code 1.
When the child exits via a signal (e.g., SIGINT from Ctrl+C), `spawnSync` sets `status` to `null` and `signal` to the signal name. Converting `null` to exit code `1` hides that signal and diverges from typical CLI behavior. You could instead propagate the signal and only fall back to `1` when neither `status` nor `signal` is set:
```js
const { status, error, signal } = spawnSync(binary, process.argv.slice(2), { stdio: "inherit" });
if (error) {
console.error(error.message);
process.exit(1);
}
if (signal) {
process.kill(process.pid, signal);
} else {
process.exit(status ?? 1);
}
```
</issue_to_address>
### Comment 2
<location path="npm/install.js" line_range="49-58" />
<code_context>
+function fetchBuffer(url, redirects = 0) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Add a timeout to network downloads to avoid hanging installs on bad connections.
`fetchBuffer` currently uses `https.get` without a timeout, so a stalled connection could cause `npm install` to hang indefinitely. Please add a per-request timeout (e.g., `request.setTimeout(...)` and aborting on expiry) so the installer fails with a clear, timely error instead of hanging.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+17
to
+22
| const result = spawnSync(binary, process.argv.slice(2), { stdio: "inherit" }); | ||
| if (result.error) { | ||
| console.error(result.error.message); | ||
| process.exit(1); | ||
| } | ||
| process.exit(result.status === null ? 1 : result.status); |
There was a problem hiding this comment.
suggestion (bug_risk): Preserve child signal termination instead of treating it as a generic exit code 1.
When the child exits via a signal (e.g., SIGINT from Ctrl+C), spawnSync sets status to null and signal to the signal name. Converting null to exit code 1 hides that signal and diverges from typical CLI behavior. You could instead propagate the signal and only fall back to 1 when neither status nor signal is set:
const { status, error, signal } = spawnSync(binary, process.argv.slice(2), { stdio: "inherit" });
if (error) {
console.error(error.message);
process.exit(1);
}
if (signal) {
process.kill(process.pid, signal);
} else {
process.exit(status ?? 1);
}
Comment on lines
+49
to
+58
| function fetchBuffer(url, redirects = 0) { | ||
| return new Promise((resolve, reject) => { | ||
| https | ||
| .get(url, (response) => { | ||
| if (response.statusCode >= 300 && response.statusCode < 400 && response.headers.location) { | ||
| response.resume(); | ||
| if (redirects >= MAX_REDIRECTS) { | ||
| reject(new Error(`too many redirects fetching ${url}`)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Add a timeout to network downloads to avoid hanging installs on bad connections.
fetchBuffer currently uses https.get without a timeout, so a stalled connection could cause npm install to hang indefinitely. Please add a per-request timeout (e.g., request.setTimeout(...) and aborting on expiry) so the installer fails with a clear, timely error instead of hanging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an npm distribution channel:
npm i -g dotagents/npx dotagents.npm/wrapper package: bin shim spawns the platform binary; postinstall downloads the goreleaser asset for darwin/linux amd64+arm64 from GitHub Releases, verifies sha256 against the releasechecksums.txt, and extracts with system tar. No archive code is executed; unsupported platforms get a clear error pointing at brew/install.sh.npm-publishjob (needs goreleaser, so assets exist first): sets the version from the tag, skips if already published, publishes with provenance. Requires theNPM_TOKENsecret.Smoke-tested end to end against the live v0.4.0 release: download, checksum verify, extract, and
dotagents helpall pass.Summary by Sourcery
Distribute the dotagents CLI through npm with checksum-verified release binaries and automated, provenance-enabled publishing.
New Features:
Enhancements:
CI:
Deployment:
Tests: