Repository navigation
build(release): install the packed release on its own before publishing, and on pull requests - #349
Conversation
|
| const [head, tail] = target.replace(/^\.\//, "").split("*"); | ||
| if (!files.some((file) => file.startsWith(head) && file.endsWith(tail ?? ""))) { |
There was a problem hiding this comment.
Repeated stars reject valid files
The pattern check rejects targets that repeat *. For "./parts/*": "./dist/*/*.js", the request parts/a points to dist/a/a.js. But split("*") makes tail equal /, so even a packed dist/a/a.js is reported missing. Match the whole target and add a fixture with repeated * substitutions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/pack-installs-alone.mjs
Line: 142-143
Comment:
**Repeated stars reject valid files**
The pattern check rejects targets that repeat `*`. For `"./parts/*": "./dist/*/*.js"`, the request `parts/a` points to `dist/a/a.js`. But `split("*")` makes `tail` equal `/`, so even a packed `dist/a/a.js` is reported missing. Match the whole target and add a fixture with repeated `*` substitutions.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Right, thanks. Fixed in a2e0e6e: the check now matches packed files against the whole target, with the same text at every *, as Node does. A new case with "./parts/*": "./dist/*/*.js" and a packed dist/a/a.js passes.
— Ruth-2424
| if (value && typeof value === "object") { | ||
| for (const [key, inner] of Object.entries(value)) { | ||
| if (key === "default" || conditions.includes(key)) { | ||
| const found = pick(inner, conditions); | ||
| if (found !== undefined) return found; |
There was a problem hiding this comment.
pick() treats a matched null condition as though no condition matched. With { import: null, require: "./legacy.cjs", default: "./index.js" }, it falls through to default and chooses import(). Node then rejects that deliberately blocked import, rather than the check trying the valid require() entry. Keep a blocked target distinct from an unmatched condition and test this case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/pack-installs-alone.mjs
Line: 40-44
Comment:
**Blocked imports get picked**
`pick()` treats a matched `null` condition as though no condition matched. With `{ import: null, require: "./legacy.cjs", default: "./index.js" }`, it falls through to `default` and chooses `import()`. Node then rejects that deliberately blocked import, rather than the check trying the valid `require()` entry. Keep a blocked target distinct from an unmatched condition and test this case.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Right, thanks. Fixed in a2e0e6e: pick() answers null when a matched condition's target is null, which stops the search as Node's does, and a subpath whose import is blocked loads by require() when require serves it. A new case with { import: null, require: "./dist/legacy.cjs", default: "./dist/index.js" } loads by require.
— Ruth-2424
| if (pkg.exports === undefined && (typeof pkg.main === "string" || existsSync(join(installed, "index.js")))) { | ||
| loads.push([pkg.name, "import"]); |
There was a problem hiding this comment.
JSON support misses main entries without exports. That branch always queues plain import(), so a package with main: "./data.json" fails with ERR_IMPORT_ATTRIBUTE_MISSING even when the file is packed correctly. Select import json for a JSON main too, and add a main-only JSON fixture.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/pack-installs-alone.mjs
Line: 158-159
Comment:
**JSON main entries fail**
JSON support misses `main` entries without `exports`. That branch always queues plain `import()`, so a package with `main: "./data.json"` fails with `ERR_IMPORT_ATTRIBUTE_MISSING` even when the file is packed correctly. Select `import json` for a JSON `main` too, and add a main-only JSON fixture.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Right, thanks. Fixed in a2e0e6e: a JSON main with no exports loads with the JSON import attribute, as a JSON exports target already did. A new main-only JSON case passes, and the new test fails with any of the three fixes removed.
— Ruth-2424
…d a JSON main as Node does From Greptile's review of anima-research#349: - An export pattern's target can repeat '*' (./dist/*/*.js), and Node puts the same text in place of each. The check matched packed files against the target's first '*' alone, so dist/a/a.js counted as missing; it now matches the whole target, with the same text at every '*'. - A null target under a matched condition blocks the subpath for those conditions, and stops Node's search. pick() read it as no match and went on to `default`, so a package with { import: null, require: ... } was imported, which Node refuses, instead of required. pick() now answers null there, and a subpath whose import is blocked loads by require() when require serves it. - A JSON main with no exports loads with the JSON import attribute, as a JSON exports target already did. Tests (test/pack-installs-alone.test.ts, 9): a case with a repeated-'*' pattern, a blocked import beside a require, and a JSON main, each passing; it fails with each of the three fixes removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ng, and on pull requests A package that works from a checkout but not from npm reaches everyone who installs it: a file left out of `files`, a dependency only devDependencies supply, or a bin npm's link can't run. scripts/pack-installs-alone.mjs packs the package as `npm publish` does, installs the tarball alone into an empty directory with its dependencies from the registry, and checks what the installed package.json offers: - every file it names is in the package: main, types, each exports target under every condition (the bun and types targets included), at least one packed file for each export pattern, and each bin. Before packing it checks those files exist in the tree, so an unbuilt tree says "build first"; - each entry point loads as its consumers load it, in its own process with a 60 s limit: import() where Node's import conditions pick a target (a JSON target with its import attribute), require() where only require serves it, nothing for a types-only subpath; without exports, the root loads from main or index.js; - each bin starts with a '#!' line, without which npm's link can't run it, and a node bin parses (node --check). Bins aren't run. Where it runs: - publish.yml: a pack-check job with contents: read only, which publish needs. Its install runs dependencies' install scripts, so it stays out of the job that holds id-token: write; publish itself now installs with --ignore-scripts, since neither tsc nor npm publish needs one (esbuild's postinstall is the tree's only one). The GitHub release notes don't wait for it. A dispatch on a branch runs it and publishes nothing. - ci.yml: the same check on pull requests and pushes to main, on ubuntu with Node 24. Its install resolves from the registry as it stands, so a new upstream release can turn it red on a change that touched nothing it installs; rerunning it on main tells the two apart. This repository passes it once agent-framework-mcp has its '#!' line (anima-research#346, which this builds on); without it, the check stops at that bin. Tests (test/pack-installs-alone.test.ts, 8), in throwaway packages packed and installed offline: every kind of entry passing, string and conditional exports, the index.js fallback, a bin-only package, an unbuilt tree, files left out of `files` (a bun target, a types file, a pattern), a devDependencies-only import, a failing subpath, a bin with no '#!' line or that doesn't parse, and a dependency that can't be installed. Each piece of the check, removed on its own, fails the tests aimed at it. Read before opening by Naomi-2422, who ran the publish job's steps: `npm publish --dry-run` after `npm install --ignore-scripts` still runs prepublishOnly's release-hold check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d a JSON main as Node does From Greptile's review of anima-research#349: - An export pattern's target can repeat '*' (./dist/*/*.js), and Node puts the same text in place of each. The check matched packed files against the target's first '*' alone, so dist/a/a.js counted as missing; it now matches the whole target, with the same text at every '*'. - A null target under a matched condition blocks the subpath for those conditions, and stops Node's search. pick() read it as no match and went on to `default`, so a package with { import: null, require: ... } was imported, which Node refuses, instead of required. pick() now answers null there, and a subpath whose import is blocked loads by require() when require serves it. - A JSON main with no exports loads with the JSON import attribute, as a JSON exports target already did. Tests (test/pack-installs-alone.test.ts, 9): a case with a repeated-'*' pattern, a blocked import beside a require, and a JSON main, each passing; it fails with each of the three fixes removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a2e0e6e to
d258695
Compare
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R1 at d258695 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: COMMENT (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.
Reviewed at d258695 against main 91e70ed (merge-base d5e6feb; what main gained since, #344, touches only src/mcpl/** and tests, so nothing here changes on the merge)
Verdict: sound, and worth merging. Three minor gaps are cheap to close, and each fix below has been tried. The job graph, permissions, the --ignore-scripts split and the release-hold interaction are all correct. The check passes this repository today and catches most of the classes it names. Where it falls short of its own prose: it misses a dependency the package imports but only devDependencies lists, whenever another dependency happens to bring that package in, which covers half of this package's dependencies (R1-1). Its parse of npm pack --json crashes under npm 12, now npm's latest (R1-2). And for bins, the bun target and the declaration files, it checks that the files exist, not that they load (R1-3).
This is round 1. There is no earlier review of ours on this PR.
Two questions this review set out to settle:
- Does it catch a broken pack before publish, and can it fail spuriously? For the most part it catches one. I mutated this repository's own
package.jsonand ran the real script. It catches a droppedsrc, afileslist that keeps only the entry files, a dropped set of declaration files, and context-manager, chokidar or discord.js moved todevDependencies. It misses chronicle, membrane or ws moved todevDependencies(R1-1), and the narrower omissions in R1-3. As for spurious failures, it fails on every run under npm 12 (R1-2). Registry drift can turn it red, as the ci.yml comment says, but the existing build job already resolves without a lockfile, so that exposure isn't new. Nothing else turned up: on this tree it passes in about 3 s, and its 9 tests pass in 4.6 s and leave no temp directories behind. - Does it interact correctly with the release-hold gate? Yes.
npm packrunsprepack/prepare/postpackand notprepublishOnly. I checked on npm 10.9.4 and 11.21.1 with aprepublishOnlythat exits 1, and pack succeeded. So the pack-check job never consults the holds and rightly has noissues: read. Inpublish,npm install --ignore-scriptsdoesn't stopnpm publishfrom runningprepublishOnly: a--dry-runpublish after an--ignore-scriptsinstall still ran a failing hook and exited 1. So the hold is still enforced there. The one trap is the obvious follow-up hardening, publishing the checked tarball, which would drop the gate silently (R1-4).
This is the agent-framework counterpart of connectome-host#258, which is still open: same script name, same contents: read job that publish needs, release notes still independent. #258 also runs the host and checks its lockfiles, which a library with gitignored locks doesn't need.
Blockers
None.
Major
None.
Minor
R1-1. A dependency the package imports but only devDependencies lists passes when another dependency brings it in. That is three of this package's six dependencies.
scripts/pack-installs-alone.mjs:128-134 installs the tarball with npm's default hoisting. @animalabs/context-manager@0.14 depends on @animalabs/chronicle and @animalabs/membrane, and discord.js@14 depends on ws, so all three land at the top of node_modules. Node resolves the package's own imports to those hoisted copies.
- Scenario: move
@animalabs/chronicle,@animalabs/membraneorwsfromdependenciestodevDependencies. The check printsinstalls on its own … loaded '@animalabs/agent-framework' (import)and exits 0. (Moving context-manager, chokidar or discord.js is caught.) The release then works for npm and bun consumers only as long as context-manager or discord.js keeps that dependency, at whatever version they ask for. Under a strict layout, such as npm'slinkedor pnpm withhoist=false, it fails outright. - This is the class the PR names first:
publish.yml:57,ci.yml:64andCONTRIBUTING.md:207-208("an import onlydevDependenciessupply, fails the PR"). - Fix (tried): add
--install-strategy=nestedto that install. Withnpm_config_install_strategy=nested, and also withlinked, the chronicle move fails withERR_MODULE_NOT_FOUND: Cannot find package '@animalabs/chronicle', and the unmodified package still passes. Tried on npm 10.9.4; Node 20, the engines floor, ships npm 10. - Test: the existing fixtures have no dependencies, so none can show this. Add a fixture that depends on a second local tarball, which itself depends on the dev-only package.
R1-2. The parse of npm pack --json crashes under npm 12, and any prepack or prepare output breaks it.
scripts/pack-installs-alone.mjs:119 does JSON.parse(out)[0]. Two things break it:
- npm 12: npm 12 prints an object keyed by package name, not an array. With npm 12.2.1 on PATH, the script dies with
TypeError: Cannot read properties of undefined (reading 'filename')at line 124, outside theCheckErrorpath, so it prints a stack trace instead of a message.npm@latestis 12.2.0 today, so a maintainer who has updated npm gets this crash on the run-by-hand path CONTRIBUTING documents. CI is fine for now: Node 24.22 and 26.12 still bundle npm 11. - Lifecycle output: npm runs
prepack/preparein the foreground and their stdout lands ahead of the JSON. I checked on npm 10.9.4, 11.21.1 and 12.2.1: aprepackthat echoes anything makes the parse throw, which the catch reports as "npm pack failed (its output is above)" with nothing above. This package has neither script. But"prepack": "npm run build"is the usual way to make a pack build itself, and with it the check would fail on every run with a wrong message. - Fix (tried): pass
--foreground-scripts=false. On npm 10, 11 and 12 the JSON stays clean,prepackstill runs, and its output is packed. Then accept both shapes:const parsed = JSON.parse(out); const [packed] = Array.isArray(parsed) ? parsed : Object.values(parsed);. Fail with aCheckErrorunless exactly one record with afilenamecomes back. - connectome-host#258's script has the same
[0]parse.
R1-3. For bins, the bun target and the declaration files, the check confirms the files exist, not that they load.
Two claims overstate it: the PR body's "each entry point loads as its consumers load it" and CONTRIBUTING's "loads each entry point" (CONTRIBUTING.md:143). Three consumers go unexercised.
- Bins:
node --check(scripts/pack-installs-alone.mjs:198-199) only parses; it doesn't resolve imports. So a bin that imports something the root doesn't reach can't be checked: a dev-only package, or a filefilesleaves out. Today both bins only import modules the root loads (src/api/mcp-server.ts:11→mcp-server-core.js, exported atsrc/index.ts:20;src/recovery/recover-cli.ts:3→offline-branch.js,src/index.ts:152), so nothing is missed yet. But a bin is how #346 broke unnoticed.- Fix (tried): load the bin's imports without running it, through a load hook (
module.register) that insertsthrow "__linked__";after the#!line. ESM evaluates a module's imports before its own body, so the imports load and the bin's code never runs. Treat that sentinel as success. On the installed tarball, both bins come backlinked, body not run. Withimport "zz-dev-only-pkg";added to the installedmcp-server.js, it fails withCannot find package 'zz-dev-only-pkg'.
- Fix (tried): load the bin's imports without running it, through a load hook (
- The
buntarget, the entry connectome-host actually runs: only its existence is checked.files: ["dist", "src/index.ts"]packs 1025 files, passes, and leaves bun consumers with anindex.tswhose imports are missing. Bun 1.3.10 imports today's installed package fine.- Fix: add
oven-sh/setup-bunto the pack-check jobs. Whenbunis on PATH, also runbun -e 'await import(<spec>)'from the install directory. #258 already usesbun build --target=bunfor the host. Otherwise, say in the prose that the bun target is checked for presence only.
- Fix: add
- Declaration files:
files: ["dist/**/*.js", "dist/src/index.d.ts", "src"]packs 360 files and passes, although every declaration fileindex.d.tsimports is missing. A cheap check: every packeddist/src/**/*.jshas its.d.tsbeside it.
Sol also flagged, and I confirmed against Node's resolver docs: a manifest with "exports": null falls back to main in Node, but here it loads nothing, because line 164 tests === undefined. The require side of a package that also has an import target is never loaded (lines 170-173), and export patterns are matched, not loaded. The last two are stated in the output. All three are edges for this package. Use == null on line 164.
Nits / design notes
R1-4. The tarball that gets checked isn't the one that gets published, and publishing the checked one would drop the release-hold check unless it moves.
pack-check builds after an install with scripts on. publish builds after an --ignore-scripts install, then builds again in prepublishOnly and packs again (publish.yml:129-136). tsc output is deterministic and I found no install script the build needs: in a fresh tree, esbuild (under tsx) is the only package with one. So the risk today is small, and nothing needs to change.
- If you later make
publishpublish exactly what was checked (upload the tarball as an artifact, thennpm publish <tgz>): npm skips the package's own lifecycle scripts for a tarball. A--dry-runpublish of a tarball with a failingprepublishOnlyexits 0. The release-hold check would then have to become an explicit step inpublish, withGH_TOKENandissues: read, or the gate disappears with no error. - Also relevant to that split: nothing exercises
publish's own--ignore-scriptsinstall-and-build path before a tag. It works today: install with--ignore-scripts, then build and run the check, and it passes. A build that came to need an install script would only fail after the tag and its release notes are out. A CI job could run the same install, or a CONTRIBUTING line could note it.
R1-5. The load-failure message names only two causes.
scripts/pack-installs-alone.mjs:184-185 attributes any load failure to a dev-only dependency or a left-out file. The cause the ci.yml comment warns about, a newly released dependency that breaks, gets the same message. Add "or a dependency's new release (rerun on main to tell)".
Verified fine
- Greptile's three findings are fixed at this head:
- repeated
*is matched with a backreference (:147-148; the./parts/*→./dist/*/*.jscase passes); - a
nullreached under an active condition stops the search (:35,:170-173; the{ import: null, require }case loads byrequire); - a JSON
maingets the import attribute (:165).
- repeated
- Real run on this tree (fresh copy; Node 22.21 / npm 10.9.4):
animalabs-agent-framework-0.22.1.tgz (1125 files) installs on its own; … bins start with '#!', and node bins parse: agent-framework-mcp, agent-framework-recover, in 2.8 s. The PR's figure of 1135 files is this plus the 10test/fixtures/*.mjsthatpretestcopies intodist/. - Mutations caught:
srcdropped fromfiles(./src/index.ts (exports['.']) isn't in …);filescut to the entry files (load fails);- every declaration file dropped (
dist/src/index.d.ts (types) … aren't in); - context-manager, chokidar or discord.js moved to
devDependencies(load fails).
- Workflow graph:
publishneeds[build-and-test, pack-check](publish.yml:92);github-releasestill needs onlybuild-and-test(:145);pack-checkruns withcontents: readand no token (:64-66);- a dispatch on a branch runs
pack-checkand skips both releasing jobs (:93,:152), and the build job's hold step skips there too (:38); pack-checktakes about 30 s and runs alongside the ~3.5 min build job, so it doesn't delaypublishor change the hold windows described at:169-172.
--ignore-scriptsinpublish:- the flag applies only to that install;
npm run build(tscpluspostbuildchmod) works after it;- TypeScript 7.0.2 has no install script;
- only esbuild does, and the build doesn't use it.
- Bins' exec bit: npm sets it when it links a bin, so its absence in the tarball rightly isn't a failure.
- Tests: 9/9 pass locally in 4.6 s. They run fully offline with an isolated
HOME, cache and user config. Each asserts the phase-specific message, not just a non-zero exit, and none passes vacuously. They clean up their own and the script's temp directories. - Changelog: CI's changelog check passes (no
src/change, so no fragment is needed).
Method. Two reviewers, merged after re-verifying every claim. (1) Codex gpt-5.6-sol at xhigh, read-only and without network, on a worktree at the head: 15.5 min, 33 commands, about 3.67M input tokens (3.51M cached) and 42k output. (2) Claude Opus 5.5, with network: ran the script and its tests on a fresh copy; mutated this repository's package.json (dependencies moved to devDependencies, files narrowed, a dev-only import added to an installed bin); tested npm 10.9.4, 11.21.1 and 12.2.1 lifecycle behaviour for pack, publish and tarball publish against fixtures; checked bun 1.3.10 against the installed package, the npm dist-tags, and the npm versions bundled with current Node releases. Who caught what: R1-1, R1-4 and R1-5 were Claude's alone. R1-2's npm 12 shape was Sol's, and both found its lifecycle-output half. R1-3's bins were found by both; its bun and declaration-file parts were Claude's, its exports: null, dual-export and pattern notes Sol's. Sol rated R1-3 major; I kept it minor because nothing in today's manifest escapes it. Sol couldn't run installs, packs or tests, or reach the registry; every one of its claims that needed those was re-checked here.
…nstall, npm 12's pack JSON, bins and the bun target loaded Answers Anarchid's round-1 review of anima-research#349 (R1-1 to R1-5). - R1-1: the install is nested (--install-strategy=nested), so the package reaches only its own dependencies. A package it imports but lists only in devDependencies now fails to load even when another dependency brings that package in, as context-manager brings chronicle and membrane, and discord.js brings ws. - R1-2: npm pack runs with --foreground-scripts=false, so a prepack's output can't land ahead of the JSON, and its --json output is read as npm 11's array or npm 12's object keyed by name. Anything else that isn't exactly one record with a filename fails by name, instead of throwing. - R1-3: a node bin's imports are loaded without running the bin: a load hook (module.register) puts a throw after its '#!' line, and ESM loads a module's imports before its body, so the bin throws a sentinel once its imports have loaded and before any of its own code. With bun on PATH, each subpath whose exports names a `bun` condition is imported under bun too, and the pack-check jobs set up bun 1.3.14 for that; without bun, the success line says the bun targets were checked for presence only. Every declaration file the build wrote beside `types` must be in the package. `exports: null` falls back to `main`, as Node does. - R1-4: the publish workflow says, beside its publish step, that it publishes from the tree, and that publishing the checked tarball would skip prepublishOnly's release-hold check. - R1-5: a failed load names a dependency's new release among its causes, with a rerun on main to tell. Tests (test/pack-installs-alone.test.ts, 13): a package imported only through another dependency's copy fails, and passes once listed; a bin whose imports the root never reaches fails to load, and one whose imports load isn't run; a bun target loads under bun, with a stand-in for bun that imports under Node's `bun` condition, and one that doesn't load fails by name; a declaration left out fails; `exports: null` loads `main`; npm 12's JSON shape and a chatty prepack pass. Each piece, removed on its own, fails the test aimed at it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…re names a new release; pack-check builds as publish does From the read of round 1: - A CommonJS bin's requires run with its code, so its imports can't be loaded without running it, and the load hook left it to run in full. The hook now throws in place of loading a bin whose format isn't `module`, before anything is evaluated, and such a bin is only parsed (`node --check`). Node decides the format, by `.cjs` or the package's `type`. The success line lists the bins that start with '#!', the ES module bins whose imports load, and the CommonJS bins parsed. - The bun-load and bin-load failures name a dependency's new release among their causes, as the import failure does. - The test of npm 12's pack JSON passes npm 12's own object through its shim: it assumed an array, so under a real npm 12 it failed. - The pack-check jobs install the tree with --ignore-scripts, as publish does, so a build that came to need a dependency's install script fails on a pull request rather than in publish after the tag. Tests: a CommonJS bin, by `type: "commonjs"` and by `.cjs` in an ES module package, is parsed and its code doesn't run; one that doesn't parse fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An internal review read round 1 (f5267d7). What it found and what changed: - Declarations. The check now takes the form R1-3 of the maintainer's review suggested: in a package with types (`types`, `typings` or a `types` condition), the `.d.ts` the build wrote beside each `.js` it packs must be in the package. The rule it replaces, every declaration under the directory of `types`, took in node_modules for a root-level `types`, ran only from a top-level `types` or `typings`, and required declarations no packed module has beside it, such as a test helper's. - A subpath with both an import target and a different require target loaded only the import; it now loads both, as each kind of consumer does. - The success line listed every bin under "start with '#!'" without naming the ones it didn't load. A bin whose '#!' line names no node is now named as not loaded. The line also says "static imports", and names the package.json fields whose files it checks. - npm pack's record is checked for its files list as well as its filename. The file count comes from that list, and the install path from this tree's package.json, so a record short of either field fails by name. - The hook's ': 0' arm, which no bin reached, is gone, and so are the Array branches of allTargets and hasCondition, which Object.values and Object.entries already cover. pick keeps its own, now tested. - Text: the header says the nested install is stricter than npm's default, that the loads run under the Node that runs the script, that export patterns are matched and not loaded, and that the bun load imports the package's name. The bun failure message no longer says it loaded the bun target. CONTRIBUTING points at the header for the list of checks and says CI installs with npm install rather than npm ci, with bun set up for the pack check. The workflow comments say why bun is 1.3.14 (connectome-host's CI runs this package under it), what the publish job's install leaves to pack-check, and that in publish.yml pack-check runs at the tag. New tests: typings and an exports array; a dual subpath; a bin whose '#!' line names no node; a bin that imports a CommonJS file; a declaration left out beside a packed module whether types, typings or a types condition gives the package types; none required beside a module the package doesn't pack (a root-level types, a test helper's and a dependency's declarations), or in a package without types; and npm pack output that isn't one record with its files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replying to this review. @Anarchid Round 1 is up at R1-1. The tarball now installs with R1-2. R1-3.
R1-4. A comment beside the publish step says it publishes from the tree, and why not the checked tarball: npm skips a package's own lifecycle scripts for a tarball, which would skip the R1-5. All three load-failure messages now name a dependency's new release among their causes, with a rerun on Evidence at
What the internal review changed, in
Written by Ruth-2424, and read by Ogden-2688. — Lottie-2661 |
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R2 at aa7f8ba · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: APPROVE (the formal review event is left to the maintainer). Finding ids R2-n are stable; cite them in fix commits.
Reviewed at aa7f8ba against main e6a0317 (merge-base d5e6feb). What main has gained since touches none of these five files or package.json, and the check passes on the merge too (see Verified fine).
Verdict: APPROVE. All five round-1 findings are fixed, and a test that fails without the fix pins each one. Two minor gaps remain. Each closes with a line or two, and neither blocks the merge. On this repository's own package.json, the check now catches every mutation round 1 found it missing. That holds under npm 10, 11 and 12, and under bun 1.3.14, the version connectome-host's CI runs. Two gaps are left:
- A dependency that asks for a package as a peer still satisfies an import the package lists only in
devDependencies(R2-1). - A child that ignores SIGTERM outlives its 60 s limit and holds the job until GitHub's 6-hour default (R2-2).
Round 1, finding by finding
| Round-1 finding | Disposition | Pinned by | |
|---|---|---|---|
| R1-1 | dev-only import satisfied by another dependency's copy | Fixed: --install-strategy=nested (scripts/pack-installs-alone.mjs:185) |
test/pack-installs-alone.test.ts:253-278 |
| R1-2 | npm pack --json crashes under npm 12; a prepack's output breaks the parse |
Fixed: --foreground-scripts=false (:156), both shapes read, anything else refused by name (:165-176) |
test :327-349 |
| R1-3 | bins, the bun target and .d.ts files only checked to exist |
Fixed: an ES module bin's static imports load through the hook and its body never runs (:43-53, :292-323); a CommonJS bin is parsed only; the bun target is imported under bun (:256-274); the .d.ts beside each packed .js is required (:198-209). The side notes are fixed too: exports: null (:231) and the dual require (:240) |
test :191-251, :280-288, :290-325 |
| R1-4 | the checked tarball isn't the published one; nothing exercised publish's --ignore-scripts build before a tag |
Fixed: the comment at publish.yml:146-148; both pack-check jobs install with --ignore-scripts and build (ci.yml:87, publish.yml:82) |
the pack-check job itself, which ran that way at this head |
| R1-5 | the load-failure message didn't name a dependency's new release | Fixed (:251-253, :270-272, :319-321) |
exact-message asserts at test :181, :204, :287 |
How these were settled:
- This repository, mutated, run through the real script (Node 22.21; npm 10.9.4 unless noted; bun 1.3.14):
@animalabs/chronicle,@animalabs/membraneorwsmoved todevDependencieseach fail withCannot find package ….files: ["dist", "src/index.ts"]fails the bun load withCannot find module './types/index.js'.files: ["dist/**/*.js", "dist/src/index.d.ts", "src"]fails, namingdist/src/agent.d.tsand the declarations beside the other packed modules.import "zz-dev-only-pkg"added to the builtmcp-server.jsfails the bin load, and the server never starts.- A
prepackthat echoes passes under npm 10.9.4 and 12.2.0. The unmodified tree passes under npm 12.2.0.
- The tests, with eight pieces of the script removed one at a time:
- the pieces: the nested flag,
--foreground-scripts=false, the npm 12 read, the hook'sthrow, the bun load, the declaration rule,== null, and the dualrequire; - each removal fails the test aimed at that piece and no other, except the hook's
throw, whose removal fails all three tests that run a node bin.
- the pieces: the nested flag,
Blockers
None.
Major
None.
Minor
R2-1. An import the package lists only in devDependencies still passes when one of its dependencies asks for that package as a peer.
The tarball installs nested (scripts/pack-installs-alone.mjs:185). npm places a dependency's peer beside that dependency, which here is inside the package's own node_modules, so the package resolves the peer too.
- Reproduced: a fixture
@zz/rootimportsmsand lists it only indevDependencies. It depends onzz-cm, whose only tie tomsispeerDependencies: { "ms": "^2.1.3" }. The check printsinstalls on its own … loaded '@zz/root' (import)and exits 0, and the install putsmsatnode_modules/@zz/root/node_modules/ms. npm 10.9.4 and 11.21.1 behave the same. - Why it matters: the header (
:5-7) and the PR body say such an import "fails to load even when another dependency brings that package in", and CONTRIBUTING (:142) says the package "reaches only its own" dependencies. For a peer, neither holds. A release with that mistake keeps working for npm consumers only while that dependency keeps the peer. Any layout that keeps a dependency's peers out of the package's reach breaks it. - Today: nothing escapes. None of this package's six dependencies declares a required peer, and
ws's two peers are optional. - Fix (tried):
--install-strategy=linked.- On npm 10.9.4, 11.21.1 and 12.2.0 the fixture then fails with
Cannot find package 'ms'. - This repository still passes under npm 10 and 11, including the bun load.
- 15 of the 16 tests pass. The 16th is the exact-stderr assert at
test/pack-installs-alone.test.ts:196-199. npm 10, which the Node 20 leg ofci.ymlruns, printsnpm warn reify The "linked" install strategy is EXPERIMENTAL and may contain bugs.; npm 11 and 12 don't. Matching that assert's message, rather than requiring equality, fixes it. - If you'd rather not rely on that strategy, narrow the texts to "as one of its own dependencies", and say that a dependency's peer still satisfies the import.
- Either way, add a fixture with a peer to pin what you choose.
- On npm 10.9.4, 11.21.1 and 12.2.0 the fixture then fails with
R2-2. A child that handles SIGTERM outlives its limit, and the job then runs until GitHub's 6-hour default.
run() (:133-138) and the bin load (:299-304) pass timeout to execFileSync/spawnSync with the default killSignal, SIGTERM. Node's docs for both say that when the child handles SIGTERM and doesn't exit, the parent waits until it does. npm pack (:156-160) has no limit, and neither pack-check job sets timeout-minutes (ci.yml:70, publish.yml:63).
- Reproduced, with the 60 s limit cut to 3 s in a copy of the script: a
mainmodule that doesprocess.on("SIGTERM", () => {})and then waits forever is still running at 40 s, when an outertimeoutstops it. Its child is orphaned, and the temp directory stays behind becausefinallynever ran. Without the handler, the same module fails at 3 s withit didn't finish within 3 s. - Scenario: a dependency's new release opens a connection at import and installs a graceful-shutdown handler. On a release tag,
pack-checkthen hangs for six hours beforepublishis skipped.github-releasedoesn't wait for it, so the release notes are out by then. - Fix (tried): set
killSignal: "SIGKILL"inrun()and in the bin'sspawnSync. With that, the same fixture fails at 3 s, with no orphan and the temp directory removed. Also givenpm packa timeout, and addtimeout-minutes: 15to both pack-check jobs, which take about 25 s today.
Nits
R2-3. module.register() is runtime-deprecated in Node 26 and doesn't exist before Node 20.6. The bin hook is installed with it (:298).
- Node 26.12.0 prints
[DEP0205] DeprecationWarning: module.register() is deprecated. Use module.registerHooks() instead.The check still works there: the test file passes 16 of 16, and so does this repository. But Node's docs say the API will be removed, and when it is, every node bin will fail as "doesn't load". enginesallows Node 20.0, andregisterarrived in 20.6, so 20.0 to 20.5 fail the same way today.- Fix (tried): use
registerHookswhen it's there (Node 22.15+), and keepregisteras the fallback. The preload becomesif (m.registerHooks) m.registerHooks({ load: (await import(url)).load }); else m.register(url);. The hook'sloadcallsnextLoadand patches its result directly, or through.then()when it returns a promise. With that change, the test file passes 16 of 16 on Node 22.21 and 26.12 with no warning, and this repository passes on 26.12. Alternatively, state in the header that the script needs Node 20.6 or later.
R2-4. Where pick() departs from Node, an entry point Node loads is left unloaded without a word.
- Inside an exports array, Node skips a
nulland tries the next target.pick()stops at thenull(:65-69). - Its condition lists (
:237-238) also leave outmodule-sync, which Node 22.12+ and 24 match. - Reproduced: with
"exports": [null, "./dist/index.js"]and a dev-only import inindex.js, Node's ownimport()fails withCannot find package 'zz-dev-only'. The check passes, and its success line has noloadedclause. - Neither shape is in this package, and neither is common.
- Fix: skip a
nullinside an array, and fail, or at least say so, when a non-pattern subpath names a runtime file that neither load reaches.
R2-5. The test's dependency packs aren't isolated like the rest. The header says npm runs "with its own cache and user config" (test :1-4). The pack() helper for the nested-install test (:258-263) spreads process.env instead, so it uses the developer's own HOME, cache and .npmrc, and writes npm's debug logs into their cache. Building its env the way check() does (:51-58) fixes it.
R2-6. Three passages of text.
publish.yml:62says "It runs on a dispatch too, which publishes nothing." A dispatch on av*tag does publish, becausepublish'sif:tests onlygithub.ref. CONTRIBUTING (:146) rightly says "on a branch"; say that here too.ci.yml:38-39says "plain npm install (matching publish.yml)".publish.yml'spublishjob now installs with--ignore-scripts, so only its build job's install matches this one.- In
CONTRIBUTING.md:138-148, the second sentence of the Releases bullet now runs to about 100 words, around a parenthesis that holds a semicolon. Giving the pack check sentences of its own would make it readable again.
Verified fine
- CI at this head: all six checks are green, including "Packed release installs on its own" in 23 s.
- This tree:
- The check passes in 3.5 s (1125 files) on Node 22.21 with npm 10.9.4, 11.21.1 and 12.2.0, and on Node 26.12.0.
- The test file passes 16 of 16 on Node 22.21, in 12.8 s, and on Node 26.12.0, and it leaves no temp directories behind.
- The hook:
- It touches only the bin's own URL; the bin's
.cjsimport loads unchanged (test:216-226). - Static imports, and errors linking them, resolve before the inserted
throw. - CRLF line endings and a missing final newline are harmless.
- A BOM before
#!fails the#!check, which is right, since the kernel wouldn't run that file either. - Neither of this package's bins makes a dynamic
import(), so "static imports" covers everything they load.
- It touches only the bin's own URL; the bin's
- Release holds: as in round 1,
npm packdoesn't runprepublishOnly, andpublish, which publishes from the tree, still does. - Workflow graph and permissions:
publishneeds both jobs, andgithub-releaseneeds only build-and-test.- pack-check runs with
contents: readand doesn't persist credentials. setup-bun@0c5077e5…is thev2.2.0release commit, and connectome-host's CI pins the same commit and bun 1.3.14.
- Pattern matching: literal parts are regex-escaped.
(.+)crosses/as Node's*does, and a repeated*uses a backreference. - The declaration rule: it looks in the build tree for the
.d.tsbeside each packed.js. So it asks for nothing beside the.mjsfixturespretestcopies, or beside a file the build didn't emit. It raises no false failure on this repository, whose tsconfig emits.jsand.d.tsside by side.
Method. Two reviewers, merged after re-verifying every claim.
(1) Codex gpt-5.6-sol at xhigh, read-only and without network, on a worktree at the head: 9.4 min, 56 commands, about 2.40M input tokens (2.30M cached) and 38k output. Its sandbox couldn't create temp directories, so it ran none of the tests.
(2) Claude Opus 5.5, with network: installed and built the head and its merge with main, then ran the script and its tests on both; mutated this repository's package.json and a built bin as described above; removed eight pieces of the script and ran the test file against each; probed where nested and linked installs place a peer, how a 3 s limit handles a SIGTERM handler, an exports array with null, and Node 26's module.register() warning; read the CI job log.
Who caught what: R2-1 and the round-1 mutation runs were Claude's. R2-2 and R2-4 were Sol's, reproduced here. In R2-3, the Node 20.0–20.5 gap was Sol's and the Node 26 deprecation Claude's. Both found R2-5. In R2-6, the first point was Sol's and the other two Claude's.
Where I differed from Sol: Sol rated R2-2 and R2-4 major and called R1-3 only partly fixed. I kept R2-2 minor because it needs a dependency that traps SIGTERM, and made R2-4 a nit because neither shape is in use. I count R1-3 fixed. Sol's Node 20.0 point is R2-3. Its other two cases were a bin that an export imports and a bin that re-imports itself under a query string. A package whose export imports its own bin runs that bin for every consumer, so the check running it too is correct, and the query-string re-import is contrived.
…-research#349 Answers R2-1 to R2-6 of the maintainer's second review of anima-research#349. - R2-1: the tarball installs linked (--install-strategy=linked), so each package reaches only the dependencies it declares. Nested, a dependency's peer was installed beside it, inside the package's own node_modules, and satisfied an import the package lists only in devDependencies. A package's own peer still loads, which a test pins. From 11.18.0 to 12.2.1, no npm release marks linked experimental, and npm recommends it for catching undeclared dependencies; npm 10 and 11.17 still warn, so the no-'#!' test matches its message rather than the whole of stderr. The header and CONTRIBUTING say "linked" and what it shows. - R2-2: a child that outlives its limit is killed (SIGKILL): npm pack, npm install and every load, since one that handles SIGTERM kept running past its limit and held the job. What a child started itself isn't killed with it, and in CI the job's timeout-minutes ends that. npm pack gets the install's limit, and both pack-check jobs get timeout-minutes: 40, above the check's own limits, so the check names what hung. PACK_INSTALLS_ALONE_INSTALL_TIMEOUT_MS and PACK_INSTALLS_ALONE_LOAD_TIMEOUT_MS set the limits, which the tests shorten; a value that isn't a positive whole number of milliseconds fails by name. - R2-3: the bin load hook is installed with module.registerHooks where Node has it, since Node 26 deprecates module.register, which stays the fallback; the hook answers registerHooks' nextLoad at once and register's through its promise. The header says the check needs Node 20.10 or later: 20.6 for the hook, and 20.10 for the import attribute a JSON target's load takes. - R2-4: pick() follows Node's rule for an exports array: a null inside one gives way to the next item, and an array of nulls or an empty one blocks its condition. The condition lists also hold node-addons, which Node matches unless run with --no-addons, and module-sync, which a Node that can require ES modules matches. With those, the lists hold every condition Node matches by default, which the loads use, so a target neither load picks is for another runtime, a bundler or a --conditions flag. The check doesn't load it, and still checks that it's in the package. - R2-5: the nested-install test packs its local dependencies with npm offline and its own home, cache and user config, as check() runs. - R2-6: publish.yml's comment says a dispatch on a branch publishes nothing, ci.yml's says its plain npm install matches publish.yml's build-and-test job, and CONTRIBUTING's Releases bullet gives the pack check sentences of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-research#349 Answers R2-1 to R2-6 of the maintainer's second review of anima-research#349. - R2-1: the tarball installs linked (--install-strategy=linked), so each package reaches only the dependencies it declares. Nested, a dependency's peer was installed beside it, inside the package's own node_modules, and satisfied an import the package lists only in devDependencies. A package's own peer still loads, which a test pins. From 11.18.0 to 12.2.1, no npm release marks linked experimental, and npm recommends it for catching undeclared dependencies; npm 10 and 11.17 still warn, so the no-'#!' test matches its message rather than the whole of stderr. The header and CONTRIBUTING say "linked" and what it shows. - R2-2: a child that outlives its limit is killed (SIGKILL): npm pack, npm install and every load, since one that handles SIGTERM kept running past its limit and held the job. What a child started itself isn't killed with it, and in CI the job's timeout-minutes ends that. npm pack gets the install's limit, and both pack-check jobs get timeout-minutes: 40, above the check's own limits, so the check names what hung. PACK_INSTALLS_ALONE_INSTALL_TIMEOUT_MS and PACK_INSTALLS_ALONE_LOAD_TIMEOUT_MS set the limits, which the tests shorten; a value that isn't a positive whole number of milliseconds fails by name. - R2-3: the bin load hook is installed with module.registerHooks where Node has it, since Node 26 deprecates module.register, which stays the fallback; the hook answers registerHooks' nextLoad at once and register's through its promise. A test wraps both, to show which one the bin load calls, since Node prints register's warning only after a main module has run, which the bin load never does. The header says the check needs Node 20.10 or later: 20.6 for the hook, and 20.10 for the import attribute a JSON target's load takes. - R2-4: pick() follows Node's rule for an exports array: a null inside one gives way to the next item, and an array of nulls or an empty one blocks its condition. The condition lists also hold node-addons, which Node matches unless run with --no-addons, and module-sync, which a Node that can require ES modules matches. With those, the lists hold every condition Node matches by default, which the loads use, so a target neither load picks is for another runtime, a bundler or a --conditions flag. The check doesn't load it, and still checks that it's in the package. - R2-5: the nested-install test packs its local dependencies with npm offline and its own home, cache and user config, as check() runs. - R2-6: publish.yml's comment says a dispatch on a branch publishes nothing, ci.yml's says its plain npm install matches publish.yml's build-and-test job, and CONTRIBUTING's Releases bullet gives the pack check sentences of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-research#349 Answers R2-1 to R2-6 of the maintainer's second review of anima-research#349. - R2-1: the tarball installs linked (--install-strategy=linked), so each package reaches only the dependencies it declares. Nested, a dependency's peer was installed beside it, inside the package's own node_modules, and satisfied an import the package lists only in devDependencies. A package's own peer still loads, which a test pins. From 11.18.0 to 12.2.1, no npm release marks linked experimental, and npm recommends it for catching undeclared dependencies; npm 10 and 11.17 still warn, so the no-'#!' test matches its message rather than the whole of stderr. The header and CONTRIBUTING say "linked" and what it shows. - R2-2: a child that outlives its limit is killed (SIGKILL): npm pack, npm install and every load, since one that handles SIGTERM kept running past its limit and held the job. What a child started itself isn't killed with it, and in CI the job's timeout-minutes ends that. npm pack gets the install's limit, and both pack-check jobs get timeout-minutes: 40, above the check's own limits, so the check names what hung. PACK_INSTALLS_ALONE_INSTALL_TIMEOUT_MS and PACK_INSTALLS_ALONE_LOAD_TIMEOUT_MS set the limits, which the tests shorten; a value that isn't a positive whole number of milliseconds fails by name. - R2-3: the bin load hook is installed with module.registerHooks where Node has it, since Node 26 deprecates module.register, which stays the fallback; the hook answers registerHooks' nextLoad at once and register's through its promise. A test wraps both, to show which one the bin load calls, since Node prints register's warning only after a main module has run, which the bin load never does. The header says the check needs Node 20.10 or later: 20.6 for the hook, and 20.10 for the import attribute a JSON target's load takes. - R2-4: pick() follows Node's rule for an exports array: a null inside one gives way to the next item, and an array of nulls or an empty one blocks its condition. The condition lists also hold node-addons, which Node matches unless run with --no-addons, and module-sync, which a Node that can require ES modules matches. With those, the lists hold every condition Node matches by default, which the loads use, so a target neither load picks is for another runtime, a bundler or a --conditions flag. The check doesn't load it, and still checks that it's in the package. - R2-5: the nested-install test packs its local dependencies with npm offline and its own home, cache and user config, as check() runs. - R2-6: publish.yml's comment says a dispatch on a branch publishes nothing, ci.yml's says its plain npm install matches publish.yml's build-and-test job, and CONTRIBUTING's Releases bullet gives the pack check sentences of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-research#349 Answers R2-1 to R2-6 of the maintainer's second review of anima-research#349. - R2-1: the tarball installs linked (--install-strategy=linked), so each package reaches only the dependencies it declares. Nested, a dependency's peer was installed beside it, inside the package's own node_modules, and satisfied an import the package lists only in devDependencies. A package's own peer still loads, which a test pins. From 11.18.0 to 12.2.1, no npm release marks linked experimental, and npm recommends it for catching undeclared dependencies; npm 10 and 11.17 still warn, so the no-'#!' test matches its message rather than the whole of stderr. The header and CONTRIBUTING say "linked" and what it shows. - R2-2: a child that outlives its limit is killed (SIGKILL): npm pack, npm install and every load, since one that handles SIGTERM kept running past its limit and held the job. What a child started itself isn't killed with it, and in CI the job's timeout-minutes ends that. npm pack gets the install's limit, and both pack-check jobs get timeout-minutes: 40, above the check's own limits, so the check names what hung. PACK_INSTALLS_ALONE_INSTALL_TIMEOUT_MS and PACK_INSTALLS_ALONE_LOAD_TIMEOUT_MS set the limits, which the tests shorten; a value that isn't a positive whole number of milliseconds fails by name. - R2-3: the bin load hook is installed with module.registerHooks where Node has it, since Node 26 deprecates module.register, which stays the fallback; the hook answers registerHooks' nextLoad at once and register's through its promise. A test wraps both, to show which one the bin load calls, since the check shows a bin load's output only when the load fails. The header says the check needs Node 20.10 or later: 20.6 for the hook, and 20.10 for the import attribute a JSON target's load takes. - R2-4: pick() follows Node's rule for an exports array: a null inside one gives way to the next item, and an array of nulls or an empty one blocks its condition. The condition lists also hold node-addons, which Node matches unless run with --no-addons, and module-sync, which a Node that can require ES modules matches. With those, the lists hold every condition Node matches by default, which the loads use, so a target neither load picks is for another runtime, a bundler or a --conditions flag. The check doesn't load it, and still checks that it's in the package. - R2-5: the nested-install test packs its local dependencies with npm offline and its own home, cache and user config, as check() runs. - R2-6: publish.yml's comment says a dispatch on a branch publishes nothing, ci.yml's says its plain npm install matches publish.yml's build-and-test job, and CONTRIBUTING's Releases bullet gives the pack check sentences of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replying to this review. @Anarchid #438, opened today, answers the six findings of your second review. Its description maps the changes, choices, and checks to those findings. Three choices to call out:
— Fiona-2757 |
tsconfig compiles test/** into dist/test/** (rootDir '.'), and `files: ["dist", "src"]` packed all of it: the published 0.22.1 has 576 of its 1065 files under dist/test, and a tree packed after `npm test` also carried the dist/test/fixtures that pretest copies in, so the package's contents depended on whether the tests ran first. Nothing consumes dist/test: main, types, every exports target and both bins sit under dist/src, and the bun target under src. `files` is now ["dist/src", "src"]. On main at 91e70ed, packed after a build and `npm run pretest`, which copies the test fixtures into dist/test, the tarball went from 1139 files (13.9 MB unpacked) to 508 (7.7 MB). The 631 files dropped are dist/test/** and dist/tsconfig.tsbuildinfo, and none is added. #349's packed-install check (scripts/pack-installs-alone.mjs on that branch), run on the narrowed package, passes: every named file is in it, the root loads under Node and under bun, and both bins' imports load. The pending TypeScript 7 fragment said the package ships dist/tsconfig.tsbuildinfo and the test build in dist/test, and its source-map counts took in dist/test's maps. It no longer says either, and its counts are dist/src's alone, measured by building this commit's source with TypeScript 7.0.2 and with 5.9.3: of dist/src's 101 modules, 42 .js.map files differ (38 in columns only, 2 mapping a line to other source lines, 2 gaining or losing a line's mapping) and 54 .d.ts.map files (48 mapping a line to other source lines, 6 gaining or losing one). dist/test's 72 and 2, which no longer ship, are out of them. The redundant `discord.js` entry in devDependencies is gone, since discord.js is a dependency at the same range. !Fixes #397 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
build(release): the pack check after the maintainer's review of #349
The release is packed, installed on its own and loaded before it publishes, and on every pull request. A file missing from
files, a dependency onlydevDependenciessupplies, or a bin npm's link can't run then fails a check, not someone's install.@antra-tess @Anarchid, this adds a job to how a release publishes, and one to CI, so please look before it merges. It follows #346, now merged, which gives the
agent-framework-mcpbin the#!line this check requires. Without that fix the check would have failed on every release so far. It also follows #348, now merged, whose release bullets it rebases over. Round 1 answers Anarchid's review ofd2586958(R1-1 to R1-5); the comment that goes with it says what changed for each finding.Why
A package that works from a checkout but not from npm reaches everyone who installs it.
maindepended onfile:../voice-kituntil chore(deps): take @animalabs/voice-kit from npm (^0.1.0) discord-mcpl#83, which no install from npm could resolve.agent-framework-mcphad never run as an installed command (fix(mcp): agent-framework-mcp runs as an installed command #346).The change
scripts/pack-installs-alone.mjspacks the package asnpm publishpacks it. It installs the tarball into an empty directory with its dependencies from the registry, nested (--install-strategy=nested), which is stricter than npm's default: the package reaches only its own dependencies. Then it checks what the installedpackage.json, the one a consumer reads, offers, under the Node that runs it:Every file its
main,types(ortypings),exportsandbinname is in the package: eachexportstarget under every condition (including thebuntarget connectome-host loads and thetypestarget, which noimport()reads), at least one packed file for each export pattern, and eachbin. In a package with types (types,typingsor atypescondition), so is the.d.tsthe build wrote beside each.jsit packs, which is what a consumer's TypeScript reads. Before packing, it checks that the filespackage.jsonnames exist in the tree, so an unbuilt tree says "build first" rather than looking like a packaging fault.Each entry point loads, each in its own process, with a 60 s limit:
import()when Node's import conditions pick a target, with its import attribute for a JSON target;require()whenrequirepicks another file, as for a dual package's CommonJS build, or when onlyrequireserves the entry;exports, or withexports: null, the root loads frommain, or fromindex.jsas Node falls back to it;bunon PATH, each subpath whoseexportsnames abuncondition is also imported under bun, by its name, as a bun consumer imports it. Without bun, the success line says those targets were checked for presence only.Because the install is nested, an import that only
devDependencieslists fails to load even when another dependency brings that package in, as context-manager brings chronicle and membrane, and discord.js brings ws.Each bin starts with a
#!line, without which npm's link can't run it as a command. A bin whose#!names node has its static imports loaded without being run: a load hook (module.register) makes an ES module bin throw once its imports have loaded, before any of its own code, which for the MCP server would start it. A CommonJS bin'srequires run with its code, so the hook stops it before it loads, and it's only parsed (node --check). A bin whose#!line names no node is checked for that line alone, and the success line names it as not loaded.npm pack's JSON is read in npm 11's shape (an array) and npm 12's (an object keyed by package name), with lifecycle scripts run in the background so aprepack's output can't land ahead of the JSON. Output that isn't one record with a filename and its files fails by name.Each failure says what it found, and where a failure has a usual cause, its message names it, a dependency's new release among them (rerunning on
maintells the two apart). npm install has a 10-minute limit.Where it runs:
pack-checkjob of its own, withcontents: readonly, whichpublishneeds.publishholdsid-token: write, so the two stay apart.publishitself now installs with--ignore-scripts. Neither its build (tsc) nornpm publishneeds a dependency's install script. On the Linux runner the only one in the tree is esbuild's postinstall, since fsevents, the other dependency with one, installs only on macOS. Before this,publishran every dependency's install scripts while holding the id-token.publishpublishes from the tree, not the tarball the check installed: npm skips a package's own lifecycle scripts for a tarball, which would skipprepublishOnly's release-hold check. A comment beside the step says so.pack-checkjob on pull requests and pushes tomain, on ubuntu with Node 24. This goes beyond the publish-time step first proposed: it finds before merge what would otherwise fail at publish, after the tag and its release notes are out.publishdoes, with--ignore-scripts, so a build that came to need a dependency's install script fails there, on a pull request, rather than inpublishafter the tag. Both set up bun 1.3.14, the version connectome-host's CI runs this package under, pinned by commit, so the bun load runs.CONTRIBUTING: the Releases bullet names the check, points at the script's header for what it checks, and says how to run it by hand (
npm run buildfirst). "Building and testing" names the CI job, with bun set up, and says CI installs withnpm installrather thannpm ci.Tests
test/pack-installs-alone.test.ts(16) runs the real script in throwaway packages, packed and installed offline with their own npm cache; their dependencies, where a case needs any, are local tarballs. A stand-in for bun, which imports under Node'sbuncondition, takes bun's place where a case loads abuntarget.mainalone andexports: nulleach load as the root;*, an import blocked bynullbeside arequire, and a JSONmain;index.jsfallback, and a bin-only package;prepackthat prints.mainnaming no file.files: abuntarget, atypesortypingsfile, a pattern, and the declaration beside a packed module, whethertypes,typingsor atypescondition gives the package types. No declaration is required beside a module the package doesn't pack, such as a test helper's or a dependency's, or in a package without types.buntarget that doesn't load.#!line; an ES module bin that doesn't parse, one whose import onlydevDependencieslists, and one whose imports, a CommonJS file among them, load, never run; a CommonJS bin, bytypeand by.cjs, parsed and never run, and one that doesn't parse.npm packoutput that isn't one record with a filename and its files.Evidence
At
aa7f8ba7, on Linux:test/pack-installs-alone.test.tspasses 16 of 16 on Node 24.22.0 with npm 11.19.1, the same with npm 12.2.0, and on Node 20.20.2.npm install --ignore-scriptsandnpm run build, which both exit 0, and thennpm run pretestfor the tests: the check reportsanimalabs-agent-framework-0.22.1.tgz (1135 files) installs on its own; every file package.json's main, types, exports and bin name is in it; loaded '@animalabs/agent-framework' (import); loaded under bun: '@animalabs/agent-framework'; bins start with '#!': agent-framework-mcp, agent-framework-recover; ES module bins' static imports load: agent-framework-mcp, agent-framework-recover., with bun 1.3.14 and with bun 1.4.3. The 1135 files include the 10 test fixtures thatpretestcopies intodist. It passes with npm 12.2.0 too, and on Node 20.20.2 without bun it reports the bun target present but not loaded.filesset to["dist/**/*.js", "dist/src/index.d.ts", "src"], the check fails, namingdist/src/agent.d.tsand 253 other declarations beside packed modules.exportslistsimportbeforebun, bun 1.3.14 and 1.4.3 both load theimporttarget by the package's name, and thebuntarget whenbuncomes first.npm pack's JSON read in npm 12's shape; its lifecycle scripts kept out of the foreground; a pack record without its files refused.exports: nullread as absent; an exports array read for the root; therequire()load beside an import, and none of the import's own file.#!names no node named as not loaded.typingsamong the named files; the declaration beside each packed module required, counted as present when it's packed, and reported once when the package also names it; atypescondition andtypingseach giving the package types; a package without types left alone.npm testran atf5267d7b, before the round's last commit, which changes only the pack check, its test, the workflow comments and CONTRIBUTING. Each test was bounded at 300 s, the run went at a load average near 100, and it reported 2349 of 2355 tests passing. The six failures are inmcpl-coalescing,tool-result-guardandworkspace-watcher, which this change doesn't touch. Each failed under that load, and run alone three times each at the same commit, those files passed, except onetool-result-guardtest, "fix(guard): a withheld tool result says where its original is (#277) #281: batches behind one stalled write cost one deadline", which failed once at a load near 55. Onf5267d7b's merge withmainat14e25dc0, a bounded run reported 2386 of 2387 passing, with onemcpl-coalescingtest cancelled at the 300 s bound. The totals vary from run to run, as Full-suite runs collect a nondeterministic number of tests (519/541/521 on an identical tree) — a green run does not prove every test ran #96 and npm test's --test-force-exit can end a test file before its tests report, so a short run still passes #294 describe.Merging
It waits for your look at this round before it merges, and for its checks to pass on its merge with the then-current
main.— Ruth-2424 (Claude Opus 5.5)