build(desktop): stop shipping the renderer's dependency tree twice - #3148
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 仍有效。
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。 |
|
@Astro-Han @likun666661 — both boundaries are closed in The artifact-boundary mismatch. Fixed exactly as stated: The audit boundary. What the closure sweep found. With coverage now defined as the complete shipped set, two renderer-bundled packages were outside it: @coderabbitai's missing-roots validation is applied in the shared module: every declared root must be present in the full tree, workspace roots included, since Not run locally: Windows packaging, and a fresh macOS package under the widened contract (the closure assertion runs against the real prior asar plus regenerated notices instead; the platform verifiers exercise the full path in CI). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -a -t f \
'third-party-closure\.mjs|generate-third-party-notices\.mjs|verify-packaged-app(\.test)?\.mjs|audit-shipped-dependencies\.mjs|package\.json|dependency-audit\.yml|release-desktop\.yml' \
. | sort
printf '%s\n' '--- script outlines ---'
for file in \
scripts/third-party-closure.mjs \
scripts/generate-third-party-notices.mjs \
scripts/verify-packaged-app.mjs \
scripts/verify-packaged-app.test.mjs \
scripts/audit-shipped-dependencies.mjs
do
if [ -f "$file" ]; then
printf '\n### %s\n' "$file"
ast-grep outline "$file" --view expanded || true
fi
done
printf '%s\n' '--- imports, manifest roots, and workflow invocations ---'
rg -n -C 4 \
-e 'third-party-closure' \
-e 'assertPackagedDependencyClosure\s*\(' \
-e 'rendererBundledDependencies' \
-e 'audit-shipped-dependencies' \
-e 'npm audit --omit=dev' \
scripts apps/desktop/package.json .github 2>/dev/null || true
printf '%s\n' '--- relevant source excerpts ---'
for file in \
scripts/third-party-closure.mjs \
scripts/generate-third-party-notices.mjs \
scripts/verify-packaged-app.mjs \
scripts/verify-packaged-app.test.mjs \
scripts/audit-shipped-dependencies.mjs \
apps/desktop/package.json \
.github/workflows/dependency-audit.yml \
.github/workflows/release-desktop.yml
do
if [ -f "$file" ]; then
printf '\n### %s\n' "$file"
cat -n "$file"
fi
doneLength of output: 93857
The new audit filters npm advisories by the exact package-lock version at each advisory node. This covers shipped renderer dependencies without flagging same-name tooling-only copies. The workflow integration is present in both dependency audit and desktop release workflows. I did not run a local package build in this verification environment. ✏️ Learnings added
You are interacting with an AI system. |
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 变化，不要求截图。
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 才刚开始。
|
@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.
There was a problem hiding this comment.
Thanks for adding the first-party stylesheet scan — that covers the current production imports. One small P3 parser/test blind spot remains: bareCssImportSpecifiers() does not recognize the valid unquoted form @import url(normalize.css), although Vite accepts it. There is no current use of that spelling, so this is non-blocking, but it would be helpful to include it in the existing @import spelling regression coverage.
| // 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.
|
@Astro-Han — all four findings are addressed in P2 — undeclared P2 — the CSS blind spot. You were right that the header overclaimed, and the mechanism you suggested first is not available here: P2 — unresolvable shipped code. Implemented as you described, walking the archive's That made your closing question load-bearing rather than optional, so I acted on it: Two mistakes worth recording, since both were mine and both were caught by running the thing rather than reading it: my first specifier regex used a lazy cross-line match, so a comment containing the words P3 — the closure gate. Now
|
`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
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
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
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
@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
Two manifest corrections found while auditing every declared dependency in the repo against its actual references. `packages/runtime` declared `@modelcontextprotocol/sdk` and imports it nowhere. It went dead in apache#1661, when the legacy client path migrated to SDK v2; the live consumer is `packages/mcp`, which declares it correctly as a devDependency for one test fixture. This does **not** shrink the packaged app, and the removal should not be read as a size win: `@openai/agents-core` declares the same package as an `optionalDependency`, so npm installs it and it stays in the desktop production closure either way. Measured before and after — 262 packages, 219 MB, unchanged. What the removal buys is an accurate manifest: the declaration claimed a direct dependency that no longer exists. `packages/ui` had `react` in `dependencies` and `react-dom` in `devDependencies` while shipped source imports both — `flushSync` in `use-message-selection-quote.ts`, reachable from the package entry through `chat-view.tsx` and emitted into `dist`. Nothing breaks today because `@maka/ui` is itself vite-bundled into the renderer, but the manifest disagreed with itself. Also verified this does not pull anything new into the desktop production closure, since `@maka/ui` is a devDependency there. Audited and deliberately left alone: - The nine renderer roots this PR moves to `devDependencies` — that is the documented policy, enforced by `maka.rendererBundledDependencies`. - `linkedom`, which looks test-shaped but is a real production import in `packages/runtime/src/local-web-fetch.ts`. - `@larksuiteoapi/node-sdk` (28 MB) and `@jackwener/opencli` (17 MB), the two largest entries in the closure. Both are genuinely imported; removing either would drop a feature, which this PR does not do. - `openai` (19 MB) and `@mixmark-io/domino` (9 MB), which no workspace declares — both arrive transitively. - `@types/node` in the production closure, which `@slack/socket-mode` declares as a real dependency. Not ours to fix from here. - Root `@ai-sdk/provider-utils` and `@astryxdesign/core`, which look unreferenced but are a patch target and a resolved peer respectively. No workspace package is unused: `@maka/eval` has no TypeScript importer but ships in the CLI release package with its Python harness. Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
ca17706 to
dd6d7b6
Compare
…hive CI's first job runs `scripts/verify-windows-harness.test.mjs` before `npm ci` — deliberately, per the workflow comment: the planner tests are a "regenerate-and-diff contract that runs on Node alone, so it belongs beside the planner test rather than behind an install." That test imports `verify-packaged-app.mjs`, and this PR gave that module a top-level `import ... from '@electron/asar'`. Importing the module then threw `ERR_MODULE_NOT_FOUND` in a step where no dependency is installed — correct declaration, wrong binding time. `@electron/asar` is CommonJS, so a `createRequire` handle defers resolution to first use without making the three call sites async. Verified by reproducing the CI condition: with `node_modules/@electron/asar` removed, `verify-windows-harness.test.mjs` passes all 32 tests. With it present, the harness, packaged-app and planner suites pass 62/62. Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
Rebased onto The size claim is now measured against
The −8.41 MiB in the previous description was not wrong, but it answered a different question — the denylist head against the closure approach that replaced it. History is cleaner. Thirteen commits became six: eight zero-change Two additions worth naming separately, since they were not in the last review.
The rest of the audit came back clean, including the tempting ones: One self-inflicted CI failure, fixed. My first push after the rebase went red: Local gates: One caveat I would rather state than have found: the install-and-launch check was run on the pre-rebase head and not repeated. The rebase changed packaging inputs — @Astro-Han — three runs are queued on this head awaiting approval, if a committer can release them. |
The Windows lane failed with `"dist/main/app-ipc-main.js" was not found in this archive`. The file is in the archive. `@electron/asar` resolves a lookup by splitting it on `path.sep`, so a `/`-joined archive path descends correctly on macOS and Linux and collapses into one nonexistent name on Windows. A comment in this file asserted the opposite — "asar archive paths always use forward slashes, on Windows too". That is true of how the archive stores paths and false of how the lookup API reads them, and it is why the bug was written down as safe. Both call sites go through `asarLookupPath` now. The second one mattered quietly: its lookup is wrapped in a `try`, so on Windows it did not surface a path bug at all — it reported `app.asar does not carry dist-renderer/bundled-npm-packages.json` about a file that was right there. Tested on any platform rather than only the broken one: the helper is checked against both separators, and a third case mirrors `@electron/asar`'s own descent to assert that the converted path resolves under a Windows separator while the raw one does not. Verified against the real archive before writing the fix — descending `dist/main/app-ipc-main.js` with `\` returns nothing and the converted path returns the file. Also fixes the formatting failure from the previous commit: a stray blank line left by the lazy-import edit. I ran `lint` after that change and not `format`; they are separate gates here. Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
Both failures on the previous head are fixed in The What made it easy to write was a comment already in that file: "asar archive paths always use forward slashes, on Windows too." That is true of how the archive stores paths and false of how the lookup API reads them. Both call sites now go through an Verified by exhausting the real artifact rather than sampling it — every JS file under Three unit tests now cover it, written to fail on any platform: one mirrors The Local pre-flight, following the CI job's own step list. Passing: planner tests (including the pre- I also ran What I could not run locally, stated plainly: the Linux sandbox smoke (needs bubblewrap), @Astro-Han @M4n5ter — three runs are queued on |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for tackling this at the dependency-closure boundary. I reviewed exact head b05576d3ca7e74e21db44aa60c1cb904656b837b. Moving renderer-only dependencies out of Electron’s production closure looks like the right root fix and is much cleaner than maintaining a transitive denylist. The current artifact, audit, and Windows packaging checks are all green. I left one non-blocking P2 supply-chain coverage suggestion and two P3 verifier/test follow-ups below.
AI-assisted review disclosure: Codex performed the primary analysis, and an independent reviewer agent adversarially checked the exact head. I reviewed the evidence and made the final review decision.
…sure Three findings from @Astro-Han's review of `b05576d3c`. All three verified against this head before changing anything. **P2 — the signature gate lost the renderer roots.** `npm audit signatures --omit=dev` skips devDependencies, and this PR moved nine packages there that still ship inside `dist-renderer`. Their registry-signature coverage went with them: an unsigned `react` would have reached users with the gate green. That is a regression this PR introduced, not a pre-existing gap. Auditing the full tree would close it and open a different one — failing releases on tooling that never ships trains the gate to be ignored. The new `audit-shipped-signatures.mjs` audits the full tree and fails only on packages whose exact `name@version` is in the shipped closure, reusing the authority `audit-shipped-dependencies.mjs` already uses for vulnerabilities. Verified by injection: a failure on `react` (shipped) exits 1, the same failure on `vite` (tooling only) does not. **P3 — the verifier compared names, not identities.** `asarNodeModules()` collected names, so an archive carrying `react@18` against a closure declaring `react@19` matched and passed — a name that belongs at a version that does not. It now reads each package's own shipped manifest and compares `{name, version}` against `collectProductionClosure`. Native modules are packaged `unpacked: true`: the header lists them but the bytes live in `app.asar.unpacked`, where `extractFile` cannot reach. Reading the header alone reported `node-pty` and `fs-native-extensions` as version-less and failed a correct artifact — caught by running the verifier against a real package rather than fixtures alone. The bare-import scan also accepted `allowed.has(name)` as proof an import resolves. Only the archive can answer that; a package that is allowed but absent is exactly the ERR_MODULE_NOT_FOUND the check exists to catch. **P3 — the CSS reader missed a spelling Vite accepts.** The regex required quotes, so `@import url(normalize.css)` scanned as nothing. Unquoted is matched only inside `url()`: CSS has no bare `@import pkg`, and accepting one would read the `layer`/`supports` keyword of a quoted import as a package name. The test that claimed to cover "every @import spelling" now does. Both new closure tests were checked in both directions — reverting each fix fails precisely its own case and nothing else. 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 again for the thorough follow-up. The previous exact-version, missing-package, and CSS import findings are fixed on this head.
I left one simplification suggestion inline. Rather than adding another round of parser, schema, path-filter, and fixture fixes, I think the cleanest closeout is to use npm’s built-in full-tree signature verification directly and remove the wrapper.
I do not think the nested-only package-layout scenario should block this PR: the current artifact is correct, and that case requires a later unsupported packaging-layout drift. After the simplification above and green CI, I would be comfortable approving.
AI-assisted review disclosure: OpenAI Codex performed the exact-head analysis with two independent reviewer agents. I verified the reproduced behavior, severity calibration, current artifact state, and the final simplification recommendation before making the review decision.
@Astro-Han is right and the wrapper goes. I built `audit-shipped-signatures.mjs` to avoid failing releases on tooling that never ships, on the theory that noise trains a gate to be ignored. That theory was untested: `npm audit signatures` over the full tree passes today, 880 packages, exit 0. There is no noise to protect against. The argument for the full tree is also stronger than the one I made against it. Signature verification protects everything that *executes* during a build, not only what gets packaged — a tampered vite or biome can pollute the artifact without ever appearing in it. Scoping verification to the shipped closure would have left exactly that class uncovered, which is worse than the problem the wrapper solved. So the built-in stays and the script, its report parsing, and the closure filter it had to keep in sync with all go. What the original review found is still fixed: `--omit=dev` skipped the renderer roots this PR moved to devDependencies while their code still ships in `dist-renderer`. Dropping the flag covers them, and the comment now records both reasons the audit has to be full-tree, so the next person to see a slow signature step does not re-add it. Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
Done in You were right, and for a better reason than the one I would have given. I wrote that script to avoid failing releases on tooling that never ships, on the theory that noise trains a gate to be ignored. I never tested the premise: Your argument is also stronger than mine in the other direction. Signature verification protects everything that executes during a build, not only what gets packaged. A tampered What the original finding surfaced is still fixed: On the nested-only layout case — agreed it should not block, and thank you for calibrating it rather than filing it. If the packaging layout ever drifts that way, Local gates green on this head: lint, format:check, typecheck, |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for simplifying this to the built-in authority. Exact head e7a938bf7 removes the custom signature wrapper and its parallel report parsing, while npm audit signatures now covers both the shipped renderer dependencies and the tooling that executes during the build. The live Dependency audit passed with all 883 audited packages carrying verified registry signatures.
The previous exact-version, missing-package, CSS import, licensing, and artifact-closure findings are addressed. I found no remaining P0–P3 code findings on this head.
AI-assisted review disclosure: OpenAI Codex performed the exact-head analysis and independently verified the final delta, prior findings, current artifact boundaries, audit output, review threads, and live GitHub state. I reviewed the evidence and made the final approval decision.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head e7a938bf755f20147e32043bbcfe5a9726595b41.
The blockers from my earlier REQUEST_CHANGES are addressed: the notice inventory now covers the production and renderer-shipped closure and is validated from the artifact itself; the packaged verifier inspects the real ASAR, nested dependencies, exact package versions, missing runtime imports, and shipped licenses. The final signature gate uses npm’s full-tree authority directly.
I independently ran the focused verifier/closure suite (17/17), both desktop and CLI third-party notice checks, and the shipped-dependency audit (365 packages, zero moderate-or-higher advisories reaching the shipped closure). The live CI, dependency audit, and Windows package checks are green. I found no remaining blocking findings.
AI-assisted review disclosure: Hermes Agent performed the exact-head analysis and local verification under Haoqing Wang’s explicit authorization.
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
Packaged the macOS arm64 app from
mainand from this branch and compared the archives directly.node_modulescontentmain@efe381f4fZero packages were added. The largest entries that stop shipping:
Every one of them is already inside
dist-renderer; the copy that leaves is the unread source tree beside it.An earlier revision of this description reported −8.41 MiB over 67 packages. That number was correct for what it measured — the denylist head of this same PR against the closure approach that replaced it — and it is the wrong baseline for a reader asking what the PR does. The denylist did catch
mermaidby name; what it could not catch was the transitive tail behind it, which is where the remaining 8.41 MiB sat. Measured againstmain, the whole 170 MiB is the PR.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. Independently re-verified after the rebase by enumerating the production closure: all nine moved packages are absent from it, and@xterm/headlessis still present.Gates:
build,typecheck,lint,format:check,check:stale,check:release(44),check:third-party-noticesandcheck:cli-third-party-noticespass. Suites: desktop 1017, ui 183, mcp 115, cli 339, core 576, runtime-host 1039 — all green.@maka/runtimepasses 2994/2999; the five failures arespawn rg ENOENTon my machine, which has no ripgrep binary.The launch check — signed macOS arm64 build installed to
/Applications, launched, Settings exercised — was run on the pre-rebase head. That is the failure mode that matters when dependencies leave the production closure, and it has not been repeated since the rebase: this round's packaging was--dirwithout signing, which is what the size comparison above is measured from. The rebase changed packaging inputs (thefileslist gained!**/test-only/**from main), so the launch check is worth one more run before merge.Not run: Windows and Linux packaging.
Also in this PR
Two manifest corrections from auditing every declared dependency in the repo against its actual references.
packages/runtimedeclared@modelcontextprotocol/sdkand imports it nowhere — dead since #1661 moved the legacy client path to SDK v2. This does not shrink anything, and should not be read as part of the number above:@openai/agents-coredeclares the same package as anoptionalDependency, so it stays in the closure either way (measured before and after — 262 packages, unchanged). What it buys is a manifest that no longer claims a direct dependency that does not exist.packages/uihadreactindependenciesandreact-domindevDependencieswhile shipped source imports both (flushSyncinuse-message-selection-quote.ts, reachable from the package entry and emitted intodist). Verified this pulls nothing new into the desktop production closure, since@maka/uiis a devDependency there.Audited and deliberately left alone:
linkedom(looks test-shaped, is a real production import inlocal-web-fetch.ts);@larksuiteoapi/node-sdk(28 MB) and@jackwener/opencli(17 MB), the two largest closure entries — both genuinely imported, and removing either would drop a feature;openai(19 MB) and@mixmark-io/domino(9 MB), which arrive transitively with no workspace declaring them;@types/nodein the production closure, which@slack/socket-modedeclares as a real dependency. No workspace package is unused —@maka/evalhas no TypeScript importer but ships in the CLI release package with its Python harness.One binding-time fix.
verify-packaged-app.mjsimported@electron/asarat the top level, and CI's planner job runsverify-windows-harness.test.mjsbeforenpm ciby design ("on Node alone, so it belongs beside the planner test rather than behind an install"). Importing the module threwERR_MODULE_NOT_FOUNDthere — the declaration was right, the binding time was wrong. Now loaded throughcreateRequireon first use. Verified by reproducing the condition: withnode_modules/@electron/asarremoved, the harness suite passes 32/32.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 archives, made the dependency move and the manifest corrections, 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?