[TEST] Ensure every feature implementation considers every install strategy

[manzoorwanijk]

Note

This document was drafted, reviewed, and evaluated with the help of Claude Code and Codex through multiple rounds of iteration.

Is there an existing issue for this?

• I have searched the existing issues

Description of Problem

Context: #9510 review thread (2026-06-08). Features keep landing that work under the default hoisted strategy but break under linked (isolated mode) — recent examples: allow-remote enforcement (#9494, #9510/#9511), optional peers via peerDependenciesMeta (#9460), workspace self-links (#9398). @owlstronaut suggested parameterizing tests over install strategies; this issue turns that into concrete workstreams.

Why this keeps happening (root causes)

1. A parallel tree-materialization path, with copied semantics. Linked mode is not a wholly separate reifier. reify() first builds the normal ideal tree, then — when installStrategy === 'linked' — converts it: it swaps this.idealTree for the result of createIsolatedTree(), diffs and reifies that, then swaps the original tree back before save (reify.js:86, :113, :127, :133). createIsolatedTree() rebuilds the tree out of lightweight IsolatedNode/IsolatedLink instances with store paths under node_modules/.store (isolated-reifier.js:302, :351). nested/shallow are not separate classes — they flow through the normal Node/PlaceDep path via installStrategy (build-ideal-tree.js:990, place-dep.js:137). So the high-risk divergence is specifically the linked conversion, not strategy branching in general.

2. The isolated classes re-implement a subset of Node/Link/Edge, and the dangerous gaps are the falsy/empty ones that silently opt out. A read that returns undefined/false/[] does not throw and does not show up as missing coverage — it just quietly takes the wrong branch. The recurring bug class:

   • isRegistryDependency — Node computes it from incoming edge specs; IsolatedNode stores a copied boolean defaulting false. Reify's allow-remote override returns false if absent/false (node.js:588, isolated-classes.js:21, reify.js:849). This is the registry-tarball allow-remote class ([BUG] install-strategy=linked rejects registry-resolved tarball URLs with EALLOWREMOTE #9494).
   • isRootDependency / isProjectRoot — store nodes can't infer from edgesIn, so isolated-reifier precomputes; IsolatedNode.get isProjectRoot() always returns false even though idealGraph marks the root isProjectRoot: true (isolated-reifier.js:92, :158, isolated-classes.js:70, reify.js:708).
   • inBundle / inDepBundle — Node computes bundling; IsolatedNode.inDepBundle is always false. Reify/diff/build/script policy branch on these (node.js:558, isolated-classes.js:75, diff.js:119, rebuild.js:177).
   • shouldOmit and dep flags — Node.shouldOmit() exists; Diff uses optional chaining so a missing shouldOmit is silently ignored. Isolated nodes carry only optional, not dev/peer/devOptional (node.js:494, diff.js:237, isolated-classes.js:27).
   • binPaths — Node computes paths; isolated starts [] and pushes only one linked-dependency path. Diff treats empty arrays as "bins exist" (node.js:313, isolated-classes.js:14, isolated-reifier.js:429, diff.js:133).
   • Edge shape — isolated edges are plain objects, while real Edge exposes getters like valid, dev, peer, spec. Reify already reads e.valid (isolated-reifier.js:458, edge.js:181, :250, reify.js:711). Any new edge-getter read becomes undefined in linked mode.

3. Tests default to hoisted. The main reify suite (workspaces/arborist/test/arborist/reify.js, 4300+ lines) runs almost entirely with the default strategy. Linked coverage lives in a separate file (test/isolated-mode.js, custom rule-based assertions) plus a handful of hand-parameterized tests. New feature tests are written against hoisted only, so linked regressions are invisible until a user hits them.

4. No process signal. Nothing in CI, the PR template, or contributor docs prompts an author (human or AI) to ask "does this behave the same under linked/nested/shallow?".

Why 100% coverage already passes (so coverage isn't the gap)

This deserves to be stated up front because it is counterintuitive: the existing test suite already enforces 100% coverage, and these bugs ship anyway.

• Arborist's test script is plain tap (workspaces/arborist/package.json:52), and CI just runs npm test --ignore-scripts --workspace @npmcli/arborist (.github/workflows/ci-npmcli-arborist.yml:112). The tap config only adds nyc excludes, no lowered thresholds (package.json:80); tap 16's installed defaults are check-coverage: true at 100 for statements/branches/functions/lines. The suite passes, so every counted line/branch — including in isolated-reifier.js/isolated-classes.js — is at 100%. (Counted, not literally every line: the code uses Istanbul ignore pragmas, including an explicitly "untested" linked branch at reify.js:827.) Because isolated-reifier.js only runs under installStrategy: 'linked', that coverage is genuinely linked-sourced.
• Therefore "increase coverage" is not the lever. The parity bugs survive because of, not in spite of, full coverage. Line/branch coverage counts lines executed, not behavior asserted per strategy. The [BUG] install-strategy=linked rejects registry-resolved tarball URLs with EALLOWREMOTE #9494 bug is the canonical case: the isolated classes were 100% covered, but a property access returned undefined/false in linked mode — a read that returns a falsy default is still a covered line. No coverage gate, at any threshold, catches that.

Potential Solution

Goals

• New features that touch reify/ideal-tree behavior get linked-mode coverage by default, not by heroics.
• Drift between Node/Link/Edge and their isolated counterparts is caught mechanically, not by field reports.
• The matrix distinguishes behaviors that must be identical across strategies from those that are intentionally strategy-specific (e.g. linked omits workspace self-links by design — isolated-mode.js:45). Parity work targets the former; the latter get explicit per-strategy expectations.
• The cost stays proportional: we should not double the test suite runtime or force every layout-assertion test to be rewritten.

How we measure progress (and why nyc % is the wrong metric)

The 100% nyc gate (see above) is not the success measure. What we actually track instead:

1. Parity matrix (the real metric). Phase 0 produces a feature × strategy matrix; the unit of progress is cells with a behavioral assertion under that strategy, not a percentage from nyc. Each row is tagged same-outcome (must match hoisted) or strategy-specific (expected to differ, with the expected linked outcome recorded). "Coverage" in this issue means "this feature's outcome is asserted under linked," computed by hand/checklist, not by a tool.
2. Conformance surface (mechanical). Phase 1's contract list gives an automatable number for the structural root cause: "% of the agreed Node/Link/Edge contract that the isolated classes implement or consciously exempt." That percentage is meaningful and can be gated in CI. Its hard limit (see Phase 1) is that it cannot see copied-but-wrong values or semantic divergence — those need behavioral tests.
3. What nyc still buys us. When a feature adds a branch to a shared file (reify.js, build-ideal-tree.js, diff.js), the 100% gate forces that branch to be executed — but it can be satisfied entirely by a hoisted test. Parameterization (Phase 2) is what makes the linked execution of that branch happen and be asserted. Net: keep the 100% nyc gate (cheap, has caught regressions) as a floor, but never treat it as evidence of strategy parity.

One mechanical option worth evaluating in CI: a linked-only coverage run (force installStrategy: 'linked' and report coverage of isolated-reifier.js/isolated-classes.js in isolation). It won't catch the falsy-read class either, but it cheaply flags isolated-path lines that only exist to support a feature whose linked test was never written. Secondary to the matrix and conformance test.

Workstreams

Phases are numbered in execution order: Phase 0 (contract audit) → Phase 1 (conformance) → Phase 2 (helper + migration) → Phase 3 (CI) → Phase 4 (docs/templates). The dependencies behind that order: Phase 1 needs the contract Phase 0 produces, and is worth doing early because it is cheap and guards the biggest bug class; Phase 3's env-var widening only affects tests that use the Phase 2 helper, so it must follow Phase 2. Phase 5 (backport) is cross-cutting, not a final step — see its section.

Phase 0 — Audit and inventory (one-time, informs everything else)

• Scope is every consumer of the isolated tree after createIsolatedTree(), not just the three obvious files. Confirmed linked-sensitive sites beyond reify.js/build-ideal-tree.js/isolated-reifier.js: diff.js (:119, :133, :237), rebuild.js (:177, :202), script-allowed.js (:329), install-scripts.js (:34), unreviewed-scripts.js, and shrinkwrap.js (:247). Also enumerate the Node/Link/Edge members each reads.
• Produce the explicit conformance contract here (the input Phase 1 needs): the set of Node/Link/Edge members the post-conversion isolated tree must provide, each marked implemented or consciously exempt (with reason).
• For each feature/config option that changes reify behavior (allow-remote, patches, overrides, scripts policy, workspaces, omit/include, global, bundled deps, optional/peer deps), record whether a linked-mode test exists and whether it is same-outcome or strategy-specific.
• Deliverable: a tracking issue with the matrix (feature × strategy × tested? × same-vs-specific) plus the contract list. Gaps become issues labelled install-strategy-parity.

Phase 1 — Structural conformance test for the isolated classes

This targets the structural half of root cause 2 and is the cheapest high-leverage guardrail. It depends on the Phase 0 contract list (a blanket member diff produces noise).

• Assert that IsolatedNode/IsolatedLink (and the isolated edge object shape) provide every member on the agreed contract, or are explicitly listed as exempt. Check must be instance-based, not prototype-only: many isolated members are class fields, not prototype methods (isolated-classes.js:14), so in/Reflect.has on a constructed instance — not Object.getOwnPropertyNames(proto) — is required.
• Failure message tells the author to either implement the member on the isolated classes or add it to the exempt list with a reason.
• Known limits (call out explicitly so no one over-trusts it): structural conformance cannot catch (a) copied-but-wrong values — isRegistryDependency exists yet defaults false; (b) semantic divergence — Node's package setter reloads edges while isolated package is plain data (node.js:377); (c) root/identity mismatch — isProjectRoot always false on isolated nodes (isolated-classes.js:70). These require the behavioral/semantic tests in Phase 2.
• Stretch: a test-only Proxy over IsolatedNode that throws on a read of any non-exempt Node/Link/Edge contract member that the isolated instance doesn't actually define, so an unanticipated read fails loudly in linked test runs instead of yielding a falsy default.
• Longer-term refactor candidate (separate effort, not blocking): share getters between Node and IsolatedNode via a common base to shrink the duplicated surface.

Phase 2 — Shared parameterization helper (the core of @owlstronaut's suggestion)

• There is already an ad-hoc precedent: for (const strategy of ['hoisted', 'linked']) in test/arborist/reify.js:2015, which creates a fresh fixture inside each strategy subtest (reify.js test:2017). Generalize it into a helper in test/fixtures, e.g.:

  // testStrategies(t, 'keeps stale package.json', { setup }, async (t, { strategy, reify }) => { ... })

• Design requirements, derived from how the suite actually works:

  • Fresh fixture per strategy. Reify mutates disk, lockfiles, and node_modules; the helper must take a setup(t, strategy) factory and call it per strategy, never share a tree across strategies.
  • Serial by default. The registry mocking disables net-connect globally in teardown-managed nock (tnock.js:6), so parallel strategy subtests race. Opt into parallelism explicitly only where safe.
  • No t.plan() surprises. Many existing tests use explicit t.plan() (reify.js test:410, :537); multiplying subtests under a parent with a fixed plan breaks the count. The helper must register exactly one subtest on the caller's t and create the per-strategy subtests inside that one, so the parent's plan counts a single child regardless of how many strategies run.
  • Strategy-namespaced names and snapshots. Test names and snapshot keys must include the strategy (the suite is snapshot-heavy — reify.js test:413), so hoisted/linked snapshots don't collide.

• What to parameterize vs. not. Target tests that assert behavior/outcomes and, for linked, disk shape and runtime/script behavior — because linked swaps the original ideal tree back before save (reify.js:133), a package-lock-only assertion can pass while the isolated on-disk materialization is wrong. Do not mass-migrate tests that assert the hoisted physical node_modules layout; linked legitimately uses .store (isolated-reifier.js:351). Such tests get strategy-specific expectations or stay single-strategy.

• "Every strategy" vs. practical default: ['hoisted', 'linked'] covers the observed bug class but does not satisfy the literal goal. nested and shallow (the global default) must be reachable — either included in the default set for behavior tests or run via the CI-expanded set (Phase 3). Decide per the open question below.

• Migrate the Phase 0 gap list onto this helper, highest-risk features first (allow-remote, patches, overrides, workspaces, optional/peer deps, bundled deps).

• New rule of thumb encoded in docs (Phase 4): a new reify-behavior test uses the helper unless it is inherently layout-specific.

Phase 3 — CI visibility

• Add an env var honored by the Phase 2 helper, e.g. ARBORIST_STRATEGIES=hoisted,linked,nested,shallow, to widen the parameterized set without code changes. Caveat to document: this only widens tests that use the helper — it does not retroactively rerun the legacy suite under linked. Coverage grows only as tests migrate onto the helper.
• Add one CI job (single platform, single Node version — not a full matrix multiplication) that runs:
  • the arborist suite with the widened strategy set, and
  • a small CLI-level smoke run (npm install/ls/update against a sandbox registry) with --install-strategy=linked. CLI-level linked coverage today is essentially one ls --install-strategy=linked group (test/lib/commands/ls.js:5305).
  • optionally, the linked-only coverage run described above.
• Keep the existing platform/Node matrix on the default strategy to control runtime.

Phase 4 — Process guardrails (humans and AI agents)

• CONTRIBUTING.md: add a short "Install strategies" section — what they are, when a change needs linked coverage, the same-vs-strategy-specific distinction, and how to use the Phase 2 helper.
• PR template: one checklist line — "If this changes install/reify behavior: covered under install-strategy=linked (or explained why not applicable)."
• Lightweight CI reminder: a GitHub Action that comments on PRs touching any linked-sensitive file without also touching the isolated classes or a linked-strategy test. Watch set must include all Phase 0 consumers, not just two files: reify.js, build-ideal-tree.js, place-dep.js, diff.js, rebuild.js, script-allowed.js, install-scripts.js, unreviewed-scripts.js, shrinkwrap.js, node.js, link.js, edge.js. Advisory comment only, never a hard failure, to avoid noise on genuinely strategy-neutral changes.
• AGENTS.md: add the same rule so AI-assisted changes get the prompt too.

Phase 5 — Backport compatibility (cross-cutting)

Development happens on latest (the next major, currently v12-to-be), while release/v11 is the active release channel and release/v10 is still maintained. Non-breaking fixes and their tests are backported down via the backport:<version> label (e.g. backport:v11, which scripts/backport.js maps to release/v11 — the label is the bare version, not backport:release/v11). The label fires .github/workflows/backport.yml → scripts/backport.js, which cherry-picks the merged change onto each target branch (this is what produced the #9511 bot backport). The cherry-pick adapts to the merge shape — -m 1 for a merge commit, the single SHA for a squash, the full commit range for a rebase — so "the change" is not always one commit. AGENTS.md's standing rule is to keep branches from diverging so future backports stay clean. This shared test infrastructure changes the backport calculus, so it has to be designed for it.

The core risk: a parameterized parity test cherry-picks cleanly only if everything it references already exists, identically, on the target branch. A test that calls testStrategies(...), relies on the Phase 1 conformance harness, or imports a new fixture utility will conflict or fail when backported to a branch that lacks that infrastructure. The likely failure mode is a contributor then hand-writing a divergent one-off linked test on the release branch — exactly the fragmentation this issue exists to prevent.

Design and sequencing implications:

• Land the shared infrastructure on the release branches first, as its own backportable commits. The Phase 2 helper, the Phase 1 conformance harness, and any common fixtures should be backported to every actively-maintained release branch (v11, and v10 while supported) before feature-level parity tests start depending on them. Treat the helper as foundation, not as part of the first feature PR that happens to use it.
• Keep the helper's public API stable and identical across branches. Test files should cherry-pick byte-for-byte. Don't change the testStrategies signature or fixture-factory contract on latest without backporting the same change; otherwise every subsequent test backport conflicts.
• Backport the fix and its linked test together, never the test ahead of the fix. Keeping the parity fix and its test in the same PR makes them travel as a unit so they stay self-consistent through the cherry-pick. This is author discipline, not a mechanism guarantee — the bot will faithfully carry whatever shape the PR was merged in (merge/squash/rebase), so the safeguard is putting fix and test in one PR, not relying on "one commit." A linked implementation can legitimately differ between branches (a fix present on latest but not yet on v11), so a test must assert the outcome correct for its branch.
• Strategy-namespaced snapshots are a backport hotspot. tap-snapshots diffs are a common cherry-pick conflict source, and the expected linked output may legitimately differ on a release branch. Resolve by regenerating on the target branch rather than force-merging conflict markers, then eyeball the linked snapshot for correctness on that branch. Two preconditions matter: first run node ./scripts/resetdeps.js after checking out the target branch (per AGENTS.md — Arborist's package.json differs across latest/v11/v10, so stale deps give wrong snapshots), and use a workspace-scoped snapshot command (npm test --ignore-scripts --workspace @npmcli/arborist -- --snapshot, or the workspace's snap target) — the root tap config excludes workspaces/**, so a top-level snapshot run silently skips Arborist.
• Keep the conformance contract/exempt list in sync across branches. The isolated classes can differ per branch, so the contract and its exemptions are branch-local data. When backporting a contract change, expect to reconcile it against the target branch's actual Node/Link/Edge surface rather than assuming a clean apply.

Process additions (extend Phase 4):

• The PR/backport checklist should remind: a backported parity fix carries its linked test in the same PR, and the parameterization helper exists on the target branch before that test lands.
• Consider running the Phase 4 advisory bot and the conformance test on release branches too, so a backport that drops or diverges linked coverage is flagged there as well.

Sequencing and effort

Phase	Effort	Payoff
0. Audit + contract	~2-3 days	Makes the gap concrete; produces the contract Phase 1 needs and the matrix Phase 2 fills
1. Conformance test	Small (one test file)	Catches the structural "missing member" bug class
2. Helper + migrate high-risk tests	Incremental, per-feature	Direct regression protection; catches semantic/value divergence conformance can't
3. CI job	Small	Keeps coverage from rotting; reaches nested/shallow + CLI
4. Docs/template/action	Small	Shifts default behavior of contributors
5. Backport compatibility	Small, ongoing	Keeps parity tests cherry-pickable; prevents release-branch divergence

Phase 4 is independent and can land first as a cheap win. Phase 1 needs Phase 0's contract; Phase 3 needs Phase 2. Phase 2 is the long-tail investment and proceeds feature-by-feature. Phase 5 is not sequential — it is a constraint on Phases 1 and 2: the conformance harness and helper must be backported to active release branches before parity tests depend on them.

Open questions for maintainers

1. Which strategies should the default parameterized set include? ['hoisted', 'linked'] covers the observed bug class; do nested/shallow belong in the default set or only the CI-expanded ARBORIST_STRATEGIES run?
2. Is added CI runtime for one extra linked/expanded job acceptable per-PR, or should it be scheduled (nightly)?
3. Appetite for the longer-term refactor to unify Node/IsolatedNode/Edge getters (shared base), vs. keeping the conformance test as the permanent guard?
4. Should the conformance contract be hand-maintained, or derived (e.g. from a curated list of members actually read in the post-conversion code path)? Hand-maintained is simpler but drifts; derived is more work but self-updating.
5. How far back is the helper/conformance infrastructure maintained — release/v11 only, or release/v10 too while it is supported? This sets how many branches the foundation commits target.