Repository navigation
feat(desktop): guard the packaged dependency closure with a reviewed ledger - #5455
Conversation
…ledger electron-builder ships the production dependency closure of package.json whether or not anything loads it. That is how apache#3146 happened: @maka/ui was a production dependency, so mermaid, lucide-react and @astryxdesign/* shipped whole — renderer sources vite had already bundled into dist-renderer. Each author saw only their own slice and nothing reported the step change. The closure is now a reviewed list. artifact-budget.json records the 212 packages that ship; changing it changes that file, so the cost lands in the diff of the pull request that causes it. Putting @maka/ui back into dependencies makes the check name all 113 packages it drags in, mermaid and react among them. It tracks the package set rather than bytes. What reaches app.asar is what survives the `files` globs — on this checkout the closure is 241 MiB on disk and 142 MiB in the archive — and reproducing those globs here would be the same hand-written exclude list that electron-builder.config.mjs already records as unable to hold. The set is exact, and it is the thing that regressed. Resolution is keyed by resolved directory rather than package name: a nested node_modules copy has its own dependencies, and keying by name lets the hoisted copy win and hides that subtree. proxy-agent-negotiate, reachable only through packages/runtime/node_modules/https-proxy-agent, is the case that catches. Verified against a real package: electron-builder --linux --x64 --dir on this checkout ships 212 packages in app.asar and the walk returns the same 212, with no difference in either direction. Runs in the same pure-Node CI stage as the e2e budget rather than in a packaging job, so a regression is reported on the pull request that adds it instead of at release. Generated-by: Claude Code
|
Hi! This is my first PR here, so the CI workflows are waiting for a maintainer to approve them. Could someone approve the run when they have a moment? Happy to address any review feedback. Thanks! |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the six-file change against this commit. The new artifact ledger checks installed production dependencies and wires the check into CI. Two issues below need correction before this guard can serve its purpose. The focused Node tests pass (9/9), but a clean checkout reproduces the check failing with all 212 ledger entries reported missing. The PR currently shows only a successful label check; I cannot verify the full hosted CI or a packaged app.asar from the available results.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| # with nothing reporting the step change (#3146). Pure Node, no build -- | ||
| # it reads package.json. | ||
| - name: Check packaged artifact budget | ||
| run: npm run check:artifact-budget |
There was a problem hiding this comment.
[P1] This unconditional step runs before setup-node (line 202) and npm ci (line 235), but the checker resolves packages from installed node_modules. On a clean checkout with no installed dependencies, I reproduced node apps/desktop/scripts/check-artifact-budget.mjs exiting 1 and reporting all 212 ledger entries as removed; the nine unit tests still pass. A fresh CI runner reaches the same missing-dependency state and fails before installation. Run the guard after npm ci on the relevant lanes, or derive the closure from an installation-independent source.
There was a problem hiding this comment.
Good catch. The step now sits right after Install dependencies in ci.yml, gated on the code selection; I confirmed on a fresh clone that the old ordering exits 1 with all 212 entries reported removed and that it passes after npm ci (ac1c9d0).
| }; | ||
|
|
||
| for (const dependency of Object.keys(manifest.dependencies ?? {})) walk(dependency, rootDirectory); | ||
| return [...new Set(visited.values())].sort(); |
There was a problem hiding this comment.
[P2] Collapsing the visited directories to a set of names loses each resolved package's version and instance count. The new nested-copy test itself resolves two shared packages but reports only one name. A version replacement or another copy with the same name can change packaged bytes while leaving this ledger and check unchanged, contrary to the stated cost guard. Track resolved instances (for example, relative path plus version), or at least version and multiplicity, and cover a same-name replacement in the tests.
There was a problem hiding this comment.
Agreed. collectClosure now returns resolved instances as path@version, artifact-budget.json is regenerated with --write (233 entries), and the tests cover a same-name version replacement and the nested duplicate (ac1c9d0).
… instances The guard resolves packages from installed node_modules, so it moves after the install step. The ledger now records each resolved instance as path@version, so a version replacement or a same-name copy changes it.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed the updated six-file change at this commit. The two issues I reported on the previous commit are addressed: the artifact-budget step now runs after npm ci on the selected code lane, and the ledger records each resolved path@version instance rather than collapsing duplicate package names. The CI planner selects that lane for this workflow change. On Node 24, npm ci completed, the artifact check matched all 233 instances, and the focused artifact tests (10/10) and CI workflow policy tests (46/46) passed. The merge tree against current main and git diff --check are clean. I found no further substantiated P0–P3 issue in the reviewed change.
The hosted CI, dependency audit, and release checks for this head are action_required with no passing check runs, so this review does not establish merge readiness. I did not independently inspect a packaged app.asar or run cross-platform packaging.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
me2seeks
left a comment
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
The issue is real: apps/desktop/electron-builder.config.mjs:70-76 records that the production dependency closure ships wholesale and that a hand-written exclude list already failed, and @maka/ui now sits in devDependencies (apps/desktop/package.json:87), so the outstanding #3146 ask — PR-time visibility of closure changes — is genuinely missing. The ledger mechanism is sound and the CI wiring is correct (the gate fires on any dependency-affecting change via scripts/ci-test-plan.mjs:29-35 and runs after npm ci, ci.yml:227-238). The problem is the layer: the PR hand-rolls a second, divergent implementation of a fact the repo already keeps under an explicit single-definition authority.
Findings
- [P2] apps/desktop/scripts/check-artifact-budget.mjs:54-92 — Duplicated authority for "what ships".
scripts/third-party-closure.mjs:17-22declares itself the single definition of the shipped closure, consumed by the security audit (scripts/audit-shipped-dependencies.mjs:81-89) and the packaged-app verifier (scripts/verify-packaged-app.mjs:893,925-927). The newcollectClosurere-implements npm resolution with different semantics (path@version instances, declared-edge walk-up) versus the existingnpm ls --workspace @maka/desktop --omit=dev --all --json(scripts/third-party-closure.mjs:30-43). Notably electron-builder's actual npm collector runsnpm list -a --include prod --include optional --omit=dev --json(node_modules/app-builder-lib/out/node-module-collector/npmNodeModulesCollector.js:15-17) — closer to the existing authority than to the new walk. If the two ever diverge (peer-installed edges, hoisting edge cases), the PR-time gate and the release-time verifier measure different sets and silently contradict each other. The Occam change: derive the ledger from the existing module, extended to instance granularity if that granularity is wanted. - [P3] apps/desktop/artifact-budget.json — The committed ledger has 233 instance entries, but the body quotes "packages returned by the walk: 212" and "Artifact budget OK — 212 packages". 212 is the unique-name count;
main()printsactual.length(instances), so on this tree it prints 233. The quoted asar comparison was therefore name-level, not instance-level — the "212=212, 0 diff" evidence does not demonstrate the instance-exactness the design claims ("The set is exact"). Correct the numbers. - [P3] apps/desktop/scripts/check-artifact-budget.test.mjs:42-171 — No test drives the enforcement path. All nine tests cover
collectClosure/compare/formatFailure; nothing runsmain()against a stale ledger to assertprocess.exitCode = 1(check-artifact-budget.mjs:142-145). Deleting that one line keeps every test green while CI passes on any closure change — exactly the silent regression this PR exists to catch. - [P3] .github/workflows/ci.yml:236-238 — The gate runs only on ubuntu-24.04. OS-gated
optionalDependenciesinstall on macOS/Windows release builds but not on the Linux runner, so a platform-only step change never touches the ledger, and a--writeledger refreshed on a contributor's Mac would fight Linux CI. app-builder-lib flags this class itself (PKG_OPTIONAL_PLATFORM_NOT_INSTALLED,moduleManager.js). Latent today; at minimum document the platform pinning in the script.
Verdict
needs-changes — the gate works and the premise is verified, but the closure should be read from the existing single authority rather than a parallel walk, and the body's verification numbers must match the committed ledger.
…tion The budget walked node_modules itself, a second definition of "what ships" next to scripts/third-party-closure.mjs, which the security audit and the packaged-app verifier already read. The two had already drifted: the walk followed only declared dependencies, so it missed peer-installed instances that npm ls (and electron-builder's npm collector) include, namely hono@4.13.7 and the undici@7.30.0 nested under @slack/socket-mode. third-party-closure.mjs now also reports the production closure as installed instances, path@version from `npm ls --long`, and the check reads that. The ledger is regenerated: 241 instances of 223 package names. main() takes its argv, ledger path and closure as parameters so the tests drive the enforcement path, including a stale ledger setting exitCode 1. The script and the CI step now say the ledger is the Linux install CI makes, since OS-gated optional dependencies install per platform.
npm ls --long reports a workspace package at its node_modules symlink, which is where the committed ledger lists them; only their nested dependencies sit under packages/<name>/node_modules. Match the fixture and the installedInstances comment to that.
|
The ledger now comes from |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review from ac1c9d09 to exact head 91463560.
The range has three commits:
c8f3b7c2, a merge ofupstream/main;6f5886f6, "read the artifact budget from the shared closure definition";91463560, "record workspace packages at their node_modules link".
Main vs. author: the merge applies cleanly. git show --remerge-diff is empty, so it carries no author edits. The head is now 0 behind and 5 ahead of main 6c63fa05. I compared the old head against its merge-base 063abf42 and the new head against 6c63fa05. The author's net change is 8 files, +611/-4:
ci.ymlstep;- two
package.jsonscripts; artifact-budget.json;check-artifact-budget.mjsand its test;- new:
scripts/third-party-closure.mjsand its test.
Previous findings (hqhq1025's two were already resolved at ac1c9d09; these are me2seeks's on ac1c9d09)
- P2, a duplicated authority for "what ships": fixed.
- The private
collectClosurewalk is gone. scripts/third-party-closure.mjsgainsinstalledInstancesandcollectProductionInstances. These read the samenpm ls --workspace @maka/desktop --omit=dev --all --jsontree, with--longfor paths. The checker imports them.- The PR-time gate and the audit/verifier now share one source.
npmWorkspaceTree's new options argument defaults to the old behavior.audit-shipped-dependenciesandgenerate-third-party-noticestests pass.
- The private
- P3, the body's numbers did not match the ledger: fixed. The body now says 241 instances (223 names) and labels the asar comparison as name-level, from before the move to instances. Locally the check prints "Artifact budget OK — 241 package instances".
- P3, no test drove enforcement: fixed. Four
main()tests now cover:- an added instance, which sets exitCode 1;
- a version change, which sets exitCode 1;
- the clean case;
--write.
- P3, the gate only runs on Linux: addressed by documentation. The script header and the failure text now say that the ledger is the Linux x64 install and must be refreshed on Linux. That is the minimum asked.
Verification (local, exact head):
- After a clean
npm ci,npm run check:artifact-budgetpasses: 7/7 tests and 241/241 instances. Thethird-party-closuretests pass 7/7, andci-test-planplusci-workflow-policypass 86/86. - Moving
@maka/uiback intodependenciesmakes the check fail, listing 131 added instances. I reverted that afterwards. - I regenerated the closure with npm 11, which is what CI's Node 24 uses, and with local npm 12. Both give exactly the committed 241-entry ledger, so the ledger is stable across the npm version CI uses.
- Biome format and lint are clean.
verify-packaged-app.test.mjsfails locally only becausepackages/runtime/distwas not built. That file is untouched by the PR.
New findings: none.
CI / merge state: every workflow on this head (CI, Dependency audit, Release Linux/Windows check, Runtime Host owner platforms) is action_required. They are waiting for maintainer approval to run, so gh pr checks reports no checks and there is no hosted result. The PR is MERGEABLE and up to date with main. Merge is BLOCKED because review is required. No protocol files are touched, so there is no epoch impact.
Verdict: all earlier findings are resolved and I found nothing new. It needs a maintainer to approve the workflow runs so hosted CI can confirm.
|
Thanks @drakeo338 for the contribution, and for working through the review feedback. Merging now. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: no blocking findings in the automated review of this head, and CI is green.
4 participants
Summary
electron-builder ships the production dependency closure of
package.jsonwhether or not anything loads it. That is how #3146 happened:
@maka/uiwas aproduction dependency, so
mermaid,lucide-reactand@astryxdesign/*shipped whole, renderer sources vite had already bundled into
dist-renderer.Each author saw only their own slice, and nothing reported the step change.
The dependency move has since landed, and I confirmed it by packaging: those
packages are no longer in
app.asar. What #3146 also asked for, "add apackaged-size check to CI so a future step change is visible in the PR that
causes it. Worth doing first", is the part still missing, and this is it.
artifact-budget.jsonrecords the 241 package instances (223 package names)that ship on current main. Changing the closure changes that file, so the cost
lands in the diff of the pull request that causes it. Putting
@maka/uibackinto
dependenciesmakes the check name all 131 instances it drags in,mermaidandreactamong them.The closure comes from
scripts/third-party-closure.mjs, the definition thesecurity audit and the packaged-app verifier already read, so this gate and
those release checks measure the same set. It reads the same
npm ls --omit=dev --alltree with--longand keys each entry by installedpath@version: a nested copy is its own entry, so a second copy or a versionchange shows up even when the name already ships.
It tracks the instance set rather than bytes, which is the part worth arguing
with. What reaches
app.asaris whatever survives thefilesglobs (on thischeckout the closure is 241 MiB on disk and 142 MiB in the archive), so a byte
budget measured here would be measuring the wrong number, and reproducing those
globs would be the same hand-written exclude list that
electron-builder.config.mjsalready records as unable to hold.The ledger is the Linux x64 install, because that is where CI runs it. npm
installs OS-gated optional dependencies only on their own platform, so the
ledger should be refreshed on Linux; the script, its failure message and the CI
step say so.
Refs #3146
Verification
Packaged the app (on the original head) and compared against what shipped. This
comparison was by package name, before the ledger moved to instances:
Regression behaviour on current main, adding
@maka/uiback todependencies:Ran locally after a fresh
npm ci, all passing:Not run: the full packaging matrix, or an instance-level comparison against a
fresh
app.asar. My sandbox has no network fornode-gyp, so the measurementabove used
--config.npmRebuild=falseand the Linux x64--dirtarget.AI use
Select exactly one:
Tool(s) and scope: Claude Code wrote the check, its tests and this
description, and ran the packaging and the verification above.
Checklist
Does this PR entail a change in behavior?