Skip to content

feat: make the package publishable to npm - #4

Merged
ethanasm merged 2 commits into
mainfrom
claude/mcp-review-pypi-npm-feasibility-723nga
Aug 14, 2026
Merged

feat: make the package publishable to npm#4
ethanasm merged 2 commits into
mainfrom
claude/mcp-review-pypi-npm-feasibility-723nga

Conversation

@ethanasm

Copy link
Copy Markdown
Owner

Makes mcp-git-reviewer publishable to npm. (PyPI was assessed and rejected — this is a TypeScript/Node project end to end; a PyPI presence would mean a rewrite or a Python shim wrapping a Node runtime the user installs separately, which is strictly worse than npx.)

The blocker: the published package didn't work

The MCP host spawned all four tool servers via the local tsx binary against src/**/*.ts. tsx is a devDependency, so consumers never get it. Installing the tarball into a clean project and running the CLI gave:

[mcp-host] Warning: Failed to start git-diff: spawn .../node_modules/.bin/tsx ENOENT
[mcp-host] Warning: Failed to start file-context: ... ENOENT
[mcp-host] Warning: Failed to start conventions: ... ENOENT
[mcp-host] Warning: Failed to start related-files: ... ENOENT

The severity is in the failure mode, not the message. initialize() treats server startup failures as non-fatal warnings ("graceful degradation"), so with a valid API key this does not error — it produces a review with zero context tools. No file reads, no convention scanning, no related-files. The entire premise of the tool, silently absent, while the CLI looks like it worked.

The existing suite can't see this: it imports tool servers directly from src/, so all 303 tests pass against a package that cannot spawn a single server.

Fix: resolveRunner now spawns process.execPath on dist/tools/<name>/server.js when running compiled, and keeps the tsx + src/ runner when running from source, so the no-build dev loop (bun run dev, vitest) is untouched. Server paths resolve relative to the host module rather than a guessed project root, so one code path serves both layouts. process.execPath rather than a bare node respects nvm/volta/asdf shims.

Both paths verified end to end: compiled via the installed tarball, source via npx tsx src/cli.ts (all four servers start, run proceeds to the expected 401 on a dummy key).

Regression guard

New npm run test:pack (scripts/smoke-pack.sh) — packs the tarball, installs it into a throwaway project, asserts every server answers an MCP initialize under plain node, runs the CLI end-to-end against a scratch git repo, and checks --version matches package.json:

▶ Pack        ✓ packed mcp-git-reviewer-0.1.0.tgz
▶ Install     ✓ installed into a clean project
              ✓ no tsx in the consumer install (as expected)
▶ Tool servers
              ✓ git-diff: responds to MCP initialize under plain node
              ✓ file-context: responds to MCP initialize under plain node
              ✓ conventions: responds to MCP initialize under plain node
              ✓ related-files: responds to MCP initialize under plain node
▶ CLI end-to-end
              ✓ CLI started every tool server
              ✓ --version matches package.json (0.1.0)

Wired into CI. Reverting the runner fix turns this red while the unit suite stays green.

Packaging hygiene

Before After
Tarball 197 files, 757 kB — shipped src/, tests/, bun.lock, CLAUDE.md, AGENT_TEAM.md, .github/ 129 files, 319 kB via a files allowlist
LICENSE absent, though the manifest claimed MIT MIT file added
repository / homepage / bugs missing added
Version hardcoded 0.1.0 in 5 places single-sourced from package.json via src/version.ts
README no install section ## Install (npx + global) and ## Publishing
.gitignore no *.tgz added — npm pack litters the root otherwise

Naming

Kept the npm package name as mcp-git-reviewer (per your instruction) with the binary staying mcp-review, and documented the split explicitly in the README so it isn't a surprise. Both names are still free on npm. Say the word if you'd rather the package be mcp-review too — it's a one-line change plus doc updates.

Verification

  • bash scripts/verify.sh — lint/format, typecheck, build, 303 tests, all green
  • npm run test:pack — green
  • Dev path re-confirmed manually (servers start from source under tsx)

Not done

  • Sourcemaps still ship in dist/. Their declarationMap/sourceMap references point at src/, which is no longer in the tarball, so they're partially dangling — harmless (file/line still resolve in stack traces) but droppable if you want a leaner package.
  • Version stays 0.1.0; npm version + npm publish is a deliberate manual step, documented in the README rather than automated here.

Generated by Claude Code

claude added 2 commits August 14, 2026 05:03
The published tarball was broken in a way the test suite could not see. The
MCP host spawned all four tool servers via the local `tsx` binary against
`src/**/*.ts`, but `tsx` is a devDependency and `src/` isn't needed at runtime,
so every server failed with ENOENT on a real install. Because startup failures
are deliberately non-fatal, this didn't error — it produced reviews with no
context tools at all, silently.

- Spawn compiled servers with `process.execPath` on `dist/tools/*/server.js`
  when running compiled, keeping the `tsx` + `src/` runner for the no-build dev
  loop. Server paths now resolve relative to the host module rather than a
  guessed project root, so one code path serves both layouts.
- Add a `files` allowlist: 197 files / 757 kB -> 129 files / 319 kB, no longer
  shipping src/, tests/, bun.lock or the internal docs.
- Add the MIT LICENSE file the manifest already claimed, plus repository,
  homepage and bugs metadata.
- Single-source the version from package.json via src/version.ts, replacing the
  five hardcoded '0.1.0' literals in the CLI and MCP serverInfo blocks.
- Add `npm run test:pack` (scripts/smoke-pack.sh): packs, installs the tarball
  into a clean project, asserts every server answers an MCP initialize under
  plain node, and runs the CLI end-to-end. Wired into CI, since the unit tests
  import servers from src/ and cannot catch a packaging break.
- Document install/publish in the README and the packaging contract in
  CLAUDE.md; ignore *.tgz.

Co-Authored-By: Claude <noreply@anthropic.com>
npm rewrites './bin/mcp-review' to 'bin/mcp-review' at publish time and warns
about the auto-correction on every run. Store the normalized form (npm pkg fix).
@ethanasm
ethanasm merged commit 41dea59 into main Aug 14, 2026
2 checks passed
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