Add npm publishing and update documentation by jbrejner · Pull Request #719 · kosli-dev/cli

jbrejner · 2026-03-23T13:48:11Z

Distribute kosli cli as npm package.
Install globally with npm install -g @kosli/cli

The above will install the wrapper package which detects the current platform and architecture and then pull the appriate package with the correct binary.

Testable packages exist, they are tagged with "snapshot" so you can test with npm install -g @kosli/cli --tag snapshot

AlexKantor87 · 2026-03-24T09:32:48Z

PR #719 Multi-Agent Review: "Add npm publishing and update documentation"

Repository: kosli-dev/cli | Author: jbrejner | Status: DRAFT Branch: 4590_cli-as-npm-package → main Stats: +503 / -5 across 30 files | 5 commits PR URL: https://github.com//pull/719

The Review Panel

Four agents with distinct personas reviewed this PR independently, then their findings were synthesized. Here's who was on the panel:

Agent	Persona	Focus Area
Sasha	Security Engineer — paranoid, cynical, thinks in attack surfaces	Secrets management, supply chain risks, token handling
Dev	DevOps/CI Engineer — battle-hardened, pragmatic, "what fails at 2am?"	GoReleaser integration, pipeline reliability, failure modes
Priya	NPM Packaging Expert — meticulous, encyclopedic npm knowledge	Package.json correctness, DX, npm ecosystem patterns
Doc	Technical Writer & DX Advocate — empathetic, user-journey focused	Documentation completeness, onboarding experience, error UX

Unanimous Verdict: DO NOT MERGE (yet)

All four agents independently reached the same conclusion: the PR has a sound architectural foundation (the esbuild-style per-platform binary distribution pattern is well-chosen) but has several blocking issues that would result in a non-functional npm package if published today. This aligns with the PR's own DRAFT status and the author's listed TODOs.

The Big Three: Issues Every Agent Flagged

These three issues were independently identified by all four reviewers, making them the highest-confidence findings:

1. The Missing `bin/kosli` JS Shim (flagged by: ALL FOUR)

The wrapper's package.json declares "bin": {"kosli": "bin/kosli"}, but the JavaScript shim file that should live at npm/wrapper/bin/kosli is nowhere in the diff. Worse, .gitignore adds npm/wrapper/bin/*, which would prevent committing it.

Sasha (Security): "Users get command not found — package is non-functional"
Dev (CI/CD): "No evidence the shim exists or is tested"
Priya (NPM): "This is the most critical issue — every installation will be broken"
Doc (DX): "The README describes a file that doesn't exist in the repo. Is it generated? By what?"

Consensus: This is likely an oversight where the shim was developed locally but blocked by the gitignore pattern. The shim must be a committed JS file (not a binary), so it shouldn't be gitignored.

2. Token Variable Mismatch (flagged by: Sasha, Dev, Priya)

The .npmrc files reference ${MY_LOCAL_NPM_TOKEN}, but the CI workflow sets NPM_TOKEN. The publish script doesn't bridge this gap.

Sasha: "npm publishing will FAIL due to authentication errors — incomplete implementation"
Dev: "Silent npm ERR! 401 Unauthorized failures in CI"
Priya: "Confusion about where tokens go; risk of accidentally committing tokens"

Consensus: The token naming needs to be unified, or the publish script needs to explicitly set the npm auth token from the CI environment variable.

3. Silent Postinstall Failures (flagged by: ALL FOUR)

The install.js postinstall script exits with code 0 on every failure path — unsupported platform, missing binary, failed validation. Users get no clear signal their installation is broken.

Sasha: "Weak validation — a trojanized binary that still executes passes silently"
Dev: "No binary validation — corrupt/truncated binaries could ship"
Priya: "Violates the postinstall contract. Users will debug for hours"
Doc: "No troubleshooting section for the errors this script silently swallows"

Consensus: At minimum, the script should exit non-zero when the binary is missing or fails validation on a supported platform. The current "never fail" approach is too permissive.

Where the Agents Diverged: The Interesting Debates

The `sed -i` Question

Dev and Priya both flagged the use of sed -i in npm-publish.sh to mutate package.json files, but for different reasons:

Dev worried about atomicity: "If sed fails partway, repo is left corrupted. No backup."
Priya worried about portability: "macOS BSD sed needs sed -i '', Linux doesn't. This will break local dev."

Sasha didn't flag this — from a security perspective, sed in a CI pipeline is unremarkable. Doc didn't flag it either — it's invisible to users.

The nfpms Template Change

Dev was the only agent to catch that the .goreleaser.yml nfpms section removed the {{ else if eq .Arch "arm64" }}{{ .Arch }}_v8.0 clause. Dev called this "undocumented arm64_v8.0 suffix removal" and questioned whether it was intentional or a regression affecting Linux ARM64 package builds.

The other three agents didn't notice this change buried in the goreleaser config — it's a subtle, domain-specific issue that only a CI specialist would spot.

Supply Chain Irony

Sasha made an observation unique to the security review: "No SHA256 verification, no SLSA attestation verification — ironic given Kosli IS a compliance tool." This philosophical point — that a compliance tool's own distribution lacks the verification it recommends — wasn't raised by the others.

Error Messages vs. Error Handling

Doc and Priya had subtly different takes on the postinstall problem:

Priya wanted the script to exit non-zero (break the install if things are wrong)
Doc wanted better error messages and a troubleshooting guide (help users fix it)

These aren't contradictory — you need both — but they reflect the different priorities of a package maintainer vs. a documentation writer.

Per-Agent Unique Findings

Sasha (Security) — Unique Catches

Attack surface analysis: Identified 5 new supply chain vectors (platform substitution, wrapper hijacking, typosquatting, token leakage, CI escape)
.npmrc files as a pattern risk: Even with variable placeholders, committing .npmrc files establishes a dangerous pattern for future contributors

Dev (DevOps) — Unique Catches

No rollback on partial publish: If npm publish fails on package 3 of 7, the script continues. Result: broken release with some platforms available, others missing
Cleanup hooks don't verify success: The before hooks assume deletion succeeded but don't check for permission issues
GoReleaser after hook conditional: Noted the snapshot/dry-run conditional is well-designed

Priya (NPM) — Unique Catches

Missing engines field: No Node.js version constraint in any package.json
Missing repository.directory: Monorepo packages should include "directory": "npm/wrapper" for npm registry navigation
Missing trailing newlines: Several package.json files lack POSIX-standard trailing newlines (will cause linter warnings and potential npm auto-fix diffs)
0.0.0 version semantics: npm interprets "0.0.0" as an exact pin, not "any version" — the publish script must update these atomically

Doc (Technical Writer) — Unique Catches

Frontmatter syntax error: Line 1 of install.md changed --- to --- (leading space) — will break Hugo rendering
No npx documentation: Users discovering the package on npmjs.org will naturally try npx @kosli/cli — is this supported?
Tree diagram formatting errors: The README has two └── at the same indentation level (cosmetic but sloppy)
Per-platform READMEs need "install @kosli/cli instead" guidance: Users who find @kosli/cli-linux-x64 on npm need to be redirected

Consolidated Recommendations (Priority Order)

Must-fix before merge

Commit the bin/kosli JS wrapper shim (and fix .gitignore to allow it)
Align token variable naming between .npmrc and CI (NPM_TOKEN vs MY_LOCAL_NPM_TOKEN)
Fix install.md frontmatter leading space (line 1)
Make postinstall exit non-zero when binary is missing/broken on supported platforms
Add error handling for partial publish failures in npm-publish.sh
Confirm the nfpms arm64 template change is intentional

Should-fix

Add "engines": { "node": ">=16.0.0" } to wrapper package.json
Add trailing newlines to all package.json files
Add troubleshooting section to install docs
Document Node.js version requirements in the NPM install tab
Fix tree diagram formatting in npm/README.md
Add repository.directory to wrapper package.json

Nice-to-have

Add binary integrity verification (checksums)
Document npx behavior
Improve per-platform README content
Add CI test step that actually installs and runs the npm package

Process Observations

What worked well about multi-agent review

Coverage breadth: Each agent caught issues the others missed (Dev's nfpms regression, Doc's frontmatter error, Priya's engines field, Sasha's supply chain vectors)
Confidence calibration: Issues flagged by 3-4 agents are clearly high-priority; issues flagged by only 1 agent are worth investigating but may be lower priority
Perspective diversity: The same issue (silent postinstall) was analyzed from four angles — security (attack surface), reliability (failure modes), correctness (npm contract), and UX (user confusion)

What was interesting about the disagreements

Agents naturally weighted the same evidence differently based on their expertise
No actual contradictions — disagreements were about priority, not facts
The "overlap zone" between agents was the most valuable (3-4 agents flagging the same issue independently = high confidence)

Review conducted: 2026-03-24 PR URL: https://github.com//pull/719

claude · 2026-03-25T19:00:07Z

Claude finished @jbrejner's task in 3m 54s —— View job

Code Review: Add npm publishing and update documentation

Run static analysis (go build, go vet, go mod tidy)
Review changed files
Post inline comments on specific issues
Post summary review

Static Analysis

go build ./..., go vet ./..., and go mod tidy all pass cleanly. No Go code issues — this PR is primarily npm packaging infrastructure.

Summary

The PR is in solid shape after the many rounds of feedback. The esbuild-style platform-package pattern is well-chosen for enterprise/air-gapped environments, the rationale is sound, and most earlier review findings have been addressed. Here's what remains:

Remaining Issues

1. prepack script has misleading error output (npm/wrapper/package.json:34)
The shell one-liner test -f bin/kosli || (echo ... && exit 1) && test -f install.js || (echo ... && exit 1) has an operator-precedence issue: if bin/kosli is missing, the exit 1 only exits the subshell, then the second || fires and prints "install.js is missing" too — even if install.js is fine. It still fails correctly, but the double error is confusing. See inline comment for a fix.

2. Tree diagram formatting (npm/README.md:29)
Cosmetic — two └── at the same indent level. Second-to-last should be ├──. This has been flagged in every prior review round.

3. release: [published] trigger on install-script-tests (.github/workflows/install-script-tests.yml:24)
Looks intentional given the test-npm job handles it, but @mbevc1 flagged it — worth a quick confirmation.

What's Working Well

install.js validation — Switched from kosli version to fs.accessSync(X_OK), eliminating false negatives from config issues. Clean approach.
prepack scripts on all platform packages — fail-fast if the binary is missing before npm pack. Good safety net.
npm-publish.sh — Two-phase pack-then-publish, retry logic with exponential backoff, proper jq temp-file cleanup with || { rm -f "$tmp"; exit 1; }, process substitution to avoid subshell issues with set -e. Well-structured.
bin/kosli JS shim — Clean platform detection, proper error messages, correct exit code forwarding via process.exit(result.status ?? 1).
GoReleaser integration — The after hook with snapshot/dry-run conditional is well-designed. The before hooks clean up stale artifacts properly.
.npmrc token alignment — All .npmrc files now consistently use ${NPM_TOKEN}, matching the CI workflow env var.
Comprehensive CI test matrix — test-npm job covers linux/x64, linux/arm64, darwin/arm64, win32/x64, and win32/arm64.
Good documentation — npm/README.md thoroughly explains the architecture, publishing flow, and goreleaser integration.

scripts/npm-publish.sh

.goreleaser.yml

npm/wrapper/install.js

npm/wrapper/package.json

npm/README.md

docs.kosli.com/content/getting_started/install.md

scripts/npm-publish.sh

.goreleaser.yml

npm/wrapper/install.js

scripts/npm-publish.sh

npm/wrapper/install.js

.goreleaser.yml

npm/cli-linux-x64/package.json

scripts/npm-publish.sh

npm/wrapper/install.js

npm/cli-linux-x64/package.json

npm/README.md

.goreleaser.yml

scripts/npm-publish.sh

npm/wrapper/install.js

npm/cli-linux-x64/package.json

- Clean up temp file on jq/mv failure for wrapper package.json update, consistent with the platform loop - Pass --provenance to npm publish when running in GitHub Actions

…SLI_GORELEASERPRO }} — that's the standard way the goreleaser-action picks up the pro license.

- Test npm install -g @kosli/cli on all 6 supported platforms - Trigger on release (published) to test newly published packages - Also trigger on push/PR to npm/**, .goreleaser.yml, and scripts/npm-publish.sh

… but it's not available in all GitHub org configuration

claude · 2026-04-01T12:54:40Z

scripts/npm-publish.sh

+# Also update the optionalDependencies version references in the wrapper
+tmp="$(mktemp)"
+jq --arg v "$VERSION" '.optionalDependencies = (.optionalDependencies | with_entries(.value = $v))' \
+  npm/wrapper/package.json > "$tmp" && mv "$tmp" npm/wrapper/package.json || { rm -f "$tmp"; exit 1; }


Bug: Missing cleanup on jq failure. The platform loop on line 38 has || { rm -f "$tmp"; exit 1; }, but this wrapper update doesn't. If jq or mv fails, the temp file leaks and the wrapper package.json is silently left unchanged.

Suggested change

npm/wrapper/package.json > "$tmp" && mv "$tmp" npm/wrapper/package.json || { rm -f "$tmp"; exit 1; }

jq --arg v "$VERSION" '.optionalDependencies = (.optionalDependencies | with_entries(.value = $v))' \

npm/wrapper/package.json > "$tmp" && mv "$tmp" npm/wrapper/package.json || { rm -f "$tmp"; exit 1; }

Already fixed — line 44 has || { rm -f "$tmp"; exit 1; }. No change needed.

scripts/npm-publish.sh

npm/README.md

npm pack silently succeeds even if the binary is absent, producing a broken package. Each platform package now checks for bin/kosli (or bin/kosli.exe on Windows) before packing; the wrapper also checks for install.js.

jbrejner · 2026-04-01T13:09:41Z

@dangrondahl - I actually ran your prompt, Claude summarizes like this: "The GoReleaser Pro npms: stanza downloads the binary from GitHub Releases at install time. This breaks in environments where GitHub is not reachable — air-gapped networks, corporate firewalls, or when --ignore-scripts is enforced. Kosli's users are disproportionately enterprise/compliance-focused, making these restrictions more likely than for a typical CLI tool.
The esbuild-style platform packages approach means the binary travels through the npm registry alongside the package metadata. Wherever npm works, the install works — no additional internet access required."

Ah, now I see. And this is because they don't already have other npm packages proxied by e.g. a remote repo in Artifactory pointing to npmjs.org? Wonder how they do then? 🤔

I do not understand your question ?

@dangrondahl

… for executable check to address @dangrondahl ai review kosli version false negatives in install.js

claude · 2026-04-01T15:48:38Z

npm/wrapper/package.json

+    "install.js"
+  ],
+  "scripts": {
+    "prepack": "test -f bin/kosli || (echo 'ERROR: bin/kosli is missing' && exit 1) && test -f install.js || (echo 'ERROR: install.js is missing' && exit 1)",


Bug: Misleading second error when bin/kosli is missing. Due to shell operator precedence, if bin/kosli is missing, the (exit 1) only exits the subshell — then && short-circuits, then the second || triggers, printing "ERROR: install.js is missing" even though install.js may be fine.

Use && to chain the two checks independently:

Suggested change

"prepack": "test -f bin/kosli || (echo 'ERROR: bin/kosli is missing' && exit 1) && test -f install.js || (echo 'ERROR: install.js is missing' && exit 1)",

"prepack": "{ test -f bin/kosli || { echo 'ERROR: bin/kosli is missing' && exit 1; }; } && { test -f install.js || { echo 'ERROR: install.js is missing' && exit 1; }; }"

Or simpler — use braces {} instead of subshells () so exit 1 terminates the whole script, not just the subshell.

claude · 2026-04-01T15:48:41Z

npm/README.md

+│   ├── bin/kosli         # the native binary — see below
+│   └── package.json      # declares os/cpu fields for platform filtering
+├── cli-win32-arm64/      # @kosli/cli-win32-arm64
+│   ├── bin/kosli.exe     # the native binary — see below


Nit: Tree diagram formatting. Two └── at the same indentation level — the second-to-last should use ├──:

Suggested change

│ ├── bin/kosli.exe # the native binary — see below

├── cli-win32-arm64/ # @kosli/cli-win32-arm64

The tree is correct as-is. cli-win32-arm64 uses ├── (not last) and cli-win32-x64 uses └── (last item). No change needed.

scripts/npm-publish.sh

npm/wrapper/install.js

claude · 2026-04-01T15:48:49Z

.github/workflows/install-script-tests.yml

+            - 'scripts/npm-publish.sh'
    workflow_dispatch:
+    release:
+        types: [published]


Confirm: release: [published] trigger is intentional? The test-npm job's logic handles release events specifically (installing @latest vs @snapshot), so this looks intentional. But @mbevc1 also flagged this — worth a quick confirmation that this is desired.

Yes, intentional. On release events with a stable tag the job installs @latest, which tests the actual release that just published. On push/PR it installs @snapshot to catch regressions against the currently published version.

jbrejner · 2026-04-01T16:10:57Z

Code Review: PR #719

Phase 1: Static Analysis

All clear — go build ./..., go vet ./..., and go mod tidy pass with no issues.

Phase 2: Issues Found

scripts/npm-publish.sh:50 — Missing node_modules exclusion in package list build

The version-injection loop (line 39) was fixed to exclude node_modules:

done < <(find npm -name package.json ! -path '*/node_modules/*')

But the package list build (line 50) still lacks the same guard:

done < <(find npm -name package.json ! -path "npm/wrapper/*" | sort)

If node_modules/ exists under npm/ (e.g. after a local npm install), this loop would add every nested package.json to PACKAGES and attempt to npm pack and publish them. Should be:

done < <(find npm -name package.json ! -path "npm/wrapper/*" ! -path "*/node_modules/*" | sort)

Phase 3: What looks good

Architecture: The esbuild-style per-platform binary distribution is the right call for enterprise/air-gapped environments. Clean separation between platform packages and wrapper.
Two-phase pack-then-publish: Catching all pack failures before publishing anything prevents partial releases.
Retry logic: Exponential backoff in npm_publish_with_retry is solid.
bin/kosli shim: Correctly forwards exit codes via result.status ?? 1, good error messages.
install.js: Clean distinction between unsupported platform (exit 1) and --no-optional skip (exit 0 with warning).
prepack scripts: Guards on all platform packages prevent publishing empty bin/ dirs.
GoReleaser integration: The after hook conditional and two-stage before cleanup are well-designed.
CI test matrix: Good coverage across linux x64/arm64, macOS arm64, Windows x64/arm64 with correct tag-based install logic.

🤖 Generated with Claude Code

jbrejner · 2026-04-01T17:34:32Z

scripts/npm-publish.sh:50 — Missing node_modules exclusion in package list build

Fixed — ! -path '*/node_modules/*' added to line 50.

jbrejner · 2026-04-01T17:35:39Z

Re: repeated tree diagram comments on npm/README.md — the tree is correct. cli-win32-arm64 uses ├── (not the last item) and cli-win32-x64 uses └── (the last item). No change needed.

@kosli

* Changes required by Gorelase version > 2 * npm-publish script supports conditional publishing. If gorelease in not in "Release" mode, npm packages are not pushed, so we can avoid multitudes of snapshot packages being pushed to public registry * Create npm packages after goreleaser via a hook script The hook runs after. If goreaser is not in release mode, npm will build in dry-run mode, so the packages are not pushed to registry * Update documentation with NPM installation instructions * Switch npm package scope to @kosli * Fix: Add the missing bin/kosli JS Shim * Fix: Token Variable Mismatch * Fix: Silent Postinstall Failures * Update npm/README.md Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * refactor(npm-publish): replace sed/perl with jq and harden publish script - Use jq instead of sed/perl for JSON version updates (portable across macOS and Linux, handles JSON correctly) - Separate pack and publish into two distinct phases so all packages are packed before any are published - Add npm_publish_with_retry with exponential backoff (3 attempts) - Fail fast with clear error messages on pack or publish failure * Fix: Frontmatter formatting * Consistent formatting of package.json files * Include npm packages in binary provenance processing * Add directory and engines specification to packages * Documention updates: - npx is not supported - package @kosli/cli should be used to install * Fix three issues in npm postinstall and publish script - postinstall: exit 1 on unsupported platform to match bin/kosli shim behaviour - npm-publish.sh: use process substitution instead of pipe to while loop so set -e catches failures inside the loop - npm-publish.sh: fix out-of-scope max_attempts variable in publish error message * Update scripts/npm-publish.sh Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * Integrate npm package build and publish into GoReleaser pipeline - Copy each platform binary into its npm package dir via per-build post hooks - Run npm-publish.sh after release (dry-run on snapshots) via after hook - Clean npm bin dirs and tarballs before each build via before hooks * Update scripts/npm-publish.sh Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * Update scripts/npm-publish.sh Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * Update scripts/npm-publish.sh Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * Update scripts/npm-publish.sh Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * Add --provenance flag to npm publish when running in GitHub Actions * Fix temp file leak and add npm provenance in GitHub Actions - Clean up temp file on jq/mv failure for wrapper package.json update, consistent with the platform loop - Pass --provenance to npm publish when running in GitHub Actions * Added distribution: goreleaser-pro and GORELEASER_KEY: ${{ secrets.KOSLI_GORELEASERPRO }} — that's the standard way the goreleaser-action picks up the pro license. * Add npm installation test job to install-script-tests workflow - Test npm install -g @kosli/cli on all 6 supported platforms - Trigger on release (published) to test newly published packages - Also trigger on push/PR to npm/**, .goreleaser.yml, and scripts/npm-publish.sh * Select npm tag snapshot for now * Removed macos-13. macos-13 is the only GitHub-hosted x64 macOS runner but it's not available in all GitHub org configuration * Refine dry-run condition in npm-publish script for clarity * Add prepack scripts to fail fast when binary is missing npm pack silently succeeds even if the binary is absent, producing a broken package. Each platform package now checks for bin/kosli (or bin/kosli.exe on Windows) before packing; the wrapper also checks for install.js. * Exclude node_modules from package.json search in npm-publish script * Refactor binary validation in postinstall script to use fs.accessSync for executable check to address @dangrondahl ai review kosli version false negatives in install.js --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>

jbrejner requested a review from pbeckham March 23, 2026 17:30

claude bot reviewed Mar 25, 2026

View reviewed changes

scripts/npm-publish.sh Outdated Show resolved Hide resolved

claude bot reviewed Mar 25, 2026

View reviewed changes

scripts/npm-publish.sh Outdated Show resolved Hide resolved

claude bot reviewed Mar 25, 2026

View reviewed changes

.goreleaser.yml Outdated Show resolved Hide resolved

claude bot reviewed Mar 25, 2026

View reviewed changes

npm/wrapper/install.js Outdated Show resolved Hide resolved

claude bot reviewed Mar 25, 2026

View reviewed changes

npm/wrapper/package.json Show resolved Hide resolved

claude bot reviewed Mar 25, 2026

View reviewed changes

npm/README.md Outdated Show resolved Hide resolved

claude bot reviewed Mar 26, 2026

View reviewed changes

docs.kosli.com/content/getting_started/install.md Outdated Show resolved Hide resolved

claude bot reviewed Mar 26, 2026

View reviewed changes

scripts/npm-publish.sh Outdated Show resolved Hide resolved

claude bot reviewed Mar 26, 2026

View reviewed changes

.goreleaser.yml Outdated Show resolved Hide resolved

claude bot reviewed Mar 26, 2026

View reviewed changes

npm/wrapper/install.js Outdated Show resolved Hide resolved