fix(cli): make the published package work outside the monorepo (#1315) - #1345
Conversation
release.yml publishes @neoboard/cli on every `v*` tag, so the first tag makes
`npx @neoboard/cli setup` real for strangers. It failed immediately, twice
over.
**Root detection required a checkout.** findProjectRoot walked up from the
CLI's own install location looking for the monorepo; under npx that is an npm
cache directory, and it THREW. A throw that deep in a path helper surfaced as
an unrelated-looking crash — setup died in root detection, reporting nothing
about standalone mode. It now returns null and resolveRoot decides: the
checkout when there is one, else $NEOBOARD_DIR, else ./neoboard, created on
demand. The monorepo always wins, so a stray NEOBOARD_DIR cannot silently
redirect a contributor.
**The package shipped nothing to start.** `files` listed only dist/README/
LICENSE. Adding "docker" to it does NOTHING — npm resolves `files` relative to
the PACKAGE root and docker/ lives one level up, at the REPO root. npm pack
succeeds and ships no compose file, silently.
That matters more than the fix: my first packaging test asserted
`pkg.files.includes("docker")`. It passed. It proved nothing, and would have
let this ship a second time. The test now runs `npm pack --dry-run --json` and
asserts on what would actually be published. A prepack step stages the four
assets a standalone install can use into cli/docker/ (gitignored, removed by
postpack).
**Commands that genuinely need the source say so.** dev, db seed and the
plugin commands run scripts/ tooling that is deliberately not shipped — 568K
to make one command work is the wrong trade. assertCheckout names the command,
gives the clone commands, and lists what does work standalone.
**Errors no longer surface as stack traces.** program.parse() left a rejected
action to Node's unhandled-rejection dump: the message buried under a trace
rooted in dist/, reading as a crash rather than as the CLI telling you
something. parseAsync().catch prints the message and exits 1.
Two existing tests asserted the throw that is now a null. The #991 one still
proves what it was written for — a non-terminating loop would hang rather than
return null — so only the assertion changed, and the comment says so.
Verified against a real packed tarball installed outside the monorepo:
neoboard --help works
neoboard config list works, creates ./neoboard/
NEOBOARD_DIR=... config list honours the override
neoboard status exit 0
neoboard dev clean message, exit 1, no stack
node cli/dist/index.js dev in-monorepo path unaffected
tarball contains docker/docker-compose.prod-full.yml
Also fixes the prepack script logging to stdout, which corrupted
`npm pack --json` for every consumer, not just this test.
Closes #1315
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alfredo1996
added
area:devex
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe CLI now supports standalone root resolution, blocks checkout-only commands outside a repository, reports asynchronous command errors cleanly, and stages selected Docker assets during package creation. Tests cover root detection, standalone behavior, command mocks, and published package contents. ChangesStandalone CLI support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLIEntrypoint
participant Config
participant Filesystem
CLIEntrypoint->>Config: Execute command and call assertCheckout
Config->>Config: Resolve checkout or standalone root
Config->>Filesystem: Create standalone root when needed
Config-->>CLIEntrypoint: Continue or reject with checkout error
CLIEntrypoint-->>CLIEntrypoint: Log rejection and set exit code 1
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/src/__tests__/lib/standalone.test.ts`:
- Around line 85-113: Update the standalone-root tests using resolveRoot to also
verify filesystem creation: import or reuse existsSync, assert existsSync(dir)
in the NEOBOARD_DIR test, and assert existsSync(resolved) in the fallback test
after resolution. Keep the existing path assertions and monorepo-root behavior
unchanged.
- Around line 27-35: Update the standalone test cleanup in afterEach to capture
the pre-suite NEOBOARD_DIR value and restore it after resetting
_setRootForTesting, rather than deleting the variable unconditionally; preserve
the current deletion behavior when no original value existed.
- Around line 150-156: Update the cliDir initialization in the package-file
discovery test to convert the URL with fileURLToPath(new URL("../../..",
import.meta.url)) instead of using .pathname, and ensure the required
URL-to-path utility is imported. Continue passing cliDir as the execFileSync
cwd.
In `@cli/src/lib/config.ts`:
- Around line 65-72: Update resolveRoot() and isStandalone() to call
findProjectRoot(process.cwd()) so published CLI invocations resolve a checkout
from the current working directory before any module-path fallback; preserve the
existing standalone directory behavior when no checkout is found, and add a
regression test covering invocation from a NeoBoard checkout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13c9d88f-e69c-422e-ae3c-f122e6483b92
📒 Files selected for processing (29)
.gitignorecli/package.jsoncli/scripts/stage-compose.mjscli/src/__tests__/commands/config.test.tscli/src/__tests__/commands/db/dump.test.tscli/src/__tests__/commands/db/migrate.test.tscli/src/__tests__/commands/db/reset.test.tscli/src/__tests__/commands/db/seed.test.tscli/src/__tests__/commands/demo.test.tscli/src/__tests__/commands/dev.test.tscli/src/__tests__/commands/doctor.test.tscli/src/__tests__/commands/env.test.tscli/src/__tests__/commands/init.test.tscli/src/__tests__/commands/logs.test.tscli/src/__tests__/commands/plugin-hints.test.tscli/src/__tests__/commands/plugin.test.tscli/src/__tests__/commands/start.test.tscli/src/__tests__/commands/status.test.tscli/src/__tests__/lib/bootstrap-status.test.tscli/src/__tests__/lib/config.test.tscli/src/__tests__/lib/credential-probe.test.tscli/src/__tests__/lib/docker-env.test.tscli/src/__tests__/lib/docker.test.tscli/src/__tests__/lib/standalone.test.tscli/src/commands/db/seed.tscli/src/commands/dev.tscli/src/commands/plugin.tscli/src/index.tscli/src/lib/config.ts
CI caught what my local run had not: the packaging test asserts on the real `npm pack` output, and CI has no cli/dist, so the tarball genuinely lacked dist/index.js. The test was right — a package packed without a build IS broken, and the release workflow only escapes it by running an explicit build step first. prepack now builds, which is what prepack is for, so `npm pack` produces a complete package for anyone rather than only for the one workflow that happens to build first. prepublishOnly's build is redundant and removed. Also replaces `postpack: rm -rf docker`. That path is relative to whatever directory npm runs it from, and one level up is the repo's real docker/. Both paths now come from import.meta.url, so cwd cannot make it delete the wrong tree. Verified from a clean dist with the exact invocation release.yml uses: `npm publish --workspace=@neoboard/cli --dry-run` stages the compose files, ships dist/index.js, and removes cli/docker afterwards. Checked and dismissed: that dry-run prints `"bin[neoboard]" script name dist/index.js was invalid and removed`. It is pre-existing on release/1.4 and harmless — the packed manifest keeps bin, and installing the tarball produces a working node_modules/.bin/neoboard (`neoboard --version` -> 1.0.0). npm is normalising an in-memory manifest for the dry-run, not the package. Refs #1315 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
CI caught a real gap my local run missed: the packaging test asserts on the actual The test was right. A package packed without a build is broken — the release workflow only escapes it because it runs an explicit Also replaced Verified from a clean One thing I checked and dismissed, since it looks alarming in the dry-run output: |
…the CLI lives Four CodeRabbit findings, all valid. The Major one is a genuine behavioural gap: resolveRoot and isStandalone searched from import.meta.url only, so a globally installed or npx'd CLI run INSIDE a checkout was treated as standalone — `dev` and `db seed` refused, and `config list` created ./neoboard inside the user's own source tree with the repo right there. Now searches process.cwd() first and falls back to the module path, which still covers the monorepo's own `node cli/dist/index.js` run from elsewhere. Verified with the real installed binary, both directions: from inside the checkout config get -> 3000, dev proceeds, no ./neoboard from a bare temp dir config get -> 3000, dev declines, ./neoboard made The three Minor ones: the standalone-root tests asserted only the RETURNED PATH, so deleting mkdirSync would still have passed — the loose-assertion shape I have been cataloguing all session, in my own test. They now assert the directory exists. afterEach deleted NEOBOARD_DIR unconditionally, clobbering a value the developer or CI may have set. Restores it now. `new URL(...).pathname` keeps the leading slash and percent-encodes spaces, so a checkout under "Program Files" would pass a broken cwd to execFileSync. fileURLToPath instead — this repo has already been bitten by a Windows path bug (#991). Refs #1315 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All four addressed — every one was valid. The Major one was a genuine behavioural gap I had missed. Verified with the real installed binary, both directions:
Plus a regression test for the checkout invocation, as suggested. The three Minor ones:
346 CLI unit tests pass; lint clean. |
|
2 participants
What
release.ymlpublishes@neoboard/clion everyv*tag, so the first tag makesnpx @neoboard/cli setupreal for strangers. It failed immediately, twice over.1. Root detection required a checkout.
findProjectRootwalked up from the CLI's own install location looking for the monorepo; undernpxthat's an npm cache directory, and it threw. A throw that deep in a path helper surfaced as an unrelated-looking crash —setupdied in root detection, reporting nothing about standalone mode.It now returns
null, andresolveRootdecides: the checkout when there is one, else$NEOBOARD_DIR, else./neoboard, created on demand. The monorepo always wins, so a strayNEOBOARD_DIRcan't silently redirect a contributor's checkout.2. The package shipped nothing to start.
The part worth reading
Adding
"docker"tofilesdoes nothing. npm resolvesfilesrelative to the package root;docker/lives one level up at the repo root.npm packsucceeds and ships no compose file, silently.That matters more than the fix, because my first packaging test asserted
pkg.files.includes("docker")— and passed. It proved nothing and would have let this ship a second time. The test now runsnpm pack --dry-run --jsonand asserts on what would actually be published. Aprepackstep stages the four assets a standalone install can use intocli/docker/(gitignored, removed bypostpack).Commands that genuinely need the source now say so
dev,db seedand the plugin commands runscripts/tooling that's deliberately not shipped — 568K to make one command work is the wrong trade.assertCheckoutnames the command, gives the clone commands, and lists what does work standalone.Errors no longer surface as stack traces
program.parse()left a rejected action to Node's unhandled-rejection dump — the message buried under a trace rooted indist/, reading as a crash rather than as the CLI telling you something.parseAsync().catchprints the message and exits 1.Two existing tests changed contract
Both asserted the throw that is now a
null. The #991 one still proves what it was written for — a non-terminating loop would hang rather than return null — so only the assertion changed, and the comment says so.Verified against a real packed tarball, installed outside the monorepo
Also fixes the prepack script logging to stdout, which corrupts
npm pack --jsonfor every consumer, not just this test.344 CLI unit tests pass; lint clean. (
integration/demo-flow.test.tsneeds a running stack and fails identically on a stashed tree.)Closes #1315
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests