refactor(workspace): resolve @mlx-node/* through each package's exports map - #151
Conversation
…ts map The root vite.config.ts kept one resolve.alias entry per `@mlx-node/*` specifier, subpaths included, rewriting each to that package's TypeScript source. Tests therefore ran against `src` while Node ran against `dist`, and the table was a second copy of every package's exports map that nothing validated. It had already drifted: `lm/model-discovery` and `lm/model-detection` were declared in exports and imported by two source files, but had no dist artifact, and `@mlx-node/vlm` had no alias at all, so it silently resolved to dist while its siblings resolved to src. Each package now declares its own source entry as an `@mlx-node/source` condition ahead of `types`/`import` in its exports map, and the test resolver opts into it through `ssr.resolve.conditions`. That key is the only one that reaches the test resolver, and Vitest mirrors it onto its Node processes as real `--conditions` flags — so anything Node loads as JavaScript must not inherit them. The dashboard's SQLite worker thread now spawns with `execArgv: []`, the same rule the desktop sidecar fork already followed; without it the worker resolved workspace packages to TypeScript and died on a parameter property Node cannot strip. Deleting the table also deleted the alias-ordering hazard: matching is prefix-based and first-match-wins, so a bare package key listed above its subpaths rewrote `@mlx-node/server/host` into `.../src/index.ts/host`. Published packages gained `src` in `files`, since the condition names files that must exist wherever the manifest does. Verified: `tsc -b` clean; `yarn mlx --help` starts the CLI from source through exports -> dist; dashboard suite 710/710 (it was 82 failures deep before the execArgv fix); an 80-file / 1375-test slice across agent, privacy, dashboard, server/host, models and core at 1367 passed / 8 skipped. docs/monorepo-paths.md documents the arrangement, the Vitest forwarding trap, and the checklist for adding a subpath.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_73308919-7e31-42e5-a1d8-fd984b2809d9) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9675933eaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Resolving the source condition through `ssr.resolve.conditions` leaked into Node: Vitest forwards that key to its processes as real `--conditions` flags, so Node's own resolution of `@mlx-node/*` selected TypeScript, and every Node-side load died on source Node cannot strip. The dashboard's SQLite worker took 82 tests down with it (fixed here with `execArgv: []`, now reverted); the desktop sidecar acceptance test's fork of the built entry was still broken and took all four `sidecar-e2e.test.ts` cases with it, failing on the parameter property in `packages/server/src/host/index.ts`. `workspaceSource()` in the root config now performs the same mapping as a `resolveId` hook (`enforce: 'pre'`) that reads each package's own `@mlx-node/source` entry at config load. The subpath list still exists once, in the package that owns it, and nothing is wired into resolver conditions — so Node, `oxnode`, spawned workers and the published map stay exactly as they were. The `execArgv: []` workaround is reverted; the doc records the measurements behind the choice. Verified: tsc -b clean; `yarn mlx --help` fine; 80-file slice 1367 passed / 8 skipped; 221-file slice 3651 passed / 40 skipped, no failures.
Adversarial review round 1: one real defect, fixedThe review returned needs-attention and it was right:
I reproduced it independently before touching anything: Rather than patch each spawn, I removed the leak. The mechanism is now a That is also the reviewer's own first recommendation ("Prefer Vite-only resolution derived from the exports maps"), and it matches where Vite is heading: Verification after the fix
Finding 2 (packed-consumer checks) is not addressed here: it is the standing gap this PR documents rather than closes, and it is a separate change. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b5c97759-95be-4ef9-90bf-43ad190869e9) |
`workspaceSource()` builds its map at config load, and a package.json is not a Vite config file: editing one restarted nothing, so a watch session kept resolving a subpath that had moved, missed one that was new, and fell back to `dist` for it. The plugin now watches the workspace manifests and restarts the server on add/change/unlink. Verified by running the dev server and touching `packages/vlm/package.json`: the log reports "server restarted.".
Adversarial review round 2: one fixed, one handed backVerdict: needs-attention. Two findings, both worth stating plainly. 1. The manifest map never refreshed — fixed. The old alias table lived in The plugin now watches the workspace manifests and restarts the server on 2. Coverage traded away — handed back, not fixed here. Correct observation: The |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d11602da-4f43-4e28-942b-33b7daf5a9b9) |
1 participant
What changed
@mlx-node/*specifiers now resolve through each package's ownexportsmap. The hand-maintainedresolve.aliastable invite.config.tsis gone — no alias entry names a workspace package any more.@mlx-node/sourcecondition, first key of eachexportsentry, ahead oftypes/import. The subpath list lives next to the subpath it names, in the package that owns it.ssr.resolve.conditions, with Vite's ssr defaults repeated because that key replaces them."src"infiles, since the condition names files that must exist wherever the manifest does.docs/monorepo-paths.mddocuments the arrangement, the trap described below, and the checklist for adding a subpath.Why
The alias table was a second, unvalidated copy of every
exportsmap, and it had already drifted:@mlx-node/lm/model-discovery+./model-detectiondeclared inexports, imported bypackages/agent/src/provider/models.tsandpackages/server/src/host/discover.ts, nodistartifactyarn mlxand any published install could not resolve them@mlx-node/vlmhad no alias entrydistwhile its siblings resolved tosrc@mlx-node/server/hostinto.../src/index.ts/host— the table carried a comment about the ordering it had to keepThe sharp edge worth knowing
ssr.resolve.conditionsis not just a bundler setting in this stack. Vitest reads it and re-emits it as real--conditionsflags on its Node processes, so Node-level resolution inside the test process picks up the condition too — andnew Worker()inherits those flags. That is how the dashboard's SQLite worker started resolving@mlx-node/*to TypeScript and dying at startup with "TypeScript parameter property is not supported in strip-only mode" (packages/server/src/host/index.tshas one).packages/dashboard/src/worker/client.tsnow spawns withexecArgv: [], mirroring the rule the desktop sidecar fork already followed insupervisor/child-node.ts. Any future nested Node spawn that loads a workspace package needs the same.The condition cannot leak into the other two surfaces: Node ignores
@mlx-node/source(it is not in its condition list, soimportstill resolvesdist), andoxnodelikewise.Verification
yarn typecheck(fulltsc -b)yarn vitest packages/dashboard/__test__execArgvfixagent,privacy,dashboard,server/host,models/doc-preprocess,coreyarn mlx --helpexports→dist, so the oxnode path is intactimport.meta.resolveinside Vitest reportspackages/*/src/**; a childnodeprocess reportspackages/*/dist/**The model-dependent tests outside that slice were not run locally.
The decision worth your eyes
Shipping
srcin the tarball is the one product-level call here. It is what makes the declared condition a real target rather than a dangling entry, and it lets a consumer opt into source intentionally. Yarn 4 cannot strip the condition at pack time (publishConfigsupports a fixed field list that excludesexports/types), so the alternative is aprepackrewrite ofpackage.json. Easy to drop: remove"src"from the sevenfilesarrays and the condition becomes repo-only.Note
Medium Risk
Changes how all Vitest runs resolve workspace imports and expands published tarballs with
src; misconfiguration could break tests or hide missingdistartifacts until runtime.Overview
Replaces the root hand-maintained
resolve.aliaslist for workspace packages with aworkspaceSource()Vite plugin that reads each package'sexportsmap and resolves@mlx-node/*imports to the@mlx-node/sourceTypeScript paths during Vitest/dev only.tsc,oxnode, and Node consumers still usedistbecause the custom condition is not wired intoresolve.conditions/ssr.resolve.conditions(that path would leak into Vitest's NodeexecArgvand break worker/sidecar loads on non-stripable TS).Every affected package
package.jsonnow declares@mlx-node/sourceon each export entry and includes"src"infilesso published manifests stay consistent. The@mlx-node/corealias is removed (itsexportsalready unifyimport/require); only the dashboard@/test alias remains.Adds
docs/monorepo-paths.md(linked fromCLAUDE.md) documenting resolution surfaces, why the Vite-plugin approach was chosen, and how to add subpaths. Updates a comment incore-exports.test.tsto match the new resolution story.Reviewed by Cursor Bugbot for commit 849d638. Bugbot is set up for automated code reviews on this repo. Configure here.