Skip to content

test(cli): three e2e spawners name the entrypoint they actually reach - #11462

Draft
claude[bot] wants to merge 1 commit into
mainfrom
claude/issue-11317-e2e-shipped-entrypoint
Draft

test(cli): three e2e spawners name the entrypoint they actually reach#11462
claude[bot] wants to merge 1 commit into
mainfrom
claude/issue-11317-e2e-shipped-entrypoint

Conversation

@claude

@claude claude Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Part of #11317.

⚠️ Part of, not the closing keyword, is deliberate — see "A ruling is
needed before this merges" below. The card's direction is settled by triage,
but one of the two facts that settled it has since become false, and the
option triage rejected now measures cheaper and truthful. Flip this line to
the closing keyword (or close the card by hand) once the ruling lands.

What was false

serve-mcp-stdio-answers.e2e.test.ts, serve-mcp-capability-collision.e2e.test.ts
and serve-stdio-stdout-purity.e2e.test.ts each carried the identical comment:

/** `bin/run.js` — the SHIPPED entrypoint, i.e. the one the card's repro names. */
const CLI = resolve(HERE, '../bin/run.js');

They spawned that stub and never reached packages/cli/dist. All three pin
NODE_ENV=development on the child for the --dev admin seed, and
@oclif/core@4.13.3 skips its TypeScript path lookup only when isProd(),
which lib/util/util.js:66 defines as a negated membership test — a
leading logical-NOT applied to
['development', 'test'].includes(process.env.NODE_ENV ?? ''). Under either of
those two values oclif rewrites the command target from the declared
./dist/commands to ./src/commands.

⚠️ The leading NOT is spelled out in words here on purpose. A bang
immediately before a bracket is image syntax, and GitHub's body sanitizer
eats it — inside a code span and inside a fence alike. The card's own body
quotes isProd() without it, and so does the dispatch, which inverts the
predicate: as written there it would mean oclif treats development and
test as production. The three source comments this PR adds carry the
correct sign, because files are not sanitized.

Re-derived here rather than transcribed, with packages/cli/dist present
(@oclif/core's own Config.load() against packages/cli, reading back the
root plugin's resolved commandsDir):

child NODE_ENV resolved commandsDir
unset packages/cli/dist/commands
development packages/cli/src/commands
test packages/cli/src/commands
production packages/cli/dist/commands

Premise correction — the card's fact (1) is stale

The card composes two facts. Fact (1) is no longer true. It states
turbo.json declares "@objectstack/cli#test": { "dependsOn": ["^build"] }
dependencies only — so dist is never built for these tests. On main today
that entry reads "dependsOn": ["build"]. It was changed by #11268
(918988ad), the same PR the card was filed from, and the card's base
(adb1b9ea8c) predates it.

Re-measured with the card's own instrument,
pnpm exec turbo run test --filter=@objectstack/cli --dry=json:

card @ adb1b9ea8c main @ 5a916c4d
tasks 58 59
#build tasks 57 58
@objectstack/cli#test deps contain @objectstack/cli#build no yes

So packages/cli/dist is built for these tests now. The defect survives on
fact (2) alone — the ts-path reroute — which is sufficient on its own, so the
card's conclusion stands and its cause is now single, not composite.

What this PR does

The triage-settled remedy (5386559160: "test-truthfulness restoration, no
product fork"
): the three files stop claiming an entrypoint they do not reach.
They now spawn bin/run-dev.js through tsx, exactly like the sibling e2e
files in this directory and like helpers/serve-process.ts's own runServe(),
and the false comment is replaced by the mechanism that made it false.

Per file the diff is four things: the CLI constant, the comment above it,
spawn(process.execPath, …) becoming spawn(TSX, …) (TSX imported from the
existing helper, so no new package-escaping path literal), and three lines
recording why the NODE_ENV override stays even though bin/run-dev.js
assigns it too — the shim's assignment runs after its own static imports have
evaluated, so only the child env pins the value for the whole process lifetime.

Nothing else changed. turbo.json is untouched (no build dependency is being
declared here — a false claim is being removed). vitest.config.ts is
untouched, for a reason given below. serve-node-env-production-default.e2e.test.ts
— the one file in packages/cli that genuinely consumes dist/, because it is
the only one spawning with NODE_ENV unset — is untouched, and nothing here
alters what it measures.

A ruling is needed before this merges

The card offered a second option and triage rejected it as "new engineering"
because it needed a build dependency declared and another route to the
--dev admin seed. Both halves have since landed on their own, so measured
today option 2 is a three-line change confined to the same three files:

  • the build dependency is already declared (the premise correction above);
  • the seed no longer needs NODE_ENV=development from the parent.
    commands/serve.ts:1272 sets it in-process: if (flags.dev) { if (!process.env.NODE_ENV) process.env.NODE_ENV = 'development'; }, and
    plugin-auth's maybeSeedDevAdmin reads process.env.NODE_ENV !== 'development' at call time, inside runtime.start() — after that
    assignment. Passing NODE_ENV: undefined therefore keeps the seed and
    leaves the reroute disabled.

Measured, all three suites, same tree, same box, one lock acquisition for the
two legs that share one:

shape spawn child NODE_ENV reaches result wall test time
before (main) node bin/run.js development src/commands 3 files / 5 tests pass 27.28s 46.76s
this PR tsx bin/run-dev.js development src/commands 3 files / 5 tests pass 39.56s 68.01s
option 2 node bin/run.js unset dist/commands 3 files / 5 tests pass 17.68s 30.26s

Option 2 passes, is the fastest of the three, and is the only one that makes
the original comment true. This PR is the slowest of the three: moving onto
tsx costs these files roughly +21s of test time, which runs against the
direction vitest.config.ts's cost header reasons in.

⛔ Not taken unilaterally. The argument against option 2 is real and is this
repo's stated policy: it would make these three suites verdicts about build
state
, which is exactly what scripts/check-test-source-alias.mjs and the
vitest.config.ts header exist to refuse — a suite that passes green against a
stale artifact, with nothing in the output saying so. Turbo now guarantees the
build in CI; it guarantees nothing for a developer running vitest directly,
who would get oclif's "command not found". Option 2 also leaves the two
properties silently self-cancelling again for the next author, which is why the
card floated a lint rule.

Maintainer/PM call. This PR implements the settled direction and is mergeable
as-is; option 2 is one NODE_ENV: 'development'NODE_ENV: undefined per
file away, in the same declared surface.

Verification

Union re-run on the final commit, 4b7a8453, working tree clean.

  • The three suites, on this branch: 3 files / 5 tests passed, 39.56s
    (pnpm --filter @objectstack/cli exec vitest run … --maxWorkers=2). The
    before/after table above is the evidence the coverage survived, not the green
    alone: same 3 files, same 5 tests, same assertions, both legs green.
  • pnpm --filter @objectstack/cli typecheck — green (tsc --noEmit). ⚠️ This
    does not cover the edited files: packages/cli/tsconfig.json has
    include: ["src"], so test/** sits outside the program. What exercises the
    edits is the vitest run above, which executed them.
  • Path-derived gate families, each exit 0 with its own verdict line:
    check:nul-bytes, check:test-source-alias, check:cross-package-test-inputs,
    check:published-files, check:slot-lookup, check:type-source-resolution,
    check:engine-double-contract, check:where-matcher,
    check:query-options-erasure, check:type-check-coverage,
    check-ci-filter-parity.mjs, check-plugin-teardown-shape.mjs,
    docs-audit/check-affected-docs.mjs.
  • check:type-check-debtnot measured locally, and it says so itself
    rather than going red: --re-measure refuses without the whole workspace
    closure built (@objectstack/service-knowledge has no built type entry
    point). That is a full-repo build and belongs to CI's lint job, which runs it
    after exactly that build. Its structural half printed OK before the refusal.
  • Declared narrowing — repo-wide pnpm lint was not run; targeted eslint
    was
    , with the three pieces of evidence that make the narrowing a
    measurement rather than a gap: (1) population read from eslint's own
    resolution — all three files came back as results with no "file ignored"
    warning, so eslint's own config puts them in scope; (2) file count read from
    --format json — 3 results, 0 errors, 0 warnings, exit 0; (3) config
    invariance — this repo runs one eslint.config.mjs which never enables
    type-aware linting for any file (no parserOptions.project, no typed @typescript-eslint rules, documented and measured with a positive control at
    eslint.config.mjs:327-328), so this diff cannot move the verdict on any
    file it does not touch.

Not changed, and why

Changeset

None, deliberately. The diff is three files under packages/cli/test/;
nothing published changes and there is nothing for release notes to say. The
skip-changeset label carries that declaration.


Generated by Claude Code


Generated by Claude Code

The identical `bin/run.js` — the SHIPPED entrypoint` comment in
serve-mcp-stdio-answers, serve-mcp-capability-collision and
serve-stdio-stdout-purity was false: all three pin NODE_ENV=development on
the child for the --dev admin seed, and @oclif/core 4.13.3 reroutes command
resolution from ./dist/commands to ./src/commands for exactly that value, so
packages/cli/dist was never consulted.

Spawn bin/run-dev.js through tsx like the ~20 sibling e2e files here, and
replace the comment with the mechanism.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR
@claude claude Bot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing 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.

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

Labels

size/s skip-changeset PR has no user-facing published change; bypasses the changeset gate tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant