From ada4a501e65cb0bca71f9d6552957e702919cfe4 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 12 Oct 2026 11:27:53 -0700 Subject: [PATCH] docs(cli): collect create-sg-rule's authoring traps into one Quirks section The recipe had its traps in four places: the `language:` traps and the Markdown note in step 4, the separator trap as step 5, and "Two ways a relational rule matches nothing" after the steps. An agent whose rule matches less than it should had no one place to look. They now live under one Quirks section. It opens with the two ways a quirk fails (a config error that aborts the scan, or a silent zero) and a table of every entry with its symptom. Each entry gives the symptom, a repro, and the workaround. Step 5 becomes "check the rule against Quirks", so the check keeps its place in the step order. Two new entries, both measured against ast-grep 0.45.3: - A keyword is not a valid `kind`. `--debug-query=cst` prints `readonly` as a node, but `kind: readonly` exits 8 with "Kind `readonly` is invalid": `kind:` takes named nodes only. `--debug-query=ast` prints named nodes only, which is the reliable way to tell them apart. - An awaited generic call hides its callee. `await rpc("x")` parses as a call of the await expression, so a callee-keyed rule silently skips it. `!`, `void` and `typeof` do the same through `unary_expression`; `yield` does not; Tsx matches TypeScript. The workaround's match spans the `await`, so a `fix:` rewrites it too, measured. The vendor contract pins both against the binary so a grammar bump that fixes either goes red, and agent-extensions pins the recipe text. Fixes #485 --- .changeset/sg-rule-quirks.md | 5 + packages/cli/src/agent/create-sg-rule.md | 266 +++++++++++++----- packages/cli/test/agent-extensions.test.ts | 32 +++ .../cli/test/ast-grep-vendor-contract.test.ts | 84 ++++++ 4 files changed, 316 insertions(+), 71 deletions(-) create mode 100644 .changeset/sg-rule-quirks.md diff --git a/.changeset/sg-rule-quirks.md b/.changeset/sg-rule-quirks.md new file mode 100644 index 00000000..fd15efb5 --- /dev/null +++ b/.changeset/sg-rule-quirks.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +`taskless agent create-sg-rule` now collects its ast-grep authoring traps into one **Quirks** section. Each entry gives the symptom (a config error that aborts the scan, or a rule that silently matches nothing), a minimal repro, and the workaround. Two new entries: a keyword such as `readonly` is not a valid `kind` (read kinds from `--debug-query=ast`, not the CST dump), and in TypeScript `await f(...)` parses as a call of the await expression, so a rule keyed on the callee silently skips every awaited generic call. diff --git a/packages/cli/src/agent/create-sg-rule.md b/packages/cli/src/agent/create-sg-rule.md index 40e41359..24563a50 100644 --- a/packages/cli/src/agent/create-sg-rule.md +++ b/packages/cli/src/agent/create-sg-rule.md @@ -1,4 +1,4 @@ -# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v5) +# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v6) ## You are here This is `create-sg-rule`. It helps you write an ast-grep rule: a check @@ -94,66 +94,21 @@ whole rule. recognizes but that names the wrong parser reports nothing and looks like a clean codebase. - Two specific traps: - - **Do not copy from `detect --json`.** It reports the - *repository's* languages in a different vocabulary. It says - `C++` where the list above says `Cpp`. - - **`Tsx` and `TypeScript` are two parsers, not aliases.** A rule - over `.tsx` files that declares `TypeScript` does not match JSX - syntax; it does not read those files at all. - - **`Markdown` sees blocks only.** tree-sitter-markdown splits - block and inline grammars and ast-grep exposes only the block - tree, so `atx_heading`, `setext_heading`, `fenced_code_block`, - `list_item`, `paragraph`, `section` and `document` are real kinds - and everything inside a line is one opaque `inline` node. There is - no `link`, `emphasis` or `strong_emphasis` kind; naming one is a - config error that exits 8 and takes the whole scan with it, and - writing it as a pattern (`[$T]($U)`) matches nothing forever with - no error at all. A rule about the words inside a line belongs to - Vale, not here. ast-grep also has no count and no absence - assertion, so "every doc has an h1" is not expressible as an `sg` - rule at any severity. - -5. **Check any variadic pattern against the separator trap.** A `$$$` - next to a comma does not mean "zero or more". The `,` in the pattern - is itself an AST node, and under ast-grep's default `smart` - strictness every node in the pattern must match, so a call with no - comma cannot match a pattern that has one. Measured against the - ast-grep this CLI ships (v%(AST_GREP_VERSION)s), given the four calls - `foo()`, `foo(1)`, `foo(1,2)`, and `foo(1,2,3)`: - -| pattern | what it matches | -|--------------------|----------------------------------------------| -| `foo($$$)` | all four, `foo()` included | -| `foo($A, $$$)` | `foo(1,2)` and `foo(1,2,3)`, never `foo(1)` | -| `foo($$$, $A)` | `foo(1)` alone | -| `foo($A, $$$, $B)` | `foo(1,2)` alone | - - A standalone `$$$` needs none of this. It is the comma beside it - that narrows the pattern. The two remedies are not the same: - - - **Trailing `$$$`**: write the pattern as an object with - `strictness: ast`, which compares named AST nodes and ignores the - separator. An object pattern also requires `context` and - `selector`: - ```yaml - rule: - pattern: - context: foo($A, $$$) - selector: call_expression - strictness: ast - ``` - This moves the boundary from two arguments to one, **not to - zero**, `$A` still has to bind something, so `foo()` is still - unmatched. And `strictness` is valid only inside the pattern - object: at rule level ast-grep rejects it as an unknown field and - fails the whole scan. - - **Leading `$$$`**: `strictness: ast` does not rescue it. Use - `any` with one branch per arity you mean to cover. - - This is upstream's intended behaviour (ast-grep/ast-grep#1365, - closed as working-as-intended), not a bug waiting on a release: - 0.45.2 behaves identically, so there is no version to wait for. + Three more `language:` mistakes have their own entries under + **Quirks** below: copying a spelling from `detect --json`, treating + `Tsx` as an alias of `TypeScript`, and expecting `Markdown` to see + inside a line. + +5. **Check the rule against Quirks.** Read the **Quirks** section below + against what you just wrote, before writing any tests. Every entry + there verifies clean and then either fails the whole scan or matches + less than it should, and a test file written from the same + assumption passes anyway. Look again at: + - a `$$$` next to a comma (the separator trap), + - any `kind:` you read off a `--debug-query=cst` dump, + - a rule that keys on a call's callee, if callers write + `await f(...)`, + - a `follows` or `precedes`, or a `not` that reuses a metavariable. 6. **Write the tests.** Write `.taskless/rules/sg//.tests/-YYYYMMDD-test.yml` with the matching @@ -170,10 +125,11 @@ whole rule. exactly like a rule that works. The `invalid:` bucket is the only thing that demonstrates the rule can fire. - Where the rule has an arity boundary (anything from step 5), put a - case on each side of it. A pattern that starts at two arguments when - it was meant to start at one passes a test suite whose fixtures all - have two. + Where the rule has an arity boundary (the separator trap under + Quirks), put a case on each side of it. A pattern that starts at two + arguments when it was meant to start at one passes a test suite whose + fixtures all have two. Where the rule keys on a callee, put an + awaited generic call among the `invalid:` cases. 7. **Run the verify feedback loop.** Both commands take the rule's directory as their argument: @@ -223,12 +179,179 @@ whole rule. - Only after the user confirms, fetch `%(TASKLESS_CLI)s agent create-remote-rule` and follow it. Do not call the service silently. -## Two ways a relational rule matches nothing +## Quirks + +Each of these comes from a grammar or from ast-grep itself, so a rule +written carefully from the upstream reference still hits them. They +fail in one of two ways: + +- **Config error.** The scan exits 8 and names the problem. Loud, but + it aborts config parsing, so every other `sg` rule in the project + goes unreported until it is fixed. +- **Silent.** The rule parses, verifies, runs, and exits 0. What it + misses goes unreported, which reads exactly like a clean codebase. + +When a rule matches less than it should, check this list before +doubting the rest of the rule. Each entry was measured against the +ast-grep this CLI ships (v%(AST_GREP_VERSION)s). + +| quirk | symptom | +|------------------------------------------------|-------------------------| +| `language:` copied from `detect --json` | config error, or notice | +| `TypeScript` declared over `.tsx` files | silent | +| an inline construct named in a `Markdown` rule | config error, or silent | +| `$$$` next to a comma | silent | +| a keyword named as a `kind` | config error | +| a callee rule over `await f(...)` | silent | +| `follows` or `precedes` without `stopBy: end` | silent | +| a `not` containing a bare metavariable | silent | + +### `language:` copied from `detect --json` + +`detect --json` reports the *repository's* languages, in a different +vocabulary. It says `C++` where ast-grep's list says `Cpp`. That one +happens to resolve, and `verify` reports it as a notice, but a +spelling ast-grep does not recognize at all is a config error. +Workaround: copy the spelling from the list in step 4. + +### `Tsx` and `TypeScript` are two parsers, not aliases + +A rule that declares `TypeScript` does not read `.tsx` files at all, +so over a `.tsx` tree it reports nothing and exits 0. Workaround: +declare `Tsx` for `.tsx` files. A check that must cover both +extensions is two rules, one per parser. + +### `Markdown` sees blocks only + +tree-sitter-markdown splits block and inline grammars and ast-grep +exposes only the block tree, so `atx_heading`, `setext_heading`, +`fenced_code_block`, `list_item`, `paragraph`, `section` and +`document` are real kinds and everything inside a line is one opaque +`inline` node. There is no `link`, `emphasis` or `strong_emphasis` +kind; naming one is a config error, and writing it as a pattern +(`[$T]($U)`) matches nothing forever with no error at all. +Workaround: a rule about the words inside a line belongs to Vale, not +here. ast-grep also has no count and no absence assertion, so "every +doc has an h1" is not expressible as an `sg` rule at any severity. + +### `$$$` next to a comma (the separator trap) + +A `$$$` next to a comma does not mean "zero or more". The `,` in the +pattern is itself an AST node, and under ast-grep's default `smart` +strictness every node in the pattern must match, so a call with no +comma cannot match a pattern that has one. Given the four calls +`foo()`, `foo(1)`, `foo(1,2)`, and `foo(1,2,3)`: -Both of these parse, verify, run, and exit 0. Neither reports anything -ever, which reads exactly like a clean codebase. +| pattern | what it matches | +|--------------------|----------------------------------------------| +| `foo($$$)` | all four, `foo()` included | +| `foo($A, $$$)` | `foo(1,2)` and `foo(1,2,3)`, never `foo(1)` | +| `foo($$$, $A)` | `foo(1)` alone | +| `foo($A, $$$, $B)` | `foo(1,2)` alone | + +A standalone `$$$` needs none of this. It is the comma beside it that +narrows the pattern. The two workarounds are not the same: + +- **Trailing `$$$`**: write the pattern as an object with + `strictness: ast`, which compares named AST nodes and ignores the + separator. An object pattern also requires `context` and `selector`: + ```yaml + rule: + pattern: + context: foo($A, $$$) + selector: call_expression + strictness: ast + ``` + This moves the boundary from two arguments to one, **not to zero**, + `$A` still has to bind something, so `foo()` is still unmatched. And + `strictness` is valid only inside the pattern object: at rule level + ast-grep rejects it as an unknown field and fails the whole scan. +- **Leading `$$$`**: `strictness: ast` does not rescue it. Use `any` + with one branch per arity you mean to cover. + +This is upstream's intended behaviour (ast-grep/ast-grep#1365, closed +as working-as-intended), not a bug waiting on a release: 0.45.2 +behaves identically, so there is no version to wait for. + +### A keyword is not a valid `kind` + +`--debug-query=cst` prints keywords as nodes. For a constructor +parameter `readonly x: number` in TypeScript: + +``` +required_parameter + readonly + pattern: identifier + type: type_annotation +``` + +but `kind: readonly` is a config error, ``Kind `readonly` is +invalid``. `readonly` is an anonymous token, and `kind:` accepts only +named nodes. The CST dump prints both the same way, so reading it +cannot tell you which names are valid kinds. This holds for every +keyword, not only `readonly`. + +Workaround: read kinds off `--debug-query=ast` instead, which prints +named nodes only. A name that appears there is a usable kind; one that +appears only in the CST dump is not. To match on the keyword, match +its named parent and test the parent's text: + +```yaml +rule: + kind: required_parameter + regex: "^readonly\\b" +``` + +### An awaited generic call hides its callee + +```ts +const r = await rpc("x"); // missed +const s = await rpc("y"); // matched +``` + +With + +```yaml +rule: + kind: call_expression + has: { field: function, regex: "^rpc$" } +``` + +only the second call matches. The TypeScript grammar parses the first +as a call OF the await expression: + +``` +call_expression + function: await_expression <- not `rpc` + await + identifier + type_arguments: + arguments: ("x") +``` + +So any rule keyed on the callee silently skips every awaited generic +call. The same happens in `Tsx`, and with `!`, `void` and `typeof` in +place of `await`, where the callee becomes a `unary_expression`. +`yield` is not affected. Workaround: accept the wrapped form as a +second branch. + +```yaml +rule: + kind: call_expression + has: + field: function + any: + - regex: "^rpc$" + - kind: await_expression + has: { kind: identifier, regex: "^rpc$" } +``` + +Add a `unary_expression` branch the same way if those prefixes matter. +The wrapped match spans the `await` too, so a `fix:` on this rule +rewrites it along with the call. + +### `follows` and `precedes` need `stopBy: end` to cross punctuation -**`follows` and `precedes` need `stopBy: end` to cross punctuation.** A sibling relation walks the immediate siblings, and in most grammars the separators are nodes too. Between two array elements sits a `,`, so the previous element is not the previous SIBLING. Measured on @@ -244,8 +367,9 @@ matches nothing, and the same rule with `stopBy: end` inside `follows` matches. If a sibling relation reports zero on input you believe should match, add `stopBy: end` before doubting the rest of the rule. -**A `not` containing a bare metavariable excludes everything.** Writing -"the name is not used in the initializer" as +### A `not` containing a bare metavariable excludes everything + +Writing "the name is not used in the initializer" as ``` rule: diff --git a/packages/cli/test/agent-extensions.test.ts b/packages/cli/test/agent-extensions.test.ts index 072a3046..aa5e592a 100644 --- a/packages/cli/test/agent-extensions.test.ts +++ b/packages/cli/test/agent-extensions.test.ts @@ -544,3 +544,35 @@ describe("create-sg-rule warns about silently inert relational rules", () => { expect(result.stdout).toContain("reads exactly like a clean codebase"); }); }); + +/** + * Two TypeScript grammar traps hit during an onboarding run (#485). The + * vendor contract pins the binary's behavior; this pins that the recipe + * says it, with the workaround. + */ +describe("create-sg-rule names the tree-sitter traps under Quirks", () => { + let cwd: string; + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "taskless-sg-quirks-")); + }); + + afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); + }); + + it("says a keyword is not a kind, and where to read real kinds", async () => { + const result = await runCli(["agent", "create-sg-rule", "-d", cwd]); + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("### A keyword is not a valid `kind`"); + expect(result.stdout).toContain("read kinds off `--debug-query=ast`"); + }); + + it("says an awaited generic call hides its callee", async () => { + const result = await runCli(["agent", "create-sg-rule", "-d", cwd]); + expect(result.stdout).toContain( + "### An awaited generic call hides its callee" + ); + expect(result.stdout).toContain("kind: await_expression"); + }); +}); diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts index 040c47ad..253dcccc 100644 --- a/packages/cli/test/ast-grep-vendor-contract.test.ts +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -379,6 +379,22 @@ const arityMatches = (body: string): string[] => .filter((line) => line !== "") .map((line) => (JSON.parse(line) as { text: string }).text); +/** Scan one TypeScript rule, `rule:` body verbatim, over one source file. */ +const typescriptFindings = (body: string, source: string) => + scan( + project({ + rules: { arity: arityRule(body) }, + sources: { "src/a.ts": source }, + }) + ); + +/** The text each TypeScript finding matched, in file order. */ +const typescriptMatches = (body: string, source: string): string[] => + typescriptFindings(body, source) + .stdout.split("\n") + .filter((line) => line !== "") + .map((line) => (JSON.parse(line) as { text: string }).text); + /** A rule whose fixtures all pass. */ const passingProject = () => project({ @@ -1041,6 +1057,74 @@ withSg("ast-grep vendor contract", () => { }); }); + /** + * TWO TYPESCRIPT GRAMMAR TRAPS, ADDED AT 0.45.3. + * + * Both were hit while authoring rules on a real project, and + * `create-sg-rule.md` lists both under Quirks with a workaround each. These + * are the measurements behind that prose: if a grammar bump fixes either, the + * recipe is describing a parser that no longer exists, and this goes red. + */ + describe("TypeScript keywords and awaited generic calls", () => { + const parameterSource = + "class C { constructor(readonly x: number, y: number) {} }\n"; + + it("rejects an anonymous keyword token as a kind", () => { + // The CST dump prints `readonly` as a node, which is what invites this. + // `kind:` takes named nodes only, so it is exit 8 and every other rule's + // report goes with it. + const outcome = typescriptFindings(" kind: readonly", parameterSource); + expect(outcome.status).toBe(8); + expect(outcome.stderr).toContain("Kind `readonly` is invalid"); + }); + + it("matches the keyword through its named parent's text", () => { + expect( + typescriptMatches( + ' kind: required_parameter\n regex: "^readonly\\\\b"', + parameterSource + ) + ).toEqual(["readonly x: number"]); + }); + + const awaitSource = [ + 'const r = await rpc("x");', + 'const s = await rpc("y");', + 'const t = rpc("z");', + "", + ].join("\n"); + + it("parses `await f()` as a call of the await expression", () => { + // The silent one: the generic awaited call is simply absent from the + // findings, exit 0. + expect( + typescriptMatches( + ' kind: call_expression\n has: { field: function, regex: "^rpc$" }', + awaitSource + ) + ).toEqual(['rpc("y")', 'rpc("z")']); + }); + + it("recovers the awaited generic call with an await_expression branch", () => { + // The wrapped match spans the `await`, which the recipe warns a `fix:` + // will rewrite along with the call. + expect( + typescriptMatches( + [ + " kind: call_expression", + " has:", + " field: function", + " any:", + ' - regex: "^rpc$"', + " - kind: await_expression", + ' has: { kind: identifier, regex: "^rpc$" }', + ].join("\n"), + awaitSource + ) + ).toEqual(['await rpc("x")', 'rpc("y")', 'rpc("z")']); + }); + }); + /** * Two matching-semantics fixes from the 0.41.0 to 0.45.2 upgrade, pinned * because they are the class no local check catches: a valid, unchanged rule