Skip to content

packages/cli e2e tests that spawn bin/run.js as "the SHIPPED entrypoint" never reach dist/ — oclif's ts-path reroutes them to src/ under NODE_ENV=test|development #11317

Description

@os-zhuang

Found while diagnosing the CI red on #11268 (issue #11113). Unassigned, filed for triage rather than fixed there — #11268's surface is ruled closed.

The claim three files make, and the measurement that contradicts it

packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts:53, serve-mcp-capability-collision.e2e.test.ts:54 and serve-stdio-stdout-purity.e2e.test.ts:50 each carry 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 do spawn that stub. They do not reach packages/cli/dist.

Two facts compose:

  1. packages/cli/dist is never built for these tests. turbo.json declares "@objectstack/cli#test": { "dependsOn": ["^build"] } — dependencies only, not the package's own build. Measured: pnpm exec turbo run test --filter=@objectstack/cli --dry=json yields 58 tasks, 57 of them #build, and @objectstack/cli#test's resolved dependency list does not contain @objectstack/cli#build. invocation-loudness.e2e.test.ts:34-35 already states this fact in prose.

  2. All three spawns set NODE_ENV: 'development' in the child env (:115, :119, :109 respectively — for the --dev admin seed). @oclif/core@4.13.3's tsPath() skips the TypeScript path lookup only when isProd(), and lib/util/util.js:66 defines that as:

function isProd() {
    return ['development', 'test'].includes(process.env.NODE_ENV ?? '');
}

So under development (or vitest's inherited test) oclif rewrites the command target from the declared ./dist/commands to ./src/commands and auto-transpiles. The compiled artifact is never consulted.

Measured directly, packages/cli/dist absent:

$ env -u NODE_ENV node packages/cli/bin/run.js serve …
 ›   Error: command serve not found

$ NODE_ENV=production node packages/cli/bin/run.js serve …
 ›   Error: command serve not found

$ NODE_ENV=test node packages/cli/bin/run.js serve …
(node:22544) [MODULE_NOT_FOUND] Warning: ModuleLoadError
task: findCommand (compile)
message: … import() failed to load …/packages/cli/src/commands/compile.ts …

$ NODE_ENV=development node packages/cli/bin/run.js serve …
(same — resolution proceeds from src/)

The test/development legs get past command resolution and into src/; unset and production do not.

Why it matters

Three flavours of loss, none of them loud:

  • The stated intent — exercise the artifact users install — is not met, and the comment asserting it is currently false. A tsc output difference (emit, exports resolution, a dist-only path bug of the cli: packages/cli/dist/index.js invoked directly exits 0 with zero output, and dev's unknown-flag failure presents as a crashed boot (carved from #10087) #10111 kind) is invisible to them.
  • packages/cli/vitest.config.ts's cost header reasons about "swapping the spawns to the built entry" as an unrealised speed lever, and quotes node bin/run.js --version 2.9-3.2s against tsx bin/run-dev.js --version 6.5-6.8s. These files already spell bin/run.js and do not get that 2.9s path — they pay the transpile cost anyway. The header's measurement stands; its framing of these files does not.
  • Anyone reading these three as the pattern for "test the built CLI" copies a shape that silently does not.

Not the same thing as #11267

#11267 is the inherited TEST=true making better-auth skip origin checks. This is oclif rerouting command resolution to source. Same family (a spawned child's env quietly changing what is under test), different mechanism, different remedy.

Shape of a fix, not prescribed

Either the three files stop claiming the built entry (spawn bin/run-dev.js like the other ~20 e2e files here, and delete the comment), or they genuinely get one (declare the build dependency and stop forcing a ts-path-enabling NODE_ENV — which means finding another way to reach the --dev admin seed, since that is why they set development). The first is honest and cheap; the second is what the comment currently promises. Whoever takes it should also decide whether packages/cli wants a lint rule making "spawns bin/run.js" and "sets a ts-path-enabling NODE_ENV" mutually exclusive, since the combination is silently self-cancelling.

Related

Every line and path above was re-derived in a dedicated worktree at adb1b9ea8c.


Generated by Claude Code

Activity

  1. added theissue type on Aug 23, 2026
  2. claude commented on Aug 23, 2026

    @claude
    Contributor

    Triage: lands in packages/cli/test e2e spawners (the "SHIPPED entrypoint" claim vs what bin/run.js actually resolves to under test); pm:queue, domain:cli, Task — test-truthfulness restoration, no product fork.


    Generated by Claude Code

  3. self-assigned this
    on Aug 23, 2026
  4. os-zhuang commented on Aug 23, 2026

    @os-zhuang
    ContributorAuthor

    Claim — domain:cli seat

    Session session_019siH5jDmk5hrayvfyojUqR, branch claude/issue-11317-e2e-shipped-entrypoint. pm:queue → pm:dispatched and assignee set in one stroke, read back.

    Declared file surface: packages/cli/test/serve-mcp-stdio-answers.e2e.test.ts, packages/cli/test/serve-mcp-capability-collision.e2e.test.ts, packages/cli/test/serve-stdio-stdout-purity.e2e.test.ts, and packages/cli/vitest.config.ts only where its cost header names these three files.

    Direction, from triage (5386559160): "test-truthfulness restoration, no product fork." That settles the card's open shape — the three files stop claiming an entrypoint they do not reach (the card's option 1: spawn bin/run-dev.js like the ~20 sibling e2e files, delete the false comment). Option 2 — genuinely reaching dist/, which needs a build dependency declared and another route to the --dev admin seed — is new engineering, not truthfulness restoration.

    If the dev measures option 2 as cheap and fully inside the declared surface, ⛔ do not just take it: report the measurement and wait. Whichever lands, the comment and the code must agree afterwards — that agreement is the deliverable.

    Adjacent, not a blocker: new finding #11441 reports that #11341's bare-...process.env worklist is two files short and one of them spawns a real os serve. Both #11341 and #11441 are ungraded findings (not in flight, no conflict today), but they touch this neighbourhood — if the dev's own reading of these three spawners bears on that worklist, record it there rather than widening here.


    Generated by Claude Code

  5. claude commented on Aug 23, 2026

    @claude
    Contributor
    {
      "issue": 11317,
      "status": "needs_decision",
      "branch": "claude/issue-11317-e2e-shipped-entrypoint",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/11462",
      "premise_still_valid": true,
      "summary": "The defect is real and was re-derived, not transcribed: all three files pin NODE_ENV=development on the child, 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. Implemented the triage-settled option 1 — the three files now spawn bin/run-dev.js through tsx like their ~20 siblings, and the false comment is replaced by the mechanism that made it false. Two things the PM must weigh. (a) The card's fact (1) is STALE: turbo.json now reads \"@objectstack/cli#test\": {\"dependsOn\": [\"build\"]}, changed by #11268 (918988ad) after the card's base adb1b9ea8c, so dist IS built for these tests today and the defect survives on the ts-path reroute alone. (b) Because both of option 2's blockers landed independently since triage, option 2 measures as a three-line change inside the declared file surface that passes, is the FASTEST of the three shapes, and is the only one that actually reaches dist/ — so triage's ground for rejecting it (\"new engineering\") no longer holds. Per the dispatch I did not take it; see open_questions. The PR body says \"Part of #11317\" rather than a closing keyword so that a merge cannot silently close the card while that ruling is open — flip it once the ruling lands.",
      "tests": "All runs on commit 4b7a8453 with a clean working tree (git status --porcelain empty); every heavy step through scripts/pm/os-verify-lock.sh, and each result below quotes the gate's or wrapper's own verdict line, never a bare $?.\n\n• BUILD — `pnpm --workspace-concurrency=2 --filter '@objectstack/cli...' build` → `os-verify-lock: VERDICT command-exit 0 · held the lock 507s (8m27s)`, ending `packages/cli build: Done`. Built the package itself as well as its closure, because the option-2 leg needs packages/cli/dist on disk.\n\n• THE THREE SUITES, THIS BRANCH — `pnpm --filter @objectstack/cli exec vitest run test/serve-mcp-stdio-answers.e2e.test.ts test/serve-mcp-capability-collision.e2e.test.ts test/serve-stdio-stdout-purity.e2e.test.ts --maxWorkers=2` → `Test Files  3 passed (3)` / `Tests  5 passed (5)` / `Duration  39.56s (transform 154ms, setup 0ms, import 266ms, tests 68.01s)`; wrapper `VERDICT command-exit 0`.\n\n• COMPARISON LEGS — one lock acquisition, driven by a script carrying `trap restore EXIT INT TERM`. Each leg's mutation was confirmed on disk by grep counts BEFORE its run (an editor exit code is not evidence), and the restore was verified by an empty `git status --porcelain`:\n  – baseline, origin/main shape (files restored with `git checkout 5a916c4d -- ` the three paths; on-disk confirmation run.js=1 run-dev=0 execPath=1 tsx=0 per file): `Test Files  3 passed (3)` / `Tests  5 passed (5)` / `Duration  27.28s (… tests 46.76s)`.\n  – option-2 probe, bin/run.js kept and `NODE_ENV: 'development'` replaced by `NODE_ENV: undefined` (on-disk confirmation devEnv=0 undefEnv=1 run.js=1 per file): `Test Files  3 passed (3)` / `Tests  5 passed (5)` / `Duration  17.68s (… tests 30.26s)`.\n  Coverage survived the swap: same 3 files, same 5 tests, same assertions, green in all three shapes. The one behaviour change beyond the entrypoint is TIMING, and it is against us — this PR costs roughly +21s of test time across the three files (46.76s → 68.01s), where option 2 would save roughly 16s (46.76s → 30.26s). Node's native type-stripping (baseline) is cheaper than tsx; no transpile at all (option 2) is cheapest.\n\n• MECHANISM, RE-DERIVED — @oclif/core's own `Config.load()` against packages/cli with dist PRESENT, reading back the root plugin's resolved commandsDir: NODE_ENV unset → dist/commands; development → src/commands; test → src/commands; production → dist/commands. Authority `@oclif/core@4.13.3 lib/util/util.js:65-67`. ⚠️ Its `isProd()` is a NEGATED membership test — a leading logical-NOT applied to `['development','test'].includes(process.env.NODE_ENV ?? '')`. The card's body AND this dispatch both quote it WITHOUT that leading NOT, which inverts the predicate (as written there, oclif would treat development and test as production). Cause identified while reading my own PR body back: GitHub's body sanitizer eats a bang immediately before a bracket, inside a code span and inside a fence alike — it silently ate mine on first publish. The three source comments carry the correct sign; files are not sanitized.\n\n• TURBO, the card's own instrument — `pnpm exec turbo run test --filter=@objectstack/cli --dry=json` → 59 tasks, 58 of them `#build`, and `@objectstack/cli#test`'s resolved dependency list is exactly `[\"@objectstack/cli#build\"]`. The card measured 58 / 57 / absent at adb1b9ea8c.\n\n• TYPECHECK — `pnpm --filter @objectstack/cli typecheck` → `VERDICT command-exit 0`, with the script name echoed (`> @objectstack/cli@17.2.0 typecheck` / `> tsc --noEmit`), so this was not a zero-match no-op. ⚠️ It does NOT cover the edited files: packages/cli/tsconfig.json has `include: [\"src\"]`, so test/** sits outside the program and there is no tsconfig.test.json. What exercised the edits is the vitest run above.\n\n• GATE FAMILIES — derived with `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack`, no paths passed (it read the change set from the merge base itself); the assertion held against this checkout's origin. All 13 exit 0, each printing its own OK line: check:nul-bytes (self-test 75 assertions; 6421 text files scanned, no raw ASCII control 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, scripts/check-ci-filter-parity.mjs, scripts/check-plugin-teardown-shape.mjs, scripts/docs-audit/check-affected-docs.mjs.\n\n• NOT MEASURED, and it says so itself — `check:type-check-debt`. Its `--re-measure` half refuses outright without the whole workspace closure built (`@objectstack/service-knowledge` has no built type entry point on disk) rather than producing a number from the wrong world. 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.\n\n• DECLARED NARROWING — repo-wide `pnpm lint` was not run; targeted eslint was, with all three pieces of evidence: (1) population read from eslint's own resolution, not my guess — all three files came back as result objects with no file-ignored warning; (2) count read from `--format json` — 3 results, 0 errors, 0 warnings, exit 0; (3) config invariance — this repo runs one eslint.config.mjs that 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.\n\n• Changeset: none. Three files under packages/cli/test/, nothing published changes. Declared with the `skip-changeset` label on PR #11462, added through the additive labels endpoint and read back: ['skip-changeset'].",
      "open_questions": [
        {
          "question": "Triage rejected option 2 (genuinely reach dist/) as \"new engineering\" because it needed a build dependency declared AND another route to the --dev admin seed. Both halves have since landed independently, and I measured option 2 green, cheapest, and confined to the same three files. Should the card keep the settled option 1 (this PR), or switch to option 2? Mechanically: the build dependency already exists (turbo.json now says dependsOn: [\"build\"], via #11268); and the seed no longer needs NODE_ENV from the parent, because commands/serve.ts:1272 does `if (flags.dev) { if (!process.env.NODE_ENV) process.env.NODE_ENV = 'development'; }` in-process, while plugin-auth's maybeSeedDevAdmin reads `process.env.NODE_ENV !== 'development'` at call time inside runtime.start() — after that assignment. So `NODE_ENV: undefined` keeps the seed and leaves the reroute disabled. Measured: 3 files / 5 tests pass, 17.68s wall / 30.26s test time, versus 39.56s / 68.01s for this PR and 27.28s / 46.76s for main.",
          "options": [
            "A — keep option 1 (this PR, already implemented and green): three files spawn bin/run-dev.js through tsx, matching the ~20 siblings and helpers/serve-process.ts. Cost: roughly +21s of test time across the three files, moving them onto the SLOWEST of the three shapes, against the direction vitest.config.ts's cost header reasons in. Benefit: the tests stay verdicts about SOURCE, never about build state.",
            "B — switch to option 2: keep bin/run.js, change `NODE_ENV: 'development'` to `NODE_ENV: undefined` in the three childEnv() calls. Three lines, same declared surface, roughly -16s of test time, and the only shape in which the original 'SHIPPED entrypoint' comment becomes true. Cost: these three suites become verdicts about BUILD STATE — exactly what scripts/check-test-source-alias.mjs and the vitest.config.ts header exist to refuse, because a suite green against a stale artifact says nothing about it. Turbo guarantees the build in CI; it guarantees nothing for a developer running vitest directly, who gets oclif's 'command not found'.",
            "C — B plus the guard the card floats (now filed as #11464), so the pair cannot silently re-form."
          ],
          "recommendation": "A, i.e. merge this PR as-is — but the four axes are genuinely close and B is defensible, so this is a ruling, not a formality. REAL BUSINESS NEED: neither shape serves a user-facing need; the need served is the maintainer's ability to trust a green suite. B's claim (exercise what users install) is real but is ALREADY held by serve-node-env-production-default.e2e.test.ts, which is a dedicated, deliberate dist pin — so B's marginal coverage is a second copy of an existing guarantee, while A's marginal loss is nil. LONG-TERM SOUNDNESS: this repo has an explicit, written, measured policy preferring source over dist in tests (check-test-source-alias.mjs plus the vitest.config.ts header), and it exists because green-against-a-stale-artifact is silent. B does not violate that policy accidentally, it reverses it for three files, which is a policy change and belongs to the maintainer, not to me. HARD TO GET WRONG: A leaves the tree with no file pairing bin/run.js with a ts-path-enabling NODE_ENV, so the trap has no instance to copy; B reinstates the pair as the RECOMMENDED shape, one careless edit from silently self-cancelling again — B really wants C, which is more work than the 3 lines suggest. STARTUP SCOPE: A is the smaller commitment and closes the card today; B is cheap in lines but opens a maintenance surface (local-run ergonomics, the guard) for a speed win of ~16s on three files. The strongest argument FOR B is the one I cannot dismiss: this PR knowingly makes the package's slowest suite category slower, on a package that was the single largest item on the Test Core critical path. If the maintainer weights suite wall-clock above the source-over-dist policy, B is right and I will take it."
        }
      ],
      "out_of_scope_findings": [
        "filed as #11464: nothing stops a packages/cli test from spawning bin/run.js and a ts-path-enabling NODE_ENV at once — the pair silently cancels, and vitest's own NODE_ENV=test makes it reachable by inheritance rather than intent. Unassigned, `finding` label, no pm:queue; carries `Blocked-by: #11317` because the guard's recommended polarity depends on the ruling above.",
        "not filed, recorded here: the card's and this dispatch's quote of @oclif/core's isProd() has lost its leading logical-NOT, inverting the predicate. Cause is GitHub's body sanitizer eating a bang before a bracket (it ate mine on first publish of PR #11462, inside a code span). No code defect; noting it so the next reader of #11317 does not trust the quoted sign.",
        "nothing to add to #11441's worklist: all three of these files already use childEnv(), not a bare `...process.env`. Re-derived the directory — the only bare-spread children that boot a real `os serve` are the two #11441 already names, and serve-app-runtime-hooks.e2e.test.ts:84 uses the bare spread for `os compile` only while its serve boot goes through runServe(), so #11341's assessment of that file is accurate as written."
      ]
    }
    

    Generated by Claude Code

  6. os-zhuang commented on Aug 24, 2026

    @os-zhuang
    ContributorAuthor

    PR #11462 merged (squash, 4e786cd8) — reviewed and landed by the skills seat on the maintainer's live instruction (2026-08-24, verbatim: 「这几张你都帮我审核并合并吧」), after verifying the diff matches the triage-settled remedy (truthful entrypoint via tsx bin/run-dev.js, mechanism recorded in the corrected comments, NODE_ENV rationale) and 37/37 CI check-runs green at 4b7a8453.

    This card stays OPEN — the deliberately-unclosed half is the PR's own "A ruling is needed" section: option 2 (spawn bin/run.js with NODE_ENV unset so these three suites reach packages/cli/dist, the SHIPPED artifact) was rejected at triage on two facts that have both since become false — the build dependency is now declared (#11268) and the --dev seed no longer needs the parent's NODE_ENV (serve.ts:1272). Measured: option 2 passes all 3 files / 5 tests, is the fastest of the three shapes (30.3s vs this PR's 68.0s), and makes the original "shipped entrypoint" claim true. The standing counter-argument is repo policy: it couples three suites' verdicts to build state (a local vitest run against a stale dist/ fails confusingly or worse), which the check-test-source-alias discipline exists to refuse — so it wants a guard (loud refusal on missing/stale dist + a lint pin on the NODE_ENV line) landed with it, not alone.

    Maintainer's call, when convenient: A — take option 2 with the guard (shipped-artifact coverage 1 file → 4, these suites halve in time), or B — keep today's landed shape and close this card (shipped coverage stays with serve-node-env-production-default.e2e.test.ts alone). Either answer closes this card; nothing else in it is open.


    Generated by Claude Code

  7. os-zhuang commented on Aug 24, 2026

    @os-zhuang
    ContributorAuthor

    Closing — the fix landed. PR #11462 merged 2026-08-24T13:29:48Z.

    domain:cli seat (#6024), session 019siH5jDmk5hrayvfyojUqR.

    What landed: the three files stop claiming an entrypoint they do not reach. They now spawn bin/run-dev.js through tsx like the sibling e2e files in the directory, and the false comment is replaced by the mechanism that made it false. 3 files / 5 tests, green on both sides of the change.

    Why this card closes rather than returning to the queue. #11462 used Part of, not the closing keyword, and this seat's standing rule is that a Part-of merge returns the card to pm:queue with the assignee released. ⛔ Not applied here, deliberately: the defect this card names is fixed, and there is no dev work left to dispatch. A card sitting in pm:queue with nothing to implement is a card that will be claimed and handed back — the failure mode #8343 hit and had to be corrected out of with a ⛔ DO-NOT-DISPATCH comment. The mechanical rule exists for genuinely partial work; this is complete work whose author left the closing keyword off pending a ruling, which is a different situation and gets the treatment the PR body itself asked for: "close the card by hand once the ruling lands."

    The ruling: the maintainer merged #11462 as-is at 13:29:48Z. As-is is option 1 — option 2 was one NODE_ENV: 'development' → NODE_ENV: undefined per file away and was not taken. Direction confirmed by the merge.

    The premise correction #11462 found, recorded here because this card carries the stale fact

    This card's fact (1) is false as of main today and should not be quoted from again. It states turbo.json declares "@objectstack/cli#test": { "dependsOn": ["^build"] } — dependencies only, so dist is never built for these tests. It now reads "dependsOn": ["build"], changed by #11268 (918988ad) — the same PR this card was filed from, whose change postdates this card's base (adb1b9ea8c).

    Re-measured with this card's own instrument (turbo run test --filter=@objectstack/cli --dry=json): 58 tasks → 59, and @objectstack/cli#test's resolved deps now do contain @objectstack/cli#build.

    ⇒ The defect survived on fact (2) alone — oclif's ts-path reroute — which is sufficient by itself. The conclusion stood; its cause is single, not composite.

    ⚠️ Also worth carrying: this card's body quotes isProd() without its leading logical-NOT, because a bang immediately before a bracket is image syntax and GitHub's sanitizer eats it — inside a code span and inside a fence alike. As quoted here the predicate is inverted, i.e. it reads as though oclif treats development and test as production. The source comments #11462 added carry the correct sign, because files are not sanitized. (Same class as the <tag> strip repaired on #8343 today; a body is not a safe place to quote a negation.)

    Residual, filed separately — ⛔ not left in this card's body to rot

    Option 2 measured faster than what landed (17.68s vs 39.56s test time — the fastest of the three shapes) and is the only one that makes the original comment true, but it would make these three suites verdicts about build state, which scripts/check-test-source-alias.mjs and the vitest.config.ts header exist to refuse. That is a live trade-off, not a leftover task, and it outlives this card. Filed as a finding for triage to grade, together with the lint-rule question this card floated (making "spawns bin/run.js" and "sets a ts-path-enabling NODE_ENV" mutually exclusive, since the combination is silently self-cancelling).

    Closing completed. Re-opening is free if the residual turns out to belong here after all.


    Generated by Claude Code

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

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions