From c5a2bcc516ebabaf61b46bc19f09c51e4a1158bc Mon Sep 17 00:00:00 2001 From: ms-lead-reviewer Date: Wed, 22 Jul 2026 14:24:14 -0500 Subject: [PATCH] fix(mosaic): DI-inject CLI-entry resolver so tests never touch real dist/ (#869 C1 review fix R3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review found the positive-path test's writeFileSync() staged a stub cli.js/index.js at the package's REAL resolved dist/ path, and afterEach only removed files that had NOT pre-existed — never restoring original CONTENT for files that had. On a host with a real pre-built dist/cli.js (ordinary `pnpm build && pnpm test`, and CI: this package's turbo.json overrides the `test` task to depend on `build`, so CI always builds a real dist/cli.js before running vitest), the test would silently overwrite the real ~26KB compiled CLI with an 87-byte stub and still report PASS. Confirmed as the exact root cause of CI1972's red `test` step: src/cli-smoke.spec.ts execs the real dist/cli.js in the same vitest process/run, so the clobber surfaced there. Fix (dependency injection, not snapshot/restore): - defaultCapabilityProbe() now takes an injectable CapabilityProbeDeps ({ resolveCliEntry }), defaulting to the real defaultResolveCliEntry() in production — no change to the real-artifact-read guarantee. - defaultResolveCliEntry() itself now takes an injectable ModuleResolver (defaults to the real require.resolve), so its resolution CHOICE (bare "@mosaicstack/mosaic" specifier vs the buggy "./package.json" subpath) can be tested in complete isolation from real package/build state. - The positive-path test now stages its stub cli.js in an mkdtempSync() scratch directory and injects resolveCliEntry to point there — it never calls the default resolver, so it structurally cannot touch the real package's dist/. It also asserts the real dist/ path's existence is unchanged by the test. - The "returns null when unbuilt" test now injects a resolver pointing at a path that cannot exist, instead of relying on this checkout happening to be unbuilt (deterministic regardless of ambient host build state). Verified: - Reintroduced the R1 bug in defaultResolveCliEntry() and confirmed the new resolver-choice test fails red against it; restored the fix (byte- identical diff against the pre-revert file) and confirmed green. - Built a real dist/cli.js (~26KB) via `turbo run build`, ran the targeted specs against it, then ran the FULL `turbo run test --filter=@mosaicstack/mosaic` CI-parity path (which builds dist/ itself per this package's turbo.json override before vitest runs, exactly matching Woodpecker's `test` step): 77/77 test files, 1451/1451 tests passed, including cli-smoke.spec.ts (22/22) and lease-activation-probe.spec.ts (15/15) in the SAME run. sha256 of dist/cli.js before and after that full run: identical (e61d8de7a2223b6578a2b733edd927707830e011f44b9d20901194c23c4a5272, 26317 bytes) — the real build artifact is untouched byte-for-byte. - python3 -m unittest runtime_tools_unittest: 25/25 pass, including both C-REGRESS-locked fail-closed cases. - typecheck/lint/format:check all pass via turbo --filter=@mosaicstack/mosaic. Co-Authored-By: Claude Opus 4.8 --- .../commands/lease-activation-probe.spec.ts | 124 +++++++++++------- .../src/commands/lease-activation-probe.ts | 56 ++++++-- 2 files changed, 120 insertions(+), 60 deletions(-) diff --git a/packages/mosaic/src/commands/lease-activation-probe.spec.ts b/packages/mosaic/src/commands/lease-activation-probe.spec.ts index 4a7e1f72..8839c6a3 100644 --- a/packages/mosaic/src/commands/lease-activation-probe.spec.ts +++ b/packages/mosaic/src/commands/lease-activation-probe.spec.ts @@ -1,12 +1,14 @@ -import { describe, it, expect, afterEach } from 'vitest'; +import { describe, it, expect } from 'vitest'; import { Command } from 'commander'; -import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; +import { tmpdir } from 'node:os'; import { fileURLToPath } from 'node:url'; import { LEASE_ACTIVATION_CAPABILITY, LEASE_CAPABILITY_PROBE_COMMAND, defaultCapabilityProbe, + defaultResolveCliEntry, defaultSupervisorProbe, leaseEnforcementActivatable, registerLeaseCapabilityProbe, @@ -111,60 +113,84 @@ describe('leaseEnforcementActivatable', () => { describe('defaultCapabilityProbe', () => { it('returns null (fail-closed) when no built CLI artifact is resolvable', () => { - // This source checkout has no dist/cli.js built for @mosaicstack/mosaic, - // so the probe must report "no capability" rather than fabricate one - // from source-tree presence — this is the exact distinction #828's - // version skew needed: source existing is not the same as the published - // artifact advertising the capability. - expect(defaultCapabilityProbe()).toBeNull(); + // Deterministic regardless of ambient host state (e.g. a host that has + // already run `pnpm build`, which would otherwise make this pass or fail + // depending on whether dist/cli.js happens to exist) — inject a resolver + // pointing at a path that cannot exist, rather than relying on this + // checkout being unbuilt. The probe must report "no capability" rather + // than fabricate one from source-tree presence — this is the exact + // distinction #828's version skew needed: source existing is not the + // same as the published artifact advertising the capability. + const result = defaultCapabilityProbe({ + resolveCliEntry: () => '/nonexistent/mosaic-lease-activation-probe-test/cli.js', + }); + expect(result).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'); + describe('positive path — injected resolver, isolated scratch dir (never the real dist/)', () => { + // A prior version of this test staged the stub cli.js at the package's + // REAL resolved dist/ path and relied on afterEach to clean up "only + // what it created" — which meant a host with a real pre-built + // dist/cli.js (ordinary `pnpm build && pnpm test`) would have its real + // ~26KB compiled CLI silently overwritten by an 87-byte stub, with no + // restoration of the original content. That is exactly the kind of + // build-artifact corruption #869 exists to prevent. This version uses + // dependency injection exclusively: defaultCapabilityProbe() is never + // called with its default resolver here, so it can never touch the real + // package dist/ at all — proven below by asserting that path's + // existence is unchanged by the test. + it('returns the real {name, version} capability from a stub cli.js in a temp dir, and leaves the real dist/ untouched', () => { + const packageRoot = join(dirname(fileURLToPath(import.meta.url)), '..', '..'); + const realDistDir = join(packageRoot, 'dist'); + const realDistPreexisted = existsSync(realDistDir); - // 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); + const scratchDir = mkdtempSync(join(tmpdir(), 'mosaic-lease-capability-probe-')); + try { + const scratchCliPath = join(scratchDir, 'cli.js'); + // 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( + scratchCliPath, + `process.stdout.write(JSON.stringify(${JSON.stringify(LEASE_ACTIVATION_CAPABILITY)}));\n`, + ); - afterEach(() => { - if (!cliPreexisted) rmSync(distCliPath, { force: true }); - if (!indexPreexisted) rmSync(distIndexPath, { force: true }); - if (!distDirPreexisted) rmSync(distDir, { recursive: true, force: true }); + const result = defaultCapabilityProbe({ resolveCliEntry: () => scratchCliPath }); + expect(result).toEqual(LEASE_ACTIVATION_CAPABILITY); + + // The real package dist/ must be byte-for-byte untouched: this test + // never invokes the default resolver, so the path's mere existence + // (created or not) must be unchanged by having run this test. + expect(existsSync(realDistDir)).toBe(realDistPreexisted); + } finally { + rmSync(scratchDir, { 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`, - ); +describe('defaultResolveCliEntry', () => { + it('resolves the bare "@mosaicstack/mosaic" specifier (the exported "." entry), never the non-exported "./package.json" subpath', () => { + // Fully isolated from the real filesystem/package state (no dependency + // on whether @mosaicstack/mosaic has been built on this host) via an + // injected fake resolver that mirrors Node's real behavior: the "." + // export resolves fine, but "./package.json" is NOT in package.json's + // `exports` map, so real `require.resolve` throws + // ERR_PACKAGE_PATH_NOT_EXPORTED for it. This is genuinely red-first + // against the reviewer-found bug: the old implementation resolved the + // "./package.json" subpath here, which this fake throws on — the new + // implementation must resolve only the bare specifier. + const requestedSpecifiers: string[] = []; + const fakeResolve = (specifier: string): string => { + requestedSpecifiers.push(specifier); + if (specifier === '@mosaicstack/mosaic') return '/fake/pkg/dist/index.js'; + throw new Error(`ERR_PACKAGE_PATH_NOT_EXPORTED: ${specifier}`); + }; - const result = defaultCapabilityProbe(); - expect(result).toEqual(LEASE_ACTIVATION_CAPABILITY); - }); + const result = defaultResolveCliEntry(fakeResolve); + + expect(result).toBe(join('/fake/pkg/dist', 'cli.js')); + expect(requestedSpecifiers).toEqual(['@mosaicstack/mosaic']); }); }); diff --git a/packages/mosaic/src/commands/lease-activation-probe.ts b/packages/mosaic/src/commands/lease-activation-probe.ts index 02385221..7c634d22 100644 --- a/packages/mosaic/src/commands/lease-activation-probe.ts +++ b/packages/mosaic/src/commands/lease-activation-probe.ts @@ -78,6 +78,46 @@ export function registerLeaseCapabilityProbe(program: Command): void { }); } +/** Injectable Node module resolver — matches `require.resolve`'s signature + * narrowly (specifier in, absolute path out, or throws). Defaults to the + * real `createRequire(import.meta.url).resolve`. Injectable so tests can + * exercise WHICH specifier {@link defaultResolveCliEntry} resolves (the + * reviewer-found bug was resolving the wrong one) without depending on + * whether `@mosaicstack/mosaic` has actually been built on the test host — + * and without ever touching the real package's `dist/` to find out. */ +export type ModuleResolver = (specifier: string) => string; + +/** + * Resolve the CLI's built entrypoint (`dist/cli.js`). Resolves 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. The "." export + * resolves to `dist/index.js`; `cli.js` is its sibling in the same built + * `dist/` directory (see package.json's `bin.mosaic`). + * + * Exported standalone (and injectable via {@link CapabilityProbeDeps}) so + * tests can exercise this resolution logic in isolation, or point + * {@link defaultCapabilityProbe} at a scratch directory instead of ever + * touching the real installed package's `dist/` — a test corrupting a real + * build artifact is exactly the artifact-integrity failure class this card + * exists to prevent (#828). + */ +export function defaultResolveCliEntry( + resolve: ModuleResolver = createRequire(import.meta.url).resolve, +): string { + const mainEntry = resolve('@mosaicstack/mosaic'); + return join(dirname(mainEntry), 'cli.js'); +} + +/** Injectable inputs for {@link defaultCapabilityProbe}. */ +export interface CapabilityProbeDeps { + /** Resolve the CLI entrypoint (`cli.js`) to probe. Defaults to + * {@link defaultResolveCliEntry}. Inject to point at an isolated scratch + * location in tests — never at the real package's `dist/`. */ + resolveCliEntry?: () => string; +} + /** * Real capability lookup. Resolves the installed `@mosaicstack/mosaic` * package's BUILT entrypoint (`dist/cli.js` — the published artifact a user @@ -91,18 +131,12 @@ export function registerLeaseCapabilityProbe(program: Command): void { * did not carry it, so this probe — reading the actually-resolvable built * artifact rather than trusting source-tree presence — would report null. */ -export function defaultCapabilityProbe(): LeaseActivationCapability | null { +export function defaultCapabilityProbe( + deps: CapabilityProbeDeps = {}, +): LeaseActivationCapability | null { try { - const req = createRequire(import.meta.url); - // 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'); + const resolveCliEntry = deps.resolveCliEntry ?? defaultResolveCliEntry; + const cliEntry = resolveCliEntry(); if (!existsSync(cliEntry)) return null; const output = execFileSync(process.execPath, [cliEntry, LEASE_CAPABILITY_PROBE_COMMAND], {