Skip to content

build(release): install the packed release on its own before publishing, and on pull requests - #349

Merged
nissa-seru merged 5 commits into
anima-research:mainfrom
nissa-seru:build/pack-installs-alone
Oct 17, 2026
Merged

nissa-seru merged 5 commits into
anima-research:mainfrom
nissa-seru:build/pack-installs-alone

Conversation

@nissa-seru

@nissa-seru nissa-seru commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

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 only devDependencies supplies, 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-mcp bin 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 of d2586958 (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.

The change

scripts/pack-installs-alone.mjs packs the package as npm publish packs 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 installed package.json, the one a consumer reads, offers, under the Node that runs it:

  • Every file its main, types (or typings), exports and bin name is in the package: each exports target under every condition (including the bun target connectome-host loads and the types target, which no import() reads), at least one packed file for each export pattern, and each bin. In a package with types (types, typings or a types condition), so is the .d.ts the build wrote beside each .js it packs, which is what a consumer's TypeScript reads. Before packing, it checks that the files package.json names 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:

    • through import() when Node's import conditions pick a target, with its import attribute for a JSON target;
    • through require() when require picks another file, as for a dual package's CommonJS build, or when only require serves the entry;
    • not at all when nothing loads it at run time, such as a types-only subpath; export patterns are matched, not loaded;
    • without exports, or with exports: null, the root loads from main, or from index.js as Node falls back to it;
    • with bun on PATH, each subpath whose exports names a bun condition 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 devDependencies lists 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's requires 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 a prepack'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 main tells the two apart). npm install has a 10-minute limit.

Where it runs:

  • publish.yml: a pack-check job of its own, with contents: read only, which publish needs.
    • The check's install of the tarball runs install scripts, as an install from npm does, and publish holds id-token: write, so the two stay apart.
    • publish itself now installs with --ignore-scripts. Neither its build (tsc) nor npm publish needs 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, publish ran every dependency's install scripts while holding the id-token.
    • publish publishes from the tree, not the tarball the check installed: npm skips a package's own lifecycle scripts for a tarball, which would skip prepublishOnly's release-hold check. A comment beside the step says so.
    • The GitHub release notes don't wait for it, keeping the two release jobs independent as CONTRIBUTING says.
    • Dispatching the workflow on a branch runs the check and publishes nothing.
  • ci.yml: a pack-check job on pull requests and pushes to main, 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.
  • Both jobs install and build the tree as publish does, with --ignore-scripts, so a build that came to need a dependency's install script fails there, on a pull request, rather than in publish after 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 build first). "Building and testing" names the CI job, with bun set up, and says CI installs with npm install rather than npm 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's bun condition, takes bun's place where a case loads a bun target.

  • Passing packages:
    • a package naming every kind of entry passes, loading root, subpath, JSON, require-only and dual entries, skipping a types-only and a null subpath, with a node bin and a shell bin, none of them run, the shell bin named as not loaded;
    • string exports, exports of conditions, an exports array, main alone and exports: null each load as the root;
    • an export pattern whose target repeats *, an import blocked by null beside a require, and a JSON main;
    • the index.js fallback, and a bin-only package;
    • npm 12's JSON shape, and a prepack that prints.
  • Before packing: an unbuilt tree, and a main naming no file.
  • Left out of files: a bun target, a types or typings file, a pattern, and the declaration beside a packed module, whether types, typings or a types condition 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.
  • Loading: a devDependencies-only import, one that another dependency's copy would have satisfied, a failing subpath, and a bun target that doesn't load.
  • Bins: one with no #! line; an ES module bin that doesn't parse, one whose import only devDependencies lists, and one whose imports, a CommonJS file among them, load, never run; a CommonJS bin, by type and by .cjs, parsed and never run, and one that doesn't parse.
  • Install: a dependency that can't be installed, and npm pack output that isn't one record with a filename and its files.

Evidence

At aa7f8ba7, on Linux:

  • Tests: test/pack-installs-alone.test.ts passes 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.
  • This repository, installed and built as the pack-check job does it, with npm install --ignore-scripts and npm run build, which both exit 0, and then npm run pretest for the tests: the check reports animalabs-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 that pretest copies into dist. 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.
  • The declarations: with files set to ["dist/**/*.js", "dist/src/index.d.ts", "src"], the check fails, naming dist/src/agent.d.ts and 253 other declarations beside packed modules.
  • bun's target: for a package whose exports lists import before bun, bun 1.3.14 and 1.4.3 both load the import target by the package's name, and the bun target when bun comes first.
  • Removals: each of 24 pieces, removed on its own, fails the tests aimed at it.
    • Installing and packing: the nested install; 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.
    • Loading: the bun load; exports: null read as absent; an exports array read for the root; the require() load beside an import, and none of the import's own file.
    • Bins: a bin counted as loaded only when its imports load; the hook stopping an ES module bin before its own code; the hook stopping a CommonJS bin before it loads, and the bin parsed instead; the hook touching the bin alone; a bin whose #! names no node named as not loaded.
    • Named files and declarations: typings among 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; a types condition and typings each giving the package types; a package without types left alone.
    • Messages: a dependency's new release named in the bun-load and bin-load failures.
  • npm test ran at f5267d7b, 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 in mcpl-coalescing, tool-result-guard and workspace-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 one tool-result-guard test, "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. On f5267d7b's merge with main at 14e25dc0, a bounded run reported 2386 of 2387 passing, with one mcpl-coalescing test 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.
  • An internal review read the first version, and another read this round; the round's last commit answers it. The version before this one also answered Greptile's three findings.

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)

@greptile-apps

greptile-apps Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5 Tier: apex

[High impact] Build and release workflow changes with new packaging validation.

The PR appears safe to merge for the current package, with three non-blocking fixes to the new check.

Findings

  1. P2 Repeated stars reject valid files ▶
  2. P2 Blocked imports get picked ▶
  3. P2 JSON main entries fail ▶
Fix with agent prompt
### Issue 1
scripts/pack-installs-alone.mjs:142-143
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.

### Issue 2
scripts/pack-installs-alone.mjs:40-44
`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.

### Issue 3
scripts/pack-installs-alone.mjs:158-159
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.

Summary

Adds standalone package checks to CI and publishing, and separates agent-framework-mcp from the importable McpServer class.

  • The packed package is installed and checked before it can ship.
  • The MCP command starts through npm’s installed link.

Reviews (1) · Last reviewed commit: "build(release): install the packed relea..." · Reviewed by Greptile

Comment thread scripts/pack-installs-alone.mjs Outdated
Comment on lines +142 to +143
const [head, tail] = target.replace(/^\.\//, "").split("*");
if (!files.some((file) => file.startsWith(head) && file.endsWith(tail ?? ""))) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +40 to +44
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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread scripts/pack-installs-alone.mjs Outdated
Comment on lines +158 to +159
if (pkg.exports === undefined && (typeof pkg.main === "string" || existsSync(join(installed, "index.js")))) {
loads.push([pkg.name, "import"]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

nissa-seru pushed a commit to nissa-seru/agent-framework that referenced this pull request Oct 14, 2026
…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>
Ruth-2424 and others added 2 commits October 15, 2026 19:22
…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>
@nissa-seru
nissa-seru force-pushed the build/pack-installs-alone branch from a2e0e6e to d258695 Compare October 15, 2026 23:22

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.json and ran the real script. It catches a dropped src, a files list that keeps only the entry files, a dropped set of declaration files, and context-manager, chokidar or discord.js moved to devDependencies. It misses chronicle, membrane or ws moved to devDependencies (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 pack runs prepack/prepare/postpack and not prepublishOnly. I checked on npm 10.9.4 and 11.21.1 with a prepublishOnly that exits 1, and pack succeeded. So the pack-check job never consults the holds and rightly has no issues: read. In publish, npm install --ignore-scripts doesn't stop npm publish from running prepublishOnly: a --dry-run publish after an --ignore-scripts install 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/membrane or ws from dependencies to devDependencies. The check prints installs 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's linked or pnpm with hoist=false, it fails outright.
  • This is the class the PR names first: publish.yml:57, ci.yml:64 and CONTRIBUTING.md:207-208 ("an import only devDependencies supply, fails the PR").
  • Fix (tried): add --install-strategy=nested to that install. With npm_config_install_strategy=nested, and also with linked, the chronicle move fails with ERR_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 the CheckError path, so it prints a stack trace instead of a message. npm@latest is 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/prepare in the foreground and their stdout lands ahead of the JSON. I checked on npm 10.9.4, 11.21.1 and 12.2.1: a prepack that 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, prepack still 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 a CheckError unless exactly one record with a filename comes 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 file files leaves out. Today both bins only import modules the root loads (src/api/mcp-server.ts:11 → mcp-server-core.js, exported at src/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 inserts throw "__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 back linked, body not run. With import "zz-dev-only-pkg"; added to the installed mcp-server.js, it fails with Cannot find package 'zz-dev-only-pkg'.
  • The bun target, 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 an index.ts whose imports are missing. Bun 1.3.10 imports today's installed package fine.
    • Fix: add oven-sh/setup-bun to the pack-check jobs. When bun is on PATH, also run bun -e 'await import(<spec>)' from the install directory. #258 already uses bun build --target=bun for the host. Otherwise, say in the prose that the bun target is checked for presence only.
  • Declaration files: files: ["dist/**/*.js", "dist/src/index.d.ts", "src"] packs 360 files and passes, although every declaration file index.d.ts imports is missing. A cheap check: every packed dist/src/**/*.js has its .d.ts beside 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 publish publish exactly what was checked (upload the tarball as an artifact, then npm publish <tgz>): npm skips the package's own lifecycle scripts for a tarball. A --dry-run publish of a tarball with a failing prepublishOnly exits 0. The release-hold check would then have to become an explicit step in publish, with GH_TOKEN and issues: read, or the gate disappears with no error.
  • Also relevant to that split: nothing exercises publish's own --ignore-scripts install-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/*/*.js case passes);
    • a null reached under an active condition stops the search (:35, :170-173; the { import: null, require } case loads by require);
    • a JSON main gets the import attribute (:165).
  • 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 10 test/fixtures/*.mjs that pretest copies into dist/.
  • Mutations caught:
    • src dropped from files (./src/index.ts (exports['.']) isn't in …);
    • files cut 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:
    • publish needs [build-and-test, pack-check] (publish.yml:92);
    • github-release still needs only build-and-test (:145);
    • pack-check runs with contents: read and no token (:64-66);
    • a dispatch on a branch runs pack-check and skips both releasing jobs (:93, :152), and the build job's hold step skips there too (:38);
    • pack-check takes about 30 s and runs alongside the ~3.5 min build job, so it doesn't delay publish or change the hold windows described at :169-172.
  • --ignore-scripts in publish:
    • the flag applies only to that install;
    • npm run build (tsc plus postbuild chmod) 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.

Ruth-2424 and others added 2 commits October 16, 2026 01:07
…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>
@nissa-seru

Copy link
Copy Markdown
Contributor Author

Replying to this review.

@Anarchid Round 1 is up at aa7f8ba7 and takes all five findings. Each change to the check has a test, and each of 24 pieces, removed on its own, fails the test aimed at it. The round's last commit, aa7f8ba7, answers an internal review of it. I'm asking for your look at this round. Below, for each finding, is what changed and the test or run that shows it, then the evidence, all on Linux.

R1-1. The tarball now installs with npm install --install-strategy=nested, so the package reaches only its own dependencies. The new test "a package imported only through another dependency's copy fails, since the install is nested" packs two local packages, zz-cm, which depends on zz-chronicle, and a fixture that imports both but lists zz-chronicle only in devDependencies. The check fails with Cannot find package 'zz-chronicle', and passes once the fixture lists it in dependencies. On this repository the nested install passes.

R1-2. npm pack runs with --foreground-scripts=false, and its JSON is read as npm 11's array or npm 12's object keyed by package name. Anything that isn't exactly one record with a filename and its files fails with a message instead of a TypeError. The test "npm pack's JSON is read in npm 12's shape too, and a prepack's output doesn't break it" passes under npm 11.19.1 and npm 12.2.0, and the check passes on this repository with npm 12.2.0.

R1-3.

  • A node bin's imports now load without the bin running, through the load hook you described: it puts a throw after the #! line, for the bin's own URL only. Tests show a bin whose import only devDependencies lists fails with Cannot find package 'zz-dev-only', and a bin whose imports load never runs its own code.
  • A CommonJS bin's requires run with its code, so its imports can't be loaded without running it. The hook throws instead of loading a bin whose format isn't module, before anything is evaluated, and such a bin is only parsed with node --check. Node decides the format, by .cjs or by the package's type. A test covers both kinds and shows the bin's code doesn't run. It also covers one that doesn't parse. This came from our own read of the round, not from your review.
  • With bun on PATH, each subpath whose exports names a bun condition is also imported under bun, by the package's name, as a bun consumer imports it. bun reaches the bun target when bun is the first condition it matches, as in this package.json. Both pack-check jobs set up bun 1.3.14, the version connectome-host's CI runs this package under. Without bun, the success line says those targets were checked for presence only. On this repository, the check's line includes loaded under bun: '@animalabs/agent-framework'.
  • The declaration check takes your suggestion. In a package with types (types, typings or a types condition), the .d.ts the build wrote beside each packed .js must be in the package. On this repository, your example's files: ["dist/**/*.js", "dist/src/index.d.ts", "src"] now fails the check, naming dist/src/agent.d.ts and the declarations beside the other packed modules. The tests are "the declaration the build wrote beside a packed module, left out of files, fails, whether types, typings or a types condition gives the package types" and "no declaration is required beside a module the package doesn't pack, or in a package without types".
  • exports: null now falls back to main, with a test. A subpath whose require target is another file, a dual package's CommonJS build, now loads through require() too. Export patterns are matched and not loaded, as the success line says.
  • A bin whose #! line names no node is checked for that line alone, and the success line now names it as not loaded.

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 prepublishOnly hold check. For your second point, both pack-check jobs now install the tree with npm install --ignore-scripts and build it, as publish does. A build that came to need a dependency's install script then fails on a pull request, not in publish after the tag. At aa7f8ba7 that install and npm run build both exit 0.

R1-5. All three load-failure messages now name a dependency's new release among their causes, with a rerun on main to tell: the import, the bun load and the bin load.

Evidence at aa7f8ba7:

  • test/pack-installs-alone.test.ts passes 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.
  • On this tree, the check reports: animalabs-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. The 1135 files include the 10 test fixtures that pretest copies into dist.
  • It also passes with bun 1.4.3 and with npm 12.2.0. On Node 20.20.2 without bun, it reports the bun target present but not loaded.
  • With your example's files, it fails, naming dist/src/agent.d.ts and 253 other declarations beside packed modules.
  • For a package whose exports lists import before bun, bun 1.3.14 and 1.4.3 both load the import target by the package's name, and the bun target when bun comes first.
  • Each of 24 pieces, removed on its own, fails the test aimed at it. The PR body lists them.
  • npm test ran at f5267d7b, 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, and at a load average near 100 it reported 2349 of 2355 tests passing. The six failures are in mcpl-coalescing, tool-result-guard and workspace-watcher, which this change doesn't touch. Each failed under that load, and run alone three times each, those files passed, except one tool-result-guard test that failed once at a load near 55. On f5267d7b's merge with main at 14e25dc0, a bounded run reported 2386 of 2387 passing, with one mcpl-coalescing test cancelled at the 300 s bound.

What the internal review changed, in aa7f8ba7:

  • the declaration check above, which first walked the directory of types and so asked for declarations no packed module has beside it, such as a test helper's;
  • the dual subpath's require() load;
  • bins whose #! line names no node, named as not loaded;
  • npm pack's record checked for its files list;
  • in the text, the header says the nested install is stricter than npm's default and that the bun load imports the package's name, and CONTRIBUTING points at the header for the list of checks.

Written by Ruth-2424, and read by Ogden-2688.

— Lottie-2661

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/membrane or ws moved to devDependencies each fail with Cannot find package ….
    • files: ["dist", "src/index.ts"] fails the bun load with Cannot find module './types/index.js'.
    • files: ["dist/**/*.js", "dist/src/index.d.ts", "src"] fails, naming dist/src/agent.d.ts and the declarations beside the other packed modules.
    • import "zz-dev-only-pkg" added to the built mcp-server.js fails the bin load, and the server never starts.
    • A prepack that 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's throw, the bun load, the declaration rule, == null, and the dual require;
    • 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.

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/root imports ms and lists it only in devDependencies. It depends on zz-cm, whose only tie to ms is peerDependencies: { "ms": "^2.1.3" }. The check prints installs on its own … loaded '@zz/root' (import) and exits 0, and the install puts ms at node_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 of ci.yml runs, prints npm 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.

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 main module that does process.on("SIGTERM", () => {}) and then waits forever is still running at 40 s, when an outer timeout stops it. Its child is orphaned, and the temp directory stays behind because finally never ran. Without the handler, the same module fails at 3 s with it 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-check then hangs for six hours before publish is skipped. github-release doesn't wait for it, so the release notes are out by then.
  • Fix (tried): set killSignal: "SIGKILL" in run() and in the bin's spawnSync. With that, the same fixture fails at 3 s, with no orphan and the temp directory removed. Also give npm pack a timeout, and add timeout-minutes: 15 to 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".
  • engines allows Node 20.0, and register arrived in 20.6, so 20.0 to 20.5 fail the same way today.
  • Fix (tried): use registerHooks when it's there (Node 22.15+), and keep register as the fallback. The preload becomes if (m.registerHooks) m.registerHooks({ load: (await import(url)).load }); else m.register(url);. The hook's load calls nextLoad and 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 null and tries the next target. pick() stops at the null (:65-69).
  • Its condition lists (:237-238) also leave out module-sync, which Node 22.12+ and 24 match.
  • Reproduced: with "exports": [null, "./dist/index.js"] and a dev-only import in index.js, Node's own import() fails with Cannot find package 'zz-dev-only'. The check passes, and its success line has no loaded clause.
  • Neither shape is in this package, and neither is common.
  • Fix: skip a null inside 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:62 says "It runs on a dispatch too, which publishes nothing." A dispatch on a v* tag does publish, because publish's if: tests only github.ref. CONTRIBUTING (:146) rightly says "on a branch"; say that here too.
  • ci.yml:38-39 says "plain npm install (matching publish.yml)". publish.yml's publish job 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.
    • Its log shows bun 1.3.14 set up from the pinned setup-bun commit, the nested install (added 34 packages), and loaded under bun.
    • That run used the merge with the main of 23:04 UTC, before #384. On the merge with today's main (e6a0317), the check passes too: 1137 files, npm 11.21.1, bun 1.3.14.
  • 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 .cjs import 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.
  • Release holds: as in round 1, npm pack doesn't run prepublishOnly, and publish, which publishes from the tree, still does.
  • Workflow graph and permissions:
    • publish needs both jobs, and github-release needs only build-and-test.
    • pack-check runs with contents: read and doesn't persist credentials.
    • setup-bun@0c5077e5… is the v2.2.0 release 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.ts beside each packed .js. So it asks for nothing beside the .mjs fixtures pretest copies, or beside a file the build didn't emit. It raises no false failure on this repository, whose tsconfig emits .js and .d.ts side 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.

@nissa-seru
nissa-seru merged commit 2482625 into anima-research:main Oct 17, 2026
6 checks passed
nissa-seru pushed a commit to nissa-seru/agent-framework that referenced this pull request Oct 17, 2026
…-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>
nissa-seru pushed a commit to nissa-seru/agent-framework that referenced this pull request Oct 17, 2026
…-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>
nissa-seru pushed a commit to nissa-seru/agent-framework that referenced this pull request Oct 17, 2026
…-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>
nissa-seru pushed a commit to nissa-seru/agent-framework that referenced this pull request Oct 17, 2026
…-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>
@nissa-seru

Copy link
Copy Markdown
Contributor Author

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:

  • R2-2. Both pack-check jobs get timeout-minutes: 40 rather than 15. The defaults for npm pack and npm install are 10 minutes each. A 15-minute job limit could interrupt those sequential stages before their own budgets were exhausted.
  • R2-3. The header names Node 20.10 as the minimum. Loading a JSON entry point uses an import attribute that Node 20.9 refuses.
  • R2-4. The condition lists also include node-addons, which Node matches by default. We left out an additional report for runtime files neither load selects. Treating those files as failures would reject packages intended for other runtimes, bundlers, or user conditions; an informational list would add output without validating those configurations. Non-pattern targets retain their package-inclusion checks.

— Fiona-2757

@nissa-seru nissa-seru mentioned this pull request Oct 17, 2026
nissa-seru pushed a commit that referenced this pull request Oct 17, 2026
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>
nissa-seru added a commit that referenced this pull request Oct 18, 2026
build(release): the pack check after the maintainer's review of #349
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants