Skip to content

Commit 35e549b

Browse files
objectstack-fleet[bot]claudehotlong
authored
fix(plugin-dev): load setup/account through literal specifiers, so init() makes no main-process round trip under vitest (#20452)
Fixes #20376 Clause-②: no ## What this changes `DevPlugin.init()`'s setup / account app-package loop imported both packages through one variable specifier, `import(/* @vite-ignore */ spec[0])`. Under vitest a variable specifier cannot be resolved when `dev-plugin.ts` is transformed, so every call went to the main vitest process, mocked packages included: two main-process round trips per `init()`, inside every clocked test window that boots `DevPlugin`. The main process is shared by the whole run, so on a busy CI shard those requests wait on other files' work. That is the shape of the `Test Core (6/6)` red on PR #20365 (job 108779589417): the mount-refusal suite's first case timed out at 5026 ms and its second took 2336 ms, while its other four took 16-72 ms, in a package run reading `transform 74.29s, import 137.46s, tests 11.87s`. Route A of the seat's answer on the card: each loop entry now carries its own literal loader (`() => import('@objectstack/setup')`, `() => import('@objectstack/account')`). The `try` / `catch` and `reportOptionalLoadFailure` around each load are byte-for-byte unchanged. No timeout, retry or test-arrangement change. Files: - `packages/plugins/plugin-dev/src/dev-plugin.ts`: the loop only (two tuple entries, one call line, one comment block). - `packages/plugins/plugin-dev/src/dev-plugin-literal-imports.pin.test.ts` (new): every dynamic `import()` in `dev-plugin.ts` takes a string literal, except `organizationsPkg` (ADR-0132); read off the TypeScript AST. - `packages/plugins/plugin-dev/src/dev-plugin-optional-load-failure.test.ts`: one case pinning that setup / account, when they cannot be imported, still reach the absent arm (one line each at `warn`, with the resolver's code and the failing specifier, never at `error`). - `packages/plugins/plugin-dev/vitest.config.ts`: two anchored alias entries for `@objectstack/setup` / `@objectstack/account`, which `check:test-source-alias` requires once the specifiers are literal (see Acceptance notes). - `.changeset/20376-plugin-dev-literal-app-imports.md`: `@objectstack/plugin-dev` `patch`. ## Takeover record This branch was started by a dev whose seat session ran out of tokens after pushing `952e1249` and `4160ecbe`, with no PR and no report. This continuation read both commits hunk by hunk as unverified work, merged `origin/main` (`0fcb10184c`) and re-ran every measurement below itself. - Kept: the `dev-plugin.ts` code lines, the changeset's frontmatter and `Clause-②` line, and the pin's `__dirname` seed (`4160ecbe`). - Corrected in `917a31bc72`: the `dev-plugin.ts` comment and the changeset said the loop now loads "like every other load / package", which the deliberate `organizationsPkg` variable specifier contradicts (now: every other declared-dependency load); the changeset said the build keeps `await import("…")`, while `dist/index.mjs` and `dist/index.js` both keep a native `import("…")` inside an arrow; the pin header carried the earlier run's load timings (44.5-102.0 ms per `init()`, a 5048 ms local timeout), which this run did not re-measure, and now states the counts below plus the CI job reading, which this run read from the job log itself. - Corrected in `35833d2a2c`: the pin's private regex comment stripper was refused by `check:comment-mask-adoption`; the pin now walks the TypeScript syntax tree (`typescript` is already a devDependency of this package). - Added in `917a31bc72`: the absent-arm pin (no existing pin covered this loop's failure path). - Added in `a24f6ea84c`: the two vitest alias entries (`check:test-source-alias` red without them). ## Measurements (H1-H4) **H1: the round trips, counted.** A throwaway vite plugin (deleted after) logged every `resolveId` and `transform` the main process served, around six back-to-back `init()`s in a throwaway probe with every package mocked absent. Same method, same run shape, both legs: | `dev-plugin.ts` | main-process requests during the six `init()`s | per `init()` | |---|---|---| | as on `origin/main` (blob `b132ad3f4`) | 20 | first: 10 (2 bare `resolveId`, 2 `/@fs` `resolveId`, 2 `dist/index.mjs` transforms, 4 `resolveId`s of their imports); each later: 2 (`@objectstack/setup`, `@objectstack/account`) | | this branch, `a24f6ea84c` | 0 | 0 | The real `dev-plugin-tenancy-mount-refusal.test.ts` under the same logger (importer `dev-plugin.ts`): on `origin/main`, 9 `resolveId`s of each package (nine `init()`s, 18 in all) plus both `dist` transforms; on this branch, none in the test phase. Every leg logged both absent-app warnings on every `init()` (6 of 6). **H2: the pin fails when the variable specifier comes back.** `node scripts/ablation-replace.mjs`, WRAP mode, at `35833d2a2c`: - the call line back to `await import(/* @vite-ignore */ spec[0])`: ① red (`1 failed | 2 passed`), received `[ 'spec[0]', 'organizationsPkg' ]`; - the whole hunk back to the `origin/main` blob `b132ad3f4`: ① and ③ red (`2 failed | 1 passed`; ③ reports 12 literal loads, none of them setup / account). Each restore was proven by the tool: blob after restore equals the HEAD blob (`1f1ac117f`) and `git diff HEAD` is empty. The pin reads `dev-plugin.ts`'s source text, so no `dist/` leg applies. **H3: the absent-package path is unchanged.** The new case is green on this branch and green with `dev-plugin.ts` swapped to the `origin/main` blob (`10 passed` both), so it pins behaviour the fix preserves. It goes red when that path breaks: a silent `catch` (`if (err) continue;`) gives `1 failed | 9 passed`, and demoting the loop's `absentLevel` to `info` gives `1 failed | 9 passed`. Restores proven as above. (A first attempt at the silent-catch leg was refused by the tool, because its replacement contained its own anchor; no test ran, and it was retried with a different anchor.) **H4: the suites pass.** `pnpm --filter @objectstack/plugin-dev test` at `a24f6ea84c`: `Test Files 9 passed (9)`, `Tests 86 passed (86)`, mount-refusal 6/6, failfast 9/9, tenancy-posture 6/6, optional-load-failure 10/10, security-enforcement-warning 4/4, malformed-stack-posture 4/4, `dev-plugin.test.ts` 24/24, i18n reader 20/20, the new pin 3/3. ## Verification at `a24f6ea84c` - `pnpm turbo run build --filter='@objectstack/plugin-dev^...' --concurrency=2`: 34/34. - `pnpm --filter @objectstack/plugin-dev typecheck`: exit 0 (`tsc --noEmit` and `check:test-typecheck`; `--listFiles` shows both touched test files in both programs). - `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands`: 62 commands, all exit 0; `--ran` reconciliation: 62 derived, 62 run, 0 NOT-MEASURED, 0 UNRUN (a derived zero: every line carries its exit code). On the way: `check:comment-mask-adoption` and `check:test-source-alias` were red on this branch's pushed state and are fixed above; `check:dual-build-cjs-loads` and `check:type-check-debt` first answered PREREQUISITE NOT MET and were re-run after `pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*' --concurrency=2` (71/71). - `node scripts/check-issue-citations.mjs --base origin/main`: exit 0 (also exit 0 with `--base 0fcb101`, the merge base). - `pnpm lint`: exit 0, no findings. - NOT MEASURED locally, declared to CI: the `Test Core`, `Dogfood`, `Build Core` and `Temporal Conformance` jobs and the workspace type-check lanes the derivation lists as CI-only. **Declared narrowing — verification ran UNLOCKED.** `scripts/pm/os-verify-lock.sh` could not take the shared verify lock on this host: no usable `flock`. The shared verify lock is declared Linux-only (`flock` is util-linux, and a stock macOS does not ship it), so the command below was run directly, without the lock — a declared narrowing, not a silent one. No serialization guarantee held for this run, nor for any sibling agent in this container while it ran. pnpm turbo run build --filter='@objectstack/plugin-dev^...' --concurrency=2 pnpm --filter @objectstack/plugin-dev build pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*' --concurrency=2 pnpm --filter @objectstack/plugin-dev typecheck pnpm --filter @objectstack/plugin-dev test pnpm --filter @objectstack/plugin-dev exec vitest run --maxWorkers=2 (suite, probe and ablation runs) ## Acceptance notes - **File surface.** `packages/plugins/plugin-dev/vitest.config.ts` is not in the claim's listed surface. It is the mechanical consequence of route A: literal specifiers make both packages visible to `check:test-source-alias`, which then requires them aliased to source. The two entries are the ones the gate printed; both packages' own imports were already aliased. The absent-arm case in `dev-plugin-optional-load-failure.test.ts` is the "smallest pin" the dispatch's H3 asked for. - **Census notes carried from the seat's answer, not re-measured under load.** `dev-plugin-malformed-stack-posture.test.ts` case 4 pays a real cold load of `objectql` and `driver-memory` inside a window widened by an explicit `60_000`, and the first case of `dev-i18n-packages-reader.test.ts` pays a first-use cost. Neither is failing (idle maxima at `a24f6ea84c`: 76 ms and 28 ms). No CPU-load generator was run on this shared host. - **Timing.** This PR rests on the deterministic counts above and the CI job reading. The earlier run's under-load timings (on the card) were not re-measured here. - **Same loop shape in the CLI.** `packages/cli/src/commands/serve.ts` (around line 4024) loads setup / account through the same kind of variable-specifier loop (with `webpackIgnore`). In a spawned CLI process there is no vitest main process to call, so it costs nothing there; a vitest suite that ran that branch in-process would pay the same round trip. Not measured. Noted, not filed. - **A stale docblock sentence.** `dev-plugin.ts`'s `MODULE_NOT_FOUND_CODES` docblock says the CJS build resolves these loads through `require()`. The current `dist/index.js` keeps a native `import("…")` for every dynamic load. It is harmless, because both codes stay in the set. Noted, not filed. - **Bundler visibility.** A consumer's bundler now sees `@objectstack/setup` and `@objectstack/account` statically, as it already saw the twelve other literal loads. Both are declared dependencies. `@objectstack/organizations`, which no framework package may declare, keeps its `webpackIgnore` variable form. --- _Generated by [Claude Code](https://claude.ai/code/session_local_1d2a197c-c20e-4e90-9be8-413d4d432289)_ --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Jack Zhuang <50353452+hotlong@users.noreply.github.com>
1 parent b98fbc2 commit 35e549b

5 files changed

Lines changed: 165 additions & 3 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"@objectstack/plugin-dev": patch
3+
---
4+
5+
`DevPlugin` now loads `@objectstack/setup` and `@objectstack/account` through literal `import('…')` specifiers, like every other declared dependency it loads, instead of one variable specifier shared by a loop (#20376).
6+
7+
Clause-②: no
8+
9+
- **What was wrong.** The setup / account app-package loop imported `spec[0]`. A variable specifier cannot be resolved when the file is transformed. Under vitest, every `DevPlugin.init()` in a test therefore made two round trips to the main test process, even with both packages mocked, inside every clocked test window that boots `DevPlugin`. The main process is shared by the whole run, so on a busy CI shard those round trips wait on other files' work, against this package's 5000 ms test budget.
10+
- **What changes.** Each loop entry carries its own literal loader. The `try` / `catch` and the absent-package report around each load are unchanged: a missing package is still logged as, for example, `✘ @objectstack/setup not installed — skipping its app`, and a present one that fails is still reported as present-but-failed.
11+
- **Unchanged.** Nothing an author or operator configures or sees changes. Both packages were already declared dependencies of `@objectstack/plugin-dev`, and both the ESM and the CJS build keep a native `import("…")` for each.
Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// #20376 — every dynamic `import()` in `dev-plugin.ts` names its package
4+
// LITERALLY, except the one ADR-0132 requires to stay a variable.
5+
//
6+
// ## Why the FORM of the specifier is pinned
7+
//
8+
// Every suite in this family mocks the packages `DevPlugin.init()` loads, so
9+
// `init()` should run in-process. For a literal `import('x')` it does: the
10+
// specifier is rewritten when `dev-plugin.ts` is transformed, and the test's
11+
// `vi.mock('x')` serves it inside the worker. A VARIABLE specifier is resolved
12+
// at CALL time instead, by a round trip to the main vitest process, and that
13+
// happens on every call, not only the first. Measured with a throwaway vite
14+
// plugin logging every `resolveId` and `transform` the main process served,
15+
// around six back-to-back `init()`s with setup and account mocked absent:
16+
//
17+
// - With the loop importing through `spec[0]` (`dev-plugin.ts` as of
18+
// `0fcb10184`), the six `init()`s made the main process serve 20
19+
// requests: a `resolveId` of `@objectstack/setup` and one of
20+
// `@objectstack/account` on EVERY `init()`, and on the first one also the
21+
// transform of both packages' `dist` entries and the resolution of their
22+
// imports. The mount-refusal suite's nine `init()`s made 18 of those
23+
// `resolveId`s.
24+
// - With literal specifiers, the same six `init()`s made ZERO. Every
25+
// specifier this file names, the twelve literal loads beside the loop
26+
// included, is resolved once, when the file is transformed at collection.
27+
// - The main process is one process shared by every worker in the run, so
28+
// a request to it waits on whatever it is doing for OTHER files. On
29+
// `Test Core (6/6)` (job 108779589417), the mount-refusal suite's first
30+
// case timed out at 5026 ms and its second took 2336 ms, while its other
31+
// four took 16-72 ms, in a package run that read `transform 74.29s,
32+
// import 137.46s, tests 11.87s`.
33+
//
34+
// So re-introducing a variable specifier puts a main-process round trip back
35+
// into every clocked window that boots this plugin, whatever the suite. A
36+
// timeout raised to absorb it would only move the cliff to the next heavier
37+
// shard.
38+
//
39+
// ## The one exception
40+
//
41+
// `@objectstack/organizations` is imported through `organizationsPkg`, with
42+
// `webpackIgnore`, on purpose: ADR-0132's entitlement boundary forbids any
43+
// framework package from declaring it (`no-framework-dependents.pin.test.ts`),
44+
// so no bundler may try to resolve it. In the suites that mock it, its mock
45+
// URL is the raw specifier, so the worker serves it without a round trip.
46+
//
47+
// ## How it reads the file
48+
//
49+
// Through the TypeScript compiler's own parser, never through the source text:
50+
// a dynamic import is a call node whose callee is the `import` keyword, so a
51+
// comment, a string or a regex that mentions `import(` is never read as one,
52+
// and the leading `/* webpackIgnore: true */` is trivia, not part of the
53+
// argument. The lit control (③) proves the walk still finds the literal
54+
// loads, so a walk that found nothing cannot pass ① vacuously.
55+
56+
import { describe, it, expect } from 'vitest';
57+
import { readFileSync } from 'node:fs';
58+
import { resolve } from 'node:path';
59+
import ts from 'typescript';
60+
61+
// `__dirname`, not `import.meta.url`: this package's build config compiles
62+
// `src/**` as CommonJS, where `import.meta` is TS1470. Vitest's evaluator
63+
// provides `__dirname` to every module it runs.
64+
const HERE = __dirname;
65+
const FILE = resolve(HERE, 'dev-plugin.ts');
66+
const SOURCE = ts.createSourceFile(FILE, readFileSync(FILE, 'utf8'), ts.ScriptTarget.Latest, true);
67+
68+
/** Every node of the parsed file, depth first. */
69+
function nodesOf(root: ts.Node): ts.Node[] {
70+
const out: ts.Node[] = [];
71+
const visit = (node: ts.Node): void => {
72+
out.push(node);
73+
ts.forEachChild(node, visit);
74+
};
75+
visit(root);
76+
return out;
77+
}
78+
79+
const NODES = nodesOf(SOURCE);
80+
81+
/** The first argument of every dynamic `import(…)` call in the file. */
82+
const DYNAMIC_IMPORT_ARGS: ts.Expression[] = NODES
83+
.filter((n): n is ts.CallExpression => ts.isCallExpression(n) && n.expression.kind === ts.SyntaxKind.ImportKeyword)
84+
.map((call) => call.arguments[0]);
85+
86+
/**
87+
* Only a plain string literal counts as naming its package: it is the form the
88+
* transform resolves ahead of time. Anything else is resolved at call time.
89+
*/
90+
const LITERAL_ARGS = DYNAMIC_IMPORT_ARGS.filter(ts.isStringLiteral).map((arg) => arg.text);
91+
const VARIABLE_ARGS = DYNAMIC_IMPORT_ARGS.filter((arg) => !ts.isStringLiteral(arg)).map((arg) => arg.getText(SOURCE));
92+
93+
describe('#20376 — dev-plugin.ts loads its packages through literal specifiers', () => {
94+
it('① every dynamic import names its package literally, except the organizations one', () => {
95+
expect(VARIABLE_ARGS, 'a variable specifier costs a main-process round trip per call under vitest').toEqual([
96+
'organizationsPkg',
97+
]);
98+
});
99+
100+
it('② the exception is the ADR-0132 package, and nothing else hides behind that name', () => {
101+
const declarations = NODES.filter(
102+
(n): n is ts.VariableDeclaration => ts.isVariableDeclaration(n) && n.name.getText(SOURCE) === 'organizationsPkg',
103+
);
104+
expect(declarations).toHaveLength(1);
105+
const init = declarations[0].initializer;
106+
expect(init !== undefined && ts.isStringLiteral(init) ? init.text : undefined).toBe('@objectstack/organizations');
107+
});
108+
109+
it('③ lit control: the walk sees the setup / account loads, and the loads beside them, as literals', () => {
110+
for (const pkg of ['@objectstack/setup', '@objectstack/account', '@objectstack/objectql']) {
111+
expect(LITERAL_ARGS, pkg).toContain(pkg);
112+
}
113+
});
114+
});

‎packages/plugins/plugin-dev/src/dev-plugin-optional-load-failure.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,4 +285,24 @@ describe('DevPlugin — an optional service that is installed and fails to const
285285
// And the false claim it used to emit instead is gone.
286286
expect(allLines(ctx).some((l) => l.includes('@objectstack/rest not installed'))).toBe(false);
287287
});
288+
289+
// #20376 changed HOW the setup / account app-package loop names its two
290+
// packages (a literal `import()` per entry, in place of one variable
291+
// specifier), never what it does when one cannot be imported. Both still
292+
// reach the absent arm: one line each, at the loop's `warn`, never at the
293+
// present-but-failed arm's `error`, and carrying the resolver's own code and
294+
// the specifier that failed.
295+
it('keeps the setup / account app packages on the absent arm when they cannot be imported', async () => {
296+
const ctx = mockCtx();
297+
await new DevPlugin({ seedAdminUser: false }).init(ctx);
298+
299+
const warnLines: string[] = ctx.logger.warn.mock.calls.map((call: unknown[]) => String(call[0]));
300+
for (const pkg of ['@objectstack/setup', '@objectstack/account']) {
301+
const lines = warnLines.filter((l) => l.includes(pkg));
302+
expect(lines, `${pkg}: reported once, at warn`).toHaveLength(1);
303+
expect(lines[0]).toContain('code: ERR_MODULE_NOT_FOUND');
304+
expect(lines[0]).toContain(`Cannot find package '${pkg}'`);
305+
expect(errorLines(ctx).some((l) => l.includes(pkg)), `${pkg}: not the present-but-failed arm`).toBe(false);
306+
}
307+
});
288308
});

‎packages/plugins/plugin-dev/src/dev-plugin.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -734,12 +734,21 @@ export class DevPlugin implements Plugin {
734734
// NOTE: @objectstack/studio is intentionally NOT default-loaded — the
735735
// console ships a dedicated Studio surface at /_console/studio/<pkg>/<pillar>,
736736
// so Studio no longer needs to exist as a navigable app tile.
737+
//
738+
// [#20376] Each entry carries its own LITERAL `import('…')`, as every
739+
// other declared-dependency load in this method does. A variable
740+
// specifier (`import(spec[0])`) cannot be resolved when this file is
741+
// transformed: under vitest every call became a round trip to the main
742+
// process — two per `init()`, measured, mocked packages included —
743+
// inside every clocked test window that boots this plugin. Both packages
744+
// are declared dependencies, and the absent-package path below is
745+
// unchanged. `dev-plugin-literal-imports.pin.test.ts` pins the form.
737746
for (const spec of [
738-
['@objectstack/setup', 'createSetupAppPlugin'],
739-
['@objectstack/account', 'createAccountAppPlugin'],
747+
['@objectstack/setup', 'createSetupAppPlugin', () => import('@objectstack/setup')],
748+
['@objectstack/account', 'createAccountAppPlugin', () => import('@objectstack/account')],
740749
] as const) {
741750
try {
742-
const mod: any = await import(/* @vite-ignore */ spec[0]);
751+
const mod: any = await spec[2]();
743752
this.childPlugins.push(mod[spec[1]]());
744753
ctx.logger.info(` ✔ App package enabled (${spec[0]})`);
745754
} catch (err) {

‎packages/plugins/plugin-dev/vitest.config.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,14 @@ export default defineConfig({
6262
},
6363
{ find: /^@objectstack\/platform-objects$/, replacement: path.resolve(__dirname, '../../platform-objects/src/index.ts') },
6464
{ find: /^@objectstack\/plugin-security$/, replacement: path.resolve(__dirname, '../plugin-security/src/index.ts') },
65+
// [#20376] The setup / account app packages. `dev-plugin.ts` names them with
66+
// literal `import('…')` specifiers now, so `check:test-source-alias` sees
67+
// both and, unaliased, both resolve through `exports` to `dist/`. Every
68+
// suite here mocks them; the mocks and the imports resolve through these
69+
// same entries. Their own imports (`@objectstack/platform-objects/apps`,
70+
// `@objectstack/spec/system`) are already aliased above and below.
71+
{ find: /^@objectstack\/setup$/, replacement: path.resolve(__dirname, '../../apps/setup/src/index.ts') },
72+
{ find: /^@objectstack\/account$/, replacement: path.resolve(__dirname, '../../apps/account/src/index.ts') },
6573
{ find: /^@objectstack\/formula$/, replacement: path.resolve(__dirname, '../../formula/src/index.ts') },
6674
{ find: /^@objectstack\/metadata-core$/, replacement: path.resolve(__dirname, '../../metadata-core/src/index.ts') },
6775
// Subpath BEFORE the bare package: `@objectstack/core` is a PREFIX match with a

0 commit comments

Comments
 (0)