Uh oh!
There was an error while loading. Please reload this page.
test(cli): reach the built CLI in the three serve e2e spawners - #12459
Merged
Conversation
Adopt the honest-and-fast spawn shape measured in #11462: spawn `bin/run.js` with plain `node` and leave the child's `NODE_ENV` UNSET, which is the value that keeps @oclif/core's TypeScript path lookup disabled so the command resolves from `packages/cli/dist/commands`. The `--dev` admin seed these fixtures sign in as survives an unset `NODE_ENV`: `serve.ts` assigns `process.env.NODE_ENV = 'development'` in-process for `--dev` before `runtime.start()`, and plugin-auth's `isDevAdminSeedArmed()` reads it at call time inside the `kernel:ready` hook, after that assignment. Each file gains a `requireBuiltCli()` preflight so an unbuilt tree fails with a sentence and a build command instead of oclif's cryptic "command serve not found". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
Contributor
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
This was referenced Aug 26, 2026
`serve-node-env-production-default.e2e.test.ts`'s "WHY THIS FILE IS THE REASON @objectstack/cli#test DECLARES build" section claimed to be the only file that consumes `packages/cli/dist` "and the only one that can be", and described the three MCP/stdio spawners in the present tense as pinning `NODE_ENV: 'development'` and therefore never touching `dist/`. This branch makes all three of those statements false. Corrected to what is measured: four files consume `dist/` today, the historical account of why the missing build stayed invisible is kept but tensed to the past, and the reason the new state is safe is named — the `dependsOn: ["build"]` declaration, not this file. Surface extension granted by the dispatching PM after the first report; nothing else in the file is rewritten. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
os-litant
marked this pull request as ready for review
August 26, 2026 04:50
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#11707
Item 1 only, per triage's scope ruling on the card. The guard (item 2) belongs to #11464 and nothing here implements it.
What changed
The three
packages/cliserve e2e spawners now use the honest-and-fast shape measured in #11462:NODE_ENVtsx bin/run-dev.jsdevelopmentsrc/commandsnode bin/run.jsundefined, which Node'sspawn()omits)dist/commandsFiles:
serve-mcp-stdio-answers.e2e.test.ts,serve-mcp-capability-collision.e2e.test.ts,serve-stdio-stdout-purity.e2e.test.ts.A fourth file,
serve-node-env-production-default.e2e.test.ts, carries a comment-only correction — see "The paragraph this change falsified" below. That file was added to this task's declared surface by the dispatching PM after the first review round, explicitly granted rather than taken; nothing in it changes behaviour and no other file is touched.The two enabling premises, re-measured on this tree — both hold
(a) the build is declared.
turbo.jsonstill reads"@objectstack/cli#test": { "dependsOn": ["build"] }(its own build, not^build), sopackages/cli/distreally is built for these tests in CI.(b) an unset
NODE_ENVstill keeps the dev-admin seed.packages/cli/src/commands/serve.ts:1298still does the in-process assignment for--dev:and the seed's gate still reads the variable at CALL time, inside the
kernel:readyhook that fires after that assignment —plugin-auth'smaybeSeedDevAdmindelegates toisDevAdminSeedArmed()inwalled-owner-verification-path.ts, whoseenv()helper readsglobalThis.process.envfreshly on every call. All five tests boot, sign in as the seeded admin, and mint a realosk_key: the seed runs.The line moved (the card cited
serve.ts:1272, it is1298today) but the shape is intact.Proof the suites now reach
dist/commands— an instrument that can say noA distinct marker was planted in each tree at once and the spawn recipe read back which one executed. Same fixture, same env, same argv:
NODE_ENVdistmarkersrcmarkernode bin/run.jsnode bin/run.jsdevelopmenttsx bin/run-dev.jsdevelopment)Rows B and C are the negative controls: the same marker pair, the same probe, the opposite answer. B is exactly the pair #11317 found in these files — it promises the built artifact and delivers source, with a green boot and nothing in the output saying so.
Suite-level ablation. Planting
process.stdout.write('OS11707_NOISE')intopackages/cli/dist/commands/serve.jsturnsserve-stdio-stdout-purity.e2e.test.tsRED, on the planted byte:dist/was mutated directly, not through a source edit plus a rebuild: the question is which tree the spawn reads, and a rebuild would land the marker in both. Both mutation legs were confirmed on disk before the run (grep -con the planted literal, plusnode scripts/ablation-dist-preflight.mjs @objectstack/cli 'OS11707_NOISE'reportingmarker present in 1 built file), and both restore legs were proved after:sha256sumofdist/commands/serve.jsidentical to the pre-mutation value,git diff HEADempty for the source leg, andablation-dist-preflight ... --absentreportingmarker absent from all 432 built files.The build prerequisite is now legible instead of cryptic
Turbo guarantees the build in CI and guarantees nothing for a developer running
vitestdirectly. Each of the three files gains arequireBuiltCli()preflight, called first inbeforeAll. Withdist/commands/serve.jsremoved, the suite now says:It fails in seconds with three tests skipped, rather than through three 150-second boot timeouts carrying oclif's wrapped
command serve not found. The command target is read from the CLI's ownoclif.commands.targetrather than restated, for the reasonscripts/cli-build-prerequisite.mjsgives; an unreadable or differently-shaped declaration defers rather than failing, so a checkout it cannot understand never turns red here.It does not catch a
dist/that is merely BEHIND its source. That residual is real and is stated in each file's header.serve-node-env-production-default.e2e.test.tshas carried the identical residual since #11113.The paragraph this change falsified, corrected in the same PR
serve-node-env-production-default.e2e.test.ts's section headed "WHY THIS FILE IS THE REASON@objectstack/cli#testDECLARESbuild" is the repo's only written account of why that turbo dependency exists — which is the premise this PR's safety rests on. This branch made three of its statements false:packages/clithat genuinely consumespackages/cli/dist, and it is the only one that can be" — there are four now.NODE_ENV: 'development'to the child" — present tense, no longer true.dist/at all" — same.Corrected to what is measured, and no further: the count is stated (four: this file plus the three below), the historical account of why the missing build stayed invisible under
dependsOn: ["^build"]is kept but tensed to the past, and one new paragraph names why the new state is safe — thedependsOn: ["build"]declaration, not this file. The surrounding prose (the origin-check rationale, the anti-vacuity section, the closing refusal to switch this file tobin/run-dev.js) is untouched. 20 insertions, 6 deletions.Measured before writing it, not recalled: of the five other files naming
bin/run.js,plugin-commands.test.tsandinvocation-loudness.e2e.test.tsonly assert the path as a string (invocation-loudnessspawnstsx bin/run-dev.jsand usesBINsolely as an expected substring), and the three that spawn it are the three this PR moved. So the original sentence's structure survives; only its outcome changed.Speed, re-measured on this box — the card's ratio holds, its absolute numbers do not
Three files, one
vitest run --maxWorkers=2, warm build, under this container's shared verify lock so neither run competed with a parallel build:Durationteststsx bin/run-dev.js)node bin/run.js)2.06x in test time (44.93s to 21.76s). The card's table claimed 2.2x (68.01s to 30.26s) from #11462 on a different box; its absolute numbers do not reproduce here — this box is roughly 1.5x faster on both rows — but the gap the card is arguing from has not narrowed. The case for this change survives on the ratio, not on the numbers.
Where this diff is bigger than the card predicted
The card said option 2 is "three lines in the same three files". The behavioural core is five lines per file, not three:
const CLI, thespawn()argv0, theNODE_ENVvalue, plus two import lines (TSXbecomes unused, andexistsSync/readFileSyncare needed by the preflight). The rest is therequireBuiltCli()preflight and the header comments, which asserted the opposite of the new code and could not be left standing.Verification
Run on
69f95990(branch head;origin/main68c5dbaabmerged in), afterturbo run build --filter=@objectstack/cli(Tasks: 56 successful, 56 total):Test Files 4 passed (4),Tests 8 passed (8),Duration 22.00sTest Files 1 passed (1),Tests 3 passed (3)pnpm lint(eslint . --no-inline-config, whole repo, not narrowed): exit 0, no output. Re-run over the four changed files after the last edit: exit 0, no output.pnpm --filter @objectstack/cli typecheck:tsc --noEmitexit 0 — butpackages/cli/tsconfig.jsonis"include": ["src"], so that program does not readtest/**at all (--listFiles: 0 hits for these files). That layer is the package's recordedTEST_DEBTentry (146 errors / 65 hidden files), which CI re-measures. Measured directly instead, same location, same synthesized config extending the package tsconfig,TS6059dropped exactly asmeasureTestDebtdoes, both tree states: 0 errors before, 0 errors after. This diff cannot move that ratchet up.check:cli-test-child-env—35 spawner source(s) among 95 under packages/cli/test/**; no new bulk process.env copy reaches a spawned childcheck:cross-package-test-inputs—OK: 18 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob(the preflight readspackages/cli/package.json, which is inside the package and escapes nothing)check:test-source-alias—OK — 72 packages with tests scanned; 61 registered as still resolving a workspace dep through dist/check:nul-bytes—OK (scanned 6892 text file(s) ...; no raw ASCII control bytes)check:comment-mask-adoption—OK — 23 private comment-stripper(s) ... all 23 recordedcheck:type-check-coverage,check:engine-double-contract,check:where-matcher,check:query-options-erasure,check:published-files,check:slot-lookup,check:page-declaration-shape,check:type-source-resolution,check-ci-filter-parity,check-plugin-teardown-shapeGate families re-derived in this worktree with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackafter the fourth file was added: change set of exactly these four paths, same family set, no new family.No changeset: test-only, nothing published changes behaviour.
One prose site left untouched on purpose
packages/cli/vitest.config.ts's cost header states the opposite policy: "Swapping the spawns to the built entry would halve per-spawn boot and is exactly the source-vs-dist tradescripts/check-test-source-alias.mjsexists to refuse." The gate itself does not contradict the ruling and is green here —check-test-source-aliasjudges in-process import resolution of test files and says nothing about what a spawned child resolves — but the header's prose does.Unlike the paragraph corrected above, this one is not falsified by this change: it is a standing policy statement that triage's ruling contradicts, and it predates this branch. The dispatching PM ruled it out of this PR and filed it separately.
Generated by Claude Code