mirror of
https://github.com/zernie/vigiles.git
synced 2026-09-14 20:53:57 +08:00
feat(cli)!: load specs in a vigiles-owned spec host, not a two-loader guess (#178)
Replaces the native-import/npx-tsx pair with ONE loader: a child process that registers vigiles' own module hooks and streams results as NDJSON. The measured bug: a consuming repo without tsx installed, where `npx tsx` went to the registry — >60s against a 15s budget, so all 50 specs failed at once with advice to run `npm run build`, a step that does not exist in a consumer install. Why a redesign and not more guards: the seam between two loaders cannot be made correct. The fallthrough guard had to answer "did the module body already run?" and Node does not expose that bit — ERR_MODULE_NOT_FOUND and SyntaxError each occur both before and during evaluation. The native path also had no time bound, and an in-flight evaluation cannot be cancelled. Three facts made it cheap: typescript is already a runtime dependency and already used at runtime; specs already round-trip through JSON (the old tsx path did exactly that); and the .js -> .ts rewrite is a rule to define, not inherit. Fixes at once: no npx and no network; a stalled spec is killed and NAMED; double evaluation is impossible by construction; resolution no longer varies by Node version or by which loader won; the Windows quoting bug dies with `npx -e`. Nine review findings closed — six by fixes, three by removing the seam. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LVXMw2PJtPPtV8BoTANAd
This commit is contained in:
zernie
Vendored
+7
-1
@@ -101,7 +101,7 @@ declare module "vigiles/generated" {
|
||||
| "internal:check"
|
||||
| "docs:api";
|
||||
|
||||
/** 393 project files. */
|
||||
/** 396 project files. */
|
||||
export type ProjectFile =
|
||||
| "src/CLAUDE.md"
|
||||
| "src/CLAUDE.md.spec.ts"
|
||||
@@ -480,6 +480,9 @@ declare module "vigiles/generated" {
|
||||
| "src/skill-refs.ts"
|
||||
| "src/skill-test.test.ts"
|
||||
| "src/skill-test.ts"
|
||||
| "src/spec-hooks.mts"
|
||||
| "src/spec-host.mts"
|
||||
| "src/spec-loader.test.ts"
|
||||
| "src/stats.test.ts"
|
||||
| "src/stats.ts"
|
||||
| "src/subagent-delivery.test.ts"
|
||||
@@ -945,6 +948,9 @@ declare module "vigiles/spec" {
|
||||
| "src/skill-refs.ts"
|
||||
| "src/skill-test.test.ts"
|
||||
| "src/skill-test.ts"
|
||||
| "src/spec-hooks.mts"
|
||||
| "src/spec-host.mts"
|
||||
| "src/spec-loader.test.ts"
|
||||
| "src/stats.test.ts"
|
||||
| "src/stats.ts"
|
||||
| "src/subagent-delivery.test.ts"
|
||||
|
||||
+206
-57
@@ -32,6 +32,7 @@ import {
|
||||
isAbsolute,
|
||||
sep as pathSep,
|
||||
} from "node:path";
|
||||
import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process";
|
||||
import { globSync } from "glob";
|
||||
import { generateTypes } from "./core/generate-types.js";
|
||||
import {
|
||||
@@ -370,65 +371,215 @@ function findSpecs(pattern?: string): string[] {
|
||||
|
||||
type AnySpec = ClaudeSpec | SkillSpec | AgentSpec | Railway;
|
||||
|
||||
/**
|
||||
* Why the last `loadSpec()` returned null.
|
||||
*
|
||||
* Kept as module state rather than a widened return type: `loadSpec` has six
|
||||
* call sites and only one of them reports to a human.
|
||||
*/
|
||||
let lastSpecLoadFailure: string | null = null;
|
||||
|
||||
/** Reason the most recent `loadSpec()` returned null, or null if it succeeded. */
|
||||
export function specLoadFailureReason(): string | null {
|
||||
return lastSpecLoadFailure;
|
||||
}
|
||||
|
||||
/**
|
||||
* How long one spec may take to evaluate before the host is killed.
|
||||
*
|
||||
* Overridable because 15s is a guess that fits the specs we have seen, not a
|
||||
* law; a repo with genuinely slow specs should be able to raise it rather than
|
||||
* discover the number by hitting it.
|
||||
*/
|
||||
const SPEC_DEADLINE_MS = Number(process.env.VIGILES_SPEC_TIMEOUT_MS) || 15_000;
|
||||
|
||||
type HostReply =
|
||||
| { path: string; phase: "start" }
|
||||
| { path: string; ok: true; value: AnySpec }
|
||||
| { path: string; ok: false; error: string };
|
||||
|
||||
type Settle = (reply: HostReply | "timeout" | "died") => void;
|
||||
|
||||
type Host = {
|
||||
child: ChildProcessWithoutNullStreams;
|
||||
/**
|
||||
* Outstanding requests keyed BY PATH.
|
||||
*
|
||||
* 🔴 This was a single slot until a `--trace-warnings` run showed
|
||||
* `checkCoverageThresholds` calling `loadSpec` through `Array.map`, i.e.
|
||||
* concurrently. A single slot is overwritten by each new caller, so a reply
|
||||
* settles whichever request happened to be last — `loadSpec` could return
|
||||
* ANOTHER spec's value for the path it was asked about.
|
||||
*
|
||||
* ⚠️ **Honest scope of that claim.** The mispairing is wrong by construction,
|
||||
* but it is NOT observable today: the one concurrent caller aggregates and
|
||||
* never asks which spec it got. Measured — the same `vigiles lint` run with a
|
||||
* last-wins dispatch produces byte-identical output. So this is a latent
|
||||
* defect closed before it had consequences, not a bug anyone hit; the visible
|
||||
* symptom was only the MaxListeners warning. I could not construct an
|
||||
* end-to-end test that fails without keying by path, and the test beside this
|
||||
* file says so rather than implying otherwise. The next concurrent caller
|
||||
* that DOES care about identity is the one this protects.
|
||||
*/
|
||||
pending: Map<string, Settle>;
|
||||
/** Last spec the host said it had STARTED — the culprit when a deadline fires. */
|
||||
started: string | null;
|
||||
buffered: string;
|
||||
};
|
||||
|
||||
let host: Host | null = null;
|
||||
|
||||
/** The compiled host entry, beside this file in `dist/`. */
|
||||
function hostEntry(): string {
|
||||
return resolve(__dirname, "spec-host.mjs");
|
||||
}
|
||||
|
||||
function startHost(): Host {
|
||||
const child = spawn(process.execPath, [hostEntry()], {
|
||||
cwd: process.cwd(),
|
||||
stdio: ["pipe", "pipe", "pipe"],
|
||||
});
|
||||
const h: Host = { child, pending: new Map(), started: null, buffered: "" };
|
||||
|
||||
child.stdout.setEncoding("utf-8");
|
||||
child.stdout.on("data", (chunk: string) => {
|
||||
h.buffered += chunk;
|
||||
let nl: number;
|
||||
while ((nl = h.buffered.indexOf("\n")) >= 0) {
|
||||
const line = h.buffered.slice(0, nl).trim();
|
||||
h.buffered = h.buffered.slice(nl + 1);
|
||||
if (!line) continue;
|
||||
let reply: HostReply;
|
||||
try {
|
||||
reply = JSON.parse(line) as HostReply;
|
||||
} catch {
|
||||
continue; // not ours; a spec writing to stdout cannot corrupt the stream
|
||||
}
|
||||
if ("phase" in reply) {
|
||||
h.started = reply.path;
|
||||
continue;
|
||||
}
|
||||
const done = h.pending.get(reply.path);
|
||||
h.pending.delete(reply.path);
|
||||
done?.(reply);
|
||||
}
|
||||
});
|
||||
|
||||
// Anything the child says on stderr is the spec's own noise; keep it out of
|
||||
// our stdout so `--json` consumers are not corrupted, but do not lose it.
|
||||
child.stderr.setEncoding("utf-8");
|
||||
child.stderr.on("data", (chunk: string) => process.stderr.write(chunk));
|
||||
|
||||
// 🔴 Unreferenced, or the CLI never exits. A piped child and its three
|
||||
// streams each hold the event loop open, so `compile` finished its work and
|
||||
// then hung forever waiting on a host that had nothing left to say. The
|
||||
// in-flight deadline timer keeps the loop alive while a request is pending,
|
||||
// which is exactly as long as we need it.
|
||||
// ONE exit listener per host, not one per request: with concurrent callers the
|
||||
// per-request version added a listener each time and Node warned at eleven.
|
||||
// It fails every outstanding request, because a dead host answers none of them.
|
||||
child.once("exit", () => {
|
||||
const waiting = [...h.pending.values()];
|
||||
h.pending.clear();
|
||||
for (const settle of waiting) settle("died");
|
||||
});
|
||||
|
||||
// The stdio types are Readable/Writable, which do not declare `unref` — the
|
||||
// objects are pipes and do have it. Optional-called so this stays correct if
|
||||
// a platform ever hands back a stream that genuinely lacks it.
|
||||
const unref = (s: unknown) => (s as { unref?: () => void })?.unref?.();
|
||||
child.unref();
|
||||
unref(child.stdin);
|
||||
unref(child.stdout);
|
||||
unref(child.stderr);
|
||||
|
||||
return h;
|
||||
}
|
||||
|
||||
/**
|
||||
* Kill the host and forget it; the next request starts a fresh one.
|
||||
*
|
||||
* Outstanding requests are failed rather than dropped: a killed host will never
|
||||
* answer them, and a promise nobody settles is a hang wearing a different hat.
|
||||
*/
|
||||
function dropHost(): void {
|
||||
if (!host) return;
|
||||
const dying = host;
|
||||
host = null;
|
||||
const waiting = [...dying.pending.values()];
|
||||
dying.pending.clear();
|
||||
dying.child.kill("SIGKILL");
|
||||
for (const settle of waiting) settle("died");
|
||||
}
|
||||
|
||||
process.on("exit", dropHost);
|
||||
|
||||
/**
|
||||
* Load one spec in the spec host.
|
||||
*
|
||||
* 🔴 **Why a child process rather than `import()` here.** A module evaluation
|
||||
* cannot be cancelled once started — `Promise.race` hands control back but the
|
||||
* evaluation keeps running and holds the event loop — so an in-process loader
|
||||
* gives a stalled spec an unbounded hang in `compile`, `test` and `audit`. It
|
||||
* also cannot tell whether a failed spec already ran (Node reports
|
||||
* `ERR_MODULE_NOT_FOUND` and `SyntaxError` both before and during evaluation),
|
||||
* which is what made the previous two-loader arrangement unfixable rather than
|
||||
* merely buggy: it had to guess whether re-running was safe.
|
||||
*
|
||||
* The host is spawned with `process.execPath` — never `npx` — so nothing is
|
||||
* fetched and nothing needs installing.
|
||||
*/
|
||||
async function loadSpec(specPath: string): Promise<AnySpec | null> {
|
||||
const fullPath = resolve(process.cwd(), specPath);
|
||||
lastSpecLoadFailure = null;
|
||||
|
||||
// Try multiple dist/ path strategies
|
||||
const candidates: string[] = [];
|
||||
|
||||
// src/ → dist/ mapping (e.g., src/CLAUDE.md.spec.ts → dist/CLAUDE.md.spec.js)
|
||||
if (fullPath.includes("/src/")) {
|
||||
candidates.push(
|
||||
fullPath.replace(/\/src\//, "/dist/").replace(/\.ts$/, ".js"),
|
||||
);
|
||||
}
|
||||
|
||||
// Root-level spec → dist/ (e.g., CLAUDE.md.spec.ts → dist/CLAUDE.md.spec.js)
|
||||
const dir = fullPath.substring(0, fullPath.lastIndexOf("/"));
|
||||
const base = fullPath.substring(fullPath.lastIndexOf("/") + 1);
|
||||
candidates.push(resolve(dir, "dist", base.replace(/\.ts$/, ".js")));
|
||||
|
||||
// examples/ → dist/examples/ mapping
|
||||
candidates.push(
|
||||
fullPath
|
||||
.replace(/\.ts$/, ".js")
|
||||
.replace(process.cwd(), resolve(process.cwd(), "dist")),
|
||||
);
|
||||
|
||||
for (const distPath of candidates) {
|
||||
if (existsSync(distPath)) {
|
||||
try {
|
||||
const mod = (await import(distPath)) as {
|
||||
default: AnySpec | { default: AnySpec };
|
||||
};
|
||||
// CJS double-default: `{ default: { default: spec } }`.
|
||||
const raw = mod.default;
|
||||
if (raw && typeof raw === "object" && "default" in raw) {
|
||||
return (raw as { default: AnySpec }).default;
|
||||
}
|
||||
return raw;
|
||||
} catch {
|
||||
// Try next candidate
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Try loading .ts directly via tsx
|
||||
try {
|
||||
const { execSync } =
|
||||
require("node:child_process") as typeof import("node:child_process");
|
||||
// Handle ESM/CJS double-default: m.default may itself have a .default
|
||||
const script = `import(${JSON.stringify(fullPath)}).then(m => { const d = m.default?.default ?? m.default; console.log(JSON.stringify(d)); })`;
|
||||
const output = execSync(`npx tsx -e '${script.replace(/'/g, "'\\''")}'`, {
|
||||
encoding: "utf-8",
|
||||
cwd: process.cwd(),
|
||||
stdio: ["pipe", "pipe", "pipe"],
|
||||
timeout: 15000,
|
||||
});
|
||||
return JSON.parse(output.trim()) as AnySpec;
|
||||
} catch {
|
||||
if (!existsSync(fullPath)) {
|
||||
lastSpecLoadFailure = `no such file: ${specPath}`;
|
||||
return null;
|
||||
}
|
||||
|
||||
host ??= startHost();
|
||||
const h = host;
|
||||
|
||||
const reply = await new Promise<HostReply | "timeout" | "died">((done) => {
|
||||
let settled = false;
|
||||
const finish: Settle = (r) => {
|
||||
if (settled) return;
|
||||
settled = true;
|
||||
clearTimeout(timer);
|
||||
h.pending.delete(fullPath);
|
||||
done(r);
|
||||
};
|
||||
const timer = setTimeout(() => {
|
||||
finish("timeout");
|
||||
}, SPEC_DEADLINE_MS);
|
||||
h.pending.set(fullPath, finish);
|
||||
h.child.stdin.write(JSON.stringify({ path: fullPath }) + "\n");
|
||||
});
|
||||
|
||||
if (reply === "timeout") {
|
||||
// The host's last `start` names the spec that stalled. Without it a hang
|
||||
// produced N identical failures and no culprit.
|
||||
const culprit = h.started ?? fullPath;
|
||||
dropHost();
|
||||
lastSpecLoadFailure =
|
||||
`evaluating ${relative(process.cwd(), culprit)} exceeded ` +
|
||||
`${SPEC_DEADLINE_MS}ms and was killed. Set VIGILES_SPEC_TIMEOUT_MS to ` +
|
||||
`raise the limit, or look for a top-level await that never settles.`;
|
||||
return null;
|
||||
}
|
||||
if (reply === "died") {
|
||||
dropHost();
|
||||
lastSpecLoadFailure = "the spec host exited unexpectedly.";
|
||||
return null;
|
||||
}
|
||||
if (!("ok" in reply) || !reply.ok) {
|
||||
lastSpecLoadFailure = `the spec did not load. ${
|
||||
"error" in reply ? reply.error : "no reason given"
|
||||
}`;
|
||||
return null;
|
||||
}
|
||||
return reply.value;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -673,9 +824,7 @@ async function compile(
|
||||
const spec = await loadSpec(specPath);
|
||||
if (!spec) {
|
||||
console.log(`\n✗ ${specPath} — failed to load`);
|
||||
console.log(
|
||||
` Ensure the spec is compiled: run \`npm run build\` first.`,
|
||||
);
|
||||
console.log(` ${specLoadFailureReason() ?? "reason unavailable"}`);
|
||||
allValid = false;
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -0,0 +1,95 @@
|
||||
/**
|
||||
* Module customization hooks for the spec host — vigiles' OWN loader for `.ts`
|
||||
* specs, replacing "whichever loader happens to be installed".
|
||||
*
|
||||
* Why vigiles owns this rather than shelling to `tsx`:
|
||||
*
|
||||
* - **No install, no network.** `typescript` is already a runtime dependency
|
||||
* of this package (`dependencies`, and `core/compile-generator.ts` uses it),
|
||||
* so `ts.transpileModule` costs nothing extra. The bug that started this
|
||||
* work was a consuming repo without `tsx`, where `npx tsx` went to the
|
||||
* registry and every one of 50 specs blew a 15s budget.
|
||||
* - **One resolution contract.** Before this, a spec's module resolution
|
||||
* depended on the user's Node version and on which loader won — so a spec
|
||||
* could load locally and fail in CI under different rules. A tool that
|
||||
* audits other tools for that kind of quiet divergence should not have it.
|
||||
*
|
||||
* Scope is deliberately small and documented as such: `.ts`/`.mts` sources, the
|
||||
* `./x.js` → `./x.ts` specifier rewrite, and bare specifiers. NOT tsconfig
|
||||
* `paths`, JSX, or decorator configuration — specs are configuration modules,
|
||||
* not applications.
|
||||
*/
|
||||
import { existsSync, readFileSync } from "node:fs";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import ts from "typescript";
|
||||
|
||||
type ResolveContext = { parentURL?: string; conditions: string[] };
|
||||
type Resolved = { url: string; format?: string | null; shortCircuit?: boolean };
|
||||
type NextResolve = (
|
||||
specifier: string,
|
||||
context: ResolveContext,
|
||||
) => Resolved | Promise<Resolved>;
|
||||
|
||||
type LoadContext = { format?: string | null; conditions: string[] };
|
||||
type Loaded = {
|
||||
format: string;
|
||||
source?: string | ArrayBuffer;
|
||||
shortCircuit?: boolean;
|
||||
};
|
||||
type NextLoad = (url: string, context: LoadContext) => Loaded | Promise<Loaded>;
|
||||
|
||||
const TS_SOURCE = /\.m?ts$/;
|
||||
|
||||
/**
|
||||
* `./x.js` → `./x.ts` when the sibling exists.
|
||||
*
|
||||
* This is the TypeScript ESM convention (`tsc` under `nodenext` requires the
|
||||
* `.js` extension in the source), which `tsx` implements and native Node does
|
||||
* not. It is the ONE divergence that matters in practice: this repository's own
|
||||
* dogfood specs import `src/core/spec.js`, a file that does not exist on disk.
|
||||
* Attempted only AFTER normal resolution fails, so it can never shadow a real
|
||||
* `.js` file.
|
||||
*/
|
||||
export async function resolve(
|
||||
specifier: string,
|
||||
context: ResolveContext,
|
||||
nextResolve: NextResolve,
|
||||
): Promise<Resolved> {
|
||||
try {
|
||||
return await nextResolve(specifier, context);
|
||||
} catch (err) {
|
||||
if (specifier.endsWith(".js") && context.parentURL) {
|
||||
const candidate = new URL(
|
||||
specifier.slice(0, -3) + ".ts",
|
||||
context.parentURL,
|
||||
);
|
||||
if (existsSync(fileURLToPath(candidate))) {
|
||||
return { url: candidate.href, format: "module", shortCircuit: true };
|
||||
}
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
/** Transpile `.ts`/`.mts` with the TypeScript this package already ships. */
|
||||
export async function load(
|
||||
url: string,
|
||||
context: LoadContext,
|
||||
nextLoad: NextLoad,
|
||||
): Promise<Loaded> {
|
||||
if (!TS_SOURCE.test(new URL(url).pathname)) return nextLoad(url, context);
|
||||
|
||||
const fileName = fileURLToPath(url);
|
||||
const { outputText } = ts.transpileModule(readFileSync(fileName, "utf-8"), {
|
||||
fileName,
|
||||
compilerOptions: {
|
||||
module: ts.ModuleKind.ESNext,
|
||||
target: ts.ScriptTarget.ES2022,
|
||||
// Erasing types is the whole job; anything that changes SEMANTICS is not
|
||||
// ours to decide for a spec.
|
||||
verbatimModuleSyntax: false,
|
||||
isolatedModules: true,
|
||||
},
|
||||
});
|
||||
return { format: "module", source: outputText, shortCircuit: true };
|
||||
}
|
||||
@@ -0,0 +1,84 @@
|
||||
/**
|
||||
* The spec host — a child process that loads specs and streams results as NDJSON.
|
||||
*
|
||||
* ONE host per CLI command, not one per spec: Node startup and the TypeScript
|
||||
* load are paid once, then each spec costs a transpile.
|
||||
*
|
||||
* 🔴 **Why a child process at all, when `import()` works in-process.** Because a
|
||||
* module evaluation cannot be cancelled once started. `Promise.race` returns
|
||||
* control to the caller but the evaluation keeps running and holds the event
|
||||
* loop, so a spec that stalls at top level hangs `compile`, `test` and `audit`
|
||||
* with no bound. A child can be killed. That is the entire argument, and it is
|
||||
* why the in-process loader this replaced could not be repaired: it also had to
|
||||
* answer "did the module body already run?" to know whether re-running was
|
||||
* safe, and Node does not expose that bit — `ERR_MODULE_NOT_FOUND` and
|
||||
* `SyntaxError` each occur both before and during evaluation.
|
||||
*
|
||||
* Protocol, one JSON object per line each way:
|
||||
* in {"path":"<abs path to spec>"}
|
||||
* out {"path":"…","phase":"start"} — emitted BEFORE evaluation
|
||||
* out {"path":"…","ok":true,"value":{…}}
|
||||
* out {"path":"…","ok":false,"error":"…"}
|
||||
*
|
||||
* The `start` line is what makes a hang diagnosable: when the parent's deadline
|
||||
* fires, the last `start` without a result NAMES the spec that stalled. Before
|
||||
* this, a stalled load produced N identical failures and no culprit.
|
||||
*
|
||||
* Values cross as JSON, which is not a new constraint — the previous `npx tsx`
|
||||
* path already did `JSON.stringify` in the child and `JSON.parse` in the parent,
|
||||
* so every spec that has ever loaded survived this round trip. Spec types carry
|
||||
* no functions; TypeScript is the authoring layer, the value is data.
|
||||
*/
|
||||
import { register } from "node:module";
|
||||
import { pathToFileURL } from "node:url";
|
||||
|
||||
register(new URL("./spec-hooks.mjs", import.meta.url));
|
||||
|
||||
function say(line: unknown): void {
|
||||
process.stdout.write(JSON.stringify(line) + "\n");
|
||||
}
|
||||
|
||||
async function loadOne(path: string): Promise<void> {
|
||||
say({ path, phase: "start" });
|
||||
try {
|
||||
const mod = (await import(pathToFileURL(path).href)) as {
|
||||
default?: unknown;
|
||||
};
|
||||
// CJS interop can nest the default one level deeper.
|
||||
const raw = mod.default as { default?: unknown } | undefined;
|
||||
const value =
|
||||
raw && typeof raw === "object" && "default" in raw ? raw.default : raw;
|
||||
if (value === undefined) {
|
||||
say({ path, ok: false, error: "the spec has no default export." });
|
||||
return;
|
||||
}
|
||||
say({ path, ok: true, value });
|
||||
} catch (err) {
|
||||
say({
|
||||
path,
|
||||
ok: false,
|
||||
error: err instanceof Error ? (err.stack ?? err.message) : String(err),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// Requests are serialised: a spec may depend on module state a previous one set
|
||||
// up, and interleaving would make a hang impossible to attribute.
|
||||
let queue: Promise<void> = Promise.resolve();
|
||||
let buffered = "";
|
||||
|
||||
process.stdin.setEncoding("utf-8");
|
||||
process.stdin.on("data", (chunk: string) => {
|
||||
buffered += chunk;
|
||||
let nl: number;
|
||||
while ((nl = buffered.indexOf("\n")) >= 0) {
|
||||
const line = buffered.slice(0, nl).trim();
|
||||
buffered = buffered.slice(nl + 1);
|
||||
if (!line) continue;
|
||||
const { path } = JSON.parse(line) as { path: string };
|
||||
queue = queue.then(() => loadOne(path));
|
||||
}
|
||||
});
|
||||
process.stdin.on("end", () => {
|
||||
queue.then(() => process.exit(0));
|
||||
});
|
||||
@@ -0,0 +1,309 @@
|
||||
/**
|
||||
* The spec host, end to end — driving the REAL built CLI the way a user does.
|
||||
*
|
||||
* WHAT THIS REPLACED, and why the shape of these tests changed. `loadSpec` used
|
||||
* to try a native `import()` and fall back to `execSync("npx tsx …")`. Two
|
||||
* defects made that arrangement unfixable rather than merely buggy:
|
||||
*
|
||||
* 1. a spec that threw was evaluated TWICE — once natively, once by the
|
||||
* fallback — so any side effect before the throw happened twice. Guarding
|
||||
* it requires answering "did the module body run?", and Node does not
|
||||
* expose that: `ERR_MODULE_NOT_FOUND` and `SyntaxError` both occur before
|
||||
* AND during evaluation;
|
||||
* 2. the native path had NO time bound. An in-flight module evaluation cannot
|
||||
* be cancelled — `Promise.race` returns control but the evaluation keeps
|
||||
* running — so a stalled spec hung `compile`, `test` and `audit` forever.
|
||||
*
|
||||
* One host process fixes both by construction: one loader means nothing to
|
||||
* re-run, and a child can be killed. The tests below assert exactly those two
|
||||
* properties, plus the resolution contract vigiles now owns.
|
||||
*
|
||||
* The measured bug that started all of it: a consuming repo without `tsx`, where
|
||||
* `npx tsx` went to the registry — `npx tsx -e 'console.log(1)'` took >60s
|
||||
* against a 15s budget, so all 50 specs failed at once with advice to run
|
||||
* `npm run build`, a step that does not exist in a consumer install.
|
||||
*
|
||||
* Deterministic, model-free, offline → free unit tier.
|
||||
*/
|
||||
import { describe, it, beforeAll, afterAll } from "vitest";
|
||||
import assert from "node:assert/strict";
|
||||
import {
|
||||
mkdtempSync,
|
||||
mkdirSync,
|
||||
writeFileSync,
|
||||
readFileSync,
|
||||
rmSync,
|
||||
} from "node:fs";
|
||||
import { join, resolve, dirname } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { execSync, spawnSync } from "node:child_process";
|
||||
|
||||
// NB: `__dirname`, not `import.meta` — this package builds to CommonJS and tsc
|
||||
// rejects import.meta here (TS1470). The idiom is legal in scripts/ next door,
|
||||
// which is outside the tsc project; copying it into src/ does not compile.
|
||||
const ROOT = resolve(__dirname, "..");
|
||||
const CLI = resolve(ROOT, "dist", "cli.js");
|
||||
|
||||
let dir: string;
|
||||
|
||||
function run(
|
||||
cwd: string,
|
||||
env: NodeJS.ProcessEnv = process.env,
|
||||
cmd = "compile",
|
||||
): { out: string; code: number } {
|
||||
// 🔴 spawnSync, not execSync: on SUCCESS execSync returns stdout only, so a
|
||||
// warning printed to stderr is invisible to assertions. That silently made
|
||||
// the listener-leak test below pass under a mutation that reinstated the leak
|
||||
// — a green mutation is a finding about the TEST, not proof of the fix.
|
||||
const r = spawnSync("node", [CLI, cmd], {
|
||||
cwd,
|
||||
encoding: "utf-8",
|
||||
env,
|
||||
timeout: 120_000,
|
||||
});
|
||||
return { out: (r.stdout ?? "") + (r.stderr ?? ""), code: r.status ?? 1 };
|
||||
}
|
||||
|
||||
/** Write a spec at `<dir>/.claude/skills/<name>/SKILL.md.spec.ts`. */
|
||||
function skill(name: string, body: string): string {
|
||||
const at = join(dir, ".claude", "skills", name, "SKILL.md.spec.ts");
|
||||
mkdirSync(dirname(at), { recursive: true });
|
||||
writeFileSync(at, body);
|
||||
return dirname(at);
|
||||
}
|
||||
|
||||
function validSpec(name: string, extra = ""): string {
|
||||
return (
|
||||
`import { experimental_skill } from "vigiles/spec";\n` +
|
||||
extra +
|
||||
`export default experimental_skill({\n` +
|
||||
` name: ${JSON.stringify(name)},\n` +
|
||||
` description: "A fixture skill. Use when testing the spec loader.",\n` +
|
||||
` body: "# ${name}\\n\\nFixture body.\\n",\n` +
|
||||
`});\n`
|
||||
);
|
||||
}
|
||||
|
||||
beforeAll(() => {
|
||||
dir = mkdtempSync(join(tmpdir(), "vigiles-spec-host-"));
|
||||
mkdirSync(join(dir, "node_modules"), { recursive: true });
|
||||
execSync(
|
||||
`ln -s ${JSON.stringify(ROOT)} ${JSON.stringify(join(dir, "node_modules", "vigiles"))}`,
|
||||
);
|
||||
writeFileSync(
|
||||
join(dir, "package.json"),
|
||||
JSON.stringify({ name: "fixture", type: "module", private: true }, null, 2),
|
||||
);
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
if (dir) rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
describe("spec host", () => {
|
||||
it("compiles a spec with npm and npx REMOVED FROM PATH, on any Node", () => {
|
||||
// The load-bearing assertion. The original bug was `npx tsx` reaching for
|
||||
// the network; the host spawns `process.execPath` and transpiles with the
|
||||
// TypeScript already in `dependencies`, so nothing is fetched.
|
||||
//
|
||||
// Note what is NOT here any more: a Node-version gate. The previous design
|
||||
// needed native type stripping (22.18+), so this test had to skip on the
|
||||
// Node 20 that this repo's own CI runs. vigiles now owns the transpile, so
|
||||
// the assertion holds on every supported runtime.
|
||||
const at = skill("alpha", validSpec("alpha"));
|
||||
|
||||
const shadow = join(dir, "no-npx");
|
||||
mkdirSync(shadow, { recursive: true });
|
||||
for (const bin of ["npx", "npm"]) {
|
||||
const f = join(shadow, bin);
|
||||
writeFileSync(
|
||||
f,
|
||||
`#!/bin/sh\necho "${bin} is unavailable in this test" >&2\nexit 127\n`,
|
||||
);
|
||||
execSync(`chmod +x ${JSON.stringify(f)}`);
|
||||
}
|
||||
|
||||
const { out } = run(dir, {
|
||||
...process.env,
|
||||
PATH: `${shadow}:${process.env.PATH ?? ""}`,
|
||||
});
|
||||
assert.match(
|
||||
out,
|
||||
/✓ .*alpha\/SKILL\.md\.spec\.ts/,
|
||||
`compile must succeed without npx on PATH, got:\n${out}`,
|
||||
);
|
||||
assert.doesNotMatch(out, /failed to load/, `no spec should fail:\n${out}`);
|
||||
rmSync(at, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("resolves a `./x.js` import to the `.ts` beside it", () => {
|
||||
// The one resolution rule vigiles deliberately owns: `tsc` under nodenext
|
||||
// requires the `.js` extension in source, so a spec that imports a local
|
||||
// helper writes `./helper.js` while only `./helper.ts` exists. Native Node
|
||||
// does not do this rewrite; tsx does. Now it is OUR documented contract
|
||||
// instead of a property of whichever loader happened to be installed.
|
||||
const at = skill(
|
||||
"sibling",
|
||||
validSpec("sibling", `import "./helper.js";\n`),
|
||||
);
|
||||
writeFileSync(join(at, "helper.ts"), "export const unused: number = 1;\n");
|
||||
|
||||
const { out } = run(dir);
|
||||
assert.match(
|
||||
out,
|
||||
/✓ .*sibling\/SKILL\.md\.spec\.ts/,
|
||||
`the .js -> .ts rewrite must resolve:\n${out}`,
|
||||
);
|
||||
rmSync(at, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("evaluates a throwing spec EXACTLY ONCE", () => {
|
||||
// With one loader there is nothing to re-run. Under the old two-loader
|
||||
// arrangement this reported «ran 2 time(s)».
|
||||
const marks = join(dir, "side-effects.log");
|
||||
const at = skill(
|
||||
"sideeffect",
|
||||
`import { appendFileSync } from "node:fs";\n` +
|
||||
`appendFileSync(${JSON.stringify(marks)}, "ran\\n");\n` +
|
||||
`throw new Error("spec blew up after its side effect");\n`,
|
||||
);
|
||||
|
||||
const { out } = run(dir);
|
||||
const ran = readFileSync(marks, "utf-8").trim().split("\n").length;
|
||||
assert.equal(ran, 1, `the spec must run once, ran ${ran} time(s):\n${out}`);
|
||||
assert.match(
|
||||
out,
|
||||
/spec blew up after its side effect/,
|
||||
`and its own error must be reported:\n${out}`,
|
||||
);
|
||||
rmSync(at, { recursive: true, force: true });
|
||||
rmSync(marks, { force: true });
|
||||
});
|
||||
|
||||
it("KILLS a spec that stalls, and names it", () => {
|
||||
// This test could not exist before. The native path had no bound, and an
|
||||
// in-flight evaluation cannot be cancelled, so a spec like this one hung
|
||||
// compile forever. The host is killable, and its `start` line tells the
|
||||
// parent WHICH spec stalled — previously a hang gave N identical failures
|
||||
// and no culprit.
|
||||
const at = skill(
|
||||
"stalls",
|
||||
`await new Promise(() => {}); // never settles\n` + validSpec("stalls"),
|
||||
);
|
||||
|
||||
const started = Date.now();
|
||||
const { out } = run(dir, {
|
||||
...process.env,
|
||||
VIGILES_SPEC_TIMEOUT_MS: "3000",
|
||||
});
|
||||
const elapsed = Date.now() - started;
|
||||
|
||||
assert.ok(
|
||||
elapsed < 60_000,
|
||||
`compile must not hang; took ${elapsed}ms:\n${out}`,
|
||||
);
|
||||
assert.match(
|
||||
out,
|
||||
/exceeded 3000ms/,
|
||||
`the deadline must be reported:\n${out}`,
|
||||
);
|
||||
assert.match(
|
||||
out,
|
||||
/stalls\/SKILL\.md\.spec\.ts/,
|
||||
`the STALLED spec must be named, not just 'a spec':\n${out}`,
|
||||
);
|
||||
rmSync(at, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("reports a spec with no default export, without re-running it", () => {
|
||||
const at = skill("nodefault", `export const notDefault = 1;\n`);
|
||||
const { out } = run(dir);
|
||||
assert.match(out, /no default export/, `got:\n${out}`);
|
||||
rmSync(at, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("pairs CONCURRENT loads by path, and leaks no listeners", () => {
|
||||
// 🔴 Two defects, one cause, and the second is the serious one.
|
||||
//
|
||||
// `--trace-warnings` on `vigiles lint` showed checkCoverageThresholds
|
||||
// calling loadSpec through Array.map — CONCURRENTLY. The client held a
|
||||
// single `pending` slot, so each new caller overwrote the previous one and
|
||||
// a reply settled whichever request happened to be last: loadSpec could
|
||||
// return ANOTHER spec's value for the path it was asked about. Silent
|
||||
// mispairing, in a loader three commands depend on.
|
||||
//
|
||||
// The MaxListeners warning was only the visible symptom of the same
|
||||
// per-request wiring. Node warns at ten, so a fixture loading specs ONE AT
|
||||
// A TIME can never see either problem — which is exactly why the first six
|
||||
// tests here did not.
|
||||
//
|
||||
// `compile` iterates serially; `lint` is the concurrent path. This asserts
|
||||
// on both: lint for the concurrency, compile for the pairing.
|
||||
const made: string[] = [];
|
||||
for (let i = 0; i < 12; i++) {
|
||||
made.push(skill(`many${i}`, validSpec(`many${i}`)));
|
||||
}
|
||||
|
||||
// Compile first so every SKILL.md exists and matches its spec…
|
||||
const { out } = run(dir);
|
||||
assert.doesNotMatch(out, /failed to load/, `all 12 must load:\n${out}`);
|
||||
|
||||
// …then lint, which loads them CONCURRENTLY and verifies each compiled file
|
||||
// against the spec it came from. That hash check is what makes mispairing
|
||||
// observable end to end: hand a path another spec's value and the file no
|
||||
// longer matches. `compile` alone cannot show this — it loads serially, so
|
||||
// "the last pending request" and "the right one" are the same thing there.
|
||||
const lint = run(dir, process.env, "lint");
|
||||
assert.doesNotMatch(
|
||||
lint.out,
|
||||
/MaxListenersExceededWarning/,
|
||||
`concurrent loads must not add a listener each:\n${lint.out}`,
|
||||
);
|
||||
// ⚠️ What this pairing check does and does NOT establish. It asserts the
|
||||
// name loaded from each spec is reported beside the path it came from, and
|
||||
// it passes on the serial `compile` path. It does NOT fail when dispatch is
|
||||
// mutated to last-wins: measured, both this fixture and a real `vigiles
|
||||
// lint` on the vigiles repo produce byte-identical output either way,
|
||||
// because the one concurrent caller aggregates and never asks which spec it
|
||||
// got. Keying by path is correct by construction; this is the honest limit
|
||||
// of the coverage, recorded rather than papered over.
|
||||
for (let i = 0; i < 12; i++) {
|
||||
assert.match(
|
||||
lint.out,
|
||||
new RegExp(`"many${i}" \\(\\.claude/skills/many${i}/SKILL\\.md\\)`),
|
||||
`many${i}'s name must be reported against its OWN path:\n${lint.out}`,
|
||||
);
|
||||
}
|
||||
assert.doesNotMatch(out, /failed to load/, `all 12 must load:\n${out}`);
|
||||
// Each compiled file must carry ITS OWN body. A mispaired reply shows up
|
||||
// here as one skill's markdown written under another's name.
|
||||
for (let i = 0; i < 12; i++) {
|
||||
const md = readFileSync(join(made[i], "SKILL.md"), "utf-8");
|
||||
assert.match(
|
||||
md,
|
||||
new RegExp(`many${i}\\b`),
|
||||
`many${i}/SKILL.md must contain its own spec, not another's:\n${md}`,
|
||||
);
|
||||
}
|
||||
for (const at of made) rmSync(at, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("never advises `npm run build` when a spec fails to load", () => {
|
||||
// The retired message: it named a build step that does not exist in a
|
||||
// consumer install, and it was printed for EVERY failure because the real
|
||||
// reason had been erased by a bare `catch`.
|
||||
const at = skill("broken", `this is not valid typescript(((\n`);
|
||||
const { out } = run(dir);
|
||||
assert.match(
|
||||
out,
|
||||
/failed to load/,
|
||||
`the broken spec must be reported:\n${out}`,
|
||||
);
|
||||
assert.doesNotMatch(
|
||||
out,
|
||||
/npm run build/,
|
||||
`the retired misleading advice must not come back:\n${out}`,
|
||||
);
|
||||
rmSync(at, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user