Repository navigation
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
Activity
Triage: lands in
packages/cli/teste2e spawners (the "SHIPPED entrypoint" claim vs whatbin/run.jsactually resolves to under test);pm:queue,domain:cli, Task — test-truthfulness restoration, no product fork.
Generated by Claude Code
Claim —
domain:cliseatSession
session_019siH5jDmk5hrayvfyojUqR, branchclaude/issue-11317-e2e-shipped-entrypoint.pm:queue→pm:dispatchedand 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, andpackages/cli/vitest.config.tsonly 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: spawnbin/run-dev.jslike the ~20 sibling e2e files, delete the false comment). Option 2 — genuinely reachingdist/, which needs a build dependency declared and another route to the--devadmin 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.envworklist is two files short and one of them spawns a realos serve. Both #11341 and #11441 are ungradedfindings (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
{ "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
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 viatsx bin/run-dev.js, mechanism recorded in the corrected comments,NODE_ENVrationale) and 37/37 CI check-runs green at4b7a8453.This card stays OPEN — the deliberately-unclosed half is the PR's own "A ruling is needed" section: option 2 (spawn
bin/run.jswithNODE_ENVunset so these three suites reachpackages/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--devseed no longer needs the parent'sNODE_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 localvitestrun against a staledist/fails confusingly or worse), which thecheck-test-source-aliasdiscipline exists to refuse — so it wants a guard (loud refusal on missing/stale dist + a lint pin on theNODE_ENVline) 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.tsalone). Either answer closes this card; nothing else in it is open.
Generated by Claude Code
Closing — the fix landed. PR #11462 merged 2026-08-24T13:29:48Z.
domain:cliseat (#6024), session019siH5jDmk5hrayvfyojUqR.What landed: the three files stop claiming an entrypoint they do not reach. They now spawn
bin/run-dev.jsthroughtsxlike 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 aPart-ofmerge returns the card topm:queuewith 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 inpm:queuewith 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: undefinedper 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
maintoday and should not be quoted from again. It statesturbo.jsondeclares"@objectstack/cli#test": { "dependsOn": ["^build"] }— dependencies only, sodistis 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 quotesisProd()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 treatsdevelopmentandtestas 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.mjsand thevitest.config.tsheader 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 "spawnsbin/run.js" and "sets a ts-path-enablingNODE_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
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:54andserve-stdio-stdout-purity.e2e.test.ts:50each carry the identical comment:They do spawn that stub. They do not reach
packages/cli/dist.Two facts compose:
packages/cli/distis never built for these tests.turbo.jsondeclares"@objectstack/cli#test": { "dependsOn": ["^build"] }— dependencies only, not the package's own build. Measured:pnpm exec turbo run test --filter=@objectstack/cli --dry=jsonyields 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-35already states this fact in prose.All three spawns set
NODE_ENV: 'development'in the child env (:115,:119,:109respectively — for the--devadmin seed).@oclif/core@4.13.3'stsPath()skips the TypeScript path lookup only whenisProd(), andlib/util/util.js:66defines that as:So under
development(or vitest's inheritedtest) oclif rewrites the command target from the declared./dist/commandsto./src/commandsand auto-transpiles. The compiled artifact is never consulted.Measured directly,
packages/cli/distabsent:The
test/developmentlegs get past command resolution and intosrc/; unset andproductiondo not.Why it matters
Three flavours of loss, none of them loud:
tscoutput difference (emit,exportsresolution, adist-only path bug of the cli:packages/cli/dist/index.jsinvoked directly exits 0 with zero output, anddev'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 quotesnode bin/run.js --version 2.9-3.2sagainsttsx bin/run-dev.js --version 6.5-6.8s. These files already spellbin/run.jsand 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.Not the same thing as #11267
#11267 is the inherited
TEST=truemaking 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.jslike 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-enablingNODE_ENV— which means finding another way to reach the--devadmin seed, since that is why they setdevelopment). The first is honest and cheap; the second is what the comment currently promises. Whoever takes it should also decide whetherpackages/cliwants a lint rule making "spawnsbin/run.js" and "sets a ts-path-enablingNODE_ENV" mutually exclusive, since the combination is silently self-cancelling.Related
NODE_ENV !== 'production'still open in a real production deployment that never sets NODE_ENV (os servedoes not force it,os startdoes) #11113 — where this was found. That PR's pin is currently the only file inpackages/clithat genuinely consumesdist/, because it is the only one that spawns withNODE_ENVunset — which is exactly the value that disables the reroute.os serveunder vitest inheritTEST=true, which makes better-auth silently skip origin/CSRF validation regardless ofNODE_ENV#11267 — sibling env-leak false-green in the same suite.packages/cli/dist/index.jsinvoked directly exits 0 with zero output, anddev's unknown-flag failure presents as a crashed boot (carved from #10087) #10111 — the card that addedinvocation-loudness.e2e.test.tsand thedist/index.jsdiagnosability work.Every line and path above was re-derived in a dedicated worktree at
adb1b9ea8c.Generated by Claude Code