fix(mosaic): resolve lease-activation CLI via exported "." entry, not "./package.json" (#869 C1 review fix)
Some checks failed
ci/woodpecker/pr/ci Pipeline failed
Some checks failed
ci/woodpecker/pr/ci Pipeline failed
Independent review of PR #870 found defaultCapabilityProbe() resolved the installed CLI via req.resolve('@mosaicstack/mosaic/package.json') — a subpath NOT present in package.json's `exports` map. Node throws ERR_PACKAGE_PATH_NOT_EXPORTED for that on every real install, which the catch-all silently turned into an always-null probe: leaseEnforcementActivatable() could never return true anywhere, defeating the card's purpose. Fix: resolve via the already-exported "." entry instead (require.resolve('@mosaicstack/mosaic') -> dist/index.js), then take cli.js as its sibling in the same built dist/ directory — matching package.json's bin.mosaic mapping. No change to the exports map itself (that would mask a separate, out-of-scope pre-existing issue in resolveTool()/launch.ts per review guidance). Adds a positive-path test that stages a realistic fake dist/index.js + dist/cli.js at the package's real resolved location and asserts defaultCapabilityProbe() returns the actual {name, version} capability object with zero mocking of the resolver. Verified red against the prior (reverted-and-restored) buggy resolution before landing the fix, and green after — the previous only-unmocked test asserted null for the wrong reason (masking this bug) and is left in place alongside the new one. Re-verified: launch.spec.ts (30), fail-closed-regression.spec.ts (2), and runtime_tools_unittest.py (25, including both C-REGRESS-locked fail-closed cases) all still green. typecheck/lint/format:check pass via `turbo run ... --filter=@mosaicstack/mosaic`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -1,5 +1,8 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { describe, it, expect, afterEach } from 'vitest';
|
||||
import { Command } from 'commander';
|
||||
import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import {
|
||||
LEASE_ACTIVATION_CAPABILITY,
|
||||
LEASE_CAPABILITY_PROBE_COMMAND,
|
||||
@@ -115,6 +118,54 @@ describe('defaultCapabilityProbe', () => {
|
||||
// artifact advertising the capability.
|
||||
expect(defaultCapabilityProbe()).toBeNull();
|
||||
});
|
||||
|
||||
describe('positive path — a real built dist/cli.js advertises the capability', () => {
|
||||
// Resolve the ACTUAL @mosaicstack/mosaic package root on disk (two levels
|
||||
// up from src/commands/), so this test exercises the exact same
|
||||
// require.resolve('@mosaicstack/mosaic') self-reference codepath
|
||||
// defaultCapabilityProbe() itself uses — no mocking of the resolver.
|
||||
// This is the regression proof for the reviewer-found bug: the previous
|
||||
// implementation resolved via the NOT-exported './package.json' subpath
|
||||
// (ERR_PACKAGE_PATH_NOT_EXPORTED on every real install), which the
|
||||
// catch-all silently turned into an always-null probe. That bug returns
|
||||
// null here regardless of a built dist/cli.js being present — so this
|
||||
// test fails red against it and only passes once resolution goes through
|
||||
// the package's already-exported "." entry.
|
||||
const mosaicRoot = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||
const distDir = join(mosaicRoot, 'dist');
|
||||
const distIndexPath = join(distDir, 'index.js');
|
||||
const distCliPath = join(distDir, 'cli.js');
|
||||
|
||||
// dist/ is gitignored and unbuilt in a fresh checkout; only remove what
|
||||
// THIS test created, never a real build that predates it.
|
||||
const distDirPreexisted = existsSync(distDir);
|
||||
const indexPreexisted = existsSync(distIndexPath);
|
||||
const cliPreexisted = existsSync(distCliPath);
|
||||
|
||||
afterEach(() => {
|
||||
if (!cliPreexisted) rmSync(distCliPath, { force: true });
|
||||
if (!indexPreexisted) rmSync(distIndexPath, { force: true });
|
||||
if (!distDirPreexisted) rmSync(distDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('returns the real {name, version} capability object', () => {
|
||||
mkdirSync(distDir, { recursive: true });
|
||||
// Minimal stand-in for the built "." export target — only needs to
|
||||
// exist for require.resolve('@mosaicstack/mosaic') to succeed; its
|
||||
// content is never loaded by defaultCapabilityProbe().
|
||||
writeFileSync(distIndexPath, 'export {};\n');
|
||||
// Minimal stand-in for the built CLI's hidden __lease-capability
|
||||
// subcommand — prints exactly what registerLeaseCapabilityProbe()
|
||||
// wires the real `mosaic __lease-capability` command to print.
|
||||
writeFileSync(
|
||||
distCliPath,
|
||||
`process.stdout.write(JSON.stringify(${JSON.stringify(LEASE_ACTIVATION_CAPABILITY)}));\n`,
|
||||
);
|
||||
|
||||
const result = defaultCapabilityProbe();
|
||||
expect(result).toEqual(LEASE_ACTIVATION_CAPABILITY);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('defaultSupervisorProbe', () => {
|
||||
|
||||
@@ -94,8 +94,15 @@ export function registerLeaseCapabilityProbe(program: Command): void {
|
||||
export function defaultCapabilityProbe(): LeaseActivationCapability | null {
|
||||
try {
|
||||
const req = createRequire(import.meta.url);
|
||||
const pkgJsonPath = req.resolve('@mosaicstack/mosaic/package.json');
|
||||
const cliEntry = join(dirname(pkgJsonPath), 'dist', 'cli.js');
|
||||
// Resolve via the package's "." export (already present in package.json's
|
||||
// `exports` map) rather than a "./package.json" subpath — the latter is
|
||||
// NOT exported, so `require.resolve('@mosaicstack/mosaic/package.json')`
|
||||
// throws ERR_PACKAGE_PATH_NOT_EXPORTED on every real install, which the
|
||||
// catch below would silently turn into an always-null probe. The "."
|
||||
// export resolves to `dist/index.js`; `cli.js` is its sibling in the same
|
||||
// built `dist/` directory (see package.json's `bin.mosaic`).
|
||||
const mainEntry = req.resolve('@mosaicstack/mosaic');
|
||||
const cliEntry = join(dirname(mainEntry), 'cli.js');
|
||||
if (!existsSync(cliEntry)) return null;
|
||||
|
||||
const output = execFileSync(process.execPath, [cliEntry, LEASE_CAPABILITY_PROBE_COMMAND], {
|
||||
|
||||
Reference in New Issue
Block a user