fix(mosaic): DI-inject CLI-entry resolver so tests never touch real dist/ (#869 C1 review fix R3)
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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']);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -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], {
|
||||
|
||||
Reference in New Issue
Block a user