refactor(release): project manifests from shipped files - #4152
Conversation
d848bdb to
fc9624c
Compare
603824e to
e96352c
Compare
ARE404
left a comment
There was a problem hiding this comment.
Approving refactor(release): project manifests from shipped files at head e96352c3.
I reviewed the 6 changed files and the shared workspaceReleaseManifest / releaseExportTarget logic end to end, and verified the release-scope behavior:
- The pruning correctly removes
src,test-only, dev-directory,.d.ts/.js.map, and*.test(.js)targets from releaseexports, while preserving every runtime branch (import/require/defaultpointing atdist). - Reachability holds: I grepped the shipped (non-test) codebase for imports of
@maka/{mcp,runtime,runtime-host}/test-only/*and other pruned targets; only e2e andtest-onlymodules reference them, so no packaged runtime branch is made unreachable. This matches the stated intent in the PR. - The source manifests are not mutated (projection builds fresh objects/arrays), covered by a test; the
filter-during-copy prunes the same set the old post-copy prune removed. - Tests added for leaf-by-leaf recursion, array targets, Electron staging, and standalone/npm consistency are behavior-accurate.
Two non-blocking [P3] notes are inline: Electron staging hardcodes only three packages for manifest pruning (npm/standalone prune all), and the .d.ts/.js.map pruning intentionally drops types and source maps for shipped dist (recorded as intended).
Note on CI: at review time the test and package checks were still settling on this head (the addon builds and label had passed); I'm approving on the code findings (no P0/P1) as requested, not on a claimed green test.
简体中文
批准 refactor(release): project manifests from shipped files,head e96352c3。
核查了 6 个改动文件与 workspaceReleaseManifest/releaseExportTarget 全链路:release 导出裁剪正确移除 src/test-only/dev 目录/.d.ts/.js.map/.test 目标,保留所有指向 dist 的运行时分支(import/require/default)。
可达性成立:grep 全部非测试代码,只有 e2e 和 test-only 模块引用被裁掉的目标,打包产物没有运行时分支变不可达;源 manifest 不被修改(有测试覆盖);filter 拷贝与旧的拷贝后裁剪等价。
两条 P3 行内:Electron 打包仅硬编码三个包做 manifest 裁剪(npm/standalone 裁剪全部);.d.ts/.js.map 裁剪会去掉发布的类型与 source map(属预期,记录为刻意行为)。
CI 备注:approve 时 test/package 尚未终态(addon 构建与 label 已过);我基于代码结论(无 P0/P1)批准,并非声称 test 已绿。
| const stage = await packager.info.tempDirManager.createTempDir({ | ||
| prefix: 'maka-release-manifests', | ||
| }); | ||
| for (const name of ['mcp', 'runtime', 'runtime-host']) { |
There was a problem hiding this comment.
[P3] The Electron staging path prunes release manifests only for mcp, runtime, and runtime-host. The npm-CLI and standalone paths prune every workspace, so the Electron asar keeps whatever exports each other @maka/* package ships as-is, including any ./test-only/* branches. If the desktop depends on a package other than these three that exposes test-only exports, they will ride into the archive. Consider deriving the full set of shipped @maka package names (e.g. from the desktop's dependencies) instead of a hardcoded triple, so staging stays consistent with the other two packaging paths.
|
|
||
| function releaseExportTarget(target) { | ||
| if (typeof target === 'string') { | ||
| return isMakaDevelopmentArtifact(target) ? undefined : target; |
There was a problem hiding this comment.
[P3] Leaf-level pruning also drops *.d.ts(map) and *.js.map targets from the release exports and strips source maps from copied dist. For CLI/internal packages that don't ship types this is intended, but the reachability consequence is worth an explicit note: production stack traces will no longer be source-mapped, and any consumer expecting types from @maka/* will not find them in the release. I verified no shipped (non-test) module imports a pruned branch (only e2e/test-only code references test-only/*), so this is not a correctness break today — just record the intended surface.
Keep test-only exports in source workspaces while deriving one release manifest shape for npm CLI, standalone CLI, and Electron staging. Project conditional exports leaf by leaf so shipped runtime branches remain importable. Generated-by: Codex
e96352c to
60e03b7
Compare
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head 60e03b76 (verified unchanged at review time; rebased onto current main since the previous push, with the exclusion moved from the beforePack push into the static files list — I re-verified the amended commit).
Single-authority manifest projection, verified rather than assumed:
- One allowlist, three consumers:
workspaceReleaseManifestinrelease-cli-file-policy.mjsreplaces the npm CLI's inline allowlist (byte-identical field set — verified against the deleted code), becomes the standalone staging manifest writer (previously a verbatim copy), and feeds the new DesktopbeforePackstaging for the nested@maka/{mcp,runtime,runtime-host}manifests. No parallel authorities added. - Exports projection is leaf-by-leaf: strings, top-level arrays, and nested conditional objects are all handled; excluded
types/test-onlytargets drop out while shipped runtime/default branches survive, and the array-before-object ordering means a legal top-level fallback array stays an array. The tests pin exactly the five real./test-only/*subpaths across the three shipped packages and prove the source manifests stay byte-identical. - Dropped fields are unreachable in every consumer: I enumerated the staged closure (
@maka/core,runtime,storage,runtime-host,eval,mcp,maka-agent) — the fields the projection drops areprivate,scripts(no lifecycle scripts exist in any of these packages),devDependencies,types,releaseFiles(repo-side policy input, read from the source manifest before projection), andmaka. Themaka.managedRuntimeHostUpdateCompatibilityfield is read at runtime only from npm-tarball deployments (writeReleaseManifestre-adds it and throws if missing), never from the standalone staging path; the CLI's own version read needs onlyversion, which the allowlist keeps. No code reads the nested@maka/*/package.jsonfiles in the Desktop bundle. - Copy-time filter equals post-copy prune: the standalone staging
cpfilter computes the sameisMakaDevelopmentArtifactpredicate on the same relative paths as the deletedpruneMakaDevelopmentArtifactswalk — same result, one less tree walk, and nothing can slip through between copy and prune anymore.
Executed on a real Windows machine at this head: release-cli-file-policy + product-release + desktop-nightly suites, 46/46 pass (including the new pins for conditional-export projection, top-level array preservation, and Desktop manifest staging). CI checks were still running at approval time.
简体中文
单一权威清单投影,全部核实而非假设:workspaceReleaseManifest 一处定义、三个消费方(npm CLI / standalone / Desktop beforePack),allowlist 与被删的内联版本逐字段一致;exports 逐叶投影,字符串/顶层数组/嵌套条件对象都正确处理;被丢的字段(private/scripts/devDependencies/types/releaseFiles/maka)逐一追过消费面,确认三条发布路径上都不可达(maka.managedRuntimeHostUpdateCompatibility 只从 npm tarball 部署读取,writeReleaseManifest 单独保留)。standalone 的 copy 期 filter 与被删的事后 prune 谓词和相对路径完全一致。本机真 Windows 三个套件 46/46 过;批准时 CI 仍在跑。
jackwener
left a comment
There was a problem hiding this comment.
I found no P0–P3 issues on exact head 60e03b76287724760cf3ea73e5b4df2522a73adf.
The five ./test-only/* export keys live only on @maka/mcp, @maka/runtime, and @maka/runtime-host. workspaceReleaseManifest projects each exports target leaf by leaf against the existing Maka artifact filter, so those keys disappear from a shipped manifest while default branches remain. npm CLI staging, standalone macOS CLI staging, and Electron beforePack all call that same function. Source package.json files are not rewritten. Top-level export arrays stay arrays. Electron excludes those three nested manifests in the static files list and overlays the projected copies from a temp dir; it does not push a glob onto electron-builder's FileSet at pack time.
I did not treat the PR body as evidence. The file-policy and product-release tests can show the claim false: the five keys are absent from the projected maps, a mixed types/default object keeps only default, a top-level fallback array does not become a numeric-key object, Electron stages those three nested manifests without ./test-only/, and standalone copy drops the matching files.
I am not merging. This is a release-packaging refactor; merge is a human call. Hosted test was not re-checked after the rebase onto current main. This review does not claim CI is green. PR #3946 overlaps the CLI staging scripts; whichever lands second has to keep this whole-value projection.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
精确 head 60e03b76287724760cf3ea73e5b4df2522a73adf 上我没有发现 P0–P3。
五个 ./test-only/* export 只出现在 @maka/mcp、@maka/runtime、@maka/runtime-host。workspaceReleaseManifest 按现有 Maka 制品过滤规则逐叶投影 exports,这些 key 从发版 manifest 里消失,default 分支留下。npm CLI、独立 macOS CLI、Electron beforePack 都走同一个函数。源仓库的 package.json 不会被改写。顶层 export 数组仍是数组。Electron 在静态 files 里排除那三个嵌套 manifest,再用临时目录里的投影覆盖;打包时不会往 electron-builder 的 FileSet 上推 glob。
我没有把 PR 正文当证据。file-policy 和 product-release 测试能证伪:投影后五个 key 不在;types/default 混在一起时只留 default;顶层 fallback 数组不会变成数字 key 的对象;Electron 给那三个嵌套 manifest 去 ./test-only/;独立拷贝会丢掉对应文件。
我不合入。这是发版打包重构,合入由人类决定。rebase 到当前 main 之后我没有重读 hosted test,这次审查不表示 CI 已绿。#3946 和 CLI staging 脚本重叠;后合的那份必须保住这次的整值投影。
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
Keep test-only exports in source workspaces while deriving one release manifest shape for npm CLI, standalone CLI, and Electron staging. Project conditional exports leaf by leaf so shipped runtime branches remain importable. Generated-by: Codex
Summary
Keep the five
./test-only/*exports in source workspace manifests for tests and E2E, while deriving every shipped workspace manifest from oneworkspaceReleaseManifestauthority tied to the existing Maka release file policy.The same projection feeds npm CLI staging, standalone macOS CLI staging, and Electron's nested
@makamanifests. It recursively projects the completeexportsvalue leaf by leaf, preserving top-level strings/arrays and nested conditional objects/arrays: excluded type/test targets disappear, while shipped runtime/default branches remain importable. Source manifests are never modified.This replaces two old production paths instead of layering beside them: the npm CLI's inline manifest allowlist is deleted, and standalone staging filters development artifacts during copy instead of recursively pruning the copied tree afterward. Electron-builder remains the only ASAR and integrity writer.
The rewrite moves this PR from its initial
+387/-5implementation to+178/-39. The 39 deletions include 36 lines of replaced production code; the net addition is the shared release boundary and contract coverage for all three consumers, including conditional exports.Verification
./test-only/fixtureandscripts.buildbefore the shared manifest projection{}for a root containing excludedtypesand shippeddefaultleavesObject.fromEntriesconverted a legal top-level export fallback array into an invalid numeric-key objectnode --test scripts/release-cli-file-policy.test.mjs scripts/product-release.test.mjs scripts/desktop-nightly.test.mjs(46 passed)npm run buildnpm run format:checknpm run lintnpm run check:stalenpm run release:cli:pack -- --development; inspected and installedmaka-agent-0.2.0-dev-cdff917c56ff.tgz, then successfully executedimport('@maka/eval')npm ci(190 packages), successfully executedimport('@maka/eval'), and confirmed the source Eval manifest remained byte-identicalimport.meta.resolveSigned DMG/ZIP and Windows packaging were not run; the macOS directory build intentionally disabled signing while retaining the production ASAR path.
Adversarial review
Three independent collaboration reviewers examined the shared authority, all three packaging paths, tests, and PR #3946 integration. No P0/P1 remained. One valid exports-shape gap was reproduced with a failing test and fixed by projecting the complete
exportstarget, which also removed the outer object-only projection path. Suggestions to duplicate the five-entry rule in the tarball verifier or reintroduce custom ASAR parsing were rejected because they would create parallel authorities.A follow-up packaging review examined the Windows verifier regression and the final delta. It found no P0-P3 issue: the package-level exclusion now remains in Electron's primary FileSet filter, and the staged manifest overlay is the only matcher appended at
beforePack.Review focus
This branch is based on
origin/mainatafc1a750b. PR #3946 remains open at76d4f20262and overlapsscripts/release-cli-file-policy.mjs,scripts/release-cli-file-policy.test.mjs, andscripts/release-cli-package.mjs. The current three-way merge has content conflicts in the test and package script because both PRs change the CLI staging boundary. Whichever PR lands second must manually preserve whole-value leaf-by-leaf conditional export projection and rerun the release suite; this PR adds no compatibility branch for a future merge.The Desktop ASAR still contains some fixture file bodies under the existing Desktop file set. This PR intentionally changes nested manifest advertising only; Desktop artifact slimming is a separate file-policy decision.
AI use
Select exactly one:
Tool(s) and scope: Codex traced the release ownership boundary, replaced duplicate npm CLI and standalone paths with one shared release-manifest projection, preserved conditional runtime export branches, staged Electron manifests before packaging, and added and ran contract and artifact checks. Collaboration reviewers performed independent adversarial checks.
Checklist
Does this PR entail a change in behavior?