Skip to content

fix(test): notice when an sg rule's globs reach no file - #487

Merged
theCodeDrift merged 1 commit into
mainfrom
fix/486-sg-empty-scope
Oct 12, 2026
Merged

theCodeDrift merged 1 commit into
mainfrom
fix/486-sg-empty-scope

Conversation

@theCodeDrift

Copy link
Copy Markdown
Member

taskless test passed an ast-grep rule whose files: glob matched nothing in the project, and check then never ran it. Nothing said so. An sg fixture is a pathless valid:/invalid: snippet, so the globs never apply to it, and a mistyped glob looked exactly like a clean codebase.

test now asks ast-grep whether the rule applies to any file, and attaches a notice when it applies to none:

✓ sg/no-console-log
    notice: no-console-log's files: globs match no file in this project, so `check` will never run it. ...

How

  • ast-grep answers, not a glob library. sg scan -r <rule> --inspect entity reports appliedRuleCount per file. That count folds in language, files:, ignores:, .gitignore, and the same walk flags check uses (sgWalkArgv([])). A second matcher would be a second opinion that could disagree with check.
  • -r, not the assembled config, so a malformed sibling rule cannot make this rule's scope read as empty.
  • Stops at the first file in scope, so the common case costs little.
  • Silent when the scan does not finish cleanly, because reporting an empty scope from a failed walk is the same false clean in reverse.

Choices

  • A notice, not a failure. A narrow glob can be deliberate, for example a rule written ahead of the code it targets.
  • Only rules declaring files: or ignores: are measured. A rule scoped by language: alone, in a project with no code in that language, is honestly clean.
  • test only. verify stays cheap and never walks the project.
  • No file count. The issue's second suggestion, a count of files in scope, would add an output field. Left for a follow-up.

Tests

Seven end-to-end cases in verify-test-commands.test.ts. Each one was mutation-checked: I broke the behavior it guards and confirmed it went red.

Case Fails when
files: matches nothing → notice the check never fires
glob reaches a file → silent the check always fires
glob reaches only another language's files → notice scope is judged by path alone
ignores: excludes everything → notice naming ignores: the check never fires
glob reaches only gitignored files → notice the walk ignores .gitignore
glob reaches only .taskless/ → notice the walk drops check's .taskless/ exclusion
no globs, no code → silent the files:/ignores: gate is removed

Fixes #486

An ast-grep fixture is a pathless snippet, so a rule's globs never apply
to it. A rule scoped to a directory that does not exist passed `test`
while `check` never ran it. `test` now asks ast-grep, through
`--inspect entity` over the same walk `check` performs, whether the rule
applies to any file, and attaches a notice when it applies to none.
@theCodeDrift

Copy link
Copy Markdown
Member Author

@claude /review

@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @theCodeDrift's task in 4m 31s —— View job


Review: fix(test): notice when an sg rule's globs reach no file

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contains no prior inline threads, no prior review summaries, and no prior top-level comments besides the triggering @claude /review comment itself — this is the first review of this PR.

  • Read prior-review data (none found — first review)
  • Read full diff via gh pr diff
  • Read changed files in full (inspect.ts, sg-scope.ts, test file) plus related context (scan.ts, engines.ts, notices.ts, sg-fixture-findings.ts, verify.ts)
  • Review for correctness, security, performance, test adequacy, clarity
  • Post findings

Summary

The approach is sound and well-isolated: asking ast-grep's own --inspect entity for appliedRuleCount rather than re-implementing glob/.gitignore/language matching avoids the "second opinion that can disagree with check" trap, and using -r <rule> rather than the assembled config correctly keeps one malformed sibling rule from making this rule's scope read as empty — consistent with how sg-fixture-findings.ts already does it. The three-way "reached" | "empty" | "unknown" result, and staying silent on "unknown", correctly avoids reporting a false empty scope from a walk that didn't finish. The gating on files:/ignores: presence (so a rule scoped only by language: is never measured) matches the stated design, and the test suite — 7 cases, each mutation-checked, covering .gitignore, .taskless/ exclusion, and language-mismatch — is solid evidence the scope question is being asked correctly against the real vendored binary rather than being reconstructed with a weaker tool, which is exactly what this repo's testing conventions ask for.

Two [New] findings posted inline, both performance-oriented, no correctness bugs found:

  1. packages/cli/src/rules/sg-scope.ts (lines 70–132) — describeEmptySgScope spawns a fresh whole-project walk per rule that declares files:/ignores:, and the worst case (glob matches nothing) is exactly the case that can't exit early at "first file in scope" — it has to walk the entire project to confirm zero. runOverPath (commands/verify.ts) runs rules sequentially, so N scoped rules means N full sequential walks on every taskless test, where check answers the equivalent question once via a single assembled-config scan. The PR's own framing — a narrow glob "written ahead of the code it targets" is a deliberate, expected pattern — means the expensive path isn't a rare misconfiguration case, it's the common one for the intended use. Not necessarily a blocker given the explicit "costs little in the common case" tradeoff already acknowledged in the PR description, but worth confirming this was measured against a realistically-sized rule tree.

  2. packages/cli/src/rules/inspect.ts (lines 505–508) — minor: describeEmptySgScope and collectSgFixtureFindings are awaited sequentially rather than concurrently, so the new walk's cost adds fully on top of existing per-fixture scan time rather than overlapping with it. Low severity, flagged alongside finding 1.

Smaller things noticed, not worth blocking on:

  • The notice text doesn't distinguish which of files:/ignores: is actually responsible when a rule declares both and only one is the empty-scope cause (e.g. a broad files: fully cancelled by ignores:). The PR explicitly deferred a related richer diagnostic (file count) to a follow-up, so this is probably an acceptable simplification in the same spirit, just noting it's not fully precise.
  • scopingKeys re-reads and re-parses the rule YAML that verify's schema layer already parsed moments earlier in the same call. Negligible next to the walk cost in finding 1, not worth a separate note.

Changeset (patch) is correctly scoped per this repo's semver guidance — this is additive surface on a 0.y.z package. test-only placement (not verify) matches the stated design rationale.
· Branch: fix/486-sg-empty-scope

Comment on lines +70 to +80
/**
* Whether ast-grep applies the rule to at least one file in the project.
*
* Three answers, because a walk that did not finish is neither: `unknown` is
* what lets the caller stay silent rather than report an empty scope it never
* measured.
*
* Stops at the first file in scope: the answer is a yes/no, and the rest of the
* walk, including running the rule over every file, would only confirm it.
*/
function reachesAnyFile(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[New] Performance: one full-project walk per scoped rule, run sequentially for every rule in the test target.

describeEmptySgScope spawns a fresh whole-project sg scan -r <rule> --inspect entity for every rule that declares files:/ignores:. The worst case — the glob matches nothing — is also the case this feature exists to catch, and it's the one case where the walk can't stop early at "first file in scope": ast-grep has to visit every file in the project before --inspect entity can confirm zero matches.

Two things compound this:

  • runOverPath in commands/verify.ts (for (const rule of rules) { results.push(await run(cwd, rule)); }) runs rules sequentially, not in parallel, so N scoped rules means N full sequential walks.
  • check answers "is this rule in scope" once per run, for every rule, via a single assembled-config scan. This feature re-derives the same answer per rule via its own dedicated walk, so a project with e.g. 30 ast-grep rules scoped to subdirectories-not-yet-written (explicitly called out in the PR description as a deliberate, expected pattern) pays 30 full-tree walks on every taskless test, where check pays one.

Not necessarily a blocker — the PR already made a deliberate "costs little in the common case" tradeoff — but worth confirming this was measured against a repo of realistic size/rule-count, since the expensive path is exactly the one the feature is meant to exercise.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Measured rather than assumed. Declining.

On this repo (298 TypeScript files in the walk), one empty-scope walk takes ~65 ms, about the same as running a real rule over every file. taskless test .taskless/rules/sg over all 9 rules, 7 of them scoped, takes 0.34–0.39 s without the walk and 0.43–0.44 s with it, discounting one 0.63 s outlier. Every one of those globs reaches a file, so each walk stops at the first hit.

The full walk only happens when a glob matches nothing, and that run also prints a notice for every such rule. Thirty rules scoped ahead of their code means thirty notices to act on. The extra latency on top of that is not where the cost lands.

The walk can't be batched into one scan either. --inspect entity reports a per-file appliedRuleCount without naming the rules, so with more than one rule loaded the zero can't be attributed to a rule. check never answers "is this rule in scope". It only reports findings, and a rule with no findings looks exactly like a rule that never ran, which is the bug.

Comment on lines +505 to +508
notices: collectNotices([
...verification.notices,
await describeEmptySgScope(cwd, ruleId),
]),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[New] Minor: these two awaits run sequentially where they could overlap.

await describeEmptySgScope(cwd, ruleId) (a full-project walk) completes before collectSgFixtureFindings(cwd, ruleId) (one scan --stdin per fixture snippet) even starts, since both are plain awaits evaluated in object-literal order. Running them via Promise.all would let the per-rule cost of this PR's new walk overlap with work test was already doing, rather than adding on top of it. Minor given the scan already dominates; flagging alongside the broader performance note on sg-scope.ts.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining. On a scoped rule whose glob reaches a file, which is every scoped rule in this repo, the walk exits at the first hit and the whole feature adds ~50 ms across 7 rules. Overlapping it with the fixture scans would save part of that. That's not enough to justify reordering how the result object is built.

@theCodeDrift
theCodeDrift merged commit a36bab1 into main Oct 12, 2026
10 checks passed
@theCodeDrift
theCodeDrift deleted the fix/486-sg-empty-scope branch October 12, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test passes an sg rule whose files: glob matches nothing, and check then never runs it

1 participant