From 0e05598c9ae0a403e5bdd9744221f1a278131d58 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 26 Oct 2026 09:15:26 -0700 Subject: [PATCH 1/3] feat(rules): hand ast-grep a rules-only tree, and add the fixture converter migration 12 will use --- packages/cli/src/rules/assemble.ts | 104 ++-- packages/cli/src/rules/engines.ts | 2 + packages/cli/src/rules/fixture-convert.ts | 413 ++++++++++++++++ packages/cli/test/assemble.test.ts | 155 ++++-- .../cli/test/ast-grep-vendor-contract.test.ts | 453 +++++++++--------- packages/cli/test/fixture-convert.test.ts | 360 ++++++++++++++ .../cli/test/vale-vendor-contract.test.ts | 97 ++++ 7 files changed, 1273 insertions(+), 311 deletions(-) create mode 100644 packages/cli/src/rules/fixture-convert.ts create mode 100644 packages/cli/test/fixture-convert.test.ts diff --git a/packages/cli/src/rules/assemble.ts b/packages/cli/src/rules/assemble.ts index 5e5a455c..f268311e 100644 --- a/packages/cli/src/rules/assemble.ts +++ b/packages/cli/src/rules/assemble.ts @@ -1,12 +1,20 @@ -import { mkdir, readFile, stat, writeFile } from "node:fs/promises"; -import { dirname, join, posix, relative, sep } from "node:path"; +import { + copyFile, + mkdir, + readFile, + rm, + stat, + writeFile, +} from "node:fs/promises"; +import { basename, dirname, join, posix, relative, sep } from "node:path"; import { ASSEMBLED_SG_CONFIG, + ASSEMBLED_SG_RULES, ASSEMBLED_VALE_CONFIG, listRuleIds, ruleConfigPath, - ruleTestsDirectory, + ruleFilePath, } from "./engines"; import { validateValeRuleConfig } from "../schemas/vale-config"; import type { RuleViolation } from "./constraints"; @@ -54,11 +62,11 @@ function toPosix(path: string): string { return path.split(sep).join(posix.sep); } -/** Whether `path` is a directory on disk. */ -async function isDirectory(path: string): Promise { +/** Whether `path` is a regular file on disk. */ +async function isFile(path: string): Promise { try { const stats = await stat(path); - return stats.isDirectory(); + return stats.isFile(); } catch { return false; } @@ -132,6 +140,35 @@ export interface ValeConfigRefusal { rejections: RuleViolation[]; } +/** + * A Vale config enabling one rule under its own matchers, and nothing else. + * + * What `verify` runs a Vale case under: the rule's `.vale.ini`, block for + * block what `check` assembles for it, behind a header whose `StylesPath` is + * absolute because the config is written outside the project. Not + * `assembleValeConfig({ ruleIds })`, which validates every rule and refuses + * the whole assembly on any rejection, deliberately, so `check --rule` cannot + * report findings an unfiltered `check` never would. Under `verify` that + * would let another rule's broken config fail this rule's cases. + * + * `undefined` when the schema rejects the config, which `validate` reports + * before `verify` ever gets here. + */ +export function buildOneRuleValeConfig( + stylesPath: string, + ruleId: string, + source: string +): string | undefined { + const verdict = validateValeRuleConfig(ruleId, source); + if (verdict.rejections.length > 0) return undefined; + return [ + `StylesPath = ${toPosix(stylesPath)}`, + "MinAlertLevel = suggestion", + "", + valeRuleBlock(ruleId, source, verdict.breadcrumbLines), + ].join("\n"); +} + /** * What `assembleValeConfig` produced when every config passed the schema: * where to point `--config`, the section patterns it wrote there, and what the @@ -282,27 +319,24 @@ export async function assembleValeConfig( } /** - * Assemble `.taskless/.sgconfig.yml`. + * Assemble `.taskless/.sgconfig.yml`, over a rules-only copy of the tree. * - * `ruleDirs` names the ast-grep rules tree, which ast-grep walks recursively — - * that recursion is why tests live in `.tests/` rather than `tests/`, since - * every `.yml` it reaches is parsed as a rule. + * ast-grep walks `ruleDirs` recursively and parses every `.yml` it reaches as + * a rule, so it is never pointed at `.taskless/rules/sg/` itself: a rule's + * `tests/` holds fixture files, and the first `.yml` fixture would fail the + * whole scan with `Cannot parse rule`. Measured on 0.51.0. Each rule's + * `.yml` is copied into `.taskless/.sg-rules/` instead, and that is what + * `ruleDirs` names, so what a rule directory may contain is the CLI's + * business rather than a property of ast-grep's discovery. * - * `testConfigs` gets one entry per rule, because each rule keeps its tests - * inside its own directory. Sorted with the rule ids, so the file is stable. + * Copied rather than linked: symlinks need privileges on Windows, and a rule + * file is a few hundred bytes. The tree is cleared first, so a rule deleted + * since the last run cannot keep reporting from a stale copy. `check` assembles + * against its snapshot, so there the tree lands inside the run directory; it + * is gitignored for any other caller. * - * A rule whose `.tests/` is not on disk is left out of `testConfigs` - * entirely. **ast-grep 0.41.0 treats a missing `testDir` as fatal to the whole - * invocation** — `Cannot read rule directory ...`, exit 6 — and `--filter` does - * not scope that away, so emitting the entry regardless would let one rule fail - * every other rule's test run, with an error naming a rule its author never - * touched. Migration `0005` now creates the directory, but that does not make - * this check redundant: `runMigrations` short-circuits once the manifest reads - * version 5, so a project a nightly already stamped never re-runs the amended - * migration, and a hand-made `mkdir .taskless/rules/sg//` reaches the same - * state on any version. Nothing becomes a silent pass — `validate` still reports - * "No test file found" and `verify` still reports "Skipped: no test file found", - * both of which read the rule directory rather than this config. + * No `testConfigs`. Fixtures are no longer run by `sg test`: `verify` runs + * each case through this same scan (see `rules/sg-fixtures.ts`). */ export async function assembleSgConfig( cwd: string @@ -310,22 +344,20 @@ export async function assembleSgConfig( const ruleIds = await listRuleIds(cwd, "sg"); if (ruleIds.length === 0) return undefined; - const rulesDirectory = tasklessRelative(RULES_DIRECTORY, "sg"); - const candidates = ruleIds.map((ruleId) => - ruleTestsDirectory(cwd, "sg", ruleId) - ); - const present = await Promise.all( - candidates.map((path) => isDirectory(path)) + const tree = join(cwd, ASSEMBLED_SG_RULES); + await rm(tree, { recursive: true, force: true }); + await mkdir(tree, { recursive: true }); + await Promise.all( + ruleIds.map(async (ruleId) => { + const source = ruleFilePath(cwd, "sg", ruleId); + if (!(await isFile(source))) return; + await copyFile(source, join(tree, basename(source))); + }) ); - const testDirectories = candidates - .filter((_, index) => present[index]) - .map((path) => toPosix(relative(join(cwd, ".taskless"), path))); const contents = [ "ruleDirs:", - ` - ${rulesDirectory}`, - "testConfigs:", - ...testDirectories.map((directory) => ` - testDir: ${directory}`), + ` - ${toPosix(relative(dirname(join(cwd, ASSEMBLED_SG_CONFIG)), tree))}`, "", ].join("\n"); diff --git a/packages/cli/src/rules/engines.ts b/packages/cli/src/rules/engines.ts index 314efbcf..ff70317d 100644 --- a/packages/cli/src/rules/engines.ts +++ b/packages/cli/src/rules/engines.ts @@ -85,6 +85,8 @@ export function ruleCapturesDirectory( */ export const ASSEMBLED_VALE_CONFIG = ".taskless/.vale.ini"; export const ASSEMBLED_SG_CONFIG = ".taskless/.sgconfig.yml"; +/** The rules-only copy of the `sg` tree that {@link ASSEMBLED_SG_CONFIG} names. */ +export const ASSEMBLED_SG_RULES = ".taskless/.sg-rules"; /** One engine's disposition for this run. */ export interface EngineDispatch { diff --git a/packages/cli/src/rules/fixture-convert.ts b/packages/cli/src/rules/fixture-convert.ts new file mode 100644 index 00000000..e208e83c --- /dev/null +++ b/packages/cli/src/rules/fixture-convert.ts @@ -0,0 +1,413 @@ +import { matchesGlob, posix } from "node:path"; + +import { parseDocument } from "yaml"; + +import { resolveAstGrepLanguage, type AstGrepLanguage } from "./capabilities"; +import type { FixtureBucket } from "./fixtures"; +import { parseValeRuleConfig } from "../schemas/vale-config"; + +/** + * Converting fixtures from the legacy `.tests/` layout into case directories. + * + * Shared by migration `12`, which converts a project's rules once, and by + * delivery, which reads a served set in the legacy layout and converts it + * before writing, so the service is never required to move. Pure: it takes + * file contents and returns file contents, and touches no filesystem. + * + * A converted fixture has to sit at a path the rule's scope accepts, because + * a case is judged by the engine's own check with the case as its root. An + * ast-grep snippet used to be scanned over stdin, which bypasses `files:`, so + * a snippet placed anywhere else would turn into a quiet `fail/` case the + * moment it was migrated. The path is derived from the scope's own patterns + * and then checked against them with `node:path`'s `matchesGlob`. That dialect + * is close to ast-grep's `globset` and to Vale's matcher globs but is not + * identical, so the check catches a derivation mistake rather than proving the + * engine agrees. The next `verify` is the proof, and it names a scope that + * excluded a case. + */ + +/** + * The file extension a snippet in each ast-grep language is written with. + * + * Keyed on {@link AstGrepLanguage}, so the type checker refuses a table that + * misses a language, and that list is itself pinned by set-equality against + * the binary in `ast-grep-vendor-contract.test.ts`. Each extension is one + * ast-grep parses as that language by default, which the same test checks by + * scanning a file of each. + */ +export const AST_GREP_LANGUAGE_EXTENSIONS: Readonly< + Record +> = { + Bash: "sh", + C: "c", + Cpp: "cpp", + CSharp: "cs", + Css: "css", + Dart: "dart", + Elixir: "ex", + Go: "go", + Haskell: "hs", + Hcl: "hcl", + Html: "html", + Java: "java", + JavaScript: "js", + Json: "json", + Kotlin: "kt", + Lua: "lua", + Markdown: "md", + Nix: "nix", + Php: "php", + Python: "py", + Ruby: "rb", + Rust: "rs", + Scala: "scala", + Solidity: "sol", + Swift: "swift", + Tsx: "tsx", + TypeScript: "ts", + Yaml: "yml", + Zig: "zig", +}; + +/** A rule's scope: the globs that admit a file, and the globs that exclude one. */ +export interface FixtureScope { + include: string[]; + exclude: string[]; +} + +/** The first alternative of the first `{a,b}` group, preferring one ending in `.`. */ +function expandFirstBrace(glob: string, extension: string): string { + const match = /\{([^{}]*)\}/.exec(glob); + if (match?.[1] === undefined) return glob; + const alternatives = match[1].split(","); + const preferred = + alternatives.find((alternative) => + `${glob.slice(0, match.index)}${alternative}`.endsWith(`.${extension}`) + ) ?? + alternatives.find((alternative) => alternative === extension) ?? + alternatives[0] ?? + ""; + return expandFirstBrace( + `${glob.slice(0, match.index)}${preferred}${glob.slice(match.index + match[0].length)}`, + extension + ); +} + +/** A concrete path for one glob, standing in `stem` for every wildcard. */ +function pathFromGlob(glob: string, stem: string, extension: string): string { + let path = expandFirstBrace(glob.replace(/^\.\//, ""), extension); + // A trailing `/**` admits anything beneath, so a file goes beneath it. + if (path.endsWith("/**")) path = `${path.slice(0, -2)}${stem}.${extension}`; + if (path === "**") path = `${stem}.${extension}`; + path = path + .replaceAll("**/", "") + .replaceAll("**", stem) + .replaceAll("*", stem) + .replaceAll("?", "x") + .replaceAll(/\[!?([^\]])[^\]]*\]/g, "$1"); + return path; +} + +/** Whether `path` is admitted by `scope`, in `node:path`'s glob dialect. */ +function admitted(path: string, scope: FixtureScope): boolean { + const included = + scope.include.length === 0 || + scope.include.some((glob) => matchesGlob(path, glob)); + return included && !scope.exclude.some((glob) => matchesGlob(path, glob)); +} + +/** + * A path for a fixture file named `stem.extension` that `scope` admits, or + * `undefined` when none of the derivations could be checked. + */ +export function deriveFixturePath( + scope: FixtureScope, + stem: string, + extension: string +): string | undefined { + const candidates = + scope.include.length === 0 + ? [`${stem}.${extension}`] + : scope.include.map((glob) => pathFromGlob(glob, stem, extension)); + return candidates.find( + (candidate) => + candidate !== "" && + !candidate.startsWith("/") && + !candidate.split("/").includes("..") && + admitted(candidate, scope) + ); +} + +/** The globs out of a `files:`/`ignores:` value, in either shape ast-grep accepts. */ +function globList(value: unknown): string[] { + if (!Array.isArray(value)) return []; + return value + .map((entry): unknown => + typeof entry === "object" && entry !== null && "glob" in entry + ? (entry as { glob: unknown }).glob + : entry + ) + .filter((glob): glob is string => typeof glob === "string"); +} + +/** Whether a parsed YAML value is a mapping, not a scalar or a list. */ +function isMapping(value: unknown): boolean { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** One converted fixture file, at a path relative to the rule directory. */ +export interface ConvertedFile { + path: string; + content: string; +} + +/** What one conversion produced, and what it had to leave alone. */ +export interface Conversion { + files: ConvertedFile[]; + /** Why something was left unconverted, one line each, naming the source. */ + unconverted: string[]; +} + +/** + * Convert one ast-grep test YAML into case directories. + * + * One case per `valid:` (pass) or `invalid:` (fail) snippet, named by bucket + * and ordinal in source order, since a snippet carries no description to name + * it by. `rule` is the rule file's content, which supplies `language:` and the + * scope. `name` is the test file's name, for messages. + * + * Unconverted (and so left in place by the caller) when the test file does not + * parse, the language has no extension, or no admitted path can be derived. + * All or nothing per test file, so a partly converted file never strands half + * its snippets. + */ +export function convertSgTestFile( + rule: string, + name: string, + content: string, + testsDirectory: string +): Conversion { + const unconvertible = (reason: string): Conversion => ({ + files: [], + unconverted: [`${name}: ${reason}`], + }); + + // `parseDocument` collects syntax errors rather than throwing, so a + // half-parsed file has to be refused here: converting what did parse would + // turn `invalid: [unterminated` into a case holding `unterminated`. + const ruleParsed = parseDocument(rule); + const testParsed = parseDocument(content); + if (ruleParsed.errors.length > 0 || testParsed.errors.length > 0) { + return unconvertible("the rule or the test file is not valid YAML"); + } + const ruleDocument: unknown = ruleParsed.toJS(); + const testDocument: unknown = testParsed.toJS(); + if (!isMapping(ruleDocument)) { + return unconvertible("the rule file is not a YAML mapping"); + } + if (!isMapping(testDocument)) { + return unconvertible("the test file is not a YAML mapping"); + } + const ruleRecord = ruleDocument as Record; + const test = testDocument as Record; + + const language = + typeof ruleRecord.language === "string" + ? resolveAstGrepLanguage(ruleRecord.language) + : undefined; + if (language === undefined) { + return unconvertible( + `language ${JSON.stringify(ruleRecord.language)} has no file extension to write a snippet with` + ); + } + const extension = AST_GREP_LANGUAGE_EXTENSIONS[language]; + const scope: FixtureScope = { + include: globList(ruleRecord.files), + exclude: globList(ruleRecord.ignores), + }; + + const files: ConvertedFile[] = []; + const buckets: [FixtureBucket, unknown][] = [ + ["pass", test.valid], + ["fail", test.invalid], + ]; + for (const [bucket, snippets] of buckets) { + if (snippets === undefined) continue; + if (!Array.isArray(snippets)) { + return unconvertible( + `its ${bucket === "pass" ? "valid" : "invalid"}: key is not a list` + ); + } + for (const [index, snippet] of snippets.entries()) { + if (typeof snippet !== "string") { + return unconvertible( + `a ${bucket === "pass" ? "valid" : "invalid"}: entry is not a string` + ); + } + const caseName = `${bucket}-${String(index + 1).padStart(2, "0")}`; + const path = deriveFixturePath(scope, caseName, extension); + if (path === undefined) { + return unconvertible( + `no path could be derived that the rule's files:/ignores: admit` + ); + } + files.push({ + path: posix.join(testsDirectory, bucket, caseName, path), + content: snippet.endsWith("\n") ? snippet : `${snippet}\n`, + }); + } + } + return { files, unconverted: [] }; +} + +/** + * The scope a Vale rule's `.vale.ini` declares: every matcher assigning + * `. = YES` admits, every one assigning `NO` excludes. + * + * Positional precedence is flattened, deliberately. A path is admitted when + * some enabling matcher matches it and no disabling one does, which is + * stricter than Vale and so never derives a path Vale would exclude. + */ +export function valeFixtureScope(ruleId: string, config: string): FixtureScope { + const key = `${ruleId}.${ruleId}`; + const scope: FixtureScope = { include: [], exclude: [] }; + for (const section of parseValeRuleConfig(config).sections) { + if (section.name === undefined) continue; + for (const node of section.nodes) { + if (node.kind !== "property" || node.key !== key) continue; + const value = node.value.trim().toUpperCase(); + if (value === "YES") scope.include.push(section.name); + if (value === "NO") scope.exclude.push(section.name); + } + } + return scope; +} + +/** + * Convert one legacy Vale fixture document, `.tests//`, into a + * case directory named for the document's stem. + * + * The document keeps its own name and extension, placed at a path the rule's + * enabling matchers admit. Unconverted when no such path can be derived. A + * rule with no enabling matcher cannot have one, and `validate` reports that + * rule as off before it reports anything about its fixtures. + */ +export function convertValeDocument( + scope: FixtureScope, + bucket: FixtureBucket, + name: string, + content: string, + testsDirectory: string +): Conversion { + const dot = name.lastIndexOf("."); + const stem = dot <= 0 ? name : name.slice(0, dot); + const extension = dot <= 0 ? "" : name.slice(dot + 1); + const derived = + extension === "" ? undefined : deriveFixturePath(scope, stem, extension); + if (derived === undefined) { + return { + files: [], + unconverted: [ + `${bucket}/${name}: no path could be derived that the rule's .vale.ini matchers admit`, + ], + }; + } + return { + files: [ + { path: posix.join(testsDirectory, bucket, stem, derived), content }, + ], + unconverted: [], + }; +} + +/** A served file, as delivery carries it: a rule-relative path and its bytes. */ +export interface LegacyFile { + path: string; + content: string; +} + +/** + * A served file set with any fixtures in the legacy `.tests/` layout rewritten + * into case directories under `tests/`. + * + * What lets delivery read a service that has not moved, permanently rather + * than as a transition step: every file outside `.tests/` passes through + * byte-identical, and a set already in the new layout is returned unchanged. + * Fixtures carry no signature, so nothing this rewrites is a signed byte. + * + * A fixture that cannot be placed keeps its bytes and moves to the same path + * under `tests/`, so the CLI still never writes `.tests/`. `verify` then names + * it, and `unconverted` says why, for the delivery to warn with. + */ +export function convertLegacyFileSet( + engine: "sg" | "vale" | "runtime", + ruleId: string, + files: readonly LegacyFile[], + legacyDirectory: string, + testsDirectory: string +): { files: LegacyFile[]; unconverted: string[] } { + const prefix = `${legacyDirectory}/`; + const legacy = files.filter((file) => file.path.startsWith(prefix)); + if (legacy.length === 0) return { files: [...files], unconverted: [] }; + + const kept = files.filter((file) => !file.path.startsWith(prefix)); + const converted: LegacyFile[] = []; + const unconverted: string[] = []; + const moveAsServed = (file: LegacyFile) => { + converted.push({ + path: `${testsDirectory}/${file.path.slice(prefix.length)}`, + content: file.content, + }); + }; + const take = (conversion: Conversion, file: LegacyFile) => { + if (conversion.unconverted.length > 0) { + unconverted.push(...conversion.unconverted); + moveAsServed(file); + return; + } + converted.push(...conversion.files); + }; + + const sibling = (name: string) => + files.find((file) => file.path === name)?.content; + + for (const file of legacy) { + const rest = file.path.slice(prefix.length); + const segments = rest.split("/"); + if (engine === "sg") { + const rule = sibling(`${ruleId}.yml`); + if (segments.length !== 1 || rule === undefined) { + unconverted.push(`${rest}: not an ast-grep test file beside its rule`); + moveAsServed(file); + continue; + } + take(convertSgTestFile(rule, rest, file.content, testsDirectory), file); + continue; + } + const [bucket, ...below] = segments; + if (bucket !== "pass" && bucket !== "fail") { + unconverted.push(`${rest}: not under pass/ or fail/`); + moveAsServed(file); + continue; + } + if (engine === "runtime") { + // Already a case directory: only the parent changes. + moveAsServed(file); + continue; + } + const [name] = below; + if (below.length !== 1 || name === undefined) { + unconverted.push( + `${rest}: a Vale fixture is one document directly in its bucket` + ); + moveAsServed(file); + continue; + } + const scope = valeFixtureScope(ruleId, sibling(".vale.ini") ?? ""); + take( + convertValeDocument(scope, bucket, name, file.content, testsDirectory), + file + ); + } + return { files: [...kept, ...converted], unconverted }; +} diff --git a/packages/cli/test/assemble.test.ts b/packages/cli/test/assemble.test.ts index 105c6e2c..a38ded17 100644 --- a/packages/cli/test/assemble.test.ts +++ b/packages/cli/test/assemble.test.ts @@ -1,13 +1,14 @@ import { mkdir, mkdtemp, + readdir, readFile, rm, stat, writeFile, } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join, relative } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { @@ -15,7 +16,12 @@ import { assembleValeConfig, type ValeAssembly, } from "../src/rules/assemble"; -import { ruleDirectory, ruleTestsDirectory } from "../src/rules/engines"; +import { + ASSEMBLED_SG_RULES, + ruleDirectory, + ruleTestsDirectory, +} from "../src/rules/engines"; +import { runAstGrepScan } from "../src/rules/scan"; let cwd: string; @@ -48,14 +54,39 @@ async function valeRule(id: string, config: string): Promise { await writeFile(join(directory, ".vale.ini"), config); } -/** Lay down an ast-grep rule with a test file. */ +/** Lay down an ast-grep rule with an (empty) tests directory. */ async function sgRule(id: string): Promise { await sgRuleWithoutTests(id); await mkdir(ruleTestsDirectory(cwd, "sg", id), { recursive: true }); } +/** Write one fixture file into `tests///` of an ast-grep rule. */ +async function sgFixture( + id: string, + bucket: "pass" | "fail", + name: string, + path: string, + body: string +): Promise { + const file = join(ruleTestsDirectory(cwd, "sg", id), bucket, name, path); + await mkdir(dirname(file), { recursive: true }); + await writeFile(file, body); +} + +/** Every file under `directory`, relative to it, sorted. */ +async function treeFiles(directory: string): Promise { + const entries = await readdir(directory, { + recursive: true, + withFileTypes: true, + }); + return entries + .filter((entry) => entry.isFile()) + .map((entry) => relative(directory, join(entry.parentPath, entry.name))) + .toSorted(); +} + /** - * Lay down an ast-grep rule that has no `.tests/` at all. + * Lay down an ast-grep rule that has no `tests/` at all. * * The state a nightly-migrated project is in, and the state a hand-made * `mkdir .taskless/rules/sg//` reaches on any version. @@ -259,18 +290,57 @@ describe("Vale config assembly", () => { }); describe("ast-grep config assembly", () => { - it("points ruleDirs at the sg tree and testConfigs at each rule", async () => { + it("points ruleDirs at the rules-only tree, with no testConfigs", async () => { await sgRule("no-eval"); await sgRule("no-debugger"); const path = await assembleSgConfig(cwd); const contents = await readFile(join(cwd, path ?? ""), "utf8"); - expect(contents).toContain("ruleDirs:\n - rules/sg"); - // Each rule keeps its tests inside its own directory, so each needs its - // own testConfigs entry. - expect(contents).toContain("- testDir: rules/sg/no-debugger/.tests"); - expect(contents).toContain("- testDir: rules/sg/no-eval/.tests"); + // Relative to the config, which sits beside the tree in `.taskless/`. + // Fixtures are judged by this same scan now, one case at a time, so + // nothing here names a test directory. + expect(contents).toBe("ruleDirs:\n - .sg-rules\n"); + }); + + it("copies each rule's .yml and nothing else into the tree", async () => { + // A `.yml` fixture is the dangerous one: ast-grep parses every `.yml` + // under `ruleDirs` as a rule, and a fixture that is not one fails the + // whole scan with `Cannot parse rule`. + await sgRule("no-eval"); + await sgRule("no-debugger"); + await sgFixture("no-eval", "fail", "workflow", "ci.yml", "run: eval(x)\n"); + await sgFixture("no-eval", "pass", "clean", "a.ts", "const x = 1;\n"); + + await assembleSgConfig(cwd); + + expect(await treeFiles(join(cwd, ASSEMBLED_SG_RULES))).toEqual([ + "no-debugger.yml", + "no-eval.yml", + ]); + // Copied verbatim, so what ast-grep loads is what the author wrote. + expect( + await readFile(join(cwd, ASSEMBLED_SG_RULES, "no-eval.yml"), "utf8") + ).toBe( + await readFile( + join(ruleDirectory(cwd, "sg", "no-eval"), "no-eval.yml"), + "utf8" + ) + ); + }); + + it("clears a rule deleted since the last assembly out of the tree", async () => { + // A stale copy would keep reporting for a rule that no longer exists. + await sgRule("keep"); + await sgRule("gone"); + await assembleSgConfig(cwd); + + await rm(ruleDirectory(cwd, "sg", "gone"), { recursive: true }); + await assembleSgConfig(cwd); + + expect(await treeFiles(join(cwd, ASSEMBLED_SG_RULES))).toEqual([ + "keep.yml", + ]); }); it("is byte-identical across runs", async () => { @@ -287,50 +357,39 @@ describe("ast-grep config assembly", () => { expect(await assembleSgConfig(cwd)).toBeUndefined(); }); - // ast-grep 0.41.0 aborts the whole invocation on a `testDir` it cannot read - // (exit 6), and `--filter` does not scope that away — so a single rule with - // no `.tests/` would fail every *other* rule's test run, naming a rule its - // author never touched. - it("omits a testDir whose directory is not on disk", async () => { - await sgRule("has-tests"); - await sgRuleWithoutTests("no-tests"); - - const path = await assembleSgConfig(cwd); - const contents = await readFile(join(cwd, path ?? ""), "utf8"); - - expect(contents).toContain("- testDir: rules/sg/has-tests/.tests"); - expect(contents).not.toContain("no-tests"); - }); - - it("emits no testDir that is missing from disk, for any rule", async () => { - await sgRule("alpha"); - await sgRuleWithoutTests("beta"); - await sgRule("gamma"); + it("still emits a config when no rule has a tests directory", async () => { + await sgRuleWithoutTests("only-rule"); const path = await assembleSgConfig(cwd); - const contents = await readFile(join(cwd, path ?? ""), "utf8"); - - const prefix = " - testDir: "; - const emitted = contents - .split("\n") - .filter((line) => line.startsWith(prefix)) - .map((line) => line.slice(prefix.length)); - expect(emitted.length).toBeGreaterThan(0); - for (const directory of emitted) { - const stats = await stat(join(cwd, ".taskless", directory)); - expect(stats.isDirectory(), `${directory} is a directory`).toBe(true); - } + expect(await readFile(join(cwd, path ?? ""), "utf8")).toBe( + "ruleDirs:\n - .sg-rules\n" + ); + expect(await treeFiles(join(cwd, ASSEMBLED_SG_RULES))).toEqual([ + "only-rule.yml", + ]); }); - // `testConfigs:` with no entries under it is accepted by ast-grep 0.41.0 — - // measured for both `sg test` and `sg scan` — so a project where no rule has - // tests yet still gets a config both commands can read. - it("still emits a config when no rule has a tests directory", async () => { - await sgRuleWithoutTests("only-rule"); + it("lets a scan complete over a rule whose tests/ holds .yml fixtures", async () => { + // The scan `check` runs, over the config this module wrote. Pointed at + // `.taskless/rules/sg/` directly, ast-grep would reach the `.yml` fixture + // below and abort with `Cannot parse rule` (measured on 0.51.0), taking + // every rule's report down with it. + await sgRule("no-eval"); + await sgFixture( + "no-eval", + "fail", + "workflow", + ".github/workflows/ci.yml", + "steps:\n - run: eval(x)\n" + ); + await mkdir(join(cwd, "src"), { recursive: true }); + await writeFile(join(cwd, "src", "a.ts"), "eval(input);\n"); const path = await assembleSgConfig(cwd); - const contents = await readFile(join(cwd, path ?? ""), "utf8"); + const scan = await runAstGrepScan(cwd, [], { configPath: path }); - expect(contents).toBe("ruleDirs:\n - rules/sg\ntestConfigs:\n"); + expect(scan.results.map((result) => [result.ruleId, result.file])).toEqual([ + ["no-eval", "src/a.ts"], + ]); }); }); diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts index 59c3dd40..97e7b9c7 100644 --- a/packages/cli/test/ast-grep-vendor-contract.test.ts +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -13,13 +13,13 @@ import { astGrepLanguageList, resolveAstGrepLanguage, } from "../src/rules/capabilities"; -import { ruleDirectory, ruleTestsDirectory } from "../src/rules/engines"; +import { ruleDirectory } from "../src/rules/engines"; +import { AST_GREP_LANGUAGE_EXTENSIONS } from "../src/rules/fixture-convert"; import { buildPath, findSgBinary, stripSgDeprecationBanner, } from "../src/rules/scan"; -import { escapeRegExp } from "../src/util/regex"; /** * ast-grep's observable behaviour, pinned. @@ -36,7 +36,7 @@ import { escapeRegExp } from "../src/util/regex"; * discovery changed is indistinguishable from a clean codebase. * * Each case says what breaks if it changes. These invoke the binary directly - * rather than through `runAstGrepScan` or `runTests`, deliberately: a test that + * rather than through `runAstGrepScan`, deliberately: a test that * went through our wrapper would be asserting our interpretation of ast-grep, * which is the thing under test everywhere else. */ @@ -85,22 +85,9 @@ const haskellKindRule = (kind: string) => "", ].join("\n"); -/** Valid test YAML, and invalid *rule* YAML — it carries no `language`. */ -const testFile = (id: string, invalid: string[], valid: string[] = []) => - [ - `id: ${id}`, - "valid:", - ...valid.map((source) => ` - ${JSON.stringify(source)}`), - "invalid:", - ...invalid.map((source) => ` - ${JSON.stringify(source)}`), - "", - ].join("\n"); - interface Project { /** Rule id to rule YAML. Each becomes `rules//.yml`. */ rules: Record; - /** Rule id to test YAML, written to `rules//.tests/-test.yml`. */ - tests?: Record; /** Path relative to the project root, to file contents. */ sources?: Record; } @@ -110,11 +97,11 @@ interface Project { * * The `sgconfig.yml` is written literally rather than through * `assembleSgConfig`, so what is pinned is the binary's response to a config, - * not our assembler's idea of one. The two relocated discovery cases at the - * bottom of this file are the deliberate exception: there, our layout is the - * thing being checked against discovery. + * not our assembler's idea of one. The discovery cases at the bottom of this + * file are the deliberate exception: there, our layout is the thing being + * checked against discovery. */ -function project({ rules, tests = {}, sources = {} }: Project): string { +function project({ rules, sources = {} }: Project): string { const cwd = mkdtempSync(join(tmpdir(), "sg-contract-")); workspaces.push(cwd); @@ -122,10 +109,6 @@ function project({ rules, tests = {}, sources = {} }: Project): string { mkdirSync(join(cwd, "rules", id), { recursive: true }); writeFileSync(join(cwd, "rules", id, `${id}.yml`), body); } - for (const [id, body] of Object.entries(tests)) { - mkdirSync(join(cwd, "rules", id, ".tests"), { recursive: true }); - writeFileSync(join(cwd, "rules", id, ".tests", `${id}-test.yml`), body); - } for (const [path, body] of Object.entries(sources)) { // The parent of the path actually given, not a hardcoded `src/` — every // case today happens to live under `src/`, and a future one that does not @@ -134,16 +117,7 @@ function project({ rules, tests = {}, sources = {} }: Project): string { writeFileSync(join(cwd, path), body); } - writeFileSync( - join(cwd, "sgconfig.yml"), - [ - "ruleDirs:", - " - rules", - "testConfigs:", - ...Object.keys(tests).map((id) => ` - testDir: rules/${id}/.tests`), - "", - ].join("\n") - ); + writeFileSync(join(cwd, "sgconfig.yml"), "ruleDirs:\n - rules\n"); return cwd; } @@ -158,17 +132,6 @@ const run = (cwd: string, argv: string[]) => const scan = (cwd: string, config = "sgconfig.yml") => run(cwd, ["scan", "--config", config, "--json=stream"]); -/** `verify`, exactly as `runTests` invokes it. */ -const test = (cwd: string, ruleId: string) => - run(cwd, [ - "test", - "-c", - "sgconfig.yml", - "--skip-snapshot-tests", - "--filter", - `^${ruleId}$`, - ]); - /** One eval call, at line 0 column 10 of `src/a.ts`. */ const evalSource = { "src/a.ts": 'const x = eval("1");\n' }; @@ -429,25 +392,29 @@ const grammarMatches = ( const arityMatches = (body: string): string[] => grammarMatches(body, aritySource["src/a.ts"]); -/** A rule whose fixtures all pass. */ -const passingProject = () => - project({ - rules: { "no-eval": rule("no-eval") }, - tests: { "no-eval": testFile("no-eval", ["eval(x)"], ["const a = 1"]) }, - }); - /** - * A failing case whose fixture text reads like a summary line. This is the - * poisoning case from issue #112, kept here as the fixture that makes the echo - * behaviour concrete. + * The files `scan` reported, run as `runAstGrepScan` runs it over a fixture + * case: under `configPath`, walking hidden directories. */ -const failingProject = () => - project({ - rules: { "no-eval": rule("no-eval") }, - tests: { - "no-eval": testFile("no-eval", ["const msg = '7 passed; 0 failed';"]), - }, - }); +const scannedFiles = (cwd: string, configPath: string): string[] => + run(cwd, [ + "scan", + "--config", + configPath, + "--json=stream", + "--no-ignore", + "hidden", + ]) + .stdout.split("\n") + .filter((line) => line !== "") + .map((line) => (JSON.parse(line) as { file: string }).file); + +/** A fresh directory for one scope case, removed after the test. */ +function scopeWorkspace(): string { + const directory = mkdtempSync(join(tmpdir(), "sg-contract-scope-")); + workspaces.push(directory); + return directory; +} withSg("ast-grep vendor contract", () => { it("reports its own name in --version", () => { @@ -661,149 +628,35 @@ withSg("ast-grep vendor contract", () => { }); }); - describe("sg test", () => { - it("prints `test result: ok. N passed; M failed;` to stdout and exits 0", () => { - // Depended on by: parseTestSummary in verify.ts, which anchors on this - // exact wording, and by runTests keying validity off exit 0. If the - // wording changes, the counts read 0/0 and verify falls through to the - // exit-code snippet — degraded rather than wrong, and this test is what - // announces it. - const result = test(passingProject(), "no-eval"); - expect(result.status).toBe(0); - expect(result.stdout).toContain("test result:"); - expect(result.stdout).toMatch( - /test result: .*ok.*\. 1 passed; 0 failed;/ - ); - }); - - it("prints `Error: test failed. N passed; M failed;` to stderr and exits 4", () => { - // The failure summary lands on STDERR, not stdout, which is why verify - // scans both streams. The exit code is 4 — not 1 — and runTests treats - // any non-zero as failure, so nothing depends on the number today; it is - // pinned because a future 0-on-failure would read as a pass. - const outcome = test(failingProject(), "no-eval"); - expect(outcome.status).toBe(4); - expect(outcome.stderr).toMatch( - /Error: test failed\. 0 passed; 1 failed;/ - ); - }); - - it("no longer colorizes the summary when stdout is not a TTY", () => { - // CHANGED AT 0.45.2. At 0.41.0 the summary arrived colorized even - // through a pipe, and this test pinned that. `--color` now honours its - // `auto` default, so a piped run is plain text. - // eslint-disable-next-line no-control-regex -- the escape IS the subject - const greenOk = /\u001B\[32mok\u001B\[0m/; - expect(test(passingProject(), "no-eval").stdout).not.toMatch(greenOk); - }); - - it("still puts the escape INSIDE the phrase under --color always", () => { - // Depended on by: parseTestSummary stripping VT control characters - // before matching, in verify.ts. Colour is gone from the piped default - // above, but it returns whenever stdout IS a TTY, and the escape lands - // between `test result: ` and `ok`, i.e. inside the phrase being - // anchored on. A parser that dropped the stripping would therefore keep - // working under CI and fail interactively, which is the worst place to - // find out. `--color always` is the testable stand-in for a TTY. - const colorized = run(passingProject(), [ - "test", - "-c", - "sgconfig.yml", - "--skip-snapshot-tests", - "--color", - "always", - "--filter", - "^no-eval$", - ]); - // eslint-disable-next-line no-control-regex -- the escape IS the subject - const inThePhrase = /test result: \u001B\[32mok\u001B\[0m\./; - expect(colorized.stdout).toMatch(inThePhrase); - }); - - it("echoes the offending source on failure, and nothing on success", () => { - // This is the root of #112: fixture text is reproduced verbatim into the - // output of a failing run, so an unanchored `/(\d+)\s+passed/` can match - // the fixture instead of the summary. A passing run echoes nothing, which - // is why the old parser was only ever wrong on failures. - expect(test(failingProject(), "no-eval").stdout).toContain( - "const msg = '7 passed; 0 failed';" - ); - expect(test(passingProject(), "no-eval").stdout).not.toContain( - "const a = 1" - ); - }); - - it("indents every echoed line, so fixture text never reaches column 0", () => { - // Depended on by: parseTestSummary anchoring on `^`. This is what makes - // the anchor close the *class* rather than the one fixture in #112 — a - // fixture whose own text is a verbatim summary line is still echoed - // indented, so it cannot be mistaken for the summary. Multi-line, since - // the indent has to hold for continuation lines too. - const poison = "Error: test failed. 99 passed; 0 failed;"; - const cwd = project({ - rules: { "no-eval": rule("no-eval") }, - tests: { - "no-eval": testFile("no-eval", [ - `const a = 1;\n${poison}\nlet b = 2;`, - ]), - }, - }); - const { stdout } = test(cwd, "no-eval"); - - expect(stdout).toContain(` ${poison}`); - expect(stdout).not.toMatch(new RegExp(`^${escapeRegExp(poison)}`, "m")); - }); - - it("needs --skip-snapshot-tests for an invalid case with no baseline", () => { - // Depended on by: runTests passing the flag. Without it, every invalid - // case in a rule that has never had snapshots recorded fails with "No - // baseline found" — a rule that is correct reported as failing. - const cwd = passingProject(); - const withoutFlag = run(cwd, [ - "test", - "-c", - "sgconfig.yml", - "--filter", - "^no-eval$", - ]); - expect(withoutFlag.status).not.toBe(0); - expect(withoutFlag.stdout).toContain("baseline found"); - }); - + describe("scan --filter", () => { it("treats --filter as an unanchored regex", () => { - // Depended on by: runTests building `^${escapeRegExp(ruleId)}$`. The - // anchors are load-bearing — without them, verifying `no-eval` would also - // run `no-eval-strict`'s cases and report its failures against the wrong - // rule. + // Depended on by: sgFilterArgv anchoring the alternation as + // `^(?:…)$`, which is how `check --rule` and each fixture case select + // one rule. Without the anchors, scanning for `no-eval` would also run + // `no-eval-strict` and report its findings under the wrong request. const cwd = project({ rules: { "no-eval": rule("no-eval"), "no-eval-strict": rule("no-eval-strict"), }, - tests: { - "no-eval": testFile("no-eval", ["eval(x)"]), - "no-eval-strict": testFile("no-eval-strict", ["eval(x)"]), - }, + sources: evalSource, }); - const anchored = run(cwd, [ - "test", - "-c", - "sgconfig.yml", - "--skip-snapshot-tests", - "--filter", - "^no-eval$", - ]); - expect(anchored.stdout).toContain("1 passed; 0 failed;"); - - const unanchored = run(cwd, [ - "test", - "-c", - "sgconfig.yml", - "--skip-snapshot-tests", - "--filter", - "no-eval", - ]); - expect(unanchored.stdout).toContain("2 passed; 0 failed;"); + const ruleIdsFor = (filter: string): string[] => + run(cwd, [ + "scan", + "--config", + "sgconfig.yml", + "--json=stream", + "--filter", + filter, + ]) + .stdout.split("\n") + .filter((line) => line !== "") + .map((line) => (JSON.parse(line) as { ruleId: string }).ruleId) + .toSorted(); + + expect(ruleIdsFor("^(?:no-eval)$")).toEqual(["no-eval"]); + expect(ruleIdsFor("no-eval")).toEqual(["no-eval", "no-eval-strict"]); }); }); @@ -1545,57 +1398,143 @@ withSg("ast-grep vendor contract", () => { }); }); - describe("the .tests/ directory is invisible to rule discovery", () => { + /** + * Why ast-grep reads rules from a rules-only tree. + * + * A rule's fixtures live beside it, at `tests///`, and a + * fixture is any file: an Actions rule's cases are `.yml` workflows. + * ast-grep parses every `.yml` under a `ruleDirs` entry as a rule, so + * pointing `ruleDirs` at the committed rules tree would read each fixture + * as a broken rule and take the whole scan down. `assembleSgConfig` copies + * each rule's `.yml` into `.taskless/.sg-rules/` instead, and points + * `ruleDirs` there. + * + * The dot in the old `.tests/` happened to keep its YAML out of discovery, + * which is behavior ast-grep does not document. The rules-only tree means no + * layout depends on it, so it is no longer pinned. + */ + describe("rule discovery reads a rules-only tree", () => { const RULE = rule("no-eval"); - const TEST_FILE = testFile("no-eval", ["eval(x)"], ["const a = 1"]); - /** A throwaway root carrying our committed rule layout. */ - function layout(): { cwd: string; directory: string } { + /** A throwaway root carrying our committed rule layout, with a YAML fixture. */ + function layout(): string { const cwd = mkdtempSync(join(tmpdir(), "sg-contract-layout-")); workspaces.push(cwd); const directory = ruleDirectory(cwd, "sg", "no-eval"); mkdirSync(directory, { recursive: true }); writeFileSync(join(directory, "no-eval.yml"), RULE); - return { cwd, directory }; - } - - it("scans clean with test YAML inside a rule's .tests/", async () => { - const { cwd } = layout(); - const tests = ruleTestsDirectory(cwd, "sg", "no-eval"); - mkdirSync(tests, { recursive: true }); - writeFileSync(join(tests, "no-eval-20260101-test.yml"), TEST_FILE); + // A literal `tests/`, not the layout constant: what is pinned is how + // ast-grep treats a plain directory, whatever the CLI calls its own. + const fixture = join(directory, "tests", "pass", "workflow", "ci.yml"); + mkdirSync(dirname(fixture), { recursive: true }); + writeFileSync(fixture, "on: push\njobs: {}\n"); mkdirSync(join(cwd, "src"), { recursive: true }); writeFileSync(join(cwd, "src", "a.ts"), 'const x = eval("1");\n'); + return cwd; + } + + it("fails the whole scan when ruleDirs reaches YAML in a plain tests/", () => { + // Depended on by: assembleSgConfig building a rules-only tree. This is + // the failure that tree exists to avoid: the fixture is not a rule, so + // ast-grep refuses it, and with it every rule in the config. + const cwd = layout(); + writeFileSync( + join(cwd, "sgconfig.yml"), + `ruleDirs:\n - ${dirname(ruleDirectory(cwd, "sg", "no-eval"))}\n` + ); + const result = scan(cwd); + expect(result.status).toBeGreaterThan(1); + expect(result.stderr).toContain("Cannot parse rule"); + expect(result.stderr).toContain(join("tests", "pass", "workflow")); + }); + + it("scans clean through the config assembleSgConfig writes", async () => { + const cwd = layout(); const configPath = await assembleSgConfig(cwd); expect(configPath).toBeDefined(); - // The rule fires — exit 1, error severity — which proves discovery ran; - // the test file beneath it was never parsed as a rule, which is the - // property being pinned. A parse failure produces no JSON at all. + // The rule fires (exit 1, error severity), which proves discovery ran; + // the fixture beside it was never offered to ast-grep as a rule. A parse + // failure produces no JSON at all. const result = scan(cwd, configPath ?? ""); expect(result.status).toBe(1); const findings = result.stdout .split("\n") .filter((line) => line !== "") - .map((line) => JSON.parse(line) as { ruleId: string }); + .map((line) => JSON.parse(line) as { ruleId: string; file: string }); expect(findings.map((finding) => finding.ruleId)).toEqual(["no-eval"]); }); + }); - it("fails the whole scan if the same file sits in a non-dot directory", async () => { - // Deliberately NOT `.tests/` — this is the layout the dot exists to - // avoid. A test file is invalid rule YAML: it has no `language`. - const { cwd, directory } = layout(); - mkdirSync(join(directory, "tests"), { recursive: true }); - writeFileSync( - join(directory, "tests", "no-eval-20260101-test.yml"), - TEST_FILE - ); + /** + * How a fixture case is scanned: from the case directory, under a config + * that lives elsewhere. + * + * Depended on by: `runSgFixtures` in `sg-fixtures.ts`, which runs each case + * with the case directory as the working directory and a one-rule config in + * a temporary directory. A fixture exercises the rule's scope only because + * `files:` resolves against the working directory, not against the config's + * location. If that moved to the config, every scoped rule's fail cases + * would go quiet, and every scoped rule would fail `verify`. + */ + describe("files: globs resolve against the scan's working directory", () => { + const SCOPED_RULE = [ + "id: no-latest", + "language: Yaml", + "severity: error", + "message: m", + "files:", + ' - ".github/workflows/*.yml"', + "rule:", + " kind: string_scalar", + " regex: '@latest'", + "", + ].join("\n"); + const OFFENDING = "steps:\n - run: npx tool@latest\n"; + + /** A config in its own directory, with an absolute `ruleDirs`. */ + function elsewhereConfig(workspace: string): string { + const rules = join(workspace, "run", "rules"); + mkdirSync(rules, { recursive: true }); + writeFileSync(join(rules, "no-latest.yml"), SCOPED_RULE); + const configPath = join(workspace, "run", "sgconfig.yml"); + writeFileSync(configPath, `ruleDirs:\n - ${rules}\n`); + return configPath; + } - const configPath = await assembleSgConfig(cwd); - const result = scan(cwd, configPath ?? ""); - expect(result.status).toBeGreaterThan(1); - expect(result.stderr).toContain("missing field `language`"); + /** A case directory holding `OFFENDING` at `path`. */ + function caseWith(workspace: string, path: string): string { + const caseDirectory = join(workspace, "parent", "case"); + mkdirSync(dirname(join(caseDirectory, path)), { recursive: true }); + writeFileSync(join(caseDirectory, path), OFFENDING); + return caseDirectory; + } + + it("fires on a file at a path the glob admits, relative to the cwd", () => { + const root = scopeWorkspace(); + const configPath = elsewhereConfig(root); + const caseDirectory = caseWith(root, ".github/workflows/ci.yml"); + expect(scannedFiles(caseDirectory, configPath)).toEqual([ + ".github/workflows/ci.yml", + ]); + }); + + it("stays quiet on the same file at the case root", () => { + const root = scopeWorkspace(); + const configPath = elsewhereConfig(root); + const caseDirectory = caseWith(root, "ci.yml"); + expect(scannedFiles(caseDirectory, configPath)).toEqual([]); + }); + + it("stays quiet when the same case is scanned from its parent", () => { + // The glob is matched against the path from the cwd, which here is + // `case/.github/workflows/ci.yml`. This is why a case is scanned with + // its own directory as the cwd rather than passed as a path. + const root = scopeWorkspace(); + const configPath = elsewhereConfig(root); + const caseDirectory = caseWith(root, ".github/workflows/ci.yml"); + expect(scannedFiles(dirname(caseDirectory), configPath)).toEqual([]); }); }); }); @@ -1692,4 +1631,64 @@ withSg("ast-grep engine capabilities", () => { } expect(rendered.split(", ")).toHaveLength(AST_GREP_LANGUAGES.length); }); + + it("parses each AST_GREP_LANGUAGE_EXTENSIONS extension as its language by default", () => { + // Depended on by: fixture-convert.ts naming a converted snippet + // `.`. ast-grep picks a file's parser from its extension + // alone, so an extension that maps to another language (or to none) + // leaves the converted fixture unparsed and every fail case quiet. + // + // One rule per language and one file per extension, in a single scan. Each + // file must be reported by its own language's rule and by no other, read + // off the `language` field ast-grep reports rather than inferred. + const rules: Record = {}; + const sources: Record = {}; + for (const [language, extension] of Object.entries( + AST_GREP_LANGUAGE_EXTENSIONS + )) { + // `pattern: zzz` parses in every grammar but Html's, where the text + // node is the smallest thing to match. + const matcher = language === "Html" ? " kind: text" : " pattern: zzz"; + rules[language.toLowerCase()] = [ + `id: ${language.toLowerCase()}`, + `language: ${language}`, + "severity: warning", + "message: m", + "rule:", + matcher, + "", + ].join("\n"); + sources[`src/a.${extension}`] = "zzz\n"; + } + + const result = scan(project({ rules, sources })); + expect(result.status, result.stderr).toBe(0); + const reported = new Map>(); + for (const line of result.stdout.split("\n")) { + if (line === "") continue; + const match = JSON.parse(line) as { + file: string; + language: string; + ruleId: string; + }; + // The Yaml rule also reads the project's own rule files and config. + if (!match.file.startsWith("src/")) continue; + const seen = reported.get(match.file) ?? new Set(); + seen.add(`${match.language}/${match.ruleId}`); + reported.set(match.file, seen); + } + + expect( + Object.fromEntries([...reported].map(([file, seen]) => [file, [...seen]])) + ).toEqual( + Object.fromEntries( + Object.entries(AST_GREP_LANGUAGE_EXTENSIONS).map( + ([language, extension]) => [ + `src/a.${extension}`, + [`${language}/${language.toLowerCase()}`], + ] + ) + ) + ); + }); }); diff --git a/packages/cli/test/fixture-convert.test.ts b/packages/cli/test/fixture-convert.test.ts new file mode 100644 index 00000000..b231f080 --- /dev/null +++ b/packages/cli/test/fixture-convert.test.ts @@ -0,0 +1,360 @@ +import { describe, expect, it } from "vitest"; + +import { + convertLegacyFileSet, + convertSgTestFile, + convertValeDocument, + deriveFixturePath, + valeFixtureScope, +} from "../src/rules/fixture-convert"; + +/** + * The converter shared by migration 12 and delivery. Pure, so every case here + * is content in and content out, with no filesystem. + * + * The directory names are passed as literals rather than read from the layout + * table: the converter takes both as arguments, and the expected paths below + * spell them out, so the two must agree in this file whatever the table says. + */ +const TESTS = "tests"; +const LEGACY = ".tests"; + +const scope = (include: string[], exclude: string[] = []) => ({ + include, + exclude, +}); + +describe("deriveFixturePath", () => { + it("puts a file at the case root when the scope has no globs", () => { + expect(deriveFixturePath(scope([]), "fail-01", "ts")).toBe("fail-01.ts"); + }); + + it("names the file for the case under a bare extension glob", () => { + expect(deriveFixturePath(scope(["*.ts"]), "fail-01", "ts")).toBe( + "fail-01.ts" + ); + }); + + it("keeps the directories a glob names, hidden ones included", () => { + // The scenario the spec names: a workflow rule's fixture has to sit where + // the rule looks, or it is a quiet `fail/` case for no reason. + expect( + deriveFixturePath(scope([".github/workflows/*.yml"]), "fail-01", "yml") + ).toBe(".github/workflows/fail-01.yml"); + }); + + it("collapses a globstar to nothing", () => { + expect(deriveFixturePath(scope(["**/src/**/*.ts"]), "pass-01", "ts")).toBe( + "src/pass-01.ts" + ); + }); + + it("takes the brace alternative ending in the snippet's extension", () => { + expect(deriveFixturePath(scope(["*.{ts,tsx}"]), "fail-01", "ts")).toBe( + "fail-01.ts" + ); + expect(deriveFixturePath(scope(["*.{ts,tsx}"]), "fail-01", "tsx")).toBe( + "fail-01.tsx" + ); + }); + + it("puts a file beneath a trailing /**", () => { + expect(deriveFixturePath(scope(["src/**"]), "fail-01", "ts")).toBe( + "src/fail-01.ts" + ); + }); + + it("returns undefined when an ignore excludes every candidate", () => { + expect( + deriveFixturePath(scope(["src/**/*.ts"], ["src/**"]), "fail-01", "ts") + ).toBeUndefined(); + }); + + it("falls through to a later glob when an ignore excludes the first", () => { + expect( + deriveFixturePath(scope(["src/**/*.ts", "*.ts"], ["src/**"]), "f", "ts") + ).toBe("f.ts"); + }); +}); + +/** An ast-grep rule file, with `extra` appended for scope keys. */ +const rule = (extra = "") => + `id: no-eval\nlanguage: TypeScript\nrule:\n pattern: eval($A)\n${extra}`; + +describe("convertSgTestFile", () => { + const tests = + "id: no-eval\nvalid:\n - JSON.parse(x)\n - const a = 1;\ninvalid:\n - eval(x)\n"; + + it("leaves a test file with a YAML syntax error unconverted", () => { + // `parseDocument` collects errors rather than throwing, so without the + // check a half-parsed `invalid:` list became a case holding its debris. + const conversion = convertSgTestFile( + rule(), + "broken-test.yml", + "id: no-eval\ninvalid: [unterminated\n", + TESTS + ); + expect(conversion.files).toEqual([]); + expect(conversion.unconverted).toEqual([ + "broken-test.yml: the rule or the test file is not valid YAML", + ]); + }); + + it("leaves a test file that is a YAML list unconverted", () => { + // A list passes a bare object check and would convert to no cases, after + // which the migration deletes the source. + const conversion = convertSgTestFile( + rule(), + "list-test.yml", + "- eval(x)\n", + TESTS + ); + expect(conversion.files).toEqual([]); + expect(conversion.unconverted).toEqual([ + "list-test.yml: the test file is not a YAML mapping", + ]); + }); + + it("writes one case per snippet, named by bucket and ordinal", () => { + const conversion = convertSgTestFile( + rule(), + "no-eval-test.yml", + tests, + TESTS + ); + expect(conversion.unconverted).toEqual([]); + expect(conversion.files).toEqual([ + { path: "tests/pass/pass-01/pass-01.ts", content: "JSON.parse(x)\n" }, + { path: "tests/pass/pass-02/pass-02.ts", content: "const a = 1;\n" }, + { path: "tests/fail/fail-01/fail-01.ts", content: "eval(x)\n" }, + ]); + }); + + it("places each snippet where the rule's files: looks", () => { + const conversion = convertSgTestFile( + rule('files:\n - "src/**/*.ts"\n'), + "no-eval-test.yml", + "invalid:\n - eval(x)\n", + TESTS + ); + expect(conversion.files.map((file) => file.path)).toEqual([ + "tests/fail/fail-01/src/fail-01.ts", + ]); + }); + + it("reads ast-grep's object form of a files: entry", () => { + const conversion = convertSgTestFile( + rule("files:\n - glob: lib/*.ts\n"), + "t.yml", + "invalid:\n - eval(x)\n", + TESTS + ); + expect(conversion.files[0]?.path).toBe("tests/fail/fail-01/lib/fail-01.ts"); + }); + + it("takes the extension from the language, any spelling ast-grep resolves", () => { + const conversion = convertSgTestFile( + "id: r\nlanguage: python\nrule:\n pattern: eval($A)\n", + "t.yml", + "invalid:\n - eval(x)\n", + TESTS + ); + expect(conversion.files[0]?.path).toBe("tests/fail/fail-01/fail-01.py"); + }); + + it.each([ + [ + "a language with no extension", + "id: r\nlanguage: Klingon\nrule:\n pattern: x\n", + "invalid:\n - x\n", + "has no file extension", + ], + [ + "a test file that is not a mapping", + rule(), + "eval(x)\n", + "not a YAML mapping", + ], + [ + "an invalid: key that is not a list", + rule(), + "invalid: eval(x)\n", + "is not a list", + ], + [ + "a scope no path can be derived for", + rule('files:\n - "src/**/*.ts"\nignores:\n - "src/**"\n'), + "invalid:\n - eval(x)\n", + "no path could be derived", + ], + ])( + "converts nothing, and says why, for %s", + (_, ruleSource, test, reason) => { + const conversion = convertSgTestFile(ruleSource, "t.yml", test, TESTS); + expect(conversion.files).toEqual([]); + expect(conversion.unconverted).toHaveLength(1); + expect(conversion.unconverted[0]).toContain("t.yml"); + expect(conversion.unconverted[0]).toContain(reason); + } + ); +}); + +describe("valeFixtureScope", () => { + it("admits under YES matchers and excludes under NO ones", () => { + const config = [ + "[*.md]", + "tskl) rule = hedge", + "hedge.hedge = YES", + "", + "[docs/legacy/**]", + "tskl) rule = hedge", + "hedge.hedge = NO", + "", + ].join("\n"); + expect(valeFixtureScope("hedge", config)).toEqual({ + include: ["*.md"], + exclude: ["docs/legacy/**"], + }); + }); + + it("ignores another rule's key", () => { + expect( + valeFixtureScope("hedge", "[*.md]\nother.other = YES\n").include + ).toEqual([]); + }); +}); + +describe("convertValeDocument", () => { + it("makes a case named for the document's stem, at an admitted path", () => { + const conversion = convertValeDocument( + scope(["docs/**/*.md"]), + "fail", + "hedges.md", + "Perhaps.\n", + TESTS + ); + expect(conversion).toEqual({ + files: [ + { path: "tests/fail/hedges/docs/hedges.md", content: "Perhaps.\n" }, + ], + unconverted: [], + }); + }); + + it("names a document no admitted path can be derived for", () => { + const conversion = convertValeDocument( + scope(["*.md"], ["*.md"]), + "pass", + "quiet.md", + "Fine.\n", + TESTS + ); + expect(conversion.files).toEqual([]); + expect(conversion.unconverted[0]).toContain("pass/quiet.md"); + }); +}); + +/** Convert a served set from `.tests/` into `tests/`, as delivery does. */ +const convert = ( + engine: "sg" | "vale" | "runtime", + ruleId: string, + files: { path: string; content: string }[] +) => convertLegacyFileSet(engine, ruleId, files, LEGACY, TESTS); + +describe("convertLegacyFileSet", () => { + const SG_RULE = + "id: no-eval\nlanguage: TypeScript\nseverity: error\nmessage: m\nrule:\n pattern: eval($A)\n"; + + it("converts a legacy ast-grep set into case directories", () => { + const result = convert("sg", "no-eval", [ + { path: "no-eval.yml", content: SG_RULE }, + { + path: ".tests/no-eval-test.yml", + content: "id: no-eval\nvalid:\n - ok()\ninvalid:\n - eval(x)\n", + }, + ]); + expect(result.unconverted).toEqual([]); + expect(result.files.map((file) => file.path)).toEqual([ + "no-eval.yml", + "tests/pass/pass-01/pass-01.ts", + "tests/fail/fail-01/fail-01.ts", + ]); + }); + + it("passes every non-fixture file through byte-identical", () => { + const signed = { path: "no-eval.yml", content: SG_RULE }; + const result = convert("sg", "no-eval", [ + signed, + { path: ".tests/no-eval-test.yml", content: "invalid:\n - eval(x)\n" }, + ]); + expect(result.files.find((file) => file.path === signed.path)).toEqual( + signed + ); + }); + + it("returns a set already in the new layout unchanged", () => { + const files = [ + { path: "no-eval.yml", content: SG_RULE }, + { path: "tests/pass/ok/ok.ts", content: "ok()\n" }, + { path: "tests/fail/bad/bad.ts", content: "eval(x)\n" }, + ]; + expect(convert("sg", "no-eval", files)).toEqual({ + files, + unconverted: [], + }); + }); + + it("converts a legacy Vale document under its rule's matchers", () => { + const result = convert("vale", "hedge", [ + { path: "hedge.yml", content: "extends: existence\n" }, + { + path: ".vale.ini", + content: "[docs/*.md]\ntskl) rule = hedge\nhedge.hedge = YES\n", + }, + { path: ".tests/fail/hedges.md", content: "Perhaps.\n" }, + ]); + expect(result.unconverted).toEqual([]); + expect(result.files.map((file) => file.path)).toContain( + "tests/fail/hedges/docs/hedges.md" + ); + }); + + it("only renames a runtime case's parent", () => { + const result = convert("runtime", "logs", [ + { path: "check.ts", content: "export default async () => [];\n" }, + { path: ".tests/fail/two-files/a.ts", content: "a\n" }, + { path: ".tests/fail/two-files/b.ts", content: "b\n" }, + ]); + expect(result.unconverted).toEqual([]); + expect(result.files).toEqual([ + { path: "check.ts", content: "export default async () => [];\n" }, + { path: "tests/fail/two-files/a.ts", content: "a\n" }, + { path: "tests/fail/two-files/b.ts", content: "b\n" }, + ]); + }); + + it("moves a fixture it cannot place under tests/ as served, and reports it", () => { + // Never back under `.tests/`: the CLI does not write a layout its own + // `verify` does not read, and `verify` names what it finds there. + const unplaceable = { + path: ".tests/no-eval-test.yml", + content: "invalid:\n - eval(x)\n", + }; + const result = convert("sg", "no-eval", [ + { + path: "no-eval.yml", + content: `${SG_RULE}files:\n - "src/**/*.ts"\nignores:\n - "src/**"\n`, + }, + unplaceable, + ]); + expect(result.files).toContainEqual({ + path: "tests/no-eval-test.yml", + content: unplaceable.content, + }); + expect(result.files.some((file) => file.path.startsWith(".tests/"))).toBe( + false + ); + expect(result.unconverted).toHaveLength(1); + expect(result.unconverted[0]).toContain("no-eval-test.yml"); + }); +}); diff --git a/packages/cli/test/vale-vendor-contract.test.ts b/packages/cli/test/vale-vendor-contract.test.ts index 4760804f..bb660974 100644 --- a/packages/cli/test/vale-vendor-contract.test.ts +++ b/packages/cli/test/vale-vendor-contract.test.ts @@ -2550,3 +2550,100 @@ withVale( }); } ); + +/** + * A styles tree, a config directory beside it, and one case tree, all + * separate, as `verifyValeRule` lays them out. + */ +function caseLayout(matcher: string, documents: string[]) { + const root = mkdtempSync(join(tmpdir(), "vale-contract-cwd-")); + workspaces.push(root); + const styles = join(root, "styles"); + mkdirSync(join(styles, "no-simply"), { recursive: true }); + writeFileSync( + join(styles, "no-simply", "no-simply.yml"), + existence("simply") + ); + const configDirectory = join(root, "config"); + mkdirSync(configDirectory); + const config = join(configDirectory, "scoped.ini"); + writeFileSync( + config, + `StylesPath = ${styles}\nMinAlertLevel = suggestion\n\n[${matcher}]\nno-simply.no-simply = YES\n` + ); + const caseDirectory = join(root, "run", "case"); + for (const path of documents) { + mkdirSync(join(caseDirectory, path, ".."), { recursive: true }); + writeFileSync(join(caseDirectory, path), "Just simply do it.\n"); + } + return { config, caseDirectory, parent: join(root, "run") }; +} + +/** The documents Vale reported a finding in. */ +function firing(cwd: string, config: string, path: string): string[] { + const result = spawnSync( + binary as string, + ["--config", config, "--output=JSON", "--no-exit", "--", path], + { cwd, encoding: "utf8" } + ); + expect(result.status, result.stderr).toBe(0); + return Object.keys( + JSON.parse(result.stdout) as Record + ).toSorted(); +} + +/** + * A matcher resolves against the working directory, not against `--config`. + * + * Depended on by: `verifyValeRule` in `rules/vale/verify.ts`, which judges a + * fixture case by running Vale with the case directory as its working + * directory, under a config written to a temp directory with an absolute + * `StylesPath`. That is how a rule's own matchers apply to a case the way + * they apply to a project: under `[docs/**\/*.md]`, a case document at + * `docs/guide/x.md` is in scope and one at `x.md` is not. + * + * If Vale ever resolved matchers against the config file's directory, every + * scoped rule's fail cases would go quiet and verification would blame the + * scope for all of them. If it resolved them against a path argument instead, + * running from the case would still work but the parent-directory case below + * would start firing, and the cwd discipline in `runCase` would be redundant + * rather than load-bearing. + */ +withVale("a matcher resolves against the working directory", () => { + it("applies [docs/**/*.md] to the tree below the cwd, with --config elsewhere", () => { + const { config, caseDirectory } = caseLayout("docs/**/*.md", [ + "docs/guide/x.md", + "x.md", + ]); + expect(firing(caseDirectory, config, ".")).toEqual(["docs/guide/x.md"]); + }); + + it("fires nothing when the same tree is passed as a path from its parent", () => { + // The path argument does not re-root the matcher: from the parent the + // document is `case/docs/guide/x.md`, which `docs/**/*.md` does not match. + // This is why a case is run FROM its directory rather than pointed at. + const { config, parent } = caseLayout("docs/**/*.md", [ + "docs/guide/x.md", + "x.md", + ]); + expect(firing(parent, config, "case")).toEqual([]); + }); + + it("walks a document under .github/", () => { + // Vale walks hidden directories, so a fixture for a rule scoped to + // `.github/**` can place its document there and be linted. If Vale ever + // skipped dot-directories, such a case would go quiet under every config, + // and the unscoped re-run would report "did not match" for a rule that + // works. + const { config, caseDirectory } = caseLayout("*", [ + ".github/x.md", + "docs/guide/x.md", + "x.md", + ]); + expect(firing(caseDirectory, config, ".")).toEqual([ + ".github/x.md", + "docs/guide/x.md", + "x.md", + ]); + }); +}); From 18b87f73a37dfbf71c3aeb8962042a874057f1c3 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 26 Oct 2026 11:18:03 -0700 Subject: [PATCH 2/3] fix(rules): number converted cases across a rule's test files, and keep each file's extension A rule improved more than once owns several dated ast-grep test files, and each numbered its snippets from one, so the second file's fail-01 landed on the first's. Case names are now claimed in a set shared across a rule's conversions; two Vale documents sharing a stem get and -2. Path derivation also preferred the first glob even when it changed the file's extension, so under [*.md] and [*.txt] a .txt document became .md and would be linted as Markdown. A path keeping the extension now wins. --- packages/cli/src/rules/assemble.ts | 9 ++- packages/cli/src/rules/fixture-convert.ts | 82 +++++++++++++++++++--- packages/cli/test/fixture-convert.test.ts | 84 +++++++++++++++++++++++ 3 files changed, 159 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/rules/assemble.ts b/packages/cli/src/rules/assemble.ts index f268311e..165930be 100644 --- a/packages/cli/src/rules/assemble.ts +++ b/packages/cli/src/rules/assemble.ts @@ -331,12 +331,11 @@ export async function assembleValeConfig( * * Copied rather than linked: symlinks need privileges on Windows, and a rule * file is a few hundred bytes. The tree is cleared first, so a rule deleted - * since the last run cannot keep reporting from a stale copy. `check` assembles - * against its snapshot, so there the tree lands inside the run directory; it - * is gitignored for any other caller. + * since the last run cannot keep reporting from a stale copy. `check`, the one + * caller, assembles against its snapshot, so the tree lands inside the run + * directory and never in the project. * - * No `testConfigs`. Fixtures are no longer run by `sg test`: `verify` runs - * each case through this same scan (see `rules/sg-fixtures.ts`). + * No `testConfigs`: `verify` writes its own one-rule config for `sg test`. */ export async function assembleSgConfig( cwd: string diff --git a/packages/cli/src/rules/fixture-convert.ts b/packages/cli/src/rules/fixture-convert.ts index e208e83c..9b8217a2 100644 --- a/packages/cli/src/rules/fixture-convert.ts +++ b/packages/cli/src/rules/fixture-convert.ts @@ -119,16 +119,25 @@ function admitted(path: string, scope: FixtureScope): boolean { /** * A path for a fixture file named `stem.extension` that `scope` admits, or * `undefined` when none of the derivations could be checked. + * + * A path keeping `.extension` is preferred over an earlier glob's that does + * not: both engines choose how to read a file by its extension, so under + * `[*.md]` and `[*.txt]` a `.txt` document written as `.md` would be linted + * as Markdown. */ export function deriveFixturePath( scope: FixtureScope, stem: string, extension: string ): string | undefined { - const candidates = + const derived = scope.include.length === 0 ? [`${stem}.${extension}`] : scope.include.map((glob) => pathFromGlob(glob, stem, extension)); + const candidates = [ + ...derived.filter((path) => path.endsWith(`.${extension}`)), + ...derived.filter((path) => !path.endsWith(`.${extension}`)), + ]; return candidates.find( (candidate) => candidate !== "" && @@ -161,6 +170,31 @@ export interface ConvertedFile { content: string; } +/** + * The cases a rule's conversion has already written, as `/`. + * + * A rule can own several legacy test files (one per authoring round, dated + * `-YYYYMMDD-test.yml`), and each numbers its snippets from one. Sharing + * this set across a rule's conversions is what keeps the second file's + * `fail-01` from landing on the first file's. + */ +export type ClaimedCases = Set; + +/** The first ``, `-2`, `-3`, … not yet in `claimed`. */ +function claimCase( + claimed: ClaimedCases, + bucket: FixtureBucket, + name: (attempt: number) => string +): string { + for (let attempt = 1; ; attempt++) { + const candidate = name(attempt); + if (!claimed.has(`${bucket}/${candidate}`)) { + claimed.add(`${bucket}/${candidate}`); + return candidate; + } + } +} + /** What one conversion produced, and what it had to leave alone. */ export interface Conversion { files: ConvertedFile[]; @@ -173,8 +207,10 @@ export interface Conversion { * * One case per `valid:` (pass) or `invalid:` (fail) snippet, named by bucket * and ordinal in source order, since a snippet carries no description to name - * it by. `rule` is the rule file's content, which supplies `language:` and the - * scope. `name` is the test file's name, for messages. + * it by. Ordinals continue past any case in `claimed`, which a successful + * conversion adds its own cases to. `rule` is the rule file's content, which + * supplies `language:` and the scope. `name` is the test file's name, for + * messages. * * Unconverted (and so left in place by the caller) when the test file does not * parse, the language has no extension, or no admitted path can be derived. @@ -185,7 +221,8 @@ export function convertSgTestFile( rule: string, name: string, content: string, - testsDirectory: string + testsDirectory: string, + claimed: ClaimedCases = new Set() ): Conversion { const unconvertible = (reason: string): Conversion => ({ files: [], @@ -226,6 +263,8 @@ export function convertSgTestFile( exclude: globList(ruleRecord.ignores), }; + // Claimed into a copy, so a file refused part-way claims nothing. + const claiming = new Set(claimed); const files: ConvertedFile[] = []; const buckets: [FixtureBucket, unknown][] = [ ["pass", test.valid], @@ -238,13 +277,17 @@ export function convertSgTestFile( `its ${bucket === "pass" ? "valid" : "invalid"}: key is not a list` ); } - for (const [index, snippet] of snippets.entries()) { + for (const snippet of snippets) { if (typeof snippet !== "string") { return unconvertible( `a ${bucket === "pass" ? "valid" : "invalid"}: entry is not a string` ); } - const caseName = `${bucket}-${String(index + 1).padStart(2, "0")}`; + const caseName = claimCase( + claiming, + bucket, + (ordinal) => `${bucket}-${String(ordinal).padStart(2, "0")}` + ); const path = deriveFixturePath(scope, caseName, extension); if (path === undefined) { return unconvertible( @@ -257,6 +300,7 @@ export function convertSgTestFile( }); } } + for (const entry of claiming) claimed.add(entry); return { files, unconverted: [] }; } @@ -288,7 +332,8 @@ export function valeFixtureScope(ruleId: string, config: string): FixtureScope { * case directory named for the document's stem. * * The document keeps its own name and extension, placed at a path the rule's - * enabling matchers admit. Unconverted when no such path can be derived. A + * enabling matchers admit. Two documents sharing a stem (`a.md`, `a.txt`) get + * `a` and `a-2`, through `claimed`. Unconverted when no such path can be derived. A * rule with no enabling matcher cannot have one, and `validate` reports that * rule as off before it reports anything about its fixtures. */ @@ -297,7 +342,8 @@ export function convertValeDocument( bucket: FixtureBucket, name: string, content: string, - testsDirectory: string + testsDirectory: string, + claimed: ClaimedCases = new Set() ): Conversion { const dot = name.lastIndexOf("."); const stem = dot <= 0 ? name : name.slice(0, dot); @@ -312,9 +358,12 @@ export function convertValeDocument( ], }; } + const caseName = claimCase(claimed, bucket, (attempt) => + attempt === 1 ? stem : `${stem}-${String(attempt)}` + ); return { files: [ - { path: posix.join(testsDirectory, bucket, stem, derived), content }, + { path: posix.join(testsDirectory, bucket, caseName, derived), content }, ], unconverted: [], }; @@ -353,6 +402,7 @@ export function convertLegacyFileSet( const kept = files.filter((file) => !file.path.startsWith(prefix)); const converted: LegacyFile[] = []; const unconverted: string[] = []; + const claimed: ClaimedCases = new Set(); const moveAsServed = (file: LegacyFile) => { converted.push({ path: `${testsDirectory}/${file.path.slice(prefix.length)}`, @@ -381,7 +431,10 @@ export function convertLegacyFileSet( moveAsServed(file); continue; } - take(convertSgTestFile(rule, rest, file.content, testsDirectory), file); + take( + convertSgTestFile(rule, rest, file.content, testsDirectory, claimed), + file + ); continue; } const [bucket, ...below] = segments; @@ -405,7 +458,14 @@ export function convertLegacyFileSet( } const scope = valeFixtureScope(ruleId, sibling(".vale.ini") ?? ""); take( - convertValeDocument(scope, bucket, name, file.content, testsDirectory), + convertValeDocument( + scope, + bucket, + name, + file.content, + testsDirectory, + claimed + ), file ); } diff --git a/packages/cli/test/fixture-convert.test.ts b/packages/cli/test/fixture-convert.test.ts index b231f080..cc99b714 100644 --- a/packages/cli/test/fixture-convert.test.ts +++ b/packages/cli/test/fixture-convert.test.ts @@ -64,6 +64,12 @@ describe("deriveFixturePath", () => { ); }); + it("prefers a glob that keeps the file's extension over an earlier one", () => { + expect(deriveFixturePath(scope(["*.md", "*.txt"]), "hedges", "txt")).toBe( + "hedges.txt" + ); + }); + it("returns undefined when an ignore excludes every candidate", () => { expect( deriveFixturePath(scope(["src/**/*.ts"], ["src/**"]), "fail-01", "ts") @@ -130,6 +136,46 @@ describe("convertSgTestFile", () => { ]); }); + it("continues ordinals past the cases another test file claimed", () => { + // A rule improved twice owns two dated test files, each numbering from + // one. Without the shared set, the second file's fail-01 overwrote the + // first's and the migration deleted its source. + const claimed = new Set(); + const first = convertSgTestFile( + rule(), + "no-eval-20260101-test.yml", + "invalid:\n - eval(a)\n", + TESTS, + claimed + ); + const second = convertSgTestFile( + rule(), + "no-eval-20260301-test.yml", + "valid:\n - ok()\ninvalid:\n - eval(b)\n", + TESTS, + claimed + ); + expect(first.files.map((file) => file.path)).toEqual([ + "tests/fail/fail-01/fail-01.ts", + ]); + expect(second.files.map((file) => file.path)).toEqual([ + "tests/pass/pass-01/pass-01.ts", + "tests/fail/fail-02/fail-02.ts", + ]); + }); + + it("claims nothing for a test file it refuses", () => { + const claimed = new Set(); + convertSgTestFile( + rule(), + "half-test.yml", + "valid:\n - ok()\ninvalid:\n - 3\n", + TESTS, + claimed + ); + expect(claimed).toEqual(new Set()); + }); + it("places each snippet where the rule's files: looks", () => { const conversion = convertSgTestFile( rule('files:\n - "src/**/*.ts"\n'), @@ -241,6 +287,25 @@ describe("convertValeDocument", () => { }); }); + it("suffixes a case whose stem another document already claimed", () => { + const claimed = new Set(); + const paths = ["hedges.md", "hedges.txt"].map( + (name) => + convertValeDocument( + scope(["*.md", "*.txt"]), + "fail", + name, + "Perhaps.\n", + TESTS, + claimed + ).files[0]?.path + ); + expect(paths).toEqual([ + "tests/fail/hedges/hedges.md", + "tests/fail/hedges-2/hedges.txt", + ]); + }); + it("names a document no admitted path can be derived for", () => { const conversion = convertValeDocument( scope(["*.md"], ["*.md"]), @@ -281,6 +346,25 @@ describe("convertLegacyFileSet", () => { ]); }); + it("keeps every snippet when a rule serves more than one test file", () => { + const result = convert("sg", "no-eval", [ + { path: "no-eval.yml", content: SG_RULE }, + { + path: ".tests/no-eval-20260101-test.yml", + content: "invalid:\n - eval(a)\n", + }, + { + path: ".tests/no-eval-20260301-test.yml", + content: "invalid:\n - eval(b)\n", + }, + ]); + expect(result.unconverted).toEqual([]); + expect(result.files.slice(1)).toEqual([ + { path: "tests/fail/fail-01/fail-01.ts", content: "eval(a)\n" }, + { path: "tests/fail/fail-02/fail-02.ts", content: "eval(b)\n" }, + ]); + }); + it("passes every non-fixture file through byte-identical", () => { const signed = { path: "no-eval.yml", content: SG_RULE }; const result = convert("sg", "no-eval", [ From 2db039a2f1c39a13b080592f0f1c999f1e5738ec Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 26 Oct 2026 11:44:36 -0700 Subject: [PATCH 3/3] fix(rules): convert fixtures without changing their extension or sharing a path From the follow-up review round: - A derived path keeps the file's extension or there is none: under [*.md] a .txt document was written as .md and linted as Markdown. A wildcard-only last segment (src/*, [*]) takes the file's extension. - Delivery's conversion claims every case the served set already carries and every name a moved fixture keeps, so a converted case no longer merges into an existing fail-01 or lands on a moved file's path. - sg test's __snapshots__ are dropped and named, not moved into tests/. - Vale enables a rule with a level as well as YES, so both admit. - A negated character class is filled with a character it admits. --- packages/cli/src/rules/fixture-convert.ts | 80 +++++++++++++++++++---- packages/cli/test/fixture-convert.test.ts | 79 +++++++++++++++++++++- 2 files changed, 145 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/rules/fixture-convert.ts b/packages/cli/src/rules/fixture-convert.ts index 9b8217a2..4493080f 100644 --- a/packages/cli/src/rules/fixture-convert.ts +++ b/packages/cli/src/rules/fixture-convert.ts @@ -104,10 +104,19 @@ function pathFromGlob(glob: string, stem: string, extension: string): string { .replaceAll("**", stem) .replaceAll("*", stem) .replaceAll("?", "x") - .replaceAll(/\[!?([^\]])[^\]]*\]/g, "$1"); + .replaceAll( + /\[(!?)([^\]]*)\]/g, + (_class, negated: string, members: string) => + negated === "" ? (members[0] ?? "x") : classOutsider(members) + ); return path; } +/** A character a negated class `[!members]` admits. */ +function classOutsider(members: string): string { + return [..."xyzq0"].find((candidate) => !members.includes(candidate)) ?? "_"; +} + /** Whether `path` is admitted by `scope`, in `node:path`'s glob dialect. */ function admitted(path: string, scope: FixtureScope): boolean { const included = @@ -120,10 +129,13 @@ function admitted(path: string, scope: FixtureScope): boolean { * A path for a fixture file named `stem.extension` that `scope` admits, or * `undefined` when none of the derivations could be checked. * - * A path keeping `.extension` is preferred over an earlier glob's that does - * not: both engines choose how to read a file by its extension, so under - * `[*.md]` and `[*.txt]` a `.txt` document written as `.md` would be linted - * as Markdown. + * The file always keeps `.extension`: both engines choose how to read a file + * by its extension, so under `[*.md]` a `.txt` document written as `.md` + * would be linted as Markdown, and a TypeScript snippet under `lib/*.js` would + * be parsed as JavaScript. A glob whose last segment names no extension + * (`src/*`, `[*]`) gets `.extension` appended, which it still admits. With no + * glob that admits the extension, there is no path, and the fixture is left + * unconverted rather than converted into a different language. */ export function deriveFixturePath( scope: FixtureScope, @@ -134,10 +146,12 @@ export function deriveFixturePath( scope.include.length === 0 ? [`${stem}.${extension}`] : scope.include.map((glob) => pathFromGlob(glob, stem, extension)); - const candidates = [ - ...derived.filter((path) => path.endsWith(`.${extension}`)), - ...derived.filter((path) => !path.endsWith(`.${extension}`)), - ]; + const candidates = derived.flatMap((path) => { + if (path.endsWith(`.${extension}`)) return [path]; + // A last segment that was all wildcard (`*`, `src/*`) names no extension, + // so it takes this file's. One naming another extension cannot hold it. + return path.endsWith(stem) ? [`${path}.${extension}`] : []; + }); return candidates.find( (candidate) => candidate !== "" && @@ -180,6 +194,26 @@ export interface ConvertedFile { */ export type ClaimedCases = Set; +/** + * The cases `paths` already occupy, for seeding a conversion's + * {@link ClaimedCases}: every `///…` path claims + * `/`. A conversion that ignored them would number its first + * case `fail-01` beside, or into, a case of that name already there. + */ +export function claimedCasesOf( + paths: Iterable, + testsDirectory: string +): ClaimedCases { + const claimed: ClaimedCases = new Set(); + for (const path of paths) { + const [directory, bucket, name] = path.split("/"); + if (directory !== testsDirectory || name === undefined) continue; + if (bucket === "pass" || bucket === "fail") + claimed.add(`${bucket}/${name}`); + } + return claimed; +} + /** The first ``, `-2`, `-3`, … not yet in `claimed`. */ function claimCase( claimed: ClaimedCases, @@ -306,7 +340,9 @@ export function convertSgTestFile( /** * The scope a Vale rule's `.vale.ini` declares: every matcher assigning - * `. = YES` admits, every one assigning `NO` excludes. + * `. = NO` excludes, and every one assigning anything else admits, + * since Vale enables a rule with `YES` or with a level (`suggestion`, + * `warning`, `error`). * * Positional precedence is flattened, deliberately. A path is admitted when * some enabling matcher matches it and no disabling one does, which is @@ -320,8 +356,8 @@ export function valeFixtureScope(ruleId: string, config: string): FixtureScope { for (const node of section.nodes) { if (node.kind !== "property" || node.key !== key) continue; const value = node.value.trim().toUpperCase(); - if (value === "YES") scope.include.push(section.name); if (value === "NO") scope.exclude.push(section.name); + else scope.include.push(section.name); } } return scope; @@ -386,7 +422,12 @@ export interface LegacyFile { * * A fixture that cannot be placed keeps its bytes and moves to the same path * under `tests/`, so the CLI still never writes `.tests/`. `verify` then names - * it, and `unconverted` says why, for the delivery to warn with. + * it, and `unconverted` says why, for the delivery to warn with. Case names + * are claimed past every case the set already carries under `tests/` and + * every name a moved fixture lands on, so no two fixtures share a path. + * + * `sg test`'s `__snapshots__/` is dropped and named: it records the output of + * the runner this layout replaced, and nothing reads it. */ export function convertLegacyFileSet( engine: "sg" | "vale" | "runtime", @@ -402,7 +443,16 @@ export function convertLegacyFileSet( const kept = files.filter((file) => !file.path.startsWith(prefix)); const converted: LegacyFile[] = []; const unconverted: string[] = []; - const claimed: ClaimedCases = new Set(); + const claimed = claimedCasesOf( + [ + ...kept.map((file) => file.path), + // A moved fixture keeps its name under its bucket, so claim it first. + ...legacy.map( + (file) => `${testsDirectory}/${file.path.slice(prefix.length)}` + ), + ], + testsDirectory + ); const moveAsServed = (file: LegacyFile) => { converted.push({ path: `${testsDirectory}/${file.path.slice(prefix.length)}`, @@ -425,6 +475,10 @@ export function convertLegacyFileSet( const rest = file.path.slice(prefix.length); const segments = rest.split("/"); if (engine === "sg") { + if (segments[0] === "__snapshots__") { + unconverted.push(`${rest}: an \`sg test\` snapshot, dropped`); + continue; + } const rule = sibling(`${ruleId}.yml`); if (segments.length !== 1 || rule === undefined) { unconverted.push(`${rest}: not an ast-grep test file beside its rule`); diff --git a/packages/cli/test/fixture-convert.test.ts b/packages/cli/test/fixture-convert.test.ts index cc99b714..a4ba6a94 100644 --- a/packages/cli/test/fixture-convert.test.ts +++ b/packages/cli/test/fixture-convert.test.ts @@ -64,12 +64,37 @@ describe("deriveFixturePath", () => { ); }); - it("prefers a glob that keeps the file's extension over an earlier one", () => { + it("takes the glob that keeps the file's extension over an earlier one", () => { expect(deriveFixturePath(scope(["*.md", "*.txt"]), "hedges", "txt")).toBe( "hedges.txt" ); }); + it("never changes the file's extension to fit a glob", () => { + // Both engines read a file by its extension: a .txt document under + // [*.md] would be linted as Markdown, a TypeScript snippet under lib/*.js + // parsed as JavaScript. + expect(deriveFixturePath(scope(["*.md"]), "a", "txt")).toBeUndefined(); + expect( + deriveFixturePath( + scope(["src/**/*.ts", "lib/*.js"], ["src/**"]), + "f", + "ts" + ) + ).toBeUndefined(); + }); + + it("gives a wildcard-only last segment the file's extension", () => { + expect(deriveFixturePath(scope(["*"]), "a", "md")).toBe("a.md"); + expect(deriveFixturePath(scope(["src/*"]), "a.b", "ts")).toBe("src/a.b.ts"); + }); + + it("fills a negated character class with a character it admits", () => { + expect(deriveFixturePath(scope(["src/[!_]*.ts"]), "f", "ts")).toBe( + "src/xf.ts" + ); + }); + it("returns undefined when an ignore excludes every candidate", () => { expect( deriveFixturePath(scope(["src/**/*.ts"], ["src/**"]), "fail-01", "ts") @@ -263,6 +288,14 @@ describe("valeFixtureScope", () => { }); }); + it("admits under a matcher that enables the rule with a level", () => { + const config = "[docs/*.md]\nhedge.hedge = error\n"; + expect(valeFixtureScope("hedge", config)).toEqual({ + include: ["docs/*.md"], + exclude: [], + }); + }); + it("ignores another rule's key", () => { expect( valeFixtureScope("hedge", "[*.md]\nother.other = YES\n").include @@ -365,6 +398,50 @@ describe("convertLegacyFileSet", () => { ]); }); + it("numbers past the cases the set already carries under tests/", () => { + const result = convert("sg", "no-eval", [ + { path: "no-eval.yml", content: SG_RULE }, + { path: "tests/fail/fail-01/fail-01.ts", content: "eval(old)\n" }, + { path: ".tests/no-eval-test.yml", content: "invalid:\n - eval(new)\n" }, + ]); + expect(result.files.map((file) => file.path)).toEqual([ + "no-eval.yml", + "tests/fail/fail-01/fail-01.ts", + "tests/fail/fail-02/fail-02.ts", + ]); + }); + + it("never converts a case onto a name a moved fixture keeps", () => { + // An extensionless Vale document cannot be converted and moves as served + // to tests/pass/a; the converted a.md must not become tests/pass/a/a.md. + const result = convert("vale", "hedge", [ + { + path: ".vale.ini", + content: "[*]\ntskl) rule = hedge\nhedge.hedge = YES\n", + }, + { path: ".tests/pass/a", content: "x\n" }, + { path: ".tests/pass/a.md", content: "Fine.\n" }, + ]); + const paths = result.files.map((file) => file.path); + expect(paths).toContain("tests/pass/a"); + expect(paths).toContain("tests/pass/a-2/a.md"); + }); + + it("drops sg test snapshots, and names them", () => { + const result = convert("sg", "no-eval", [ + { path: "no-eval.yml", content: SG_RULE }, + { path: ".tests/__snapshots__/no-eval-snapshot.yml", content: "x\n" }, + { path: ".tests/no-eval-test.yml", content: "invalid:\n - eval(x)\n" }, + ]); + expect(result.files.map((file) => file.path)).toEqual([ + "no-eval.yml", + "tests/fail/fail-01/fail-01.ts", + ]); + expect(result.unconverted).toEqual([ + "__snapshots__/no-eval-snapshot.yml: an `sg test` snapshot, dropped", + ]); + }); + it("passes every non-fixture file through byte-identical", () => { const signed = { path: "no-eval.yml", content: SG_RULE }; const result = convert("sg", "no-eval", [