From 43f64932b7f3d429e99fbf3817070daed86544da Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 11 Nov 2026 20:02:32 -0800 Subject: [PATCH] test: stop the src/node_modules race between platform-binary and the source walk platform-binary.test.ts planted a fake binary at packages/cli/src/node_modules/.bin to exercise the resolver's node_modules/.bin tier, while api-deprecated-paths.test.ts walked packages/cli/src with a recursive readdir. Run in parallel, the walk could list src/node_modules and then hit ENOENT descending into it after the other test removed it. - platform-binary: relocate the resolver's own module path to a temp directory via a passthrough mock of fileURLToPath, so its `..`/node_modules/.bin computation is still what finds the binary and nothing is written under src/. - api-deprecated-paths: walk by hand, prune node_modules and generated/ before descending, and skip directories or files that vanish mid-walk. Closes #694 --- .../cli/test/api-deprecated-paths.test.ts | 80 ++++++++++++++----- packages/cli/test/platform-binary.test.ts | 51 +++++++++--- 2 files changed, 100 insertions(+), 31 deletions(-) diff --git a/packages/cli/test/api-deprecated-paths.test.ts b/packages/cli/test/api-deprecated-paths.test.ts index b6b02cbb..3f4a6a6a 100644 --- a/packages/cli/test/api-deprecated-paths.test.ts +++ b/packages/cli/test/api-deprecated-paths.test.ts @@ -1,5 +1,5 @@ import { readFile, readdir } from "node:fs/promises"; -import { join, dirname, relative, sep } from "node:path"; +import { join, dirname, relative } from "node:path"; import { fileURLToPath } from "node:url"; import { describe, it, expect } from "vitest"; @@ -48,23 +48,63 @@ function resourcePrefix(path: string): string { return prefix.replace(/\/+$/, ""); } +/** Whether `error` is a filesystem "no such file or directory". */ +function isMissing(error: unknown): boolean { + return (error as NodeJS.ErrnoException | undefined)?.code === "ENOENT"; +} + +/** + * Every `.ts` file under `directory`, walked by hand rather than with + * `readdir({ recursive: true })` so that two kinds of directory can be pruned + * before the walk descends into them: + * + * - `node_modules`, which is never source. A recursive `readdir` cannot skip + * it, only filter its contents afterwards, and it lists a directory and then + * descends into it as separate steps. When a parallel test created and + * removed `src/node_modules` between those steps, the walk threw `ENOENT` + * (taskless/cli#694). + * - `generated/`, the schema's own transcript: it necessarily names every + * path, deprecated ones included, and is not a call site. Matched at any + * depth, because that reasoning is about what a generated directory IS and + * does not depend on where it sits. + * + * A directory that vanishes between being listed and being read is skipped + * rather than failing the walk: something that no longer exists cannot + * reference a deprecated path. + */ async function typeScriptSources(directory: string): Promise { - const entries = await readdir(directory, { - recursive: true, - withFileTypes: true, - }); - return entries - .filter((entry) => entry.isFile() && entry.name.endsWith(".ts")) - .map((entry) => join(entry.parentPath, entry.name)) - .filter( - // `generated/` is the schema's own transcript: it necessarily names every - // path, deprecated ones included, and is not a call site. Matched at any - // depth, because that reasoning is about what a generated directory IS - // and does not depend on where it sits. `startsWith` reached only a - // top-level `src/generated`, which is a no-op today and would have - // reported a nested one as a wall of false offenders. - (file) => !relative(directory, file).split(sep).includes("generated") - ); + let entries; + try { + entries = await readdir(directory, { withFileTypes: true }); + } catch (error) { + if (isMissing(error)) return []; + throw error; + } + + const files: string[] = []; + for (const entry of entries) { + const path = join(directory, entry.name); + if (entry.isDirectory()) { + if (entry.name === "node_modules" || entry.name === "generated") continue; + files.push(...(await typeScriptSources(path))); + } else if (entry.isFile() && entry.name.endsWith(".ts")) { + files.push(path); + } + } + return files; +} + +/** + * The file's contents, or `undefined` when it was removed after the walk + * listed it, for the same reason the walk tolerates a vanished directory. + */ +async function readSource(file: string): Promise { + try { + return await readFile(file, "utf8"); + } catch (error) { + if (isMissing(error)) return undefined; + throw error; + } } describe("deprecated API paths", () => { @@ -88,7 +128,8 @@ describe("deprecated API paths", () => { const offenders: string[] = []; for (const file of await typeScriptSources(SOURCE_ROOT)) { - const source = await readFile(file, "utf8"); + const source = await readSource(file); + if (source === undefined) continue; for (const prefix of deprecated) { // The negative lookahead keeps `/cli/api/rule` from matching // `/cli/api/rule-hash-vectors`, a live path that merely shares a stem. @@ -122,7 +163,8 @@ describe("v1 API routes", () => { const pattern = /["'`}]\/cli\/api\/(?!v2\/)/; const offenders: string[] = []; for (const file of await typeScriptSources(SOURCE_ROOT)) { - const source = await readFile(file, "utf8"); + const source = await readSource(file); + if (source === undefined) continue; for (const [index, line] of source.split("\n").entries()) { if (pattern.test(line)) { offenders.push(`${relative(SOURCE_ROOT, file)}:${String(index + 1)}`); diff --git a/packages/cli/test/platform-binary.test.ts b/packages/cli/test/platform-binary.test.ts index dce92d48..1f26efbd 100644 --- a/packages/cli/test/platform-binary.test.ts +++ b/packages/cli/test/platform-binary.test.ts @@ -9,7 +9,7 @@ import { import { tmpdir } from "node:os"; import { join } from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { AST_GREP_BINARY } from "../src/rules/ast-grep-binary"; import { @@ -20,6 +20,36 @@ import { } from "../src/rules/platform-binary"; import { VALE_BINARY } from "../src/rules/vale/binary"; +/** + * Where the resolver believes its own module lives, when a test relocates it. + * + * `resolvePlatformBinary` finds its `node_modules/.bin` tier relative to its + * own file (`fileURLToPath(import.meta.url)`). Under vitest that file is + * `src/rules/platform-binary.ts`, so planting a binary at the real location + * meant creating `src/node_modules` inside the source tree, where a parallel + * test walking `src/` could list it and then hit ENOENT once it was removed + * (taskless/cli#694). Relocating the module path instead keeps the resolver's + * own `..`/`node_modules/.bin` computation under test while every file the + * test creates lives in a temp directory. + * + * Every other call passes straight through to the real `fileURLToPath`. + */ +const relocation = vi.hoisted(() => ({ + modulePath: undefined as string | undefined, +})); + +vi.mock("node:url", async (importOriginal) => { + const actual = await importOriginal(); + const fileURLToPath: typeof actual.fileURLToPath = (url, options) => { + const path = actual.fileURLToPath(url, options); + return relocation.modulePath !== undefined && + /[/\\]rules[/\\]platform-binary\.ts$/.test(path) + ? relocation.modulePath + : path; + }; + return { ...actual, default: { ...actual, fileURLToPath }, fileURLToPath }; +}); + /** * The two engines' packages are named differently, and the difference is not * cosmetic: ast-grep publishes a libc/toolchain suffix and Vale does not. @@ -207,18 +237,14 @@ describe("candidate order", () => { onUnix( "reports node_modules/.bin as the source when that tier answers", () => { - // Mirrors the computation `resolvePlatformBinary` uses internally - // (relative to the compiled module's own directory) — this is the exact - // location that tier searches, not a stand-in for it. - const localBin = join( - import.meta.dirname, - "..", - "src", - "node_modules", - ".bin" - ); + // Pretend the resolver's module sits at `/rules/platform-binary.ts`, + // the same depth as `src/rules/platform-binary.ts`, so the tier it + // computes is `/node_modules/.bin`. Nothing is written under `src/`. + const root = mkdtempSync(join(tmpdir(), "candidate-order-bin-")); + workspaces.push(root); + relocation.modulePath = join(root, "rules", "platform-binary.ts"); + const localBin = join(root, "node_modules", ".bin"); mkdirSync(localBin, { recursive: true }); - workspaces.push(join(import.meta.dirname, "..", "src", "node_modules")); const binaryPath = join(localBin, "sg"); writeFileSync(binaryPath, "#!/bin/sh\necho 'fake-tool 1.0.0'\n"); chmodSync(binaryPath, 0o755); @@ -233,6 +259,7 @@ describe("candidate order", () => { expect(resolution.source).toBe("node_modules/.bin"); } finally { process.env.PATH = originalPath; + relocation.modulePath = undefined; } } );