Conversation
npm's install-scripts approval gate flags seven packages on a fresh clone: two allowlist entries trail the lockfile (esbuild 0.27.7 vs the pinned 0.28.2, @jackwener/opencli 1.8.4 vs 1.8.7), and five packages with install scripts were never allowlisted at all (electron-winstaller, protobufjs, tree-sitter-javascript, @astryxdesign/cli, @astryxdesign/core). Every entry is now the exact version pinned in package-lock.json, verified against node_modules, and a fresh install completes with no unreviewed-script warnings. Generated-by: GLM-5.3-Flash (ZCode)
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 1e899b5dc088e3cbfdbb9f0854696f27591e6731. All eight allowScripts entries match non-optional packages and exact versions in package-lock.json whose lock entries have hasInstallScript: true. The only other dependency with that flag is the optional, Darwin-only fsevents, which this PR does not approve.
P3 — Do not approve lifecycle scripts that this repository does not need (package.json:130-132,136). The published @astryxdesign/cli@0.6.2 and @astryxdesign/core@0.6.2 postinstall scripts only print setup nudges; protobufjs@7.6.5 only checks the version notation and prints a warning; and @jackwener/opencli@1.8.7 skips its shell-completion postinstall on this repository's local install (and in CI). None produces an install artifact needed by Maka. Setting these entries to true grants four packages install-time code execution solely to remove npm's unreviewed-script warning. npm's install-scripts deny records an explicit false decision and also clears that warning without executing the scripts. Please deny these nonessential scripts and retain approval only where installation/build behavior requires it.
The one-file diff is whitespace-clean and merges cleanly with current main. Visible checks on this head are successful. I inspected the lock entries and the published lifecycle scripts but did not run npm 11's install-scripts gate or a fresh install locally, so this is not an independent runtime confirmation of the author's warning-free install claim. This COMMENTED review is not merge approval.
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.
Review follow-up: the @astryxdesign/cli and @astryxdesign/core postinstalls only print setup nudges, protobufjs only checks its version notation, and @jackwener/opencli skips its shell-completion postinstall on this repository's installs and in CI — none produces an artifact Maka needs. Record an explicit deny (which also clears npm's unreviewed-script warning without executing the scripts) and keep approval only for the scripts installation genuinely requires (esbuild's binary install, tree-sitter-javascript's native build, electron-winstaller's arch selection, node-pty's native build). Generated-by: GLM-5.3-Flash (ZCode)
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head 7bdbbe1e. This follow-up changes only package.json:130-132,136, explicitly denying the four nonessential lifecycle scripts identified in my previous review. The four remaining approved scripts retain exact-version matches to non-optional hasInstallScript entries in package-lock.json; optional Darwin-only fsevents is not approved. I found no remaining P0–P3 issue in this one-file change.
The current-head test, package, package-linux, audit, and platform owner checks succeeded, and the diff is whitespace-clean and merges cleanly with current main. I did not independently run npm 11's install-scripts gate or a fresh install locally (this host has Node 18/npm 9), so the warning-free install claim remains unverified here. This COMMENTED review is not merge approval.
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
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 PR refreshes the root allowScripts map to match package-lock.json for npm ≥ 11.6's install-script approval gate: two stale exact-version keys are corrected (esbuild 0.27.7→0.28.2, @jackwener/opencli 1.8.4→1.8.7) and five previously unlisted script-carrying packages are added. The issue is real — base package.json:129-133 trails the lockfile (package-lock.json:9571, 3291) — and a config-only edit is the right-sized fix. However, the "exactly the 8 allowlisted packages" completeness claim is false per the lockfile itself.
Findings
- [P2] package.json:129-138 —
fsevents@2.3.3is missing from the map.package-lock.json:10178-10185showsfseventswith"hasInstallScript": true,optional: true,os: ["darwin"]. The author's "full node_modules scan" was evidently run on Linux, where this darwin-only optional dependency is never installed, so the ninth script-carrying package was missed. macOS is a first-class platform here (CIowner (macos-latest), Electron desktop app), so a freshnpm installon a Mac will still trip the gate — or silently block fsevents'installscript — exactly what this PR claims to eliminate. Fix: add a deliberate"fsevents@2.3.3": true|falseentry. - [P3] package.json:129 — no guard prevents recurrence. Nothing in CI or scripts cross-checks
allowScriptsagainst the lockfile (the only in-repo references are the staging-strip atscripts/package-macos-arm64-cli.mjs:184and its test; no.githubworkflow mentions it). The PR body itself states drift is why the list broke ("Devs on npm versions without the approval gate never see the warnings"), so without a check in the existingauditjob the map goes stale again on the next esbuild/opencli bump.
Non-issue verified: the @jackwener/opencli true→false flip is safe and arguably more correct — its postinstall only installs shell completions and a global-install adapter sync (node_modules/@jackwener/opencli/scripts/postinstall.js, fetch-adapters.js header: "Only runs on global install"), while the desktop app consumes it purely as a library (apps/desktop/src/main/browser/session.ts:20). Likewise electron-winstaller@5.4.0: true is harmless cross-platform (select-7z-arch.js copies prebuilt vendor/7z-{x64,arm64}.exe, both present).
Verdict
needs-changes — add the missing fsevents@2.3.3 entry so the completeness claim holds on macOS; otherwise the version-refresh itself is verified correct.
fsevents@2.3.3 is a darwin-only optional dev dependency with hasInstallScript: true in the lockfile; a fresh install on macOS trips the approval gate exactly like the eight entries this PR already refreshed. The allowlist was scanned on Linux, where the os: ["darwin"] guard keeps the package out of node_modules - that is how it was missed. Generated-by: GLM-5.3-Flash (ZCode)
|
Addressed the P2 at On the P3 (no guard against recurrence): agreed the map can drift again, but I'd keep this PR config-only and land the cross-check as a follow-up — a small script comparing The |
npm's install-script approval gate keys approvals on exact name@version, so a routine bump silently stales the map and a new optional script- carrying dependency (fsevents) escapes it entirely. The new check-allow-scripts script fails on any script-carrying lockfile entry the map does not key, any map key trailing the lockfile, and any non-boolean decision; the dependency-audit workflow runs it beside the shipped-closure audit so the drift cannot land. Generated-by: GLM-5.3-Flash (ZCode)
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head. The added fsevents@2.3.3 decision matches the optional Darwin-only, install-script-bearing lockfile entry; the four previously unnecessary scripts remain denied. The new guard compares every script-bearing lockfile package with exact allowScripts keys, rejects stale or non-boolean decisions, and runs in the dependency-audit workflow. Its four focused tests and the real-lockfile check pass locally. I found no substantiated P0–P3 issue in these changes. I could not exercise npm 11 installation on macOS, and the current head’s hosted checks were still running at review time. This is not a merge approval.
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.
|
Both review threads are now closed on head
The |
The affected-test lane failed on a single timing-sensitive assertion in
code-mode.test.js ("excludes host waiting from the execution budget and
preserves dependent Promise.race progress", false !== true). The same
test file passes three consecutive local runs (33/33 each) on this
exact tree; nothing in this PR touches the runtime. Tree-identical
retrigger.
Generated-by: GLM-5.3-Flash (ZCode)
|
The test red on |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit a5b07c5. The new tip is an empty CI retrigger after merging main; the effective PR change remains the exact-version allowScripts inventory and its drift guard. On this head, node scripts/check-allow-scripts.mjs reports all nine install-script dependencies covered, and the four guard tests pass. The merge-tree and diff-check against fresh main de4fc5f are clean. I found no new substantiated P0-P3 issue in the changed code.
This is not merge-ready yet: the current-head Windows package job failed while electron-builder's icon-tool process exited with 3221225477 during ICO conversion. I cannot establish from this run whether it is transient or caused by the integrated main/toolchain, so the required gate remains red. The current-head test, audit, package-linux, and Windows/macOS owner jobs passed. I did not run npm 11 installation on macOS or a Windows package build locally.
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.
The Release Windows package job crashed twice at the same point: the electron-builder icons@1.2.3 icon-tool child process died with 0xC0000005 on the Windows runner (the ico output had already been written). main's own package lane passes on the same sources, so this is runner-side, not this PR. Third attempt; if it crashes at the same point again I will file an issue with the three logs. Generated-by: GLM-5.3-Flash (ZCode)
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 58d09e0. This tip is an empty-tree CI retrigger: its tree is byte-for-byte identical to the preceding reviewed head a5b07c5. The effective change is still the exact-version allowScripts inventory plus its lockfile drift guard. The preceding same-tree local guard run covered all nine script-bearing packages and passed 4/4 focused tests; no new code finding is introduced by this tip.
The current-head test, audit, package-linux, Windows package, and Windows/macOS owner jobs are now SUCCESS. Fresh main de4fc5f merge-tree and diff-check are clean. The earlier Windows icon-tool crash did not recur, but a single successful retry does not establish its cause. I did not run macOS npm 11 installation or local Windows 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.
|
Heads-up for reviewer bandwidth: #4865 (open since Sep 5, cross-referenced above) overlaps this PR on both halves — it refreshes the same stale The allowlist-refresh half is duplicated labor — whichever PR merges first settles it. For the guard, #4865's npm-native semantics may be the better long-term replacement for this PR's lockfile cross-check; if the maintainers prefer that, I'm happy to rebase this PR's guard onto the prune-based approach or drop it in favor of #4865's — whichever avoids two competing scripts at the same path. |
Small hygiene fix for npm's install-scripts approval gate (npm ≥ 11.6): on a fresh clone of this repository, the gate flags seven packages whose install scripts are not covered by the
allowScriptsmap inpackage.json.esbuild@0.27.7is allowlisted while the lockfile pins0.28.2, and@jackwener/opencli@1.8.4while the lockfile pins1.8.7. The exact-version keys no longer match what actually installs, so the gate still flags them.electron-winstaller,protobufjs,tree-sitter-javascript,@astryxdesign/cli, and@astryxdesign/coreall carry install/postinstall scripts but had no entry at all.Every allowlist entry is now the exact version pinned in
package-lock.json(verified againstnode_modules), and a freshnpm installcompletes with no unreviewed-script warnings.Verification
npm install-scripts lsafter installnpm installnode_modulesscan for install/postinstall scriptsbiome format package.jsonDevs on npm versions without the approval gate never see the warnings, which is why the list drifted.
AI use
Implemented with GLM-5.3-Flash (ZCode): the lockfile cross-check, the allowlist refresh, and the completeness scan.
package-lock.json