Uh oh!
There was an error while loading. Please reload this page.
fix(desktop): harden core barrel against node-only value exports - #1734
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed the current head. Moving runtime-boundary values to the existing subpath fixes the renderer import boundary without adding a second implementation. The renderer build and CI coverage are appropriate. No findings.
The branch currently conflicts with main, so the resolved head will need another review.
Astro-Han
left a comment
There was a problem hiding this comment.
Review findings (independent deepseek-v4-flash review, 2 passes)
The fix itself (moving runtime-boundary value exports out of the core barrel) is correct and mechanically complete — but the earlier approval no longer covers the head (the merge commits changed the branch), and two P2s need a decision.
P2 — the startup contract test you added was swallowed by the merge; the head has no regression guard
startup-loading-shell-contract.test.ts (3 tests added in 6da3471be, including the barrel-purity assertion) was deleted on main by #1727 and the merge commit fc099b75 kept main's deletion — git ls-tree 4bd7b29a has no such file. The PR body's "startup loading shell contract: 3 passed" verification is therefore not reproducible on the current head, and the exact #1573 regression class this PR fixes can silently return. Please re-land the contract in an executable form (e.g. assert the renderer build output contains no node: imports rather than a source-regex test), or explicitly defer with a reason.
P2 — the barrel still value-exports 3 other Node-only modules; the new comment overstates the invariant
packages/core/src/index.ts:255-260 (tool-ledger-scanner → node:util), :291 (tool-args-identity → node:crypto), :292-295 (canonical-runtime-event → node:util), and :285-290 (tool-recovery-bundle → node:crypto) are still value-exports, while :304-306 claims "renderer imports of @maka/core do not evaluate Node-only modules". Today the renderer graph happens to tree-shake them (verified: 0 node: references in the production bundle; dev-mode Vite uses a browser-external proxy that only throws on call, not on import), so there is no current white screen — but the fix relies on luck, not construction. Either move the remaining Node-only value exports behind subpaths (the subpaths already exist in package.json exports), or correct the comment to state the actual invariant.
Needs discussion — the root-cause mechanism is not reproducible with the pinned toolchain
We rebuilt the PR base (barrel with runtime-boundary value export), the head, and the fix-parent state with the pinned Vite 8.1.5: the production renderer build succeeds in all three states with zero node: references (tree-shaken), and dev mode does not throw on module evaluation (the proxy throws only when a function is called; the renderer never calls them). Current main re-added all four value exports and the renderer still works. If the mechanism is "barrel evaluation of node:crypto throws before createRoot", it doesn't hold under the pinned toolchain — the white screen you observed may have a different root cause, and this PR may be defensive hardening rather than the fix. Please reproduce the original white screen on a clean environment with the pinned toolchain (with bundle evidence), or re-scope the PR as defensive hardening.
Gate: no P0/P1 → not a FAIL; the two P2s need to be handled (fix or explicit deferral) before merge.
sunheyi6
commented
Aug 4, 2026
Merged latest Conflict resolution notes:
The two P2s from the review are addressed on the head:
Local validation: full |
Astro-Han
commented
Aug 8, 2026
Thanks — the revised direction looks sound. Moving Node-only value exports behind the existing subpaths keeps the renderer-facing The earlier review concerns appear resolved. The remaining work is mainly integration with current P2 — Please rebase and preserve the browser-safe root invariant while resolving conflicts This branch now conflicts with Please rebase onto the latest P3 — The test comments overstate the reproduced failure mechanism The comments currently say that Vite imports of It would be more accurate to describe the test as intentionally enforcing a stronger structural invariant: the renderer-facing root barrel must remain loadable without Node built-ins, regardless of current tree-shaking behavior. The unused The implementation itself does not need another redesign. After the rebase and a fresh CI run, this should be ready for another quick review. Thanks! |
Drop value exports for tool-ledger-scanner (node:util), tool-args-identity (node:crypto), canonical-runtime-event (node:util), and tool-recovery-bundle (node:crypto) from the browser-consumed @maka/core barrel; keep type exports and route value consumers to the explicit subpaths. Re-lands the renderer regression guard as an executable test against the built barrel.
a102889 to
b4c8657Compare…#2258 - shell-run.ts gained a node:util dependency in apache#2258; replace the single isDeepStrictEqual use with a local structural comparison so the renderer-facing @maka/core barrel stays free of node:* evaluation - migrate main's new barrel consumers of node-only values to their existing subpaths (tool-args-identity, tool-ledger-scanner, canonical-runtime-event) - reword the barrel boundary test comments to describe the enforced structural invariant instead of a reproduced failure mechanism; drop the unused FIXTURES_DIR constant
b4c8657 to
03f8199CompareUh oh!
There was an error while loading. Please reload this page.
Summary
Defensive hardening of the renderer-facing
@maka/corebarrel: the browser-consumed barrel no longer value-exports any module whose implementation depends onnode:*, and an executable regression guard prevents the Node-only export graph from returning to the renderer entry path.Scope (per review
4bd7b29a, re-scoped from "fix" to defensive hardening — the white-screen mechanism was not reproducible under the pinned Vite 8.1.5 toolchain, where tree-shaking removes thenode:*references and dev-mode proxies throw only on call, not on import):packages/core/src/index.ts— drop value exports fortool-ledger-scanner(node:util),tool-args-identity(node:crypto),canonical-runtime-event(node:util), andtool-recovery-bundle(node:crypto); keep only type exports in the barrel. The explicit subpaths (@maka/core/tool-args-identity,@maka/core/tool-ledger-scanner,@maka/core/canonical-runtime-event,@maka/core/tool-recovery-bundle,@maka/core/runtime-boundary) already exist inpackage.jsonexports and now carry the value consumers (runtime, storage, runtime-host).apps/desktop/src/main/__tests__/renderer-core-barrel-node-boundary.test.ts(+ fixtures) — re-lands the regression guard in executable form against the built barrel artifact the renderer actually consumes: a child Node process registers a loader hook that throws on everynode:*import and then imports@maka/core; it exits non-zero the moment the barrel graph evaluates Node-only code. This replaces the source-regex contract pruned by chore: prune source-regex tests and parallelize the e2e suite #1727.Why defensive
Under the pinned toolchain the production renderer bundle tree-shakes the
node:*references and dev mode never calls the proxied functions, so there is no current white screen — but the invariant ("importing@maka/corein a browser-like environment evaluates nonode:*module") relied on tree-shaking luck rather than construction. Keeping Node-only values behind explicit subpaths makes the boundary structural and test-enforced.Validation
npm test(full suite: clean + build:test + script tests + all workspacetest:dist)@maka/coreevaluates nonode:*modulegit diff --check