From acc9ff0dd0a090ace9bfd12fb20ff23be8c8d41a Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 19 Aug 2026 00:45:56 -0700 Subject: [PATCH 1/2] fix(cli): anchor sg test count parsing, and pin ast-grep's contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `runTests` scraped `ast-grep test`'s pass/fail counts with two unanchored regexes over stdout+stderr combined, taking the first match anywhere. A failing run echoes the offending fixture's source, so a fixture containing `'7 passed; 0 failed'` was read as the summary and verify reported `7 passed, 0 failed` for a run whose summary said `0 passed; 1 failed`. This corrupts reported counts only — it never turns a failure into a pass. Validity comes from the exit code, and a passing run echoes no fixture source, so the sole match on a clean run is the genuine summary. The counts still matter: `improve-rule` feeds them back to an agent iterating on a rule. The counts now come from the summary line itself, in one of the two exact forms ast-grep emits, with ANSI stripped first (the escape sits inside the phrase) and the last such line winning. Both streams are read through a `StringDecoder`, as `vale/run.ts` already does, so a multi-byte character split across a chunk boundary survives. `ast-grep test` has no structured output at 0.41.0, so the parse cannot be deleted. What makes it defensible is the new `packages/cli/test/ast-grep-vendor-contract.test.ts`, mirroring the Vale contract file: it invokes the pinned binary directly and pins the summary wording and exit codes (0 on pass, 4 on failure), the colorization, the fixture echo, `--skip-snapshot-tests`, `--filter`'s regex semantics, the `--json=stream` line protocol and match shape, 0-based ranges, the severity vocabulary (five values, not four), and the scan exit-code boundary. The two `.tests/`-discovery cases move here from `engine-layout.test.ts`, which existed only for them and is removed. Fixes #108 Fixes #112 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3 --- .changeset/anchor-sg-test-summary.md | 18 + packages/cli/src/rules/engines.ts | 3 +- packages/cli/src/rules/verify.ts | 78 ++- .../cli/test/ast-grep-vendor-contract.test.ts | 519 ++++++++++++++++++ packages/cli/test/engine-layout.test.ts | 110 ---- packages/cli/test/verify.test.ts | 48 ++ 6 files changed, 655 insertions(+), 121 deletions(-) create mode 100644 .changeset/anchor-sg-test-summary.md create mode 100644 packages/cli/test/ast-grep-vendor-contract.test.ts delete mode 100644 packages/cli/test/engine-layout.test.ts diff --git a/.changeset/anchor-sg-test-summary.md b/.changeset/anchor-sg-test-summary.md new file mode 100644 index 00000000..0f910986 --- /dev/null +++ b/.changeset/anchor-sg-test-summary.md @@ -0,0 +1,18 @@ +--- +"@taskless/cli": patch +--- + +Fix the pass/fail counts reported when a rule's `ast-grep` tests fail. + +`ast-grep test` echoes the source of a failing test case, and `verify` scraped +its counts with unanchored regexes over stdout and stderr combined — so a +fixture containing text like `'7 passed; 0 failed'` was read as the summary and +`verify` reported `✗ failed (7 passed, 0 failed)` for a run that actually had 0 +passed and 1 failed. The counts are now read from the summary line itself +(`test result: ok.` / `Error: test failed.`), with ANSI colors stripped first. + +This only affected the reported numbers, never the pass/fail verdict, which +comes from the exit code — but those numbers are handed to the agent driving +`improve-rule`, where a wrong count can steer the next edit. Test output is also +now decoded with a `StringDecoder` per stream, so a multi-byte character split +across a chunk boundary is no longer mangled. diff --git a/packages/cli/src/rules/engines.ts b/packages/cli/src/rules/engines.ts index d8151aae..e4290efb 100644 --- a/packages/cli/src/rules/engines.ts +++ b/packages/cli/src/rules/engines.ts @@ -63,7 +63,8 @@ export const RULES_DIRECTORY = "rules"; * * That is undocumented behavior, and three things make depending on it * acceptable. The failure is loud — a parse error naming the file, never a test - * silently reinterpreted as a rule. `engine-layout.test.ts` pins it, so it is + * silently reinterpreted as a rule. `ast-grep-vendor-contract.test.ts` pins it + * alongside the rest of ast-grep's observed behavior, so it is * checked on every run rather than remembered. And the binary is pinned to an * exact version, so it cannot change without a deliberate bump, which is * exactly where that test fires. diff --git a/packages/cli/src/rules/verify.ts b/packages/cli/src/rules/verify.ts index 0163309a..a2ed3ece 100644 --- a/packages/cli/src/rules/verify.ts +++ b/packages/cli/src/rules/verify.ts @@ -1,5 +1,6 @@ import { readFile, readdir } from "node:fs/promises"; import { spawn } from "node:child_process"; +import { StringDecoder } from "node:string_decoder"; import { parse } from "yaml"; @@ -158,6 +159,50 @@ async function validateRequirements( // --- Layer 3: Test execution --- +/** + * Drop SGR escape sequences. + * + * ast-grep colorizes even when stdout is not a TTY, and the escape lands + * *inside* the phrase we anchor on — `test result: \u001B[32mok\u001B[0m.` — so + * stripping is a precondition for matching the summary at all, not cosmetics. + * Pinned by `ast-grep-vendor-contract.test.ts`. + */ +function stripAnsi(text: string): string { + // eslint-disable-next-line no-control-regex + return text.replaceAll(/\u001B\[[\d;]*m/g, ""); +} + +/** + * The counts from `ast-grep test`'s summary line, or `undefined` if there is + * none. + * + * `ast-grep test` has no structured output at 0.41.0 — `--help` offers nothing + * machine-readable — so the counts can only come from prose. What makes that + * defensible is anchoring: the match is the whole summary line, in one of the + * two exact forms ast-grep emits, rather than the first number in the stream + * that happens to be followed by "passed". + * + * That distinction is the bug in #112. A *failing* run echoes the offending + * test's source, so a fixture reading `const msg = '7 passed; 0 failed';` was + * matched ahead of `Error: test failed. 0 passed; 1 failed;` and the CLI + * reported 7 passed, 0 failed. Wrong counts, not a false pass — validity comes + * from the exit code — but those counts are fed back to the agent driving + * `improve-rule`, so they steer the next edit. + * + * The last summary line wins, since only the final one describes the whole run. + */ +function parseTestSummary( + output: string +): { passed: number; failed: number } | undefined { + const summary = + /^(?:test result: ok\.|Error: test failed\.) (\d+) passed; (\d+) failed;/gm; + let found: { passed: number; failed: number } | undefined; + for (const match of stripAnsi(output).matchAll(summary)) { + found = { passed: Number(match[1]), failed: Number(match[2]) }; + } + return found; +} + async function runTests(cwd: string, ruleId: string): Promise { // Assembly names every rule's `.tests/` as its own `testConfigs` entry, so // the filter below selects a rule whose tests ast-grep already knows how to @@ -192,14 +237,21 @@ async function runTests(cwd: string, ruleId: string): Promise { } ); + // One decoder per stream, not `chunk.toString()` per chunk. A multi-byte + // UTF-8 sequence split across a chunk boundary would otherwise have each + // half independently replaced with U+FFFD, and ast-grep echoes fixture + // source — arbitrary user text — into the output of a failing run. Same + // treatment `vale/run.ts` already gives its streams. + const stdoutDecoder = new StringDecoder("utf8"); + const stderrDecoder = new StringDecoder("utf8"); const stdoutChunks: string[] = []; const stderrChunks: string[] = []; child.stdout.on("data", (chunk: Buffer) => { - stdoutChunks.push(chunk.toString()); + stdoutChunks.push(stdoutDecoder.write(chunk)); }); child.stderr.on("data", (chunk: Buffer) => { - stderrChunks.push(chunk.toString()); + stderrChunks.push(stderrDecoder.write(chunk)); }); child.on("error", () => { @@ -212,14 +264,20 @@ async function runTests(cwd: string, ruleId: string): Promise { }); child.on("close", (code) => { - const output = stdoutChunks.join("") + stderrChunks.join(""); - - // Parse pass/fail counts from sg test output - // sg test outputs: "test result: ok. 3 passed; 0 failed;" - const passedMatch = /(\d+)\s+passed/i.exec(output); - const failedMatch = /(\d+)\s+failed/i.exec(output); - const passed = passedMatch ? Number(passedMatch[1]) : 0; - const failed = failedMatch ? Number(failedMatch[1]) : 0; + // Flush whatever partial multi-byte sequence each decoder is holding, so + // a stream that ends mid-character contributes its replacement char once + // rather than leaving bytes unaccounted for. + stdoutChunks.push(stdoutDecoder.end()); + stderrChunks.push(stderrDecoder.end()); + + // Both streams, because ast-grep prints the passing summary to stdout and + // the failing one to stderr. Joined with a newline so the summary stays + // at the start of a line for the anchored match. + const output = `${stdoutChunks.join("")}\n${stderrChunks.join("")}`; + + const summary = parseTestSummary(output); + const passed = summary?.passed ?? 0; + const failed = summary?.failed ?? 0; if (code === 0) { resolve({ valid: true, errors: [], passed, failed }); diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts new file mode 100644 index 00000000..1fad5737 --- /dev/null +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -0,0 +1,519 @@ +import { spawnSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it } from "vitest"; + +import { assembleSgConfig } from "../src/rules/assemble"; +import { ruleDirectory, ruleTestsDirectory } from "../src/rules/engines"; +import { buildPath, findSgBinary } from "../src/rules/scan"; + +/** + * ast-grep's observable behaviour, pinned. + * + * Everything here is a property of a **vendored third-party binary**, exact- + * pinned at `0.41.0` in `packages/cli/package.json`. Our own tests assert that + * our code behaves correctly *given* these; this file asserts the givens, so an + * ast-grep bump that changes one fails here — naming the assumption and the + * code that rests on it — instead of surfacing downstream. + * + * It matters more here than for Vale, because the ast-grep failure mode is + * quiet: `runAstGrepScan` treats exit 0 and 1 as normal and silently discards + * stdout lines that do not parse, so a scan that matched nothing because + * 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 + * went through our wrapper would be asserting our interpretation of ast-grep, + * which is the thing under test everywhere else. + */ + +/** + * `findSgBinary` throws rather than returning `undefined` — ast-grep has no + * degraded mode — so absence is caught here to skip rather than to fail. + */ +const binary = ((): string | undefined => { + try { + return findSgBinary(); + } catch { + return undefined; + } +})(); +const withSg = binary === undefined ? describe.skip : describe; + +const workspaces: string[] = []; +afterEach(() => { + for (const workspace of workspaces.splice(0)) { + rmSync(workspace, { recursive: true, force: true }); + } +}); + +const rule = (id: string, severity = "error") => + [ + `id: ${id}`, + "language: TypeScript", + `severity: ${severity}`, + `message: no eval`, + "note: prefer a real parser", + "rule:", + " pattern: eval($$$A)", + "", + ].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; +} + +/** + * A throwaway ast-grep 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. + */ +function project({ rules, tests = {}, sources = {} }: Project): string { + const cwd = mkdtempSync(join(tmpdir(), "sg-contract-")); + workspaces.push(cwd); + + for (const [id, body] of Object.entries(rules)) { + 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)) { + mkdirSync(join(cwd, "src"), { recursive: true }); + writeFileSync(join(cwd, path), body); + } + + writeFileSync( + join(cwd, "sgconfig.yml"), + [ + "ruleDirs:", + " - rules", + "testConfigs:", + ...Object.keys(tests).map((id) => ` - testDir: rules/${id}/.tests`), + "", + ].join("\n") + ); + return cwd; +} + +const run = (cwd: string, argv: string[]) => + spawnSync(binary as string, argv, { + cwd, + encoding: "utf8", + env: { ...process.env, PATH: buildPath() }, + }); + +/** `scan`, exactly as `runAstGrepScan` invokes it. */ +const scan = (cwd: string, config = "sgconfig.yml") => + run(cwd, ["scan", "--config", config, "--json=stream"]); + +/** `test`, 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' }; + +/** Exit status of scanning one finding declared at `severity`. */ +const statusAt = (severity: string) => + scan( + project({ + rules: { "no-eval": rule("no-eval", severity) }, + sources: evalSource, + }) + ).status; + +/** 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. + */ +const failingProject = () => + project({ + rules: { "no-eval": rule("no-eval") }, + tests: { + "no-eval": testFile("no-eval", ["const msg = '7 passed; 0 failed';"]), + }, + }); + +withSg("ast-grep vendor contract", () => { + it("reports its own name in --version", () => { + // Depended on by: AST_GREP_BINARY.identity (/ast-grep/i) in scan.ts. The + // resolver runs each candidate because `@ast-grep/cli`'s postinstall can + // leave a placeholder text file at the binary path. If ast-grep stops + // saying "ast-grep" here, findSgBinary rejects the real binary and throws + // "ast-grep binary not found" — fatal for every sg rule. + const result = spawnSync(binary as string, ["--version"], { + encoding: "utf8", + }); + expect(result.status).toBe(0); + expect(`${result.stdout}${result.stderr}`).toMatch(/ast-grep/i); + }); + + describe("--json=stream", () => { + it("emits one JSON object per line on stdout, with nothing else", () => { + // Depended on by: runAstGrepScan reading stdout through readline and + // JSON.parsing each line, discarding anything that does not parse. An + // interleaved status line would be swallowed silently; a change to a + // pretty-printed array would make EVERY line unparseable and report an + // empty scan as a clean codebase. + const cwd = project({ + rules: { "no-eval": rule("no-eval") }, + sources: { + ...evalSource, + "src/b.ts": 'const y = eval("2");\n', + }, + }); + const result = scan(cwd); + const lines = result.stdout.split("\n").filter((line) => line !== ""); + expect(lines).toHaveLength(2); + for (const line of lines) { + expect(() => { + JSON.parse(line); + }).not.toThrow(); + } + // The "N error(s) found" banner goes to stderr, not into the stream. + expect(result.stderr).toContain("error(s) found"); + }); + + it("carries the field names AstGrepMatch reads", () => { + // Depended on by: AstGrepMatch in types/check.ts and toCheckResult. A + // rename arrives as `undefined` inside a CheckResult rather than as an + // error — a finding with no message, or no file. + const cwd = project({ + rules: { "no-eval": rule("no-eval") }, + sources: evalSource, + }); + const match = JSON.parse(scan(cwd).stdout.split("\n")[0] ?? "") as Record< + string, + unknown + >; + for (const field of [ + "ruleId", + "severity", + "message", + "note", + "text", + "file", + "range", + ]) { + expect(match, `missing ${field}`).toHaveProperty(field); + } + expect(match.ruleId).toBe("no-eval"); + expect(match.text).toBe('eval("1")'); + expect(match.file).toBe("src/a.ts"); + // `replacement` appears only for a rule with a `fix`; toCheckResult maps + // it to an optional `fix`, so its absence here is the contract too. + expect(match).not.toHaveProperty("replacement"); + }); + + it("reports range line and column 0-based", () => { + // Depended on by: util/format.ts and rules/runtime/narrow.ts, which both + // render `line + 1` / `column + 1`. If ast-grep ever emitted 1-based + // positions, every reported location would be off by one — no error, + // just quietly wrong. `const x = eval("1");` puts the match at line 0, + // columns 10-19. + const cwd = project({ + rules: { "no-eval": rule("no-eval") }, + sources: evalSource, + }); + const match = JSON.parse(scan(cwd).stdout.split("\n")[0] ?? "") as { + range: { + start: { line: number; column: number }; + end: { line: number; column: number }; + }; + }; + expect(match.range.start).toEqual({ line: 0, column: 10 }); + expect(match.range.end).toEqual({ line: 0, column: 19 }); + }); + }); + + describe("severity", () => { + it("emits hint, info, warning and error verbatim", () => { + // Depended on by: AstGrepMatch.severity, typed as exactly those four. + // Anything else arrives as a CheckResult with a severity outside the + // union — TypeScript believes a value the binary never promised. + for (const severity of ["hint", "info", "warning", "error"]) { + const cwd = project({ + rules: { "no-eval": rule("no-eval", severity) }, + sources: evalSource, + }); + const match = JSON.parse(scan(cwd).stdout.split("\n")[0] ?? "") as { + severity: string; + }; + expect(match.severity).toBe(severity); + } + }); + + it("accepts exactly [hint info warning error off] and rejects the rest", () => { + // The vocabulary is FIVE values, one more than AstGrepMatch's union — + // `off` is accepted in a rule but disables it, so it can never appear in + // output (asserted below). A rule at any other severity fails the parse + // rather than reaching us, which is what keeps the four-value union safe. + const cwd = project({ + rules: { "no-eval": rule("no-eval", "catastrophe") }, + sources: evalSource, + }); + const result = scan(cwd); + expect(result.stderr).toContain( + "unknown variant `catastrophe`, expected one of `hint`, `info`, `warning`, `error`, `off`" + ); + }); + + it("runs no rule at severity off, so `off` never reaches the stream", () => { + const cwd = project({ + rules: { "no-eval": rule("no-eval", "off") }, + sources: evalSource, + }); + const result = scan(cwd); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe(""); + }); + }); + + describe("scan exit codes", () => { + // Depended on by: runAstGrepScan's boundary — "exit 1 means error-severity + // matches were found; exit > 1 means the binary or config failed." That + // single comparison is the whole difference between findings and engine + // failure. If a config error ever exited 1, a broken engine would be + // reported as a clean scan with no results. + + it("exits 0 for findings below error severity", () => { + expect(statusAt("warning")).toBe(0); + expect(statusAt("info")).toBe(0); + }); + + it("exits 1 for error-severity findings", () => { + expect(statusAt("error")).toBe(1); + }); + + it("exits above 1 when the config or a rule cannot be read", () => { + // Measured at 0.41.0: 6 for a missing config, 8 for an unparseable rule. + // Only `> 1` is depended on; the exact numbers are recorded so a change + // in them is visible without being treated as a break. + const missing = scan( + project({ rules: { "no-eval": rule("no-eval") } }), + "nope.yml" + ); + expect(missing.status).toBeGreaterThan(1); + expect(missing.status).toBe(6); + + const unparseable = scan( + project({ + rules: { "no-eval": rule("no-eval", "catastrophe") }, + sources: evalSource, + }) + ); + expect(unparseable.status).toBeGreaterThan(1); + expect(unparseable.status).toBe(8); + }); + }); + + 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("colorizes the summary even when stdout is not a TTY", () => { + // Depended on by: stripAnsi in verify.ts. The escape sits between + // `test result: ` and `ok`, i.e. INSIDE the phrase being matched, so a + // parser that skipped stripping would fail to anchor on a passing run. + // eslint-disable-next-line no-control-regex -- the escape IS the subject + const greenOk = /\u001B\[32mok\u001B\[0m/; + expect(test(passingProject(), "no-eval").stdout).toMatch(greenOk); + }); + + 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("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"); + }); + + 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. + 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)"]), + }, + }); + 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;"); + }); + }); + + /** + * Relocated from `engine-layout.test.ts`, which existed only for these two. + * + * ast-grep's `ruleDirs` recurses and parses every `.yml` beneath it as a + * rule, so a rule's tests have to live somewhere the rule walk does not + * reach. A dot-directory is skipped; `tests/` and `__tests__/` are not, and + * either fails the whole scan with `missing field 'language'`. + * + * Documented at `engines.ts` (RULE_TESTS_DIRECTORY) and `assemble.ts` + * (assembleSgConfig). These two cases go through our own layout helpers + * deliberately — what is being pinned is that OUR directory names survive + * ast-grep's discovery. If it ever breaks, the recorded fallback is to + * materialize a rules-only tree for ast-grep (design D2). + */ + describe("the .tests/ directory is invisible to rule discovery", () => { + 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 } { + 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); + mkdirSync(join(cwd, "src"), { recursive: true }); + writeFileSync(join(cwd, "src", "a.ts"), 'const x = eval("1");\n'); + + 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. + 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 }); + 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 + ); + + const configPath = await assembleSgConfig(cwd); + const result = scan(cwd, configPath ?? ""); + expect(result.status).toBeGreaterThan(1); + expect(result.stderr).toContain("missing field `language`"); + }); + }); +}); diff --git a/packages/cli/test/engine-layout.test.ts b/packages/cli/test/engine-layout.test.ts deleted file mode 100644 index 1051961c..00000000 --- a/packages/cli/test/engine-layout.test.ts +++ /dev/null @@ -1,110 +0,0 @@ -import { execFile } from "node:child_process"; -import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { promisify } from "node:util"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; - -import { assembleSgConfig } from "../src/rules/assemble"; -import { ruleDirectory, ruleTestsDirectory } from "../src/rules/engines"; -import { findSgBinary, buildPath } from "../src/rules/scan"; - -const execFileAsync = promisify(execFile); - -let cwd: string; - -beforeEach(async () => { - cwd = await mkdtemp(join(tmpdir(), "tskl-layout-")); -}); - -afterEach(async () => { - await rm(cwd, { recursive: true, force: true }); -}); - -const RULE = [ - "id: no-eval", - "language: TypeScript", - "severity: error", - "message: no eval", - "rule:", - " pattern: eval($$$A)", - "", -].join("\n"); - -/** A test file is valid test YAML and invalid *rule* YAML — it has no `language`. */ -const TEST_FILE = [ - "id: no-eval", - "valid:", - ' - "const a = 1"', - "invalid:", - ' - "eval(x)"', - "", -].join("\n"); - -/** - * Pins the one undocumented behavior this layout depends on. - * - * ast-grep's `ruleDirs` recurses and parses every `.yml` beneath it as a rule, - * so a rule's tests have to live somewhere the rule walk does not reach. A - * dot-directory is skipped; `tests/` and `__tests__/` are not, and either fails - * the whole scan with `missing field 'language'`. - * - * The binary is pinned to an exact version, so this cannot change without a - * deliberate bump — and this test is what fires at that bump, turning the - * discovery into a migration task with a changelog to read rather than a - * mystery in someone's CI. If it ever does break, the recorded fallback is to - * materialize a rules-only tree for ast-grep (design D2). - */ -describe("the .tests/ directory is invisible to ast-grep rule discovery", () => { - it("scans clean with test YAML inside a rule's .tests/", async () => { - const rule = ruleDirectory(cwd, "sg", "no-eval"); - await mkdir(rule, { recursive: true }); - await writeFile(join(rule, "no-eval.yml"), RULE); - - const tests = ruleTestsDirectory(cwd, "sg", "no-eval"); - await mkdir(tests, { recursive: true }); - await writeFile(join(tests, "no-eval-20260101-test.yml"), TEST_FILE); - - await mkdir(join(cwd, "src"), { recursive: true }); - await writeFile(join(cwd, "src", "a.ts"), 'const x = eval("1");\n'); - - const configPath = await assembleSgConfig(cwd); - expect(configPath).toBeDefined(); - - // ast-grep exits non-zero when it finds error-severity results, which is - // the success case here — the rule fired. A parse failure is what would - // make this test meaningful, and that produces no JSON at all. - let stdout: string; - try { - ({ stdout } = await execFileAsync( - findSgBinary(), - ["scan", "-c", configPath ?? "", "--json"], - { cwd, env: { ...process.env, PATH: buildPath() } } - )); - } catch (error) { - stdout = (error as { stdout: string }).stdout; - } - - // The rule fires, which proves discovery ran; the test file beneath it was - // never parsed as a rule, which is the property being pinned. - const findings = JSON.parse(stdout) as { ruleId: string }[]; - expect(findings.map((f) => f.ruleId)).toEqual(["no-eval"]); - }); - - it("would fail if the same file sat in a non-dot directory", async () => { - const rule = ruleDirectory(cwd, "sg", "no-eval"); - await mkdir(join(rule, "tests"), { recursive: true }); - await writeFile(join(rule, "no-eval.yml"), RULE); - // Deliberately NOT `.tests/` — this is the layout the dot exists to avoid. - await writeFile(join(rule, "tests", "no-eval-20260101-test.yml"), TEST_FILE); - - const configPath = await assembleSgConfig(cwd); - - await expect( - execFileAsync(findSgBinary(), ["scan", "-c", configPath ?? "", "--json"], { - cwd, - env: { ...process.env, PATH: buildPath() }, - }) - ).rejects.toThrow(); - }); -}); diff --git a/packages/cli/test/verify.test.ts b/packages/cli/test/verify.test.ts index a8c7fd29..06ce9569 100644 --- a/packages/cli/test/verify.test.ts +++ b/packages/cli/test/verify.test.ts @@ -318,6 +318,54 @@ describe("verifyRule", () => { expect.stringContaining("message") ); }); + + it("reads the counts off the summary line, not off echoed fixture text", async () => { + // Regression for #112. `ast-grep test` echoes the source of a FAILING case + // into its output, and the old parser took the first `(\d+)\s+passed` + // anywhere in stdout+stderr — so this fixture made a run whose summary says + // `0 passed; 1 failed;` report 7 passed, 0 failed. Wrong counts rather than + // a false pass (validity comes from the exit code), but the counts are what + // `improve-rule` feeds back to an agent iterating on the rule. + const rulesDirectory = join(temporaryDirectory, ".taskless", "sg", "rules"); + const testsDirectory = join( + temporaryDirectory, + ".taskless", + "sg", + "rule-tests" + ); + await mkdir(rulesDirectory, { recursive: true }); + await mkdir(testsDirectory, { recursive: true }); + + await writeFile( + join(rulesDirectory, "no-eval.yml"), + stringify({ + id: "no-eval", + language: "typescript", + severity: "error", + message: "Do not use eval()", + rule: { pattern: "eval($$$)" }, + }), + "utf8" + ); + + // The invalid case contains no `eval(...)`, so the rule does not fire and + // ast-grep echoes it back as a failure — carrying its numbers with it. + await writeFile( + join(testsDirectory, "no-eval-20260330-test.yml"), + stringify({ + id: "no-eval", + valid: [], + invalid: ["const msg = '7 passed; 0 failed';"], + }), + "utf8" + ); + + const result = await verifyRule(temporaryDirectory, "no-eval"); + + expect(result.tests.valid).toBe(false); + expect(result.tests.passed).toBe(0); + expect(result.tests.failed).toBe(1); + }); }); describe("getSchemaPayload", () => { From b46ca13213fd08634655d388b035a91c6867f28f Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 19 Aug 2026 12:07:24 -0700 Subject: [PATCH 2/2] fix(cli): use the builtin ANSI stripper, and pin the echo indent Addresses PR #117 review: - Replace the hand-rolled `stripAnsi` (plus its `no-control-regex` suppression) with `node:util`'s `stripVTControlCharacters`. - `project()` in the vendor-contract suite now mkdirs the parent of the path actually given rather than a hardcoded `src/`. - Pin that `sg test` indents every echoed fixture line by two spaces, which is what makes the `^` anchor close the poisoning class rather than only the one fixture from #112, and say so in the doc comment. Refs #108 Refs #112 --- packages/cli/src/rules/verify.ts | 25 ++++++------- .../cli/test/ast-grep-vendor-contract.test.ts | 36 +++++++++++++++++-- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/rules/verify.ts b/packages/cli/src/rules/verify.ts index a2ed3ece..9c041dbd 100644 --- a/packages/cli/src/rules/verify.ts +++ b/packages/cli/src/rules/verify.ts @@ -1,6 +1,7 @@ import { readFile, readdir } from "node:fs/promises"; import { spawn } from "node:child_process"; import { StringDecoder } from "node:string_decoder"; +import { stripVTControlCharacters } from "node:util"; import { parse } from "yaml"; @@ -159,19 +160,6 @@ async function validateRequirements( // --- Layer 3: Test execution --- -/** - * Drop SGR escape sequences. - * - * ast-grep colorizes even when stdout is not a TTY, and the escape lands - * *inside* the phrase we anchor on — `test result: \u001B[32mok\u001B[0m.` — so - * stripping is a precondition for matching the summary at all, not cosmetics. - * Pinned by `ast-grep-vendor-contract.test.ts`. - */ -function stripAnsi(text: string): string { - // eslint-disable-next-line no-control-regex - return text.replaceAll(/\u001B\[[\d;]*m/g, ""); -} - /** * The counts from `ast-grep test`'s summary line, or `undefined` if there is * none. @@ -190,6 +178,15 @@ function stripAnsi(text: string): string { * `improve-rule`, so they steer the next edit. * * The last summary line wins, since only the final one describes the whole run. + * + * Stripping first is a precondition, not cosmetics: ast-grep colorizes even + * when stdout is not a TTY, and the escape lands *inside* the phrase being + * anchored on — `test result: \u001B[32mok\u001B[0m.`. + * + * The anchor closes the poisoning class rather than just the one reported + * fixture: echoed source is indented two spaces by ast-grep, so a fixture whose + * own text reads `Error: test failed. 99 passed; 0 failed;` still cannot reach + * column 0. Both properties are pinned by `ast-grep-vendor-contract.test.ts`. */ function parseTestSummary( output: string @@ -197,7 +194,7 @@ function parseTestSummary( const summary = /^(?:test result: ok\.|Error: test failed\.) (\d+) passed; (\d+) failed;/gm; let found: { passed: number; failed: number } | undefined; - for (const match of stripAnsi(output).matchAll(summary)) { + for (const match of stripVTControlCharacters(output).matchAll(summary)) { found = { passed: Number(match[1]), failed: Number(match[2]) }; } return found; diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts index 1fad5737..f896e489 100644 --- a/packages/cli/test/ast-grep-vendor-contract.test.ts +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -1,7 +1,7 @@ import { spawnSync } from "node:child_process"; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -103,7 +103,10 @@ function project({ rules, tests = {}, sources = {} }: Project): string { writeFileSync(join(cwd, "rules", id, ".tests", `${id}-test.yml`), body); } for (const [path, body] of Object.entries(sources)) { - mkdirSync(join(cwd, "src"), { recursive: true }); + // 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 + // should not fail with ENOENT. + mkdirSync(dirname(join(cwd, path)), { recursive: true }); writeFileSync(join(cwd, path), body); } @@ -375,7 +378,8 @@ withSg("ast-grep vendor contract", () => { }); it("colorizes the summary even when stdout is not a TTY", () => { - // Depended on by: stripAnsi in verify.ts. The escape sits between + // Depended on by: parseTestSummary stripping VT control characters + // before matching, in verify.ts. The escape sits between // `test result: ` and `ok`, i.e. INSIDE the phrase being matched, so a // parser that skipped stripping would fail to anchor on a passing run. // eslint-disable-next-line no-control-regex -- the escape IS the subject @@ -396,6 +400,32 @@ withSg("ast-grep vendor contract", () => { ); }); + 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( + `^${poison.replaceAll(/[.*+?^${}()|[\]\\]/g, String.raw`\$&`)}`, + "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