Repository navigation
feat(rules): hand ast-grep a rules-only tree, and add the fixture converter - #584
Conversation
github-actions
Bot
added
the
Open OpenSpec
theCodeDrift
changed the title
c5ceac8 to
62a16c4
Compare
theCodeDrift
changed the title
|
@claude /review |
|
Claude finished @theCodeDrift's task in 6m 5s —— View job Review complete
Review mode: incremental — read 0 prior review thread(s) before reviewing. Findings[New] Case numbering collides across a rule's multiple legacy test files, silently dropping fixtures — posted inline on
[New]
Minor / non-blocking
What looked solid
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
62a16c4 to
c8ed830
Compare
|
@claude /review This is a follow-up review after the review-fix round (c8ed830, shared
|
|
Claude finished @theCodeDrift's task in 6m 13s —— View job Review complete
Review mode: incremental — read 1 prior review thread before reviewing. The one thread (case-numbering collision across a rule's multiple legacy test files) is resolved — fixed in c8ed830 via the shared Findings
|
| /** Whether `path` is admitted by `scope`, in `node:path`'s glob dialect. */ | ||
| function admitted(path: string, scope: FixtureScope): boolean { | ||
| const included = | ||
| scope.include.length === 0 || | ||
| scope.include.some((glob) => matchesGlob(path, glob)); | ||
| return included && !scope.exclude.some((glob) => matchesGlob(path, glob)); | ||
| } |
There was a problem hiding this comment.
[New] admitted checks node:path's glob dialect, which diverges from ast-grep's globset exactly where it matters: whether a single * crosses /.
globset (the Rust crate ast-grep's files:/ignores: are matched with) matches * across path separators by default — only literal_separator(true) would turn that off, and nothing pins which mode ast-grep actually scans with. node:path's matchesGlob, by contrast, documents * as explicitly excluding separators.
Repro: files: ['src/**/*.ts'], ignores: ['*01.ts'], case src/fail-01.ts.
admitted()here:matchesGlob('src/fail-01.ts', '*01.ts')isfalse(the pattern has no/, so under this dialect it can only match a bare filename) → the ignore never fires → the converter calls this admitted.- ast-grep's actual
globset: if*crosses/asglobset's default does,*01.tsmatchessrc/fail-01.tstoo → the real scan excludes the file.
For a fail/ case this just produces a loud, caught failure (no violation reported where one was expected). For a pass/ case it's silent: the file is never scanned at all, so "no violation" looks identical to "the rule correctly didn't fire." verify can't tell those apart from its own output.
Separately, pathFromGlob's negated-class handling looks like it has the opposite problem — a false negative. [!_]*.ts goes through .replaceAll(/\[!?([^\]])[^\]]*\]/g, "$1"), which for a negated class extracts the very character the class excludes: [!_] → _. So src/[!_]*.ts derives src/_fail-01.ts, and then admitted() correctly rejects its own derivation (the class literally excludes _), leaving a legitimately-placeable fixture marked unconverted. Substituting any character not in the excluded set (e.g. x) would derive a path the class actually admits.
Neither case is covered in fixture-convert.test.ts or pinned by a vendor-contract test today, so nothing currently catches either direction of this drift.
| /** | ||
| * A path for a fixture file named `stem.extension` that `scope` admits, or | ||
| * `undefined` when none of the derivations could be checked. | ||
| * | ||
| * A path keeping `.extension` is preferred over an earlier glob's that does | ||
| * not: both engines choose how to read a file by its extension, so under | ||
| * `[*.md]` and `[*.txt]` a `.txt` document written as `.md` would be linted | ||
| * as Markdown. | ||
| */ | ||
| export function deriveFixturePath( | ||
| scope: FixtureScope, | ||
| stem: string, | ||
| extension: string | ||
| ): string | undefined { | ||
| const derived = | ||
| scope.include.length === 0 | ||
| ? [`${stem}.${extension}`] | ||
| : scope.include.map((glob) => pathFromGlob(glob, stem, extension)); | ||
| const candidates = [ | ||
| ...derived.filter((path) => path.endsWith(`.${extension}`)), | ||
| ...derived.filter((path) => !path.endsWith(`.${extension}`)), | ||
| ]; | ||
| return candidates.find( | ||
| (candidate) => | ||
| candidate !== "" && | ||
| !candidate.startsWith("/") && | ||
| !candidate.split("/").includes("..") && | ||
| admitted(candidate, scope) | ||
| ); | ||
| } |
There was a problem hiding this comment.
[New] The extension-fallback in deriveFixturePath still returns a wrong-extension candidate when it's the only admitted one — it doesn't just prefer the right extension, it will settle for the wrong one.
The ordering fix (c8ed830) tries every extension-preserving candidate before any non-preserving one, but candidates.find(...) still falls through to a non-preserving candidate rather than returning undefined. Traced two repros against the current code:
- Vale, scope
{include: ['*.md']}, documentpass/a.txt:derived = ["a.md"](only one glob, no extension-preserving option exists).candidates = ["a.md"],admitted("a.md", scope)is true, so this returns"a.md"— a.txtdocument gets written and then linted as Markdown, exactly the failure mode the docstring says this function exists to avoid. - sg,
language: TypeScript(extensionts),files: ['src/**/*.ts', 'lib/*.js'],ignores: ['src/**']:"src/fail-01.ts"is derived first but excluded byignores, so the search falls to"lib/fail-01.js", which is admitted. The snippet is written with a.jsextension under aTypeScript-language rule. ast-grep selects a file's language from its extension before applying a rule'slanguage:filter, so this file is classified as JavaScript and the TypeScript rule never considers it — same vacuous-pass / loud-fail-miss as the sibling glob-dialect finding above.
Given the module's own stated invariant ("both engines choose how to read a file by its extension"), a candidate that loses the extension isn't a degraded-but-usable fallback, it's actively wrong. This probably wants to return undefined (and let the caller mark it unconverted) when no extension-preserving candidate is admitted, rather than accepting one that silently changes which engine/language reads the file.
| const moveAsServed = (file: LegacyFile) => { | ||
| converted.push({ | ||
| path: `${testsDirectory}/${file.path.slice(prefix.length)}`, | ||
| content: file.content, | ||
| }); | ||
| }; | ||
| const take = (conversion: Conversion, file: LegacyFile) => { | ||
| if (conversion.unconverted.length > 0) { | ||
| unconverted.push(...conversion.unconverted); | ||
| moveAsServed(file); | ||
| return; | ||
| } | ||
| converted.push(...conversion.files); | ||
| }; |
There was a problem hiding this comment.
[New] moveAsServed never touches claimed, so a moved-as-served path can collide with a case directory a later conversion in the same rule claims.
Traced against the current code with: legacy Vale files .tests/pass/a (no extension) and .tests/pass/a.md, same rule, in that order.
.tests/pass/a→convertValeDocumentseesextension === ""→derived = undefined→ unconverted →moveAsServedwrites{ path: "tests/pass/a", ... }.claimedis untouched..tests/pass/a.md→convertValeDocument(scope, "pass", "a.md", ..., claimed)→ stem"a", and sinceclaimedhas no"pass/a"entry (the move above never added one),claimCasehappily hands out"a"→ writes{ path: "tests/pass/a/a.md", ... }.
The output set now holds both tests/pass/a (a file) and tests/pass/a/a.md (which requires tests/pass/a to be a directory) — a file/directory collision that only surfaces when something actually writes these paths to disk.
This is the general shape of the gap: every moveAsServed call (here, and the two unconverted+moveAsServed pairs above for the "not under pass/fail" and "not beside its rule" cases) places a file at a path derived independently of claimed, so it's never reserved and never checked against what the case-directory conversions are about to claim. Worth either claiming the moveAsServed destination in claimed before conversion proceeds, or deriving moved-as-served paths from a namespace that can't overlap with <bucket>/<case>/.
| /** | ||
| * The scope a Vale rule's `.vale.ini` declares: every matcher assigning | ||
| * `<id>.<id> = YES` admits, every one assigning `NO` excludes. | ||
| * | ||
| * Positional precedence is flattened, deliberately. A path is admitted when | ||
| * some enabling matcher matches it and no disabling one does, which is | ||
| * stricter than Vale and so never derives a path Vale would exclude. | ||
| */ | ||
| export function valeFixtureScope(ruleId: string, config: string): FixtureScope { | ||
| const key = `${ruleId}.${ruleId}`; | ||
| const scope: FixtureScope = { include: [], exclude: [] }; | ||
| for (const section of parseValeRuleConfig(config).sections) { | ||
| if (section.name === undefined) continue; | ||
| for (const node of section.nodes) { | ||
| if (node.kind !== "property" || node.key !== key) continue; | ||
| const value = node.value.trim().toUpperCase(); | ||
| if (value === "YES") scope.include.push(section.name); | ||
| if (value === "NO") scope.exclude.push(section.name); | ||
| } | ||
| } | ||
| return scope; | ||
| } |
There was a problem hiding this comment.
[New] valeFixtureScope reads only literal YES/NO, but the config schema (schemas/vale-config.ts's vale-config-value-yes-no rejection) says plainly that Vale treats any other value as a level override that still enables the rule: "Vale {VERSION} reads a level name here as an override of the style's own level, and anything else as off, without saying so."
So for a matcher like [docs/**] / hedge.hedge = error, Vale enables hedge under docs/**, but valeFixtureScope registers neither an include nor an exclude for that matcher (the value matches neither "YES" nor "NO"). With scope.include left empty, deriveFixturePath reads that as "no restriction, admit everywhere" (scope.include.length === 0 branch) — which is the opposite of "stricter than Vale, never derives a path Vale would exclude" that this module's docstring claims. A fixture derived this way lands at the bare stem (e.g. a.md) instead of under docs/**, i.e. outside the one matcher that actually scopes the rule.
This is reachable today: convertLegacyFileSet's vale branch calls valeFixtureScope(ruleId, sibling(".vale.ini") ?? "") directly on the served .vale.ini text, with no gate on validateValeRuleConfig passing first. If migration 12 or delivery ever calls this converter before (or instead of) validation — which is plausible, since convertLegacyFileSet is documented as usable by "delivery, which reads a served set in the legacy layout" — a level-value rule's fixtures get silently placed outside its real scope.
Separately, confirmed as not a bug: the root section (no name) is correctly skipped — the same schema rejects a root-level assignment as a run-level key that Vale reads as enabling nothing, so the root is never an enabling matcher.
…verter migration 12 will use
…ep each file's extension A rule improved more than once owns several dated ast-grep test files, and each numbered its snippets from one, so the second file's fail-01 landed on the first's. Case names are now claimed in a set shared across a rule's conversions; two Vale documents sharing a stem get <stem> and <stem>-2. Path derivation also preferred the first glob even when it changed the file's extension, so under [*.md] and [*.txt] a .txt document became .md and would be linted as Markdown. A path keeping the extension now wins.
…ing a path From the follow-up review round: - A derived path keeps the file's extension or there is none: under [*.md] a .txt document was written as .md and linted as Markdown. A wildcard-only last segment (src/*, [*]) takes the file's extension. - Delivery's conversion claims every case the served set already carries and every name a moved fixture keeps, so a converted case no longer merges into an existing fail-01 or lands on a moved file's path. - sg test's __snapshots__ are dropped and named, not moved into tests/. - Vale enables a rule with a level as well as YES, so both admit. - A negated character class is filled with a character it admits.
c8ed830 to
2db039a
Compare
Stack (root → tip):
Groundwork for unified rule fixtures. Everything here is correct on its own, below the layer that renames the fixture directory.
assembleSgConfigcopies each rule's<id>.ymlinto.taskless/.sg-rules/and pointsruleDirsthere, so ast-grep never walks a rule's fixtures. On 0.51.0 a plaintests/holding a.ymlinsideruleDirsstill fails the whole scan; the old.tests/only escaped that through undocumented dot-directory skipping.checkassembles against its snapshot, so the tree lands in the run directory.buildOneRuleValeConfig. A Vale config enabling one rule under its own matchers, for the runner in feat(rules): every engine’s fixtures are case directories under tests/, judged through its own check #585.fixture-convert.ts. The pure converter migration 12 and delivery will share: language → extension table, a path derived from the rule's scope and checked withpath.matchesGlob(no warning on Node 22.22), sg snippets and Vale documents into case directories, and a legacy file-set converter. It is unused until feat(rules): every engine’s fixtures are case directories under tests/, judged through its own check #585.files:resolves against the scan's cwd when--configlives elsewhere; a plaintests/insideruleDirsfails the scan; every extension in the converter's table parses as its language. Vale 3.24.0: matchers resolve against the cwd, and hidden directories are walked. Thesg testoutput pins are dropped, since nothing will callsg test.Fixed after review: case ordinals restarted in each of a rule's legacy test files, so a rule with two dated test files had two
fail-01s. Names are now claimed across a rule's conversions. A derived path also keeps the file's extension: a.txtVale document under[*.md]and[*.txt]had been written as.md.Second review round: a converted file keeps its extension or stays unconverted; delivery reserves names already in a served set and names moved files keep;
sg testsnapshots are dropped; Vale level values (warning,error) count as enabling; negated character classes convert.Typecheck, lint and the full suite pass on this layer alone (2,786 tests).
Stack: #583 spec → #584 groundwork → #585 the
tests/rename → #586 recipes & archive. Every layer is green on its own; land it withgh stack merge. Design:openspec/changes/unified-rule-fixtures/design.md(archived on #586).