From fd2dba957e14f9a0e9da270f8ce3ebe15c222e00 Mon Sep 17 00:00:00 2001 From: paulohenriquevn Date: Mon, 7 Sep 2026 09:30:52 -0300 Subject: [PATCH] fix(dep-check): substitute a sibling whose contract moved without its version (usetheokit/theokit#659) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check D skipped any sibling whose version number the registry already had. That answers "does this version exist?", which is a different question from "is the published copy the one this workspace is about to publish?". A release that widens a peer range across two workspace packages moves the contract and not the version — changesets bumps at version time, not at pull-request time. So the leg packed tomorrow's package, resolved yesterday's sibling from npm, and reported two copies of a peer: a pair publication never produces. It is a required check, so the release could not merge, and no ordering of the old design avoided it — bumping first makes the packed tarball ask for a version npm does not have, and the install answers ETARGET instead. A sibling now also earns its local tarball when the registry serves its version with different dependencies, peerDependencies or optionalDependencies. The narrowness is deliberate: an identical contract is still installed from the registry, which is what makes the leg test what a consumer resolves. Absent evidence answers "unchanged" — a registry that returns no manifest must not read as "the contract moved", or every sibling gets a local tarball and the leg tests nothing. Each substitution now records WHICH gap it filled, and the note says so. The single message claimed the registry did not have the version, which is false for this case; a note naming the wrong reason is worse than none, because it is read and believed. Proved in both directions against the real usetheokit/theokit checkout, with the condition reconstructed on disk (a peer range widened without a version bump): with the fix @theokit/presenter@0.9.0-next.0 substituted, note explains why fix reverted no substitution at all — the blind spot, reproduced 109 unit tests pass, including the pre-existing one asserting that a sibling the registry already serves is NOT substituted, which is the guarantee this must not break. --- CHANGELOG.md | 24 ++++++++++++++ packages/dep-check/index.mjs | 21 ++++++++++-- packages/dep-check/src/checks.mjs | 43 +++++++++++++++++++++++-- packages/dep-check/src/registry.mjs | 13 ++++++++ packages/dep-check/test/checks.test.mjs | 43 +++++++++++++++++++++++++ 5 files changed, 138 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae4845e..e5e1b9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] + +### Fixed + +- **The install leg substitutes a sibling whose contract moved without its version moving + (usetheokit/theokit#659).** Check D skipped any sibling whose version number the registry already + had. That answers "does this version exist?", which is a different question from "is the published + copy the one this workspace is about to publish?". + + A release that widens a peer range across two workspace packages moves the contract and not the + version, because changesets bumps at version time and not at pull-request time. So the leg packed + tomorrow's package, resolved yesterday's sibling from npm, and reported two copies of a peer — a + pair publication never produces. It is a required check, so the release could not merge, and no + ordering of the old design avoided it: bumping first makes the packed tarball ask for a version npm + does not have, and the install answers ETARGET instead. + + A sibling now also earns its local tarball when the registry serves its version with different + `dependencies`, `peerDependencies` or `optionalDependencies`. The narrowness is deliberate — an + identical contract is still installed from the registry, which is what makes the check test what a + consumer resolves. Absent evidence answers "unchanged": a registry that returns no manifest must + not read as "the contract moved", or every sibling gets a local tarball and the leg tests nothing. + + Each substitution now says WHICH gap it filled. The single note claimed the registry did not have + the version, which is false for this case, and a note that names the wrong reason is worse than + none because it is read and believed. ### Added - **The back-merge says how long its pull request has been open (usetheokit/theokit-sdk#566).** diff --git a/packages/dep-check/index.mjs b/packages/dep-check/index.mjs index d952f89..52d6e91 100755 --- a/packages/dep-check/index.mjs +++ b/packages/dep-check/index.mjs @@ -23,7 +23,7 @@ import { parseArgs } from "node:util"; import { ceilingDrift, consumersLeftBehind, floorsInOwnWorkspace, groupUntestedFloors, installedDrift, isSibling, peerInstallSpecs, pinnableSiblings, rangeFloor, sharedFloor, unpublishedSiblings, unpublishedWorkspaceVersions, untestedFloors } from "./src/checks.mjs"; import { findPublishablePackages, resolveInstalledVersion, siblingReferences } from "./src/ecosystem.mjs"; -import { consumersOf, discoverEcosystemPackages, latestVersion, packument, publishedVersions } from "./src/registry.mjs"; +import { consumersOf, discoverEcosystemPackages, latestVersion, packument, publishedManifest, publishedVersions } from "./src/registry.mjs"; import { batchedWithDeps, detectBuildScript, detectPackageManager, pinOverrides } from "./src/package-manager.mjs"; import { installFromTarball } from "./src/tarball.mjs"; @@ -222,12 +222,23 @@ async function commandInstall(root) { dir: p.dir, // Carried so the substitution can follow a substituted tarball's own unpublished asks. references: siblingReferences(p.manifest, isSibling), + // Carried so the substitution can compare the contract this workspace declares against the one + // the registry serves at the same version number (usetheokit/theokit#659). + manifest: p.manifest, })); const substitutions = []; // Built once for every workspace package, not per reference: the substitution walks // transitively, so it needs an answer for siblings the package under test never names. const published = {}; for (const p of workspace) published[p.name] = await publishedVersions(p.name); + // What the registry serves AT the workspace's own version, so a contract that moved without the + // version moving is visible. Only that one version is fetched: it is the only one the substitution + // decision compares against (usetheokit/theokit#659). + const manifests = {}; + for (const p of workspace) { + const served = await publishedManifest(p.name, p.version); + if (served) manifests[p.name] = { [p.version]: served }; + } // What the registry SERVES, as opposed to everything it holds. `peerInstallSpecs` asks whether // that version satisfies a declared range, which is the only way to tell a real prerelease floor // (`>=4.63.4-next.0`, latest does not satisfy) from a prerelease SENTINEL (`>=0.1.0-alpha.0`, @@ -238,7 +249,7 @@ async function commandInstall(root) { const refs = siblingReferences(pkg.manifest, isSibling); // What the registry cannot answer yet, taken from the workspace instead. Only the gap — // see `unpublishedSiblings`. - const localSiblings = unpublishedSiblings({ references: refs, workspace, published }); + const localSiblings = unpublishedSiblings({ references: refs, workspace, published, manifests }); // Computed AFTER the substitution and from it: a peer this cut is about to publish must not // also be asked for as `@latest`, or the registry copy overrides the packed one on the same // command line — see `peerInstallSpecs`. @@ -265,7 +276,11 @@ async function commandInstall(root) { // consumer can install today — and a reader deciding what a green D means has to know which // one they got. Same reason `untestedFloors` prints (#6). for (const s of substitutions) { - console.log(` note: ${s.pkg} was installed against ${s.name}@${s.version} packed from this workspace — the registry does not have that version yet, so this cut is testing what it is about to publish`); + const why = + s.reason === 'contract-moved' + ? 'the registry serves that version with different dependency ranges, so this cut is testing the pair it is about to publish rather than one publication never produces' + : 'the registry does not have that version yet, so this cut is testing what it is about to publish'; + console.log(` note: ${s.pkg} was installed against ${s.name}@${s.version} packed from this workspace — ${why}`); } return findings.length === 0 ? 0 : 1; } diff --git a/packages/dep-check/src/checks.mjs b/packages/dep-check/src/checks.mjs index 15a5d66..b0d0499 100644 --- a/packages/dep-check/src/checks.mjs +++ b/packages/dep-check/src/checks.mjs @@ -314,7 +314,30 @@ export function peerInstallSpecs({ references, localSiblings = [], latest = new return [...specs]; } -export function unpublishedSiblings({ references, workspace, published }) { +/** The fields a consumer's installer resolves. `devDependencies` never reaches a consumer's tree. */ +const INSTALLED_FIELDS = ["dependencies", "peerDependencies", "optionalDependencies"]; + +/** + * Does the workspace copy declare a different installed contract than the registry serves at the + * SAME version number? + * + * Absent evidence answers NO, deliberately. A registry that did not return a manifest, or a caller + * that passed none, must not read as "the contract changed" — that would hand a local tarball to + * every sibling and stop the check testing what a consumer actually resolves, which is the reason + * the version skip exists (usetheokit/theokit#659). + */ +function contractMoved(local, publishedManifest) { + if (!publishedManifest || !local.manifest) return false; + return INSTALLED_FIELDS.some((field) => !sameRanges(local.manifest[field], publishedManifest[field])); +} + +function sameRanges(a = {}, b = {}) { + const keys = new Set([...Object.keys(a), ...Object.keys(b)]); + for (const k of keys) if (a[k] !== b[k]) return false; + return true; +} + +export function unpublishedSiblings({ references, workspace, published, manifests = {} }) { const byName = new Map(workspace.map((p) => [p.name, p])); const out = []; const seen = new Set(); @@ -333,8 +356,22 @@ export function unpublishedSiblings({ references, workspace, published }) { // Missing is not empty: an unreachable registry must not read as "nothing is published", // or every sibling gets a local tarball and the check tests nothing real. if (!Array.isArray(versions) || !versions.length) continue; - if (versions.includes(local.version)) continue; - out.push({ name: dep, version: local.version, dir: local.dir }); + // "The registry has this version number" is a DIFFERENT question from "the registry has the + // copy this workspace is about to publish". A release that changes a dependency contract across + // two workspace packages moves the contract without moving the version, because changesets bumps + // at version time and not at pull-request time. Skipping on the number alone installed tomorrow's + // package against yesterday's sibling and reported a duplicate peer — a pair publication never + // produces, and one no ordering of the old design could avoid (usetheokit/theokit#659). + if (versions.includes(local.version) && !contractMoved(local, manifests[dep]?.[local.version])) continue; + out.push({ + name: dep, + version: local.version, + dir: local.dir, + // Which of the two gaps this substitution fills. The report explains itself with this, + // and "the registry does not have that version" is FALSE for a contract that moved + // without the version moving (usetheokit/theokit#659). + reason: versions.includes(local.version) ? 'contract-moved' : 'unpublished', + }); for (const r of local.references ?? []) queue.push(r.dep); } return out; diff --git a/packages/dep-check/src/registry.mjs b/packages/dep-check/src/registry.mjs index 55df5a0..48dbe30 100644 --- a/packages/dep-check/src/registry.mjs +++ b/packages/dep-check/src/registry.mjs @@ -34,6 +34,19 @@ export async function latestVersion(name) { return doc?.["dist-tags"]?.latest ?? null; } +/** + * The manifest the registry SERVES for one exact version, or null when it serves none. + * + * A version number is not a contract: the registry can hold `0.8.0` while the workspace is about to + * republish a different `0.8.0` with a widened peer range, because changesets moves the version at + * version time and not at pull-request time. Reading the served manifest is what lets the install + * leg tell those two apart (usetheokit/theokit#659). + */ +export async function publishedManifest(name, version) { + const doc = await packument(name); + return doc?.versions?.[version] ?? null; +} + /** Every published version, in registry order. */ export async function publishedVersions(name) { const doc = await packument(name); diff --git a/packages/dep-check/test/checks.test.mjs b/packages/dep-check/test/checks.test.mjs index 3ecb2fd..5aee5ca 100644 --- a/packages/dep-check/test/checks.test.mjs +++ b/packages/dep-check/test/checks.test.mjs @@ -247,6 +247,7 @@ describe("unpublishedSiblings — what check D cannot get from the registry yet" const out = unpublishedSiblings({ references: [{ dep: "@theokit/agents" }], workspace, published }); expect(out.map((s) => s.name)).toEqual(["@theokit/agents"]); expect(out[0].version).toBe("12.1.0"); + expect(out[0].reason).toBe("unpublished"); }); it("test_leaves_a_sibling_the_registry_already_has_to_be_installed_from_the_registry", () => { @@ -261,6 +262,48 @@ describe("unpublishedSiblings — what check D cannot get from the registry yet" expect(unpublishedSiblings({ references: [{ dep: "@theokit/nowhere" }], workspace, published })).toEqual([]); }); + it("test_substitutes_a_published_version_whose_local_contract_no_longer_matches", () => { + // usetheokit/theokit#659. The skip above asks "is this version number published?", which is a + // different question from "is the published copy the one this workspace is about to publish?". + // A release that widens a peer range across two workspace packages moves the CONTRACT without + // moving the version number — changesets bumps at version time, not at pull-request time. So the + // gate installed tomorrow's package against yesterday's sibling and reported two copies of a + // peer, a pair publication never produces. Measured on usetheokit/theokit#658, which it blocked. + const ws = [ + { + name: "@theokit/presenter", + version: "0.8.0", + dir: "/w/packages/presenter", + manifest: { peerDependencies: { "@theokit/sdk": "^4.49.0 || ^5.0.0" } }, + }, + ]; + const pub = { "@theokit/presenter": ["0.7.0", "0.8.0"] }; + const manifests = { "@theokit/presenter": { "0.8.0": { peerDependencies: { "@theokit/sdk": "^4.49.0" } } } }; + const out = unpublishedSiblings({ references: [{ dep: "@theokit/presenter" }], workspace: ws, published: pub, manifests }); + expect(out.map((s) => s.name)).toEqual(["@theokit/presenter"]); + // The report explains every substitution, and the older explanation — "the registry does not + // have that version yet" — is FALSE here: it has it, with a different contract. A note that + // names the wrong reason is worse than none, because it is read and believed. + expect(out[0].reason).toBe("contract-moved"); + }); + + it("test_leaves_a_published_version_alone_when_the_local_contract_is_identical", () => { + // The narrowness is the point (#659). Substituting whenever a package is in the workspace would + // stop testing what a consumer resolves — the reason the skip exists at all. Only a contract + // that DIFFERS earns the local tarball. + const ws = [ + { + name: "@theokit/presenter", + version: "0.8.0", + dir: "/w/packages/presenter", + manifest: { peerDependencies: { "@theokit/sdk": "^4.49.0" }, dependencies: {} }, + }, + ]; + const pub = { "@theokit/presenter": ["0.8.0"] }; + const manifests = { "@theokit/presenter": { "0.8.0": { peerDependencies: { "@theokit/sdk": "^4.49.0" } } } }; + expect(unpublishedSiblings({ references: [{ dep: "@theokit/presenter" }], workspace: ws, published: pub, manifests })).toEqual([]); + }); + it("test_follows_a_substituted_tarballs_own_unpublished_asks", () => { // `@theokit/tauri` depends on `theokit@0.57.0`, which is substituted — and that tarball then // requests `@theokit/agents@^12.1.0`, still absent. Substituting only the direct reference