ci: enforce the sibling devDependency rule, not just the protocol

bestax-migrate/CLAUDE.md

@@ -94,13 +94,15 @@ to how packing works: `pnpm -C bestax-migrate pack`.
 
 `@allxsmith/bestax-bulma` still stays a **devDependency** — it is only the
 typecheck target for the e2e, never imported at runtime, and consumers of a
-codemod CLI must not be made to install the component library. That is a policy
-rule, not a protocol one, and the conformance check enforces only part of it.
-The pack-time exemption this package gets is narrow: `workspace:`/`catalog:` in
-**devDependencies** only. Moving the library to `dependencies` as `workspace:^`
-is flagged (consumers would be made to install it), but re-adding it as a
-**plain semver range** still passes CI, because that is a policy question rather
-than a protocol one. That one is on review.
+codemod CLI must not be made to install the component library. Both halves of
+that are now enforced by `publishable-manifests` (#537): the protocol rule
+flags `workspace:^` in a consumer section, and the sibling rule flags a
+workspace package name in `dependencies`/`optionalDependencies` **whatever
+the specifier says** — so the plain-semver spelling that used to pass on
+review attention alone fails CI with the move-it-back fix named. One
+appearance that is NOT a counterexample: the e2e asserts the **migrated
+app's** manifest depends on the library (`e2e/kitchen-sink.test.ts`) — that
+is the codemod's output, which should depend on it, and no check reads it.
 
 The skill lives at repo-root
 `skills/bestax-migrate/`. It **is** bundled into create-bestax (settled in #385): the

scripts/check-conformance.mjs

@@ -1506,7 +1506,7 @@ const PNPM_PUBLISHED = new Set([
  * not re-derive the predicate that produced it. Two copies of one rule inside
  * one function is the drift this repo keeps paying for.
  */
-export function manifestViolations(dir, pkg) {
+export function manifestViolations(dir, pkg, siblings = new Map()) {
   if (pkg?.private) return [];
 
   // The declaration is consulted HERE rather than by the caller, so that a test
@@ -1582,8 +1582,13 @@ export function manifestViolations(dir, pkg) {
       }
       // pnpm turns this into a real range, so it installs. It is still wrong in
       // a section consumers resolve: it makes everyone installing this package
-      // install that one too.
-      if (CONSUMER_SECTIONS.includes(section)) {
+      // install that one too — but when the target is a workspace SIBLING,
+      // siblingViolations owns the case: its move-to-devDependencies fix is
+      // the right one, while this rule's pin-a-range advice would keep the
+      // sibling dependency in place and just trade one violation for another
+      // (#546 review). A `catalog:` entry can point at an external package,
+      // which is no sibling, so the skip is by name, not by protocol.
+      if (CONSUMER_SECTIONS.includes(section) && !siblings.has(name)) {
         offenders.push({ section, name, spec, protocol, why: 'consumer' });
       }
     }
@@ -1660,6 +1665,80 @@ export function manifestViolations(dir, pkg) {
   return violations;
 }
 
+/**
+ * No published package may depend on a workspace sibling in a section
+ * consumers resolve, however the specifier is spelled (#537).
+ *
+ * The protocol rule above is one spelling of this policy caught by side
+ * effect: `workspace:^` in `dependencies` is flagged because pnpm resolving
+ * it does not stop consumers installing it. But since every package publishes
+ * with pnpm (#532), `workspace:^` and a plain `^5.11.2` land in the tarball
+ * as the same installable range — so the likelier spelling, a hand-written
+ * semver range, passed every check while making every consumer of a
+ * four-file codemod CLI install the component library. bestax-migrate's
+ * CLAUDE.md carried "that one is on review" as the only enforcement.
+ *
+ * The rule is blanket over published packages rather than an opt-in set: no
+ * package has a sibling dep in a consumer section today, so nothing is
+ * grandfathered, and a scaffolder or an MCP server has no more business
+ * pulling in the component library than the codemod does. If a package ever
+ * legitimately needs one, that PR adds the exemption — a decision at the
+ * moment it is cheap, the PNPM_PUBLISHED shape.
+ *
+ * peerDependencies are deliberately OUTSIDE the rule, and this departs from
+ * the protocol rule above, which does fire on a `workspace:` peer (with
+ * pin-a-range advice). The departure is the point: a peer on a sibling is
+ * the one section where "consumers install this" is the intended semantic —
+ * a component add-on peering on the core library is a normal thing to want.
+ *
+ * `siblings` (a Map of name → { private }) arrives as an argument because
+ * this function is pure and fixture-driven like manifestViolations, and the
+ * names live in manifests only the async walk can read. It is DERIVED from
+ * the workspace there, never declared — a hardcoded name list is the drift
+ * class #540 closed. Private-ness rides along because the fix differs: a
+ * private sibling cannot become a peerDependency, since no consumer could
+ * resolve it.
+ */
+export function siblingViolations(dir, pkg, siblings) {
+  if (pkg?.private) return [];
+  const violations = [];
+  for (const section of ['dependencies', 'optionalDependencies']) {
+    for (const [name, spec] of Object.entries(pkg?.[section] ?? {})) {
+      // An npm alias installs its TARGET, so `"ui": "npm:@allxsmith/…@^5"`
+      // pulls in the sibling under another key. Compared on the target, or
+      // the rule is bypassable by renaming — review on the PR caught exactly
+      // that hole. The last `@` splits off the range; a scoped name's leading
+      // `@` survives because the slice starts past `npm:`.
+      let target = name;
+      if (typeof spec === 'string' && spec.startsWith('npm:')) {
+        const aliased = spec.slice(4);
+        const at = aliased.lastIndexOf('@');
+        target = at > 0 ? aliased.slice(0, at) : aliased;
+      }
+      const sibling = siblings.get(target);
+      if (!sibling || target === pkg?.name) continue;
+      // A PRIVATE sibling gets different advice: it does not exist on the
+      // registry, so "make it a peerDependency" would leave every consumer
+      // unable to install — the dependency cannot ship in any section
+      // consumers resolve.
+      const fix = sibling.private
+        ? `"${target}" is private and unpublishable, so no consumer could ` +
+          `ever resolve it — this dependency cannot ship at all. Move it ` +
+          `to devDependencies.`
+        : `Move it to devDependencies, or if consumers really must resolve ` +
+          `it, make it a peerDependency with an explicit range.`;
+      violations.push(
+        `${dir}/package.json depends on workspace sibling "${target}" in ` +
+          `${section}${target === name ? '' : ` (aliased as "${name}")`}. ` +
+          `Whatever the specifier says, consumers of ${dir} would be made ` +
+          `to install it and its whole tree — a codemod CLI pulling in the ` +
+          `component library is the case this rule exists for (#537). ${fix}`
+      );
+    }
+  }
+  return violations;
+}
+
 /**
  * Lifecycle hooks that run during a pack or publish, and therefore name scripts
  * whose absence would fail a release rather than CI.
@@ -1734,21 +1813,47 @@ async function checkPublishableManifests() {
     return ['pnpm-workspace.yaml has no `packages:` entries — cannot check.'];
   }
 
+  // First pass: read every manifest once, so the sibling-name set exists
+  // before any package is judged. Derived from the workspace, not declared —
+  // a hardcoded name list is the drift class #540 closed. Private packages'
+  // NAMES still count as siblings (depending on one is broken for consumers
+  // either way), but private packages are not themselves judged.
+  const manifests = [];
   for (const dir of packages) {
-    let pkg;
     try {
-      pkg = JSON.parse(await readFile(join(REPO, dir, 'package.json'), 'utf8'));
+      const pkg = JSON.parse(
+        await readFile(join(REPO, dir, 'package.json'), 'utf8')
+      );
+      // JSON.parse succeeds on `null`, `42`, `"x"` — shapes a truncated write
+      // or merge artifact really produces. Dereferencing one later would
+      // throw a TypeError past the runner's loop and abort every remaining
+      // check with a stack trace, when this exact case has a violation
+      // written for it. Review caught the sibling-map pass doing just that.
+      // An ARRAY passes typeof and instead vanishes silently — no name, so
+      // it never joins the sibling map. Same outcome, quieter road.
+      if (!pkg || typeof pkg !== 'object' || Array.isArray(pkg)) {
+        throw new Error('not an object');
+      }
+      manifests.push({ dir, pkg });
     } catch {
       violations.push(
         `pnpm-workspace.yaml lists "${dir}" but ${dir}/package.json is missing ` +
           `or unparseable.`
       );
-      continue;
     }
+  }
+  const siblings = new Map(
+    manifests
+      .filter(m => m.pkg.name)
+      .map(m => [m.pkg.name, { private: Boolean(m.pkg.private) }])
+  );
+
+  for (const { dir, pkg } of manifests) {
     // The private check lives in manifestViolations, not here, so there is one
     // copy of it. hookScripts still runs for private packages: a broken pack
     // hook is worth reporting whether or not the package publishes.
-    violations.push(...manifestViolations(dir, pkg));
+    violations.push(...manifestViolations(dir, pkg, siblings));
+    violations.push(...siblingViolations(dir, pkg, siblings));
 
     // Naming a script is not the same as shipping it. A hook pointing at a
     // moved path fails during the release rather than in CI, which is the one

scripts/publishable-manifests.test.mjs

@@ -23,6 +23,7 @@ import {
   hookScripts,
   manifestViolations,
   parseWorkspacePackages,
+  siblingViolations,
 } from './check-conformance.mjs';
 import {
   execOptions,
@@ -619,3 +620,160 @@ test('an unresolvable protocol in devDependencies is explained honestly', () =>
   assert.doesNotMatch(v, /no consumer can resolve it/);
   assert.doesNotMatch(v, /plain semver range/);
 });
+
+// --- the sibling-at-runtime rule (#537) --------------------------------------
+//
+// Driven with an explicit name set because the rule is pure and the real set
+// is derived inside the async walk. No published package carries a sibling in
+// a consumer section today, so none of these branches executes on the real
+// tree — the same seam rationale as everything above.
+
+const SIBLINGS = new Map([
+  ['@allxsmith/bestax-bulma', { private: false }],
+  ['create-bestax', { private: false }],
+  ['bestax-migrate', { private: false }],
+  ['bestax-mcp', { private: false }],
+  ['@allxsmith/bestax-docs', { private: true }],
+]);
+
+test('a plain-semver sibling in dependencies is flagged, whatever the range', () => {
+  const v = siblingViolations(
+    'bestax-migrate',
+    {
+      name: 'bestax-migrate',
+      dependencies: { '@allxsmith/bestax-bulma': '^5' },
+    },
+    SIBLINGS
+  );
+  assert.equal(v.length, 1, v.join('\n'));
+  assert.match(v[0], /consumers of bestax-migrate/);
+  assert.match(v[0], /#537/);
+});
+
+test('an optionalDependencies sibling is flagged the same way', () => {
+  const v = siblingViolations(
+    'bestax-mcp',
+    { name: 'bestax-mcp', optionalDependencies: { 'create-bestax': '^4' } },
+    SIBLINGS
+  );
+  assert.equal(v.length, 1);
+});
+
+test('a peer sibling is outside this rule, deliberately', () => {
+  // A departure from the protocol rule, which does fire on a workspace: peer:
+  // peers are the one section where "consumers install this" is the intended
+  // semantic. The protocol rule still polices HOW a peer is spelled.
+  assert.deepEqual(
+    siblingViolations(
+      'bulma-ui',
+      {
+        name: '@allxsmith/bestax-bulma',
+        peerDependencies: { 'bestax-mcp': '^1' },
+      },
+      SIBLINGS
+    ),
+    []
+  );
+});
+
+test('a devDependencies sibling stays legal — it reaches no consumer', () => {
+  assert.deepEqual(
+    siblingViolations(
+      'bestax-migrate',
+      {
+        name: 'bestax-migrate',
+        devDependencies: { '@allxsmith/bestax-bulma': 'workspace:^' },
+      },
+      SIBLINGS
+    ),
+    []
+  );
+});
+
+test('a private package may depend on any sibling it likes', () => {
+  // docs really does dep the library; nobody installs docs from a registry.
+  assert.deepEqual(
+    siblingViolations(
+      'docs',
+      {
+        name: '@allxsmith/bestax-docs',
+        private: true,
+        dependencies: { '@allxsmith/bestax-bulma': 'workspace:*' },
+      },
+      SIBLINGS
+    ),
+    []
+  );
+});
+
+test("a non-sibling dependency is still nobody's business", () => {
+  assert.deepEqual(
+    siblingViolations(
+      'bestax-migrate',
+      { name: 'bestax-migrate', dependencies: { bulma: '^1.0.4' } },
+      SIBLINGS
+    ),
+    []
+  );
+});
+
+test('an npm alias pointing at a sibling is still a sibling', () => {
+  // `"ui": "npm:@allxsmith/bestax-bulma@^5"` installs the sibling under
+  // another key, so a key-only comparison was bypassable by renaming —
+  // review caught the hole. The message names both the target and the alias.
+  const v = siblingViolations(
+    'bestax-migrate',
+    {
+      name: 'bestax-migrate',
+      dependencies: { ui: 'npm:@allxsmith/bestax-bulma@^5' },
+    },
+    SIBLINGS
+  );
+  assert.equal(v.length, 1, v.join('\n'));
+  assert.match(v[0], /@allxsmith\/bestax-bulma/);
+  assert.match(v[0], /aliased as "ui"/);
+});
+
+test('a private sibling is not offered the peerDependency escape', () => {
+  // docs is unpublishable, so "make it a peerDependency" would leave every
+  // consumer unable to install. The advice must not name an impossible fix.
+  const v = siblingViolations(
+    'bestax-migrate',
+    {
+      name: 'bestax-migrate',
+      dependencies: { '@allxsmith/bestax-docs': 'workspace:*' },
+    },
+    SIBLINGS
+  );
+  assert.equal(v.length, 1, v.join('\n'));
+  assert.match(v[0], /private and unpublishable/);
+  assert.doesNotMatch(v[0], /make it a peerDependency/);
+});
+
+test('one violation, one fix: the sibling rule owns a workspace: sibling dep', () => {
+  // Before the dedupe, a `workspace:^` sibling in dependencies drew BOTH the
+  // protocol rule (pin a range) and the sibling rule (move to devDependencies)
+  // — contradictory advice for one defect (#546 review). The sibling rule's
+  // fix is the correct one, so the protocol rule stands down by name; a
+  // `catalog:` entry pointing at an EXTERNAL package is no sibling and stays
+  // protocol-flagged.
+  const siblings = new Map([['@allxsmith/bestax-bulma', { private: false }]]);
+  const pkg = {
+    name: 'bestax-migrate',
+    dependencies: { '@allxsmith/bestax-bulma': 'workspace:^' },
+  };
+  const protocol = manifestViolations('bestax-migrate', pkg, siblings).filter(
+    v => v.includes('bestax-bulma')
+  );
+  const sibling = siblingViolations('bestax-migrate', pkg, siblings);
+  assert.equal(protocol.length, 0);
+  assert.equal(sibling.length, 1);
+  assert.match(sibling[0], /Move it to devDependencies/);
+
+  const external = manifestViolations(
+    'x',
+    { name: 'x', dependencies: { leftpad: 'catalog:' } },
+    siblings
+  );
+  assert.ok(external.some(v => v.includes('leftpad')));
+});