test: Add CI for npm-installed CLI#5542
Conversation
The boxel-cli suite only ever called command functions in-process inside the monorepo, where pnpm's nested node_modules masks how the package behaves once installed from npm. `boxel parse` shipped broken — glint silently resolved nothing under npm's hoisted node_modules layout — despite a fully green suite. Add a subprocess-driven harness whose only per-context variable is the binary under test (BOXEL_CLI_BIN): - tests/helpers/run-boxel.ts drives the CLI as a subprocess (argv + env + stdin -> stdout/stderr/exit). - tests/helpers/integration.ts seeds a JWT profile on disk so a spawned CLI authenticates via a temp HOME with no Matrix round-trip. - scripts/run-cli-suite.ts installs the target and runs a test command against it: `pnpm pack` (resolves catalog:/workspace: specifiers) then `npm install` the tarball for real hoisting, or `npm install` from the registry for the published artifact. Wire two contexts into CI: the PR gate runs the suite against the packed npm install (test:cli:tarball), and the publish workflow smoke-tests the just-published registry artifact (verify-unstable / verify-stable). Add parse fixtures covering the reported resolution gaps plus a deliberate type error that proves glint actually ran, and port smoke.test.ts onto the harness. This is the test/CI half of CS-12083. The parse fix is a follow-up, so the parse fixtures are expected to fail against the installed binary until it lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rewrite the tests/integration suite to drive the installed `boxel` binary via runBoxel instead of calling command functions in-process, so the same tests exercise the CLI as installed from npm (the tarball PR gate and the published post-release smoke). - Add createTestRealmViaCli(home): create a realm through `boxel realm create` and read its URL back from the profile the CLI wrote to disk. - Each test seeds a profile on disk (createTestHome + setupTestProfile / setupJwtTestProfile), runs the command under test via runBoxel, and verifies realm/DB/checkpoint state in-process via reloadProfile(home). - Text content rides stdin; binary content is staged to a temp --file. - Commands with --json are asserted via res.json(); others via stdout (the CLI drops ANSI colors when stdout is not a TTY) and exit code. A few tests stay in-process by necessity, coexisting with the subprocess tests in the same files: white-box cases that mock/spy the internal fetch (request-body shape, injected non-2xx responses) can't cross the process boundary, and realm-watch is the long-running `watch start` daemon plus the RealmWatcher internal API, which a subprocess can't drive without weakening assertions. Not run locally (needs the realm-server + Postgres + Matrix stack); eslint clean across the suite. Expect CI iteration to settle argv and output-assertion details. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a77f4dc3f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (result.status === 0 && result.stdout.trim()) { | ||
| console.log(`Resolved ${PKG_NAME}@${version} → ${result.stdout.trim()}`); | ||
| return; |
There was a problem hiding this comment.
When unstable or latest already points to an older release, this condition succeeds immediately even if the newly published dist-tag has not propagated, because the workflows pass those tags at .github/workflows/boxel-cli-publish.yml:550 and :565. npmInstall() can consequently install the previous artifact and report a green post-publish verification for code that was never tested; propagate the concrete published version from the publish jobs and wait until that exact version resolves.
Useful? React with 👍 / 👎.
| name: 'tracked-format-class', | ||
| covers: '@tracked inside a static isolated format class', | ||
| }, | ||
| { name: 'runtime-common', covers: 'bare @cardstack/runtime-common import' }, |
There was a problem hiding this comment.
The packed-install integration run cannot pass this fixture: @cardstack/runtime-common is only a devDependency in packages/boxel-cli/package.json, so npm does not install it for the packed CLI, and the generated parse tsconfig in src/commands/parse.ts has no path for the bare package. Glint therefore reports a module-resolution diagnostic for runtime-common/home.gts, contradicting the errors === [] assertion and making the new Boxel CLI CI job fail until this dependency is shipped or vendored/mapped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this is recorded in CS-12254
| covers: '@tracked inside a static isolated format class', | ||
| }, | ||
| { name: 'runtime-common', covers: 'bare @cardstack/runtime-common import' }, | ||
| { name: 'boxel-host-tools', covers: '@cardstack/boxel-host/tools/* import' }, |
There was a problem hiding this comment.
This clean fixture imports @cardstack/boxel-host/tools/save-card, but src/commands/parse.ts only maps @cardstack/boxel-host/commands/*, and scripts/build-types.ts excludes the host app's tools directory from HOST_APP_REACHABLE_SUBDIRS. Thus the npm-installed CLI has neither a path mapping nor bundled source for this import, so the fixture necessarily produces a resolution error and fails the newly enabled integration job.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
the failure is deliberate, and fixed in #5551
The post-publish verify jobs installed @cardstack/boxel-cli@unstable / @latest. A dist-tag always resolves to some prior release, so waitForPublishedVersion returned on the first poll regardless of whether the version just published had propagated, and npmInstall could pull the previous artifact — a green smoke-test of code this run never installed. Thread the concrete version out of the publish jobs (a `version` job output, read from package.json in the unstable publish step and from the strip step for stable) and pass it to run-cli-suite, so the poll waits for that exact version to resolve and the install fetches it. Also warn in run-cli-suite when --version is a dist-tag rather than a concrete version, so this can't silently regress. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖]
Lens. This reviews the new test-execution machinery — the runBoxel subprocess helper and the run-cli-suite.ts install-and-run harness — the CI wiring that drives it, and whether the ~30 in-process→subprocess test conversions preserve behavior. I checked out the head branch and traced the module-resolution and packaging claims against the actual parse tsconfig and bundled-types/.
Bottom line: the design is sound and I found no blocking correctness bug in the new machinery. The one thing to get right before merge is sequencing — the suite is deliberately red and can only go green once two separate follow-ups land, not one (see thread 2 below). Two non-blocking cleanups are in inline threads.
What lands right.
- The split between which binary runs (
BOXEL_CLI_BIN, read once in the parent) and how the suite drives it (identicalrunBoxelcalls) is the right seam: one suite, three execution contexts (localdist, packed tarball, published registry), no per-context test forks. sanitizedParentEnv()strippingBOXEL_*is a real correctness win, not just hygiene — it makes a developer's shell match CI, and the smoke test asserting a leakedBOXEL_ENVIRONMENTcan't shift behavior pins it.- The conversions strengthen coverage rather than merely relocating it: the error-path tests now assert
exitCodealongside the JSON payload, which the in-process calls couldn't observe.parse.test.ts'splain-glimmerfixture is a faithful superset of the removedparse-glimmer-types.test.ts, and thedeliberate-type-errorfixture that proves glint actually ran (guarding the "produced no TS diagnostics" false-pass) is the crux of the whole PR and is well-constructed. pnpm pack(notnpm pack) to rewritecatalog:/workspace:*, thennpm installfor hoisting, is the correct combination to reproduce a real user'snode_moduleslayout.
On the open review threads (verified against the branch):
-
"Wait for the exact published version" (P1) — fixed correctly.
boxel-cli-publish.ymlnow exposes aversionoutput from each publish job (read frompackage.json), and both verify jobs installneeds.<job>.outputs.version— a concrete semver, not the movingunstable/latestdist-tag.waitForPublishedVersiononly meaningfully polls for a concrete version and loudly warns on a dist-tag. The chain is airtight. (As a bonus confirmation:verify-stablecorrectly omits thepublishedguard thatverify-unstableneeds — the stable job always publishes when it succeeds, whereas the unstable push path can no-op.) -
"
runtime-commonfixture can't resolve" (P1) — confirmed, and this is the sequencing point.@cardstack/runtime-commonis adevDependency(workspace:*), so it isn't installed in the npm layout. The generated parse tsconfig maps@cardstack/base/*,@cardstack/host/*,@cardstack/boxel-host/commands/*,@cardstack/boxel-ui/*, and a'*'catch-all →bundled-types/host-types/. The bare@cardstack/runtime-commoninfixtures/parse/runtime-common/home.gtsfalls through to that catch-all, andbundled-types/host-types/@cardstack/ships onlyhost/, notruntime-common/— so that fixture'sexpect(errors).toEqual([])fails today. Per your own thread notes, theboxel-host-toolsgap is fixed in the companion PR butruntime-commonis tracked separately, so the parse suite needs both fixes to go green; the companion PR alone leaves theruntime-commoncase red. -
"
boxel-host-toolsfixture" (P1) — deliberate, no action here. Same mechanism (no path mapping, andtoolsis excluded from the bundled host subdirs); addressed by the companion PR.
Recommendations.
- Sequence the merge so
mainis never left with a red CLI job. This flips the existing green "Boxel CLI Tests" job to run the packed install, which fails until both theboxel-host-toolsfix and theruntime-commonfollow-up land — turning that job red on every unrelated PR and failingverify-unstableon every subsequent unstable publish. Land them together, or, if they can't ship together, mark the two known-failing fixtures pending (a documentedit.fails/skip keyed to the missing mappings) so the demonstration is preserved without reddeningmain. (Non-blocking on code; blocking on merge order.) run-boxel.ts: set the stream encoding to avoid multi-byte UTF-8 corruption on chunk boundaries — see thread. (Non-blocking.)- Drop the dead
new ProfileManager(...)statement duplicated across ~15 converted tests — see thread. (Non-blocking.)
Adjacent, out of scope. waitForPublishedVersion shells out to sleep via execFileSync('sleep', ...), which is POSIX-only. Harmless because it runs only on the Linux publish runners, but it's the one spot where the otherwise cross-platform helpers (the rest honors USERPROFILE for Windows) wouldn't run on Windows. Not asking for a change.
Generated by Claude Code
Set the stdout/stderr encoding so Node's StringDecoder buffers partial multi-byte sequences across `data` events. Accumulating `Buffer` chunks with `+=` decoded each chunk independently, so a multi-byte character split across a pipe boundary became U+FFFD — a latent flake for realm content with non-ASCII text read back through `file read` and `--json` payloads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ests The `new ProfileManager(emptyHome/.boxel-cli)` line in the no-active- profile tests is never used — the behavior comes entirely from spawning the CLI with HOME pointed at an empty dir, which has no profiles.json. Remove the statement (and its now-unused import, or narrow it to `import type` where ProfileManager is still referenced as a type) so these tests read as exactly what they check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fix PR is already approved. My plan is to merge this once approved, then rebase the fix in #5551, then merge it. I know this will result in |
2 participants
boxel parseis silently broken in the npm release of Boxel CLI. The test suite didn’t catch this because it runs using monorepo conventions, not the npm package.This makes several changes:
runBoxeltest helper to test CLI behaviour, not in-code behaviourdist/index.jsas before or path to an npm-installed packageDeliberate test failure
The failed CI job is known, to demonstrate the problem of undetected brokenness, and that this new CI structure would fix it. The fix that causes the test to pass is in #5551, separated for demonstration and ease of review.