Skip to content

Add npm publishing and update documentation#719

Draft
jbrejner wants to merge 5 commits intomainfrom
4590_cli-as-npm-package
Draft

Add npm publishing and update documentation#719
jbrejner wants to merge 5 commits intomainfrom
4590_cli-as-npm-package

Conversation

@jbrejner
Copy link
Contributor

@jbrejner jbrejner commented Mar 23, 2026

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.

Draft because I still need TODO

  • add api token for publishing to npm registry
  • kosli attest on the packages created
  • have @jbpros to take a look at packages. I currently have snapshot packages at @jbrejner/cli - are there things a good npm package should have ?

jbrejner and others added 5 commits March 21, 2026 19:22
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
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
@jbrejner jbrejner requested a review from pbeckham March 23, 2026 17:30
@AlexKantor87
Copy link
Contributor

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

Repository: kosli-dev/cli | Author: jbrejner | Status: DRAFT Branch: 4590_cli-as-npm-packagemain 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

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

Should-fix

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

Nice-to-have

  1. Add binary integrity verification (checksums)
  2. Document npx behavior
  3. Improve per-platform README content
  4. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants