build(desktop): stop shipping the renderer's dependency tree twice - #3148
build(desktop): stop shipping the renderer's dependency tree twice#3148Joob1n wants to merge 15 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughProblem solvedDesktop packaging included renderer dependencies twice. This PR moves nine renderer-only packages to
Source of truth and solution scopeThe desktop manifest is the source of truth for renderer-bundled dependency roots. Third-party notice generation and packaged-app verification read this configuration. The PR removes the unreliable denylist path. It does not add a parallel exclusion authority. The solution is the smallest coherent change described in the PR. Dependency classification removes the duplicate production closure. Shared closure traversal includes renderer-bundled packages in third-party notices. Archive-level verification checks the final The archive check replaces the initial dependency-classification test. No existing code or test can be removed without weakening dependency-closure, notice-coverage, or archive-level regression coverage. Validation and risksThe archive check rejects the nine renderer-only packages, requires both PTY packages, and verifies that every shipped renderer package has a notice. The check was tested with failing and passing archives, including an archive with one notice entry removed. Notice generation now combines the Node production closure with the renderer dependency closure. Notices were added for Reported validation includes build, typecheck, lint, format checks, CI, signed macOS arm64 packaging, and successful macOS launch with Settings rendering. A Windows renderer smoke test passed after one environmental timeout. Windows and Linux packaging were not otherwise run locally. Required-check status remains unverified without direct check evidence. Follow-up risks include incomplete coverage from Complexity delta
Total maintenance complexity decreases. The added traversal and archive checks are justified by packaging, hoisting, and licensing risks. Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe desktop manifest classifies renderer-only packages for bundling. Notice generation includes their dependency trees. Packaged-app verification checks renderer duplication, PTY dependencies, and third-party notices on macOS and Windows. ChangesDesktop dependency closure
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to The PR removes renderer-only dependencies from the production archive and reduces shipped size, but release readiness still has bounded risks: Windows validation may fail against older releases, and third-party license notices may be incomplete for dependencies used by shipped renderer code. These should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant PlatformVerifier
participant PackagedAppVerifier
participant AppAsar
participant DesktopManifest
PlatformVerifier->>PackagedAppVerifier: validate packaged dependency closure
PackagedAppVerifier->>DesktopManifest: read renderer dependency roots
PackagedAppVerifier->>AppAsar: inspect packaged node_modules
AppAsar-->>PackagedAppVerifier: return packaged dependency names
PackagedAppVerifier-->>PlatformVerifier: report validation result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the Electron packaging configuration for @maka/desktop to prevent shipping a redundant copy of renderer-only dependencies inside app.asar. It targets the “double-shipped renderer dependency tree” problem described in #3146 by explicitly excluding known renderer-bundled packages from electron-builder’s default production dependency closure.
Changes:
- Expands
electron-builder.config.mjsfilesconfiguration to explicitly exclude a set of renderer-onlynode_modulespackages. - Adds in-file documentation explaining why these dependencies are safe to exclude (and why
@xterm/headlessis intentionally not excluded). - Keeps the existing packaging inputs (
dist/**/*,dist-renderer/**/*,package.json, and test exclusions) while reducing packaged artifact size.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
86e3b14 to
93ba39c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/desktop/electron-builder.config.mjs:40
filesexcludesnode_modules/d3-*/**, but thed3meta-package itself exists in the dependency tree (package-lock.jsonhasnode_modules/d3) and isn’t excluded here. There are noimport/requirereferences tod3inapps/desktop/src/mainorapps/desktop/src/preload, so this will still get packaged as part of the production dependency closure even though it’s renderer-only (via mermaid), adding back avoidable size.
'!node_modules/katex/**',
'!node_modules/d3-*/**',
'!node_modules/dagre-d3-es/**',
likun666661
left a comment
likun666661
left a comment
There was a problem hiding this comment.
Found one packaging issue: part of the renderer-only dependency closure is still included in app.asar.
93ba39c to
a19601f
Compare
|
You were right on both counts, and the measurement is what made it obvious the approach was wrong rather than merely incomplete. Reworked and rebased onto The denylist is gone. The nine renderer-only direct dependencies moved to Moved: Measured the archive the same way you did:
That lands on your 69 / 8.84 MiB. The set that left is the one you named — Safety of the move was checked against built output rather than sources: Added Windows and Linux packaging not run locally. |
0d35e78 to
1659179
Compare
|
Note on the failed The packaged-renderer smoke failed once inside the upgrade-lifecycle step: I re-ran the identical tree and it passed, so it was environmental rather than something this change causes. Two details support that beyond the re-run: the captured stderr is that one line, with no module-resolution error — and a renderer that failed to start would have taken the I could not diagnose it further from the log alone, and two hypotheses I tested were wrong: the CDP port is reserved and released before Electron binds it, but an occupied port makes Electron print no What blocks a conclusive read is that |
|
Thanks for the rework — the core claim holds up under real verification: I checked the compiled output and the 9 packages are loaded only by the renderer (the only non-test hit in Conclusion: PASS with one P2 — a licensing regression that must be handled (fix or explicit deferral with reason) before merge. P2 — the move out of the production closure silently removes license notices for code that still ships and runs. P3 (optional): AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：PASS（1 个 P2 许可回归，合并前需处理或显式延后）。核心声明全部成立：9 个包只被 renderer 加载（dist/main 唯一非测试命中是注释）、闭包机制正确（electron-builder 26.x 用 npm list --omit=dev / 依赖遍历收集，都排除 devDependencies；PR lockfile 闭包 227 个包、9 个一个不在）、@xterm/headless 与 addon-unicode11 经 PTY 栈正确保留且不泄漏回 @xterm/xterm、无其它 workspace 在生产代码引用、CI 布局不破坏。移入 devDependencies 而非 files denylist 是正确形状（denylist 覆盖不了传递 hoisting：mermaid 尾部、d3-* 别名）；新测试钉住分类防回归。P2：移出生产闭包后 THIRD_PARTY_NOTICES.txt 同步丢失这些包及其传递尾的许可声明（383→249 包，react/react-dom/@dnd-kit/@astryxdesign/@xterm/xterm/@maka/ui+mermaid/d3/katex/dompurify/es-toolkit/dayjs/lodash-es/tslib/lucide-react/intl-messageformat 等），但它们的代码仍随 dist-renderer 打进 asar 发布——MIT 许可要求随副本附带声明，notices 文件是这些包唯一的声明载体（renderer 自己的 THIRD_PARTY_LICENSES.txt 只覆盖 vendored SVG 资产、无 vite license 插件）。这是 AGENTS.md 保护区域（licensing）的回归。建议：为 renderer 打包依赖单独生成 notices（基于 vite 构建清单）或显式记录"由制品内嵌机制承担"的决策，并 diff 验证 asar 内 dist-renderer 第三方包与 shipped notices 的覆盖差。P3（可选）：npm audit --omit=dev 不再覆盖随 app 发布的 renderer 依赖（React 出 CVE 不会报警）；packages/ui 仍把 react/@astryxdesign/core 列在生产 dependencies（今天不在闭包内，但未来 @maka/ui 进闭包会静默把 react 等带回 asar，新测试只钉 desktop 分类）；新测试是静态清单断言不是 asar 级检查，防不了传递泄漏和 notices 回归；"before" 基线是 denylist head 而非 main（307→240 与 ~69 自称自洽，但 lockfile 闭包算法与实测数值有绝对差异——方向与终态一致，基线呈现需诚实）。 |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
对抗性复核结论：依赖移到 devDependencies 的方向正确，确实比 files denylist 更符合依赖闭包模型；但当前 revision 引入了发布物许可声明回归，因此不可合并。
P1：renderer 代码仍随 dist-renderer 发布，但对应许可证已从 THIRD_PARTY_NOTICES.txt 删除。
apps/desktop/package.json 将 React、React DOM、@maka/ui、Astryx、dnd-kit、renderer xterm 等九个包移入 devDependencies。Vite 仍把它们以及 Mermaid、D3、DOMPurify、dayjs、es-toolkit、lodash-es 等传递代码打进 renderer bundle；electron-builder.config.mjs:18 继续把 dist-renderer/**/* 放进 app.asar。
但 scripts/generate-third-party-notices.mjs:167-195 只从 npm ls --workspace @maka/desktop --omit=dev 生成 notices。PR 产出的 notice 已找不到上述包，所以当前 check 会对同一个错误闭包自证通过：Node production closure 变小了，实际 shipped renderer module graph 没变，许可清单却跟着缩小。
请保留这次依赖分类和体积收益，但让 notice inventory 覆盖最终发布的两部分：Node production closure + Vite renderer module graph，并在制品层验证 shipped third-party code 均有对应 notice。不能通过把九个包移回 dependencies 来回避问题。
P2：新增的 packaged-dependency-closure.test.ts:44-69 没有验证 packaged closure，只验证 manifest 分类。
即使 electron-builder 行为变化、传递包重新泄漏进 asar、renderer 不再 bundle 某个声明为 devDependency 的包，或者 runtime xterm 从真实闭包消失，这两个测试仍会绿。第二个测试只断言 @xterm/headless / addon-unicode11 不在 desktop devDependencies，甚至不证明它们实际存在于发布闭包。
建议删除这 70 行静态分类测试，改由 package verifier 直接检查 app.asar：九个 renderer-only source packages 缺席，main/runtime 必需包存在，renderer smoke 真实启动；同一 verifier 同时校验 notices 覆盖。
结论：生产改法的核心方向是最优的，没有必要删除或回退依赖移动；需要重构的是“发布依赖/许可来源”的判定，从 package.json 标签改为最终 shipped graph。CI 当前全绿，Windows 也完成真实打包与 smoke，但许可 P1 修复前不 ready to merge。
1659179 to
4285cb8
Compare
|
@hqhq1025 @Astro-Han — both P1s addressed, and the second one changed the shape of the check rather than patching it. P1 — the licensing regression. Correct, and it was the more serious of the two: moving the nine packages out of the production closure made
P2 — the test asserted classification, not the artifact. You were right, and I deleted the 70 lines rather than defend them. Replaced by
Verified in both directions rather than asserted:
On the baseline in the table, since you asked for it stated honestly: the 307 → 240 measurement compares this branch against the denylist revision, not against Not addressed here, and worth their own issues rather than a quiet fix: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/verify-windows-x64.mjs (1)
100-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip dependency-closure checks for baseline releases.
verify-windows-installer-lifecycle.mjspassesexpectedVersionfor the pinned previous installer.assertPackagedDependencyClosure(resources)remains unconditional and uses the current manifest, while the previous manifest declared all renderer roots as direct dependencies. The baseline can therefore contain those roots and fail withapp.asar carries renderer-only packages a second time. Skip this check whenexpectedVersion !== undefined, or use the baseline manifest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a09ef6f4-b572-4fae-bdf2-4a8cfdc78e4f
📒 Files selected for processing (7)
apps/desktop/electron-builder.config.mjsapps/desktop/package.jsonapps/desktop/resources/licenses/npm/THIRD_PARTY_NOTICES.txtscripts/generate-third-party-notices.mjsscripts/verify-macos-arm64-dmg.mjsscripts/verify-packaged-app.mjsscripts/verify-windows-x64.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/electron-builder.config.mjs
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
4285cb8 to
26d0926
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 440d20ee-b425-4031-87c6-0d28138860d5
📒 Files selected for processing (1)
scripts/generate-third-party-notices.mjs
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Moving renderer-only sources out of Electron's production dependency closure is the right way to remove duplication, and deriving notices plus artifact checks from the declared renderer roots is much stronger than a transitive denylist. Two verification boundaries need to follow that model consistently.
The simplest first-principles rule is: inspect each artifact against the contract that produced it, and define security/license coverage from what ships rather than npm dependency class. That means current artifacts get the new closure assertion, historical upgrade baselines retain their historical contract, and bundled renderer roots remain inside audit coverage even though npm labels them dev dependencies.
Review performed with two Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I reproduced the P1 in live Windows release-check logs and verified the latest head.
中文评论
把 renderer-only sources 移出 Electron production dependency closure 是消除重复的正确方案；从声明的 renderer roots 派生 notices 与 artifact checks，也明显优于维护传递依赖 denylist。但两个验证边界必须一致跟随这一模型。
更符合第一性原理的规则是：每个 artifact 按生成它的契约验证，安全/许可覆盖按实际发布内容定义，而不是按 npm dependency 分类。当前产物应用新 closure assertion；历史 upgrade baseline 保留历史契约；renderer roots 即使被标记为 devDependencies，只要仍被 bundle 发布，就必须继续进入 audit。
本次审查使用了两位 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已在实时 Windows release-check 日志中复现 P1，并复核最新 head。
| "electron-builder": "26.15.3", | ||
| "esbuild": "^0.27.7", | ||
| "linkedom": "^0.18.13", | ||
| "react": "^19.2.1", |
There was a problem hiding this comment.
P2 — Shipped renderer code falls out of the release vulnerability audit. The workflows still use npm audit --omit=dev; after this move, React and the other declared renderer roots remain in dist-renderer but are excluded from that audit solely because npm calls them dev dependencies. Extend the existing shipped-artifact graph to an auditable renderer closure (or run an equivalent dedicated bundled-graph audit) so the security boundary matches what is released.
There was a problem hiding this comment.
Rechecked on exact head 9e74bcc95. The dedicated audit is a good step, but this P2 remains: it still derives the renderer closure from the hand-maintained maka.rendererBundledDependencies list, while the dependency-audit workflow does not build/read Vite's actual dist-renderer/bundled-npm-packages.json. A newly imported renderer package omitted from the list can therefore ship without this security lane auditing it; only the later release artifact verifier detects the drift. Please make the audit consume the actual bundled graph, or build and validate that graph in the audit workflow.
中文说明
当前 head 已新增专用 audit，但它仍以手工 rendererBundledDependencies 为权威，而 dependency-audit workflow 不生成或读取 Vite 的真实 bundled graph。新增但漏列的 renderer import 仍会绕过安全审计，只在更晚的 release verifier 才被发现，因此这个 P2 仍有效。
`app.asar` carried a second copy of the renderer's dependency sources. Vite emits everything the renderer loads into `dist-renderer`; electron-builder then walked the production dependency closure of `apps/desktop/package.json` and packaged those same packages again, as sources nothing ever loads. A `files` denylist was the first attempt and could not hold. It has to name every transitive package too, so excluding `mermaid` did not stop electron-builder from independently collecting what `mermaid` hoists, and `d3-*` never matched the bare `d3` meta-package. Move the nine renderer-only direct dependencies to `devDependencies` instead. That removes them and everything only reachable through them from the closure, so the denylist is deleted rather than extended. Notices follow what ships, not what npm places in node_modules. The generator now unions the Node production closure with the closure of the renderer roots, declared once in `maka.rendererBundledDependencies` and read by both the generator and the packaged-artifact check so the two cannot drift. Without that, moving these packages out of the production closure would have dropped the notices for code that still ships inside `dist-renderer`. Verification moved from the manifest to the artifact. `assertPackagedDependencyClosure` reads `app.asar` directly: the nine must be absent, `@xterm/headless` and `@xterm/addon-unicode11` must be present because the PTY stack loads them, and every shipped renderer package must have a notice. A manifest assertion would have stayed green through a change in how electron-builder walks the closure, a transitive package leaking back in, or the notices regressing. Measured on the archive: 307 packages / 131.39 MiB of `node_modules` before, 240 / 122.98 MiB after — 67 packages and 8.41 MiB out, none added. Generated-by: Claude Code
26d0926 to
9d1eaf6
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The latest increment fixes the prior Windows-baseline blocker correctly: current artifacts still receive the new closure assertion, while a historical upgrade baseline is verified against its historical contract. The dependency reclassification, notice-union generation, and removal of the transitive denylist remain the right first-principles direction; current-main merge is clean and all live checks are green.
One artifact-boundary mismatch remains. The verifier inspects app.asar from the artifact but reads notices from the checkout, so it can certify a package whose shipped notice is stale or empty. The smallest correction is to read the notice from resourcesPath, then use the same renderer closure as the generator for complete coverage. This keeps the rule simple: validate what ships using what ships.
Reviewed with Codex using two independent reviewer agents and an external DeepSeek review; I verified the latest fix, artifact paths, current-main merge, prior discussion, and live Windows/CI checks.
中文
最新增量正确修复了此前的 Windows baseline 阻塞：当前产物仍执行新的 closure assertion，历史升级基线则按其历史契约验证。依赖重新分类、notice union 生成和删除传递依赖 denylist，仍是符合第一性原理的方向；与当前 main 可干净合并，实时检查全绿。
仍有一个 artifact 边界不一致：verifier 检查的是 artifact 中的 app.asar，但读取的是 checkout 中的 notices，因此即使发布物携带的 notice 过期或为空也会通过。最小修复是从 resourcesPath 读取实际发布的 notice，并用与 generator 相同的 renderer closure 做完整覆盖。规则保持简单：用发布物自身验证发布物。
本次由 Codex 配合两个独立 reviewer agent 和外部 DeepSeek 审查；我核验了最新修复、artifact 路径、与当前 main 的合并、已有讨论和实时 Windows/CI 检查。
|
复核最新 head 不过当前仍不建议合并，两个发布边界尚未闭合：
另外，artifact verifier 目前只检查九个 direct roots 是否有 notice，没有拿 generator 的完整 renderer transitive closure 做覆盖对比；建议两边复用同一个 closure authority，避免再次出现“direct roots 有 notice、传递发布代码漏 notice”的自证通过。现有相关 threads：#discussion_r3804505722、#discussion_r3804737815。 |
The closure verifier inspected app.asar from the artifact but read THIRD_PARTY_NOTICES.txt from the checkout, so an artifact carrying a stale or empty notice could still be certified. It now reads the notice inside resourcesPath and checks it against the complete shipped closure (Node production plus the renderer bundle) instead of only the declared roots, with the closure definition extracted to third-party-closure.mjs so the generator and the verifier cannot drift; regression tests build real asar fixtures and run under check:release. Security coverage follows the same boundary: npm audit --omit=dev no longer sees renderer roots that still ship, so the audit workflows gain audit-shipped-dependencies.mjs, which fails on any advisory whose affected installed copy is version-exact in the shipped closure (a vulnerable copy on a tooling-only path stays out). Two renderer-bundled packages were outside the declared roots: simple-icons and @ant-design/icons-svg are imported by renderer source and ship in dist-renderer, so they join rendererBundledDependencies and the generated notices; the manual asset notice now separates the paths vendored at simple-icons@15.22.0 from the ones bundled at the package-lock version, which had drifted. The missing-roots validation also covers workspace roots, whose absence would silently drop their third-party tails. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The latest head resolves the earlier packaged-notice, real-asar fixture, and historical-baseline findings. One closure gap remains: the new audit treats a hand-maintained root list as the authority, while the renderer actually ships additional CSS-imported npm packages.
The first-principles authority is the renderer build graph/artifact, not a second manually synchronized manifest. At minimum, include both Fontsource packages as roots and add a regression; the durable Occam solution is to derive the shipped renderer closure from Vite's module graph so licensing and security audit share the artifact's real dependency set.
Reviewed with Codex using two independent review passes and DeepSeek V4 Flash as an external adversarial pass; I verified the closure inputs against this exact head and current main.
中文
最新 head 已修复 packaged notice、真实 asar fixture 和历史 baseline 问题。仍有一个 closure 缺口：新 audit 把人工维护的 root 列表当权威，但 renderer 实际还发布了通过 CSS import 引入的 npm 包。
第一性原理下，权威应是 renderer build graph/artifact，而不是第二份人工同步清单。最小修复是加入两个 Fontsource roots 并补回归；长期最简方案是从 Vite module graph 推导 closure，让许可证与安全审计共用真实 artifact 依赖集。
本次由 Codex 两轮独立审查，并使用 DeepSeek V4 Flash 做外部对抗审查；我核对了当前 head 与最新 main。
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Three consecutive review rounds found the same failure class: a package entered the renderer bundle through a path the hand-maintained root list did not anticipate (a direct import, a deep import, a CSS @import). The durable fix is to stop trusting the list: the vite build now records every npm package the bundle actually contains — module-graph entries plus emitted assets' source packages, which is how CSS-only chains like Fontsource surface — into dist-renderer/bundled-npm-packages.json. The notices gate fails when the record names a package outside the declared closure, and the release verifier reads the same record out of app.asar, so the artifact is judged by its own account of itself. That check immediately demanded the two OFL Geist font packages, which ship but must not enter the ASF-policy npm notices; they join the roots with an asset-license channel — audited and closure-checked as shipped packages, licensed by the vendored GEIST license files the artifact already carries, with both the generator and the verifier enforcing that the file actually ships. Two closure bugs from review are fixed with it: a workspace root's slot in the full npm tree carries dev edges, so @maka/ui was dragging @types/react, @types/react-dom into the shipped notices — workspace roots now walk their own production closure; and the asar check compared the archive against the declared roots only, so a renderer-only transitive package could leak back in silently — it now requires the archive to stay inside the production closure, which the real artifact satisfies exactly. Verified against a freshly packaged unsigned macOS app: the full assertion passes; removing one Fontsource declaration fails the notices gate naming it; each rejection path is covered by fixture tests that build real asar archives. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
@Astro-Han — you called the durable option correctly, and after three rounds of the same failure class (a direct import, a deep import, now a CSS import) I took it rather than patching the instance again. The bundle-graph record. The vite build now writes Fontsource specifically could not just join the npm notices: both packages are OFL-1.1, which the generator's ASF policy set deliberately excludes, and the artifact already carries their license texts as @Astro-Han's earlier line-comment asks and both remaining Qodo findings land in the same commit:
Verified end to end against a freshly packaged (unsigned, Not derived from the graph: the Node production closure itself (electron-builder's own walk is the authority there, and the asar-subset check pins it) and the OFL channel's license file list, which is two entries with an existence check on both ends. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The latest head correctly makes the Vite bundle graph authoritative for renderer roots, closes the Fontsource gap, keeps historical baselines on their historical contract, and validates notices from the artifact. Required checks are green. One artifact traversal gap remains below.
Two independent fixture probes reproduced the same result: an unknown package under node_modules/foo/node_modules is invisible to the current asar walk, so the verifier can certify an artifact whose complete package closure it has not inspected. The external DeepSeek pass treated this as an untested gap because today's nested production dependencies are normally in-closure; I am retaining P2 because the stated authority is the artifact itself, and a shallow walk cannot establish that invariant. Recursive traversal is small and removes the ambiguity.
I am leaving a review comment rather than approving. AI-assisted review by Codex with two independent reviewer passes and OpenCode Go DeepSeek V4 Flash (high); I verified the current head, real-asar fixture behavior, shipped-closure inputs, prior findings, and live release checks.
中文
最新 head 已正确把 Vite bundle graph 作为 renderer roots 的权威，补齐 Fontsource、历史 baseline contract 和 artifact 内 notices 校验；必需检查全绿。仍有一个 artifact 遍历缺口。
两次独立 fixture probe 都复现：node_modules/foo/node_modules 下的未知包不会被当前 asar walk 看到，因此 verifier 可能认证一个并未完整检查 package closure 的 artifact。DeepSeek 将其视作未测试缺口，因为当前正常嵌套 production dependencies 通常都在 closure 内；主线程仍保留 P2，因为既然 artifact 是权威，浅层遍历就无法证明该不变量。递归遍历改动很小，也能彻底消除歧义。
本次由 Codex、两个独立 reviewer 与 OpenCode Go DeepSeek V4 Flash high 辅助；已核对当前 head、真实 asar fixture、shipped closure、历史 findings 和实时 release checks。
The asar walk stopped at the top level, so a package nested under another (node_modules/foo/node_modules/bar — what npm produces on a version conflict) was invisible, and the verifier could certify an archive whose complete package closure it had not inspected. The walk now recurses through every nested node_modules, skipping dot entries (.bin, .package-lock.json); the production-closure allowlist already carries nested names, so the real artifact passes unchanged, and a fixture with a leak hidden one level down turns red — removing the recursion alone fails that test. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
@Astro-Han — fixed in |
The Windows package job failed with the exact signature issue maka-agent#3196 records — renderer CDP not exposed within 30s, no port in the log to classify further — which a rerun of an identical tree has previously cleared on this PR. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
…face The slash-command-menu e2e case timed out on toBeVisible; this branch changes release scripts, licensing, and the vite bundle-graph plugin only, and the same suite passed on the previous head of this PR. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The new recursive walk correctly closes the previously reported nested node_modules bypass, including scoped packages, and the real ASAR regression is good.\n\nOne remaining artifact-authority gap is inline below: the verifier certifies names, not the exact package versions that shipped. This matters precisely where nested version conflicts are possible. The simplest final state is one exact {name, version} production closure compared with package metadata read from the archive; the same set can drive notice validation.\n\nThe current Windows package check is failing and two checks are still running, so CI is not merge-ready independently.\n\nReviewed with Codex using two independent reviewer agents and OpenCode Go DeepSeek V4 Flash (high); I verified the exact delta, the prior nested-package finding, real ASAR fixtures, production closure semantics, and live CI.\n\n
中文
\n\n新的递归遍历已经正确修复此前 nestednode_modules 绕过，包括 scoped package；真实 ASAR 回归测试也很好。\n\n下面仍有一个 artifact authority 缺口：verifier 只认证 package name，没有认证实际发布的精确 version；这恰好在 nested version conflict 场景中最重要。最简单的最终状态是从 archive 读取 package metadata，与精确 {name, version} production closure 对比；同一集合还可复用到 notice validation。\n\n当前 Windows package check 失败，另有两项检查仍在运行，因此 CI 本身也尚未 merge-ready。\n\n本次由 Codex 配合两个独立 reviewer agent，以及 OpenCode Go DeepSeek V4 Flash（high）审查；我核验了精确增量、既有 nested-package finding、真实 ASAR fixture、production closure 语义和实时 CI。\n\n| { collectClosure, collectPackagedAllowlist } = {}, | ||
| ) { | ||
| const asarPath = join(resourcesPath, 'app.asar'); | ||
| const packaged = asarNodeModules(asarPath); |
There was a problem hiding this comment.
[P2] Verify the exact versions present in app.asar, not only package names. A real archive containing react@18.0.0 currently passes against an allowed production closure containing react@19.2.0, because both sets reduce to react. That can certify the wrong vulnerable or differently licensed nested version. Read each package’s package.json during this recursive walk, compare {name, version} against the exact production closure, and add a wrong-version ASAR fixture.
There was a problem hiding this comment.
Rechecked on exact head 9e74bcc95; this P2 is still present. The recursive walk now finds nested package names, but asarNodeModules() still discards each package version. The allowlist comparison therefore remains name-only, so a wrong or vulnerable version with an allowed name can pass. Please read each archived package's package.json, compare exact {name, version} pairs, and add the wrong-version ASAR fixture requested here.
中文说明
当前 head 已能递归发现 nested package，但仍只比较包名，没有读取 archive 内 package.json 的版本。因此允许名下的错误或有漏洞版本仍可能通过。请比较精确 name+version，并补 wrong-version ASAR fixture。
Same signature issue maka-agent#3196 records, in the same upgrade-lifecycle step as its original filing. The diagnosability fix is tracked there. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the follow-up fixes. The packaging boundary is now much cleaner: the renderer dependency closure, shipped notices, nested modules, historical baseline behavior, and release audit are aligned, and the latest two commits only retrigger CI.
I’m keeping COMMENT on this exact head because one concrete verifier gap remains in the existing inline thread at scripts/verify-packaged-app.mjs:308-355: the ASAR walk compares package names but not the versions actually present in each package's package.json. That means an allowed name at the wrong shipped version can pass even though the expected production closure is version-specific. Please compare exact {name, version} identities and add a wrong-version ASAR fixture. I have not duplicated the inline finding here.
After that thread is addressed and the remaining checks finish green, this should be ready for another pass.
AI-assisted review disclosure: Codex re-reviewed exact head 4bfa261 and verified the surviving issue against the current verifier and tests.
中文说明
这轮修复已经把 renderer 依赖闭包、发布 notices、嵌套依赖、历史 baseline 和 release audit 基本统一。当前仍有一个具体缺口：ASAR 检查只比较包名，没有比较实际打包进去的版本，因此同名错误版本仍可能通过。建议按精确 {name, version} 校验，并增加 wrong-version ASAR fixture。该问题已有行内线程，这里不重复发表。
Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The current design now correctly removes the renderer-only production closure, includes CSS/Vite bundle evidence, traverses nested archive packages, checks the artifact's own notices, and avoids applying the new invariant to the historical upgrade baseline. The PR body and all eight commits have complete Claude Code disclosure/trailers. This is packaging/audit behavior rather than product UI/UX, so no screenshot is required.
Two P2s remain in existing threads, which I rechecked and replied to rather than duplicating: the shipped vulnerability audit still trusts the hand-maintained renderer-root list instead of the actual Vite bundled graph, and the ASAR verifier still compares package names without exact versions.
The exact head is also CONFLICTING / DIRTY, has a prior changes-requested decision, and currently has no GitHub Actions run beyond CodeRabbit. Please resolve the two live threads, rebase onto current main, resolve stale fixed threads, and request an exact-head rereview with green checks.
Reviewed with Codex as an AI-assisted code review. I verified the exact-head changes, current thread state, dependency/license/audit authorities, merge state, CI, UI scope, and commit provenance; no external model output was used.
中文说明
当前方案已经解决 renderer-only production closure、CSS/Vite bundle evidence、nested archive、artifact 内 notice 与历史 baseline 等问题，AI 说明也完整，且不涉及产品 UI/UX，无需截图。但仍有两个已存在的 P2：安全审计仍信任手工 renderer root 列表而不是真实 Vite bundle graph；ASAR 校验仍只比较包名、不比较精确版本。我已在原 thread 回复确认，没有重复开新 inline。当前 head 还与 main 冲突且没有有效 Actions 结果；请修复、rebase、清理 stale threads 后再按新 head review。
Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
…deps-from-asar # Conflicts: # apps/desktop/electron-builder.config.mjs # package.json
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 4cede165267ec4f1914e5704cb9020243c66997b.
The branch is mergeable and most checks are now green, but the two existing P2 findings remain unchanged, so I am keeping this at COMMENT rather than adding duplicate inline threads:
- The shipped-dependency security audit still derives the renderer closure from the hand-maintained
maka.rendererBundledDependencieslist instead of consuming the actual Vite bundle graph. A newly imported renderer package omitted from that list can still evade this audit. - The ASAR verifier still collects and compares package names only, not the versions found inside the archive. A packaged wrong version can therefore satisfy an allowlist entry for the expected name.
Please address the existing threads with focused regressions. The AI disclosure and trailers are complete, and this PR does not introduce a user-visible UI/UX change requiring screenshots. The remaining package check is still running.
AI-assisted review disclosure: Codex re-reviewed the exact head, current implementation, existing threads, CI, UI scope, and provenance metadata. No external model was used. Astro-Han authorized this review campaign.
中文说明
当前 head 已可合并且大部分 CI 已绿，但两个既有 P2 仍未修复：安全审计仍依赖手工 renderer 依赖列表而非真实 Vite bundle graph；ASAR 校验仍只比包名、不比 archive 内实际版本。为避免重复，没有新增 inline。AI 披露完整；本 PR 无用户可见 UI/UX 变化，不要求截图。
…te step Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head 1ae4b4a56b89398ccc0a3ef34ac9d714e78a2cd9.
This head only retriggers CI and does not change either affected implementation, so the two existing P2 findings remain current: the shipped-dependency audit still relies on the hand-maintained renderer dependency list rather than the actual Vite bundle graph, and the ASAR verifier still compares package names without verifying the archive’s exact package versions. I am not duplicating the existing inline threads.
The new commit carries Generated-by: Claude Code; the PR-level disclosure remains complete. The fresh CI run has only just started.
AI-assisted review disclosure: Codex reviewed the exact-head delta, current implementations, existing threads, live CI, and provenance metadata. No external model was used. Astro-Han authorized this review campaign.
中文说明
这个新 head 只用于重触发 CI，没有修改两个问题所在的实现，因此两个既有 P2 仍然有效：依赖审计未消费真实 Vite bundle graph；ASAR 校验只比包名、不核对 archive 内精确版本。为避免重复，没有新增 inline。新提交的 Claude Code trailer 与 PR 披露完整，新的 CI 才刚开始。
Same tree passed the suite on the previous run; the successor-admission shutdown test timed out waiting for root close. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
@M4n5ter @Astro-Han — the latest head's workflows are sitting in |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for rerunning the workflows. I re-reviewed exact head 41870ad881f81a4e6602428336425e9c0bd3729a and am keeping this at COMMENT for now.
The two existing P2 threads remain actionable on this head:
- The shipped-dependency audit still relies on the hand-maintained
rendererBundledDependencieslist instead of the actual Vite bundle graph, so a newly bundled renderer import can still escape the audit. - The ASAR verifier still checks package names but not the exact archived package versions, so a wrong-version package can still pass verification.
I did not add duplicate inline findings. The other five unresolved threads appear fixed and can be resolved. The newly approved exact-head workflows are still running, so a green result is also pending.
Smallest path: make the audit consume the generated Vite bundle manifest, make ASAR verification read and compare each archived package.json version, and add the corresponding omission/wrong-version regressions. I’ll be happy to re-review the next head.
AI-assisted review disclosure: OpenAI Codex performed the exact-head code and review-thread analysis; I verified the cited code paths, severity, deduplication, provenance, and live CI state before posting.
中文说明
当前 head 只是重跑 CI，没有修复两个既有 P2：依赖审计仍使用手工清单而非真实 Vite bundle graph；ASAR 校验仍只看包名、不校验归档内的精确版本。其余 5 个旧线程已由代码修复，可以关闭。最小修复是让审计直接消费生成的 bundle manifest，并读取 ASAR 内各包的 package.json 做精确版本比较，再补漏列和错误版本回归测试。
|
Thanks for approving the runs. The one failure is the pre-#3241 CDP smoke fault again — fifth occurrence of that signature in two days, and this PR cannot escape it on its own because merge-CI takes the smoke from main. Rather than spending another approval on a dice roll: reviewing #3241 first (14/14 green, small, self-contained in the release verifier) removes the fault class, and a rerun here afterwards should settle cleanly. Everything else on this head is green. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 360ff2d7. The removal itself is safe, and I verified that rather than taking it on trust. My findings are about the new guards and the scope around them, not about a package that stops shipping.
I resolved both package-lock.json states with an npm-resolution walk from apps/desktop's dependencies alone: 129 name@version entries leave the production closure and 0 enter, while the lockfile's packages map is byte-identical — 0 added, 0 removed, 0 version changes. So the installed node_modules layout, hoisting, and which copy of a transitive dependency wins are unchanged; only the closure electron-builder copies into app.asar shrinks. That matters because build:main is plain tsc, so the main process really does resolve from app.asar/node_modules at runtime.
The evidence that nothing load-bearing left:
- Grepping all 120 removed names as import specifiers across
apps/desktop/src/{main,preload,shared}and every production workspace'ssrcyields two hits, both type-onlyimport type { … } from '@maka/ui'(erased — confirmed in the emitteddist/main/*.js), plus the literal string'scheduler'as a CLI argument. - The 184 built renderer chunks in
dist-renderer/assets/*.jscontain zero bare module specifiers. The renderer never resolved fromapp.asar/node_modules; the second copy really was dead weight. - I enumerated every bare dynamic
import()/require()in production source —node-pty,@xterm/headless,@xterm/addon-unicode11,qrcode,electron,ai,@maka/eval— and theimport.meta.resolve('@maka/runtime-host/…')inruntime-host-boot.ts. All remain in the after-closure. No removed package ships a.node/.dll/.dylib/.so/.exe. There is noasarUnpackkey on either side, so electron-builder's automatic native-module unpacking is untouched. - Notices go 384 → 386 packages: nothing removed, two added. The closure change strictly expands license coverage.
The architectural point worth stating: this PR moves the renderer's license provenance from "whatever npm says is a production dependency" to "whatever vite-bundled-packages.js records plus a declared roots list". That is the right seam — the rollup module graph really is closer to the truth than the npm closure — but it makes that one recorder the sole authority, and validateBundledPackageRecord only asserts record ⊆ closure, never closure ⊇ actual bundle. A package the recorder misses is silently missing from the notices with no check failing. Today's four bare CSS @imports are all covered by accident of also being JS imports or emitting hashed font assets; the inline finding explains the case that would not be.
I ran the PR's new scripts/verify-packaged-app.test.mjs in isolation on macOS: 9/9 pass, and the fixtures build real asars so the header parsing and nested-node_modules walk are genuinely exercised. The existing packaged smoke drives the real binary over CDP and asserts React mounted and the app shell rendered, which covers the renderer half of this change well.
Two mechanics for the record, not findings: the head has no reported status checks and reviewDecision is CHANGES_REQUESTED, so the test check and a non-author committer approval are still outstanding; and the branch carries six empty retrigger commits that a squash-merge will absorb.
Reviewed with Claude Opus as an analysis assistant. Closure and lockfile deltas, the bare-specifier scan, the native-binary scan, and the new test run are reproduced by execution; everything else is confirmed by reading source at this head. Windows and macOS reasoning is by inspection — I did not package on a Windows host.
| import { createServer } from 'node:net'; | ||
| import { join } from 'node:path'; | ||
| import { join, resolve } from 'node:path'; | ||
| import { extractFile, getRawHeader } from '@electron/asar'; |
There was a problem hiding this comment.
[P2] Declare @electron/asar instead of relying on hoisting. It is not in any package.json in this repository and resolves only because electron-builder → app-builder-lib happens to hoist it to the root node_modules. This import is now on the release path: check:release runs scripts/verify-packaged-app.test.mjs, and check:release itself runs inside scripts/package-macos-arm64.mjs — so an electron-builder bump that pulls a different major, or that stops depending on asar at all, turns this into ERR_MODULE_NOT_FOUND and aborts release packaging before any artifact exists. Not user-visible; release-blocking on the day it lands. Confirmed by reading code at this head and by inspecting package-lock.json (the installed directory exists, no manifest declares it). Add it to root devDependencies, pinned to the version electron-builder currently resolves. Same import in scripts/verify-packaged-app.test.mjs:6.
| if (name === '.vite') return; | ||
| packages.add(name); | ||
| }; | ||
| for (const id of this.getModuleIds()) collect(id); |
There was a problem hiding this comment.
[P2] Collect the CSS pipeline's dependencies too, or weaken the claim in this file's header. getModuleIds() is described here as "the one authority that sees every way a package can enter the bundle", but a package reached only through a bare CSS @import never becomes a rollup module: Vite registers postcss @import dependencies with this.addWatchFile(file) and inlines them at transform time (confirmed in the installed vite@8.1.5 source). The asset fallback below rescues such a package only if its CSS emits a url() asset. Because validateBundledPackageRecord asserts only record ⊆ closure and never the reverse, a miss is silent. Concretely: adding a pure-rules CSS dependency — normalize.css, or an icon-font CSS using data: URIs — to apps/desktop/src/renderer/styles.css would ship its rules inside dist-renderer/assets/*.css in app.asar with no entry in THIRD_PARTY_NOTICES.txt and no failing check. That is an ASF licensing miss in a released artifact, and this PR creates the exposure: before it, such a package was a production dependency and the notice generator picked it up from the npm closure. Today's four bare CSS imports are all covered — @astryxdesign/core is also a JS import and both Fontsource packages emit .woff2 assets I confirmed are present. Vite behaviour confirmed by reading the installed source; current coverage confirmed by grep plus the emitted assets. Record the postcss dependency set from a transform hook, or scan the emitted CSS text. Regression test: a bare CSS @import of a package with no url() assets must appear in bundled-npm-packages.json.
| // renderer-only transitive package included, not just the declared roots. | ||
| const allowed = collectPackagedAllowlist | ||
| ? await collectPackagedAllowlist() | ||
| : collectProductionNames('@maka/desktop'); |
There was a problem hiding this comment.
[P2] Assert that the archive's code resolves inside the closure, not just that its node_modules matches it. assertPackagedDependencyClosure proves no package directory leaked in and none was trimmed out, but nothing checks that the shipped JavaScript's bare imports are satisfiable — and this PR is what creates that failure mode. Two live instances at this head: files: ['dist/**/*'] with only !**/__tests__/** still ships 65 tsc side-files under dist/renderer/, ten of which statically import react, react-dom, @maka/ui, or @dnd-kit/* — packages no longer in the archive — so app.asar now carries ESM whose static imports cannot resolve; and @maka/ui is now a devDependency while runtime-host-skills-ipc-main.ts and preload.ts import types from it, so converting either to a value import passes typecheck, lint, build, and this closure assertion. Nothing loads the side-files today: the only main→dist/renderer edge is dist/main/computer-use/cursor-overlay-window.js:23, which I walked and which reaches no removed package. The failure that would escape is a value import of @maka/ui from a lazily loaded main module — it throws ERR_MODULE_NOT_FOUND only in the packaged app, only when the user opens that surface; an eagerly loaded one would fail boot and the packaged smoke would catch it. Confirmed by reading code and by grep over the built tree. Walk the archive's dist/**/*.js for bare specifiers and assert each is in collectProductionNames('@maka/desktop') — one check covers both instances. Separately worth asking whether dist/renderer/** should ship at all.
| // Same seam the sandbox check uses: a baseline install predates this | ||
| // classification, so requiring it of a previously released build would fail | ||
| // a release that was correct when it shipped. | ||
| if (expectedVersion === undefined) await assertPackagedDependencyClosure(resources); |
There was a problem hiding this comment.
[P3] Make the closure gate overridable, like the sandbox gate this comment cites. The added comment says "same seam the sandbox check uses", but the sandbox seam above is requireWindowsSandbox = expectedVersion == null and is an overridable option — its comment exists precisely to explain why a strict undefined check with no override is wrong. scripts/verify-windows-autoupdate.mjs:436-442 verifies a genuinely current build (only the version is bumped) and explicitly passes requireWindowsSandbox: true, requireDisclaimer: true to defeat this skip; it has no way to re-enable the closure assertion, so the upgraded install is never closure-checked. Impact is low because verify:windows-x64 checked the same bytes earlier in the release job. Confirmed by reading code at this head. Add requireDependencyClosure = expectedVersion == null to the options destructure and set it true at the autoupdate call site.
@electron/asar was imported by the verifier and its test but declared in no manifest, resolving only because electron-builder hoists it. That import is on the release path, so an electron-builder bump that moved or dropped it would abort packaging before any artifact existed. Declared at the version currently resolved; the lockfile gains one line and the installed tree is unchanged. The bundle recorder's header claimed the module graph sees every way a package enters the bundle. It does not: Vite inlines a CSS `@import` at transform time, so a package of pure rules never becomes a module and, emitting no `url()` asset, leaves no trace — it would ship its rules inside the archive with no notice and nothing failing. `getWatchFiles` is unavailable in this Vite's plugin context, so the generator reads the first-party stylesheets instead and requires every package they import by name to be in the shipped closure. A third-party stylesheet importing another package needs no scan: that package is its dependency, so the closure already reaches it. Matching node_modules against the closure said nothing about whether the shipped code can resolve what it imports. It now walks the archive's `dist/**/*.js` and requires each bare specifier to be carried, provided by Electron, or a builtin. That surfaced the real instance review named: `dist/renderer/**` ships 19 tsc side-files importing react, @maka/ui and @astryxdesign/core, which the renderer bundles rather than ships. Main reaches exactly one subtree there — the cursor overlay engine — so only that subtree is packaged now. The closure gate is defaulted and overridable like the sandbox and disclaimer gates beside it, and the auto-update check asks for it back on the upgraded install, which is a current build rather than a baseline. Verified against a freshly packaged app: the full assertion passes, the renderer smoke still mounts, `dist/renderer` carries only the overlay subtree, and repackaging without the exclusion fails the new check naming all three packages and their files. Reported by @Astro-Han. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Summary
app.asarshipped a second copy of the renderer's dependency sources. Vite emits everything the renderer loads intodist-renderer; electron-builder then walked the production dependency closure ofapps/desktop/package.jsonand packaged those same packages again, as sources that are never loaded.The first version of this PR excluded them with a
filesdenylist. That was the wrong shape, and @likun666661's review showed why with measurements: a denylist has to name every transitive package too, so excludingmermaiddoes not stop electron-builder from independently collecting the hoisted packagesmermaidpulls in, andd3-*never matched the bared3meta-package. On that head the archive still carried ~69 renderer-only packages.So the fix moved to the closure itself. The nine renderer-only direct dependencies now live in
devDependencies, which takes them and everything only reachable through them out of the production closure. The denylist is gone rather than extended — there is nothing left for it to catch, and nothing left to keep in sync.Moved:
@maka/ui,react,react-dom,@astryxdesign/core,@astryxdesign/theme-neutral,@dnd-kit/core,@dnd-kit/sortable,@xterm/xterm,@xterm/addon-fit.@xterm/headlessand@xterm/addon-unicode11stay dependencies:@maka/runtimeimports them for the PTY stack (packages/runtime/src/pty-stack.ts).Refs #3146
Verification
Measured the archive itself, before and after, by enumerating
node_modulesentries inapp.asarwith their sizes:The 67 that left are the ones the review named —
es-toolkit(2.65 MiB),dompurify(1.63),d3(0.97),lodash-es(0.61),dayjs(0.51) — plus their transitive tail: the whole@types/d3-*set,@formatjs/*,@iconify/*,cose-base,layout-base,robust-predicates,scheduler,stylis,tslib,uuid. Zero packages were added.Nothing renderer-only remains: the only xterm entries left are the two the PTY stack imports.
Safety of the move was checked against compiled output rather than sources —
dist/mainanddist/preloadreference these packages only from__tests__(already excluded from the archive), with one non-test hit that is a comment inworkspace-file-search.jssaying the main process deliberately does not import@maka/ui.Gates:
build,typecheck,lint,format:checkpass. Packaged the signed macOS arm64 app, installed it to/Applications, launched it, and exercised Settings — the renderer renders and the app runs, which is the failure mode that matters when dependencies move out of the production closure.Not run: Windows and Linux packaging.
Review focus
The load-bearing claim is that no main-process or preload code path resolves any of the nine moved packages at runtime. The evidence above is from built output, but a second pair of eyes on that list is worth more than my grep.
AI use
Select exactly one:
Tool(s) and scope: Claude Code — measured the archive, made the dependency move, and ran the verification above. Reviewed and submitted by the contributor of record.
Generated-by: Claude Codeis on the commit.Checklist
Does this PR entail a change in behavior?