From d6d80ae97febeffe772f5771d7834e8471bada43 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 24 Aug 2026 21:11:55 -0700 Subject: [PATCH 1/2] fix(cli): validate an sg rule's language field in verify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vendored ast-grep rule schema types `language:` as a bare string with no enum and `verify` never read it, so any spelling returned `ok: true` and the binary was the first thing with an opinion — at `check` time. An unrecognized name aborts config parsing and takes every other sg rule down with it; a recognized name for the wrong parser reports nothing and reads as a clean codebase. `verify` now resolves the field the way ast-grep does: case-insensitively, against the canonical list plus the sixteen aliases the binary accepts. An unresolvable name is an error naming the accepted spellings; a resolvable but off-list one is a notice naming the canonical spelling, so the lowercase `typescript` rules already in the wild keep passing. The Tsx/TypeScript split is checked against `files:`, which is the form that trap takes in practice. The alias table is the one list `sg run -h` will not enumerate, so it is pinned by probing: the vendor contract feeds every alias to the binary through a real config and reads the resolution back out of the scan stream's own `language` field, and sweeps a set of near-misses to catch a bump that adds one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017cEN93Acyp4zBwP3oDnyy1 --- .changeset/verify-sg-language.md | 28 +++ packages/cli/src/rules/capabilities.ts | 117 ++++++++++- packages/cli/src/rules/inspect.ts | 21 +- packages/cli/src/rules/verify.ts | 170 +++++++++++++++- packages/cli/src/schemas/rules-verify.ts | 20 +- .../cli/test/ast-grep-vendor-contract.test.ts | 156 ++++++++++++++- packages/cli/test/verify.test.ts | 184 ++++++++++++++++++ 7 files changed, 676 insertions(+), 20 deletions(-) create mode 100644 .changeset/verify-sg-language.md diff --git a/.changeset/verify-sg-language.md b/.changeset/verify-sg-language.md new file mode 100644 index 00000000..87124ee7 --- /dev/null +++ b/.changeset/verify-sg-language.md @@ -0,0 +1,28 @@ +--- +"@taskless/cli": patch +--- + +Validate an sg rule's `language:` field in `verify`, instead of leaving it to +ast-grep at `check` time. + +Nothing local had an opinion on the field. The vendored rule schema types it as +a bare string with no enum, so `verify` returned `ok: true` for any spelling and +the binary was the first thing to object — in the two ways it objects, both of +them late: + +- A name ast-grep does not recognize fails `SgLang` deserialization, which + aborts parsing of the single config Taskless assembles per run. One typo takes + every _other_ sg rule down with it. `verify` now fails that rule by name, + prints the accepted spellings, and suggests the obvious canonical one where + there is one (`C#` → `CSharp`). +- A recognized name pointing at the wrong parser reports nothing and reads as a + clean codebase. `Tsx` and `TypeScript` are two parsers, not aliases, so a + `TypeScript` rule scoped to `**/*.tsx` matches nothing and exits zero. + `verify` fails that rule, and notices the half-dead case where a `{ts,tsx}` + glob reaches both. + +Case variants and ast-grep's extension aliases are accepted rather than +rejected, since ast-grep accepts them itself: `typescript`, `TYPESCRIPT` and +`ts` all reach TypeScript. They get a notice naming the canonical spelling, so +rules already written the lowercase way — including the ones in this +repository — keep passing. diff --git a/packages/cli/src/rules/capabilities.ts b/packages/cli/src/rules/capabilities.ts index 25c60a33..004dae02 100644 --- a/packages/cli/src/rules/capabilities.ts +++ b/packages/cli/src/rules/capabilities.ts @@ -13,8 +13,10 @@ * render time: * * - `src/generated/ast-grep-rule-schema.json` types `$defs.Language` as a bare - * string with no enum, and `verify` never validates a rule's `language`, so - * any spelling passes our own checks and fails only inside ast-grep. + * string with no enum — its only hint is an `example` reading `"typescript"`, + * which is not even the canonical spelling — so the vendored schema cannot + * answer the question. `verify` answers it from the constants below instead + * (see `validateLanguage` in `verify.ts`). * - `detect --json` reports the *repository's* languages in a different * vocabulary — `C++` where ast-grep says `Cpp` — and says nothing about what * an engine can parse. @@ -46,13 +48,14 @@ export const AST_GREP_VERSION = "0.41.0"; * * SPELLINGS ARE ast-grep's, NOT ours and not `detect`'s. `Cpp`, `CSharp`, * `JavaScript`, `Tsx` — a rule's `language:` field is handed to ast-grep - * unchanged and `verify` does not check it, so the binary is the first thing - * with an opinion. MEASURED at 0.41.0: it accepts some off-list aliases - * (`C++` and `cpp` both resolve to Cpp), so an off-list spelling is not - * reliably an error. The two real failures are a name ast-grep does not know - * at all (`C#`), which aborts config parsing so every rule goes unreported, - * and a valid name for the wrong parser (`TypeScript` over `.tsx`), which - * reports nothing and reads as a clean codebase. Neither is caught locally. + * unchanged, so the binary has the final opinion. MEASURED at 0.41.0: it + * accepts more than this list — case variants and a fixed set of extension + * aliases, both enumerated in {@link AST_GREP_LANGUAGE_ALIASES} — so an + * off-list spelling is not on its own an error. The two real failures are a + * name ast-grep does not know at all (`C#`), which aborts config parsing so + * every rule goes unreported, and a valid name for the wrong parser + * (`TypeScript` over `.tsx`), which reports nothing and reads as a clean + * codebase. `verify` catches both; see `verify.ts`. * * Pinned by set-equality against the binary in * `test/ast-grep-vendor-contract.test.ts`, so a version bump that adds or drops @@ -88,6 +91,102 @@ export const AST_GREP_LANGUAGES = [ "Yaml", ] as const; +/** One of the spellings {@link AST_GREP_LANGUAGES} lists, canonically cased. */ +export type AstGrepLanguage = (typeof AST_GREP_LANGUAGES)[number]; + +/** + * The spellings ast-grep also accepts that are not on the canonical list, + * mapped to the language each resolves to. + * + * Keys are lowercase because ast-grep's own matching is case-insensitive: + * `TYPESCRIPT`, `Cs` and `GOLANG` all resolve at 0.41.0. That makes the whole + * accepted vocabulary "the canonical list plus these, compared lowercased", + * which is what {@link resolveAstGrepLanguage} implements. + * + * A RULE'S `language:` FIELD AND `sg run --lang` DO NOT SHARE A VOCABULARY. + * Measured at 0.41.0: `--lang C++` is rejected outright while a rule declaring + * `language: C++` parses fine. Every value here was probed through a real + * config, because that is the only thing a rule file is ever fed to. + * + * THIS IS THE ONE LIST HERE THE BINARY CANNOT BE ASKED TO ENUMERATE. `sg run + * -h` prints the canonical list, so `AST_GREP_LANGUAGES` above is checked by + * set-equality against it; nothing prints the aliases. Each entry is instead + * pinned by *probing*, in the "language aliases" suite of + * `test/ast-grep-vendor-contract.test.ts`: every key is fed to the binary in a + * config and the resolution is read back out of the scan stream's own + * `language` field — ast-grep reports the canonical name it settled on, so the + * mapping is the binary's answer rather than ours. The same suite feeds a + * sweep of near-misses (`h`, `mjs`, `sh`, `tf`, `csx`) and asserts they are + * rejected, so a bump that ADDS an alias fails there too. + * + * Whitespace is not folded, deliberately: `language: "ts "` is rejected by the + * binary, so accepting it here would pass a rule that cannot run. + */ +export const AST_GREP_LANGUAGE_ALIASES: Readonly< + Record +> = { + "c++": "Cpp", + cc: "Cpp", + cs: "CSharp", + cxx: "Cpp", + ex: "Elixir", + golang: "Go", + hs: "Haskell", + js: "JavaScript", + jsx: "JavaScript", + kt: "Kotlin", + py: "Python", + rb: "Ruby", + rs: "Rust", + sol: "Solidity", + ts: "TypeScript", + yml: "Yaml", +}; + +/** Every canonical name, keyed by its own lowercase spelling. */ +const CANONICAL_BY_LOWERCASE = new Map( + AST_GREP_LANGUAGES.map((name) => [name.toLowerCase(), name]) +); + +/** + * The language ast-grep would parse `spelling` as, or `undefined` if it would + * reject the config outright. + * + * `undefined` is the fatal case, not a stylistic one: an unrecognized name + * fails `SgLang` deserialization, which aborts parsing of the single config + * Taskless assembles for the run — so every *other* sg rule goes unreported + * with it. + */ +export function resolveAstGrepLanguage( + spelling: string +): AstGrepLanguage | undefined { + // NOT trimmed. Measured at 0.41.0, ast-grep rejects `"ts "` — folding the + // whitespace here would call a rule valid that the binary refuses to load. + const key = spelling.toLowerCase(); + return CANONICAL_BY_LOWERCASE.get(key) ?? AST_GREP_LANGUAGE_ALIASES[key]; +} + +/** + * The `.ts` / `.tsx` split — the one pair of ast-grep languages that share a + * family and read disjoint file extensions. + * + * MEASURED at 0.41.0: a `TypeScript` rule over a `.tsx` tree exits zero having + * matched nothing, and a `Tsx` rule scans `.tsx` only. That is the quiet + * failure of the two, because "no findings" is exactly what a clean codebase + * looks like. Pinned by "treats Tsx and TypeScript as different parsers, not + * aliases" in `test/ast-grep-vendor-contract.test.ts`. + * + * Kept to this pair deliberately. Every other language's extensions would be a + * second vendored table with no measured backing, and the trap only exists + * where two languages look like spellings of one thing. + */ +export const AST_GREP_TSX_SPLIT: Readonly< + Partial> +> = { + TypeScript: "ts", + Tsx: "tsx", +}; + /** * The Vale release carried by the `@taskless/vale-` packages pinned * in `packages/cli/package.json`. Their npm versions append a build stamp diff --git a/packages/cli/src/rules/inspect.ts b/packages/cli/src/rules/inspect.ts index 28b042fe..61ce04b2 100644 --- a/packages/cli/src/rules/inspect.ts +++ b/packages/cli/src/rules/inspect.ts @@ -19,6 +19,14 @@ export interface RuleVerification { ruleId: string; ok: boolean; errors: string[]; + /** + * Something true about the rule that does not make it invalid. An sg rule + * spelled `language: typescript` reaches the right parser and fails nothing, + * but the canonical spelling is `TypeScript` — worth saying, not worth + * failing. Surfaced even on a pass, for the same reason + * {@link RuleTestResult.notice} is. + */ + notice?: string; } /** What `test` concluded about one rule. */ @@ -65,7 +73,15 @@ async function verifySgRule( // test layer is `test`'s business, so it is not part of the verdict here. const errors = [...result.schema.errors, ...result.requirements.errors]; return { - verification: { engine: "sg", ruleId, ok: errors.length === 0, errors }, + verification: { + engine: "sg", + ruleId, + ok: errors.length === 0, + errors, + ...(result.schema.notice === undefined + ? {} + : { notice: result.schema.notice }), + }, result, }; } @@ -233,6 +249,9 @@ export async function testOneRule( ok: result.tests.valid, errors, ran: true, + ...(verification.notice === undefined + ? {} + : { notice: verification.notice }), }; } diff --git a/packages/cli/src/rules/verify.ts b/packages/cli/src/rules/verify.ts index 281066b7..6f3a2811 100644 --- a/packages/cli/src/rules/verify.ts +++ b/packages/cli/src/rules/verify.ts @@ -11,6 +11,13 @@ import { TASKLESS_REQUIRED_FIELDS, findRegexWithoutKind, } from "../schemas/ast-grep-rule"; +import { + AST_GREP_TSX_SPLIT, + AST_GREP_VERSION, + astGrepLanguageList, + resolveAstGrepLanguage, + type AstGrepLanguage, +} from "./capabilities"; import { ensureTasklessDirectory } from "../filesystem/directory"; import { assembleSgConfig } from "./assemble"; import { @@ -38,6 +45,19 @@ export interface LayerResult { errors: string[]; } +/** + * Layer 1's verdict, plus anything true about the rule that is worth saying + * without failing it. + * + * The `notice` carries the non-fatal half of the `language:` check — an + * accepted-but-off-list spelling, or a `files:` glob a valid language cannot + * reach. Separate from `errors` for the same reason `RuleTestResult.notice` is: + * it must be sayable on a rule that passed, and it must not turn CI red. + */ +export interface SchemaLayerResult extends LayerResult { + notice?: string; +} + export interface RequirementsResult extends LayerResult { hasTestFile: boolean; } @@ -77,7 +97,7 @@ export interface TestLayerResult extends LayerResult { export interface VerifyResult { success: boolean; ruleId: string; - schema: LayerResult; + schema: SchemaLayerResult; requirements: LayerResult; tests: TestLayerResult; } @@ -114,6 +134,131 @@ function validateSchema(ruleData: unknown): LayerResult { return { valid: false, errors }; } +/** + * The canonical language a misspelling was probably reaching for, if there is + * an obvious one. + * + * Deliberately not a fuzzy match. It folds exactly the two shapes a human + * types when the canonical name is a word for a symbol — `C#` and `c-sharp` + * both fold to `csharp` — and otherwise offers nothing, because a wrong guess + * here costs more than silence: the full accepted list is printed either way. + */ +function suggestLanguage(spelling: string): AstGrepLanguage | undefined { + return resolveAstGrepLanguage( + spelling + .toLowerCase() + .replaceAll("#", "sharp") + .replaceAll(/[\s_-]+/g, "") + ); +} + +/** + * The file extensions a `files:` glob explicitly names, lowercased. + * + * Only what the glob *states*. `src/**` names none and is not evidence of + * anything, which is the point: this feeds a check that must never fire on a + * rule whose scope it cannot actually read. + */ +function globExtensions(glob: string): string[] { + const segment = glob.split("/").at(-1) ?? ""; + const dot = segment.lastIndexOf("."); + if (dot === -1) return []; + const suffix = segment.slice(dot + 1).toLowerCase(); + // `**\/*.{ts,tsx}` — globset supports brace alternation, so one glob can name + // several extensions. + const braced = /^\{([^}]*)\}$/.exec(suffix); + return (braced?.[1] === undefined ? [suffix] : braced[1].split(",")) + .map((extension) => extension.trim()) + .filter((extension) => /^[a-z\d]+$/.test(extension)); +} + +/** + * The `language:` field, against what ast-grep will actually do with it. + * + * Nothing else in the pipeline asks. The vendored JSON Schema types the field + * as a bare string (its only hint is an `example` of `"typescript"`, which is + * not even the canonical spelling), so Layer 1's zod pass accepts anything, and + * the first component with an opinion is the binary at `check` time. Both of + * its verdicts are bad places to learn this: + * + * - A name ast-grep does not recognize fails `SgLang` deserialization, which + * aborts parsing of the ONE config Taskless assembles per run. Every other sg + * rule goes unreported with it, so a single typo blinds the engine. That is + * an error here. + * - A recognized name for the wrong parser reports nothing at all and is + * indistinguishable from a clean codebase. Only the `TypeScript`/`Tsx` pair + * can be checked from the rule file alone — see {@link AST_GREP_TSX_SPLIT} — + * and it is checked against `files:`, which is the form the trap takes in + * practice. + * + * Case variants and ast-grep's extension aliases are ACCEPTED, not rejected. + * ast-grep resolves them itself (`typescript`, `TYPESCRIPT`, `ts` all reach + * TypeScript at 0.41.0), so failing them would fail rules that demonstrably + * work — including every rule already written against the lowercase spelling + * ast-grep's own schema shows as its example. They get a notice instead. + * + * See taskless/cli#165. + */ +function validateLanguage(ruleData: Record): { + errors: string[]; + notices: string[]; +} { + const errors: string[] = []; + const notices: string[] = []; + + const declared = ruleData.language; + // A missing or non-string `language` is already reported — by Layer 2's + // required-fields pass and by zod respectively — and saying it twice in + // different words would only make the real message harder to find. + if (typeof declared !== "string" || declared === "") { + return { errors, notices }; + } + + const canonical = resolveAstGrepLanguage(declared); + if (canonical === undefined) { + const suggestion = suggestLanguage(declared); + errors.push( + `language: "${declared}" is not a language ast-grep ${AST_GREP_VERSION} accepts. ` + + `It aborts config parsing, so every other sg rule in the project goes unreported too. ` + + (suggestion === undefined ? "" : `Did you mean "${suggestion}"? `) + + `Accepted spellings: ${astGrepLanguageList()}.` + ); + return { errors, notices }; + } + + if (declared !== canonical) { + notices.push( + `language: "${declared}" works — ast-grep resolves it to ${canonical} — but ${canonical} is how ast-grep spells it.` + ); + } + + const own = AST_GREP_TSX_SPLIT[canonical]; + const files = ruleData.files; + if (own !== undefined && Array.isArray(files)) { + const sibling = own === "ts" ? "tsx" : "ts"; + const siblingLanguage = own === "ts" ? "Tsx" : "TypeScript"; + const named = new Set( + files + .filter((glob): glob is string => typeof glob === "string") + .flatMap((glob) => globExtensions(glob)) + ); + if (named.has(sibling)) { + const message = + `${canonical} does not parse .${sibling} files — ${siblingLanguage} is a separate parser, not an alias, ` + + `so those globs match nothing and check reports a clean codebase.`; + if (named.has(own)) { + notices.push(`files: some globs name .${sibling}. ${message}`); + } else { + errors.push( + `files: every glob names .${sibling}, but language is ${canonical}. ${message}` + ); + } + } + } + + return { errors, notices }; +} + // --- Layer 2: Taskless requirements --- async function validateRequirements( @@ -550,17 +695,28 @@ export async function verifyRule( }; } - // Layer 1 - const schemaResult = validateSchema(ruleData); + const ruleRecord = ( + ruleData && typeof ruleData === "object" && !Array.isArray(ruleData) + ? ruleData + : {} + ) as Record; + + // Layer 1, plus the `language:` field the vendored JSON Schema cannot type. + const parsed = validateSchema(ruleData); + const language = validateLanguage(ruleRecord); + const schemaResult: SchemaLayerResult = { + valid: parsed.valid && language.errors.length === 0, + errors: [...parsed.errors, ...language.errors], + ...(language.notices.length === 0 + ? {} + : { notice: language.notices.join(" ") }), + }; // Layer 2 const requirementsResult = await validateRequirements( cwd, ruleId, - (ruleData && typeof ruleData === "object" ? ruleData : {}) as Record< - string, - unknown - > + ruleRecord ); // Layer 3 — only when asked for, and only if a test file exists (Layer 2 diff --git a/packages/cli/src/schemas/rules-verify.ts b/packages/cli/src/schemas/rules-verify.ts index 45f6ace0..0b5906cf 100644 --- a/packages/cli/src/schemas/rules-verify.ts +++ b/packages/cli/src/schemas/rules-verify.ts @@ -36,6 +36,22 @@ const layerResultSchema = z.object({ errors: z.array(z.string()).describe("Human-readable error messages"), }); +/** + * Layer 1, which alone can also report something true that is not a failure. + * + * An sg rule spelled `language: typescript` reaches the right parser and + * verifies clean, but `TypeScript` is how ast-grep spells it. That is worth + * saying on a rule that passed, so it cannot ride in `errors`. + */ +const schemaLayerResultSchema = layerResultSchema.extend({ + notice: z + .string() + .optional() + .describe( + "Something true about the rule that does not make it invalid — an accepted-but-off-list `language:` spelling above all. Present only when there is something to say" + ), +}); + const testLayerResultSchema = layerResultSchema.extend({ passed: z.number().describe("Number of test cases that passed"), failed: z.number().describe("Number of test cases that failed"), @@ -49,7 +65,9 @@ export const verifyOutputSchema = z.object({ ), success: z.boolean().describe("True if all layers passed"), ruleId: z.string(), - schema: layerResultSchema.describe("Layer 1: Zod schema validation"), + schema: schemaLayerResultSchema.describe( + "Layer 1: Zod schema validation, plus the `language:` check the vendored JSON Schema cannot express" + ), requirements: layerResultSchema.describe( "Layer 2: Taskless requirement checks" ), diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts index 52198d42..3699f51f 100644 --- a/packages/cli/test/ast-grep-vendor-contract.test.ts +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -7,9 +7,11 @@ import { afterEach, describe, expect, it } from "vitest"; import { assembleSgConfig } from "../src/rules/assemble"; import { + AST_GREP_LANGUAGE_ALIASES, AST_GREP_LANGUAGES, AST_GREP_VERSION, astGrepLanguageList, + resolveAstGrepLanguage, } from "../src/rules/capabilities"; import { ruleDirectory, ruleTestsDirectory } from "../src/rules/engines"; import { buildPath, findSgBinary } from "../src/rules/scan"; @@ -157,6 +159,110 @@ const evalSource = { "src/a.ts": 'const x = eval("1");\n' }; const atLanguage = (language: string) => rule("no-eval").replace("language: TypeScript", `language: ${language}`); +/** + * A rule declaring `language` verbatim, over a pattern given verbatim. + * + * `atLanguage` cannot serve the alias cases: it keeps `eval($$$A)`, which only + * parses as JavaScript-family source. Each language below needs a pattern its + * own grammar accepts. + */ +const langRule = (language: string, pattern: string) => + [ + "id: lang", + `language: ${language}`, + "severity: error", + "message: lang", + "note: n", + "rule:", + ` pattern: ${pattern}`, + "", + ].join("\n"); + +/** + * One file per language that has an alias, holding an identifier `zzz` the + * pattern beside it matches. + * + * Sources are deliberately the smallest thing each grammar accepts, because + * the assertion is about which parser ast-grep picked, not about matching. + * `CSharp` is the one that needs more than a bare identifier pattern — measured + * at 0.41.0, `pattern: zzz` parses but matches nothing there. + */ +const languageFixture: Record< + string, + { file: string; source: string; pattern: string } +> = { + Cpp: { file: "src/a.cpp", source: "int zzz = 1;\n", pattern: "zzz" }, + CSharp: { + file: "src/a.cs", + source: "class A { void M() { int zzz = 1; } }\n", + pattern: "int zzz = 1", + }, + Elixir: { file: "src/a.ex", source: "zzz = 1\n", pattern: "zzz" }, + Go: { + file: "src/a.go", + source: "package main\n\nvar zzz = 1\n", + pattern: "zzz", + }, + Haskell: { file: "src/a.hs", source: "zzz = 1\n", pattern: "zzz" }, + JavaScript: { file: "src/a.js", source: "var zzz = 1\n", pattern: "zzz" }, + Kotlin: { file: "src/a.kt", source: "val zzz = 1\n", pattern: "zzz" }, + Python: { file: "src/a.py", source: "zzz = 1\n", pattern: "zzz" }, + Ruby: { file: "src/a.rb", source: "zzz = 1\n", pattern: "zzz" }, + Rust: { + file: "src/a.rs", + source: "fn main() { let zzz = 1; }\n", + pattern: "zzz", + }, + Solidity: { + file: "src/a.sol", + source: "contract A { uint zzz = 1; }\n", + pattern: "zzz", + }, + TypeScript: { file: "src/a.ts", source: "var zzz = 1\n", pattern: "zzz" }, + Yaml: { file: "src/a.yml", source: "zzz: 1\n", pattern: "zzz" }, +}; + +/** + * The canonical language ast-grep says it used, for a rule that declared + * `spelling`. + * + * READ OUT OF THE BINARY, NOT ASSUMED. Every match on the `--json=stream` + * output carries a `language` field naming the parser that produced it, so an + * alias's resolution is ast-grep's own answer rather than a claim of ours + * inferred from which files got scanned. + */ +const resolvedLanguage = (spelling: string, expected: string) => { + const fixture = languageFixture[expected]; + if (fixture === undefined) throw new Error(`no fixture for ${expected}`); + const line = scan( + project({ + rules: { lang: langRule(spelling, fixture.pattern) }, + sources: { [fixture.file]: fixture.source }, + }) + ) + .stdout.split("\n") + .find((text) => text !== ""); + return line === undefined + ? undefined + : (JSON.parse(line) as { language: string }).language; +}; + +/** + * Whether ast-grep recognized `language: ` as a language at all. + * + * KEYED ON `SgLang`, NOT ON THE EXIT STATUS. Every rule that fails to load + * exits 8 with the same top-line `Cannot parse rule` message, and `pattern: + * zzz` is legitimately unparseable in several grammars (`Html` wants a `kind`) + * — so status alone cannot tell "not a language" from "not a pattern". The + * `Caused by` chain does: `did not match any variant of untagged enum SgLang` + * appears only for an unrecognized name, and it is the exact failure this + * whole suite exists to keep out of `check`. + */ +const languageRecognized = (spelling: string) => + !scan( + project({ rules: { lang: langRule(spelling, "zzz") } }) + ).stderr.includes("SgLang"); + /** Four calls at increasing arity, one per line, for the `$$$` cases below. */ const aritySource = { "src/a.ts": "foo();\nfoo(1);\nfoo(1,2);\nfoo(1,2,3);\n", @@ -570,6 +676,51 @@ withSg("ast-grep vendor contract", () => { .stdout ).toContain("eval(x)"); }); + + it("accepts every canonical name lowercased", () => { + // `verify` compares lowercased (see `resolveAstGrepLanguage`), which is + // only safe because the binary does too. If a bump ever made one name + // case-sensitive, `verify` would start passing a rule ast-grep refuses. + for (const language of AST_GREP_LANGUAGES) { + expect( + languageRecognized(language.toLowerCase()), + `${language.toLowerCase()} was rejected` + ).toBe(true); + } + }); + + it("resolves every alias to the language AST_GREP_LANGUAGE_ALIASES claims", () => { + // The alias table is the ONE thing in `capabilities.ts` the binary + // cannot be asked to enumerate — `sg run -h` prints only the canonical + // list. So each entry is probed instead, and the resolution is read back + // out of the scan stream rather than inferred. + for (const [alias, canonical] of Object.entries( + AST_GREP_LANGUAGE_ALIASES + )) { + expect(resolvedLanguage(alias, canonical), alias).toBe(canonical); + } + }); + + it("rejects the near-misses that are not aliases", () => { + // The direction the table cannot self-check: a bump that ADDS an alias + // leaves `verify` failing a rule that now works. These are the spellings + // an author is most likely to reach for — a header extension, a module + // extension, a shell name, a Terraform name, a script suffix — and all + // five were measured as rejected at 0.41.0. + for (const spelling of ["h", "hpp", "mjs", "cjs", "sh", "tf", "csx"]) { + expect(languageRecognized(spelling), `${spelling} was accepted`).toBe( + false + ); + } + }); + + it("does not fold surrounding whitespace", () => { + // Why `resolveAstGrepLanguage` deliberately does not trim: a trimming + // resolver would call this rule valid and ast-grep would still refuse to + // load the config. + expect(languageRecognized('"ts "')).toBe(false); + expect(resolveAstGrepLanguage("ts ")).toBeUndefined(); + }); }); /** @@ -742,8 +893,9 @@ withSg("ast-grep vendor contract", () => { * The bracketed list from `sg run -h`, which is the only place ast-grep * enumerates this. Not derived from anything we generate: the vendored * `src/generated/ast-grep-rule-schema.json` types `$defs.Language` as a bare - * string with no enum, and `verify` never validates a rule's `language`, so our - * own artifacts cannot answer the question. + * string with no enum, so our own generated artifacts cannot answer the question. + * `verify` does validate a rule's `language` — against `AST_GREP_LANGUAGES` and + * `AST_GREP_LANGUAGE_ALIASES`, which is exactly why both are pinned here. */ function reportedLanguages(): string[] { const help = spawnSync(binary as string, ["run", "-h"], { diff --git a/packages/cli/test/verify.test.ts b/packages/cli/test/verify.test.ts index 9fc34592..701cdaaa 100644 --- a/packages/cli/test/verify.test.ts +++ b/packages/cli/test/verify.test.ts @@ -37,6 +37,32 @@ async function coverageProject( ); } +/** + * `no-eval`, with its `language:` swapped and any extra top-level keys merged + * in. No test file, so Layer 3 is skipped — every case below reads + * `result.schema`, which Layers 2 and 3 cannot reach. + */ +async function ruleWithLanguage( + cwd: string, + language: unknown, + extra: Record = {} +): Promise { + const rulesDirectory = join(cwd, ".taskless", "sg", "rules"); + await mkdir(rulesDirectory, { recursive: true }); + await writeFile( + join(rulesDirectory, "no-eval.yml"), + stringify({ + id: "no-eval", + language, + severity: "error", + message: "Do not use eval()", + rule: { pattern: "eval($$$)" }, + ...extra, + }), + "utf8" + ); +} + describe("verifyRule", () => { let temporaryDirectory: string; @@ -398,6 +424,164 @@ describe("verifyRule", () => { expect(result.tests.failed).toBe(1); }); + /** + * The `language:` field — taskless/cli#165. + * + * Layer 1's zod pass cannot answer any of these: the vendored JSON Schema + * types `language` as a bare string. What ast-grep does with each spelling is + * pinned separately, against the binary, in + * `test/ast-grep-vendor-contract.test.ts`. + */ + describe("the language field", () => { + it("fails a rule whose language ast-grep does not recognize", async () => { + // The loud failure, and the reason it is an error rather than a notice: + // ast-grep cannot deserialize the name, so it abandons the whole config + // and every other sg rule in the project goes unreported with it. + await ruleWithLanguage(temporaryDirectory, "nonsense"); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(false); + expect(result.schema.errors.join("\n")).toContain( + 'language: "nonsense" is not a language ast-grep' + ); + }); + + it("names the accepted spellings in the error", async () => { + // An author who reached for `C#` needs to be told the word `CSharp`, not + // merely that they were wrong. The full list is printed too, so a + // spelling the suggestion cannot fold still lands somewhere useful. + await ruleWithLanguage(temporaryDirectory, "C#"); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + const errors = result.schema.errors.join("\n"); + expect(errors).toContain('Did you mean "CSharp"?'); + expect(errors).toContain("Accepted spellings: Bash, C, Cpp"); + }); + + it("accepts a case variant and says how ast-grep spells it", async () => { + // The deliberate non-breaking half. ast-grep resolves `typescript` + // itself — it is the very spelling its own JSON Schema shows as the + // field's `example` — so rules already written this way keep working and + // hear about the canonical name instead of failing. + await ruleWithLanguage(temporaryDirectory, "typescript"); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(true); + expect(result.schema.notice).toContain( + "TypeScript is how ast-grep spells it" + ); + }); + + it("accepts an extension alias ast-grep resolves", async () => { + // `ts` is not on the canonical list and is not a case variant of + // anything on it, but ast-grep accepts it. Rejecting it would fail a + // rule that demonstrably works — the opposite of the bug being fixed. + await ruleWithLanguage(temporaryDirectory, "ts"); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(true); + expect(result.schema.notice).toContain('"ts" works'); + }); + + it("says nothing at all about the canonical spelling", async () => { + await ruleWithLanguage(temporaryDirectory, "TypeScript"); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(true); + expect(result.schema.notice).toBeUndefined(); + }); + + it("fails a TypeScript rule scoped only to .tsx files", async () => { + // The quiet failure. `Tsx` and `TypeScript` are separate parsers, so + // this rule matches nothing, exits zero, and reads as a clean codebase — + // the state that is indistinguishable from success at check time. + await ruleWithLanguage(temporaryDirectory, "TypeScript", { + files: ["src/**/*.tsx"], + }); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(false); + expect(result.schema.errors.join("\n")).toContain( + "every glob names .tsx, but language is TypeScript" + ); + }); + + it("only notices when some globs do reach the declared language", async () => { + // Half the scope is dead, half works. Failing here would fail a rule + // that reports real findings, so this is worth saying and not worth + // failing — and brace alternation is one glob naming two extensions. + await ruleWithLanguage(temporaryDirectory, "TypeScript", { + files: ["src/**/*.{ts,tsx}"], + }); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(true); + expect(result.schema.notice).toContain("some globs name .tsx"); + }); + + it("says nothing about globs that name no extension", async () => { + // `src/**` states nothing about extensions, so there is nothing to + // conclude. A check that guessed here would fire on correct rules. + await ruleWithLanguage(temporaryDirectory, "TypeScript", { + files: ["src/**"], + }); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(true); + expect(result.schema.notice).toBeUndefined(); + }); + + it("catches the mirror image: Tsx scoped only to .ts", async () => { + await ruleWithLanguage(temporaryDirectory, "Tsx", { + files: ["src/**/*.ts"], + }); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(false); + expect(result.schema.errors.join("\n")).toContain( + "every glob names .ts, but language is Tsx" + ); + }); + + it("leaves a missing language to the required-fields layer", async () => { + // Reported once, in the layer that owns it. Two messages for one + // omission only makes the useful one harder to find. + const rulesDirectory = join( + temporaryDirectory, + ".taskless", + "sg", + "rules" + ); + await mkdir(rulesDirectory, { recursive: true }); + await writeFile( + join(rulesDirectory, "no-eval.yml"), + stringify({ + id: "no-eval", + severity: "error", + message: "Do not use eval()", + rule: { pattern: "eval($$$)" }, + }), + "utf8" + ); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.errors.join("\n")).not.toContain("Accepted"); + expect(result.requirements.errors).toContain( + "Missing required field: language" + ); + }); + }); + describe("fixture coverage", () => { it("does not report success for a rule with no fixtures", async () => { // Both buckets present and both empty. `ast-grep test` calls this From 081db0b0a891fc336597962c17ff1cf788775e5a Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 25 Aug 2026 14:08:02 -0700 Subject: [PATCH 2/2] fix(verify): read both shapes of an sg files: glob entry RuleFileGlob is a plain pattern string OR { glob, caseInsensitive }, and assemble.ts hands files: to ast-grep untouched, so both shapes run. The TypeScript/Tsx wrong-parser scan filtered to strings, so an object entry contributed no extensions: no error and no notice, indistinguishable from a rule whose globs name nothing. The trap the check exists to catch went unreported for one of the two legal shapes. Unwrap { glob } before extracting extensions, and cover the object form and a mixed-shape files: list in the tests. --- .changeset/verify-sg-language.md | 4 ++++ packages/cli/src/rules/verify.ts | 23 ++++++++++++++++++++++- packages/cli/test/verify.test.ts | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/.changeset/verify-sg-language.md b/.changeset/verify-sg-language.md index 87124ee7..f891dd69 100644 --- a/.changeset/verify-sg-language.md +++ b/.changeset/verify-sg-language.md @@ -26,3 +26,7 @@ rejected, since ast-grep accepts them itself: `typescript`, `TYPESCRIPT` and `ts` all reach TypeScript. They get a notice naming the canonical spelling, so rules already written the lowercase way — including the ones in this repository — keep passing. + +The `files:` scan reads both shapes ast-grep allows for a glob entry, the plain +string and the `{ glob, caseInsensitive }` object, so the wrong-parser check is +not silently skipped for rules written the second way. diff --git a/packages/cli/src/rules/verify.ts b/packages/cli/src/rules/verify.ts index 6f3a2811..e799d76f 100644 --- a/packages/cli/src/rules/verify.ts +++ b/packages/cli/src/rules/verify.ts @@ -172,6 +172,26 @@ function globExtensions(glob: string): string[] { .filter((extension) => /^[a-z\d]+$/.test(extension)); } +/** + * The glob string out of one `files:` entry, in either shape ast-grep accepts. + * + * `RuleFileGlob` is `anyOf` a bare pattern string and `{ glob, caseInsensitive? }` + * (see `src/generated/ast-grep-rule-schema.json`), and `assemble.ts` passes + * `files:` through untouched, so both shapes reach the binary as written. + * Reading only the string form would leave the object form scanning nothing, + * which is a silent pass rather than a missed error message: the check below + * fires on what a glob *names*, so an entry it cannot read looks exactly like + * an entry that names nothing. + */ +function globPattern(entry: unknown): string | undefined { + if (typeof entry === "string") return entry; + if (typeof entry === "object" && entry !== null && "glob" in entry) { + const { glob } = entry as { glob: unknown }; + if (typeof glob === "string") return glob; + } + return undefined; +} + /** * The `language:` field, against what ast-grep will actually do with it. * @@ -239,7 +259,8 @@ function validateLanguage(ruleData: Record): { const siblingLanguage = own === "ts" ? "Tsx" : "TypeScript"; const named = new Set( files - .filter((glob): glob is string => typeof glob === "string") + .map((entry) => globPattern(entry)) + .filter((glob): glob is string => glob !== undefined) .flatMap((glob) => globExtensions(glob)) ); if (named.has(sibling)) { diff --git a/packages/cli/test/verify.test.ts b/packages/cli/test/verify.test.ts index 701cdaaa..09af7d68 100644 --- a/packages/cli/test/verify.test.ts +++ b/packages/cli/test/verify.test.ts @@ -552,6 +552,38 @@ describe("verifyRule", () => { ); }); + it("reads the object form of a files: entry", async () => { + // `RuleFileGlob` is a string OR `{ glob, caseInsensitive? }`, and + // assemble.ts passes `files:` to ast-grep untouched, so both shapes run. + // Reading only the string form makes the object form scan nothing, which + // looks identical to a rule whose globs name no extension: no error, no + // notice, and the wrong-parser trap goes unreported. + await ruleWithLanguage(temporaryDirectory, "TypeScript", { + files: [{ glob: "src/**/*.tsx" }], + }); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(false); + expect(result.schema.errors.join("\n")).toContain( + "every glob names .tsx, but language is TypeScript" + ); + }); + + it("mixes the two files: shapes in one rule", async () => { + // Nothing requires an author to pick one form, and `caseInsensitive` + // is the reason to reach for the object one. Half the scope is dead, so + // this is the notice case, exactly as it is for two plain strings. + await ruleWithLanguage(temporaryDirectory, "TypeScript", { + files: ["src/**/*.ts", { glob: "src/**/*.TSX", caseInsensitive: true }], + }); + const result = await verifyRule(temporaryDirectory, "no-eval", { + runTests: false, + }); + expect(result.schema.valid).toBe(true); + expect(result.schema.notice).toContain("some globs name .tsx"); + }); + it("leaves a missing language to the required-fields layer", async () => { // Reported once, in the layer that owns it. Two messages for one // omission only makes the useful one harder to find.