Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/scan-hidden-directories.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
---
"@taskless/cli": patch
---

Let ast-grep rules see inside hidden directories such as `.github/`.

ast-grep's file walker skips dot-directories unless told otherwise, and
`runAstGrepScan` never told it otherwise. No `sg` rule could match anything
under `.github/`, `.circleci/`, `.vscode/` or `.husky/`, so `check` reported
nothing and exited 0 on a workflow file it flags correctly the moment the same
bytes live in a non-hidden directory. Vale has no such blind spot, which left
the two static engines disagreeing about whether `.github/` existed at all.
Both `check` and the runtime engine's ast-grep narrow now pass
`--no-ignore hidden`.

Only `hidden` is passed, and deliberately not `vcs`: `.gitignore` is still
respected, so the wider walk does not start reporting findings in `dist/` or
anywhere else a project has already said it does not want scanned. Rule
discovery is untouched — `ruleDirs` walks by its own rules, so a rule's
`.tests/` directory is still skipped rather than parsed as a rule.

`.taskless/` is excluded from the wider walk, because it is hidden too and
reaching it is not a fix. A rule definition is structured YAML full of `id:`,
`language:`, `severity:` and `rule:` keys, so an ordinary user-written Yaml rule
fires on the CLI's own rule files — a finding in a directory the user did not
author and cannot edit without disabling their rule. The exclusion applies only
when `check` walks the whole project on its own; an explicit path stays a
request, which is the rule the Vale runner already follows.

`.git/` is excluded on the same terms. ast-grep has no exclusion of its own for
it and `.gitignore` does not list it, so the default hidden-directory skip was
the only thing holding it back: without this, a whole-project `check` descended
into `.git/objects` and `.git/logs` on every run, and `.git/hooks/*` scripts
matched language rules never meant to lint VCS internals.

Both engines now decide "whole project" the same way, and it is no longer
`paths.length === 0`. An explicit `.` is normalized to the literal path `"."`
before it reaches either runner, so a length test read the most ordinary way of
asking for a whole-project check as a user-named path and skipped the exclusions
— `check` was clean while `check .` reported findings inside `.taskless/`. Vale
was already wrong in the same way and for the same reason, independently of the
hidden-directory change, so the predicate is now shared rather than written
twice.
8 changes: 7 additions & 1 deletion packages/cli/src/rules/runtime/narrow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { StringDecoder } from "node:string_decoder";

import { stringify } from "yaml";

import { buildPath, findSgBinary } from "../scan";
import { buildPath, findSgBinary, sgWalkArgv } from "../scan";
import type { Match } from "../../types/runtime-rule";
import type { LoadedCaptureRule, RuntimeRule } from "./discover";

Expand Down Expand Up @@ -38,6 +38,12 @@ function runSg(
"scan",
"--config",
configPath,
// The narrow is a `sg scan` over the user's project, so it walks it on
// the same terms `runAstGrepScan` does — reaching `.github/`, and not
// reaching `.taskless/`. A capture rule blind to the first would report
// nothing for a workflow file; one that reached the second would anchor a
// runtime rule on the CLI's own config.
...sgWalkArgv(paths),
...extraArguments,
...(paths.length > 0 ? ["--", ...paths] : []),
];
Expand Down
87 changes: 87 additions & 0 deletions packages/cli/src/rules/scan.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { fileURLToPath } from "node:url";
import type { AstGrepMatch } from "../types/check";
import { toCheckResult, type CheckResult } from "../types/check";
import { ASSEMBLED_SG_CONFIG } from "./engines";
import { isWholeProjectWalk } from "./walk-scope";
import {
isPlatformBinary,
pathCommandName,
Expand Down Expand Up @@ -126,6 +127,91 @@ export interface ScanOptions {
configPath?: string;
}

/**
* The project directory this CLI owns. Declared locally rather than imported
* from the Vale runner, which keeps its own copy for the same reason.
*/
const TASKLESS_DIRECTORY = ".taskless";
const GIT_DIRECTORY = ".git";

/**
* Directories kept out of a whole-project walk, each with its own reason.
*
* ast-grep honors repeated `--globs` flags — measured against 0.41.0, two
* exclusions both apply, unlike Vale's `--glob`, where the last one silently
* wins. So this is a list rather than one combined brace pattern.
*/
const EXCLUDED_DIRECTORIES = [TASKLESS_DIRECTORY, GIT_DIRECTORY] as const;

/**
* How every `sg scan` we spawn is told to walk the project.
*
* One function rather than a constant per call site, because both scan call
* sites — {@link runAstGrepScan} and the runtime narrow — need exactly these
* flags on exactly these terms, and a third would too. ast-grep's
* `sgconfig.yml` has no equivalent knob, so this cannot live in the assembled
* config; the argv is the only place it can be expressed.
*
* **`--no-ignore hidden`**, always. It lets the walker descend into
* dot-directories, which it refuses to do by default. Without it no `sg` rule
* could match anything under `.github/`, `.circleci/`, `.vscode/` or
* `.husky/` — a silent false negative, since `check` reported nothing and
* exited 0. Vale has no such blind spot, so the two static engines disagreed
* about whether `.github/` existed at all. Measured against the pinned ast-grep
* 0.41.0, `hidden` is the only value that reaches those directories: `dot`,
* `exclude`, `global` and `parent` all left `.github/` unscanned. Deliberately
* **not** passed is `vcs`, which stops `.gitignore` being respected and was
* measured to pull `dist/` into the scan — a rule has no business reporting
* findings in build output or vendored dependencies.
*
* **A `--globs` exclusion of `.taskless/`**, when we are the ones who chose to
* walk the whole project. That directory is hidden, so it was never scanned
* before and reaching it is not a fix: it is CLI-managed config the user did
* not author. Every rule definition in it is structured YAML carrying `id:`,
* `language:`, `severity:`, `message:` and `rule:` keys, so any reasonable
* user-written Yaml rule fires on the CLI's own rule files — an unfixable false
* positive in a directory the user cannot edit without disabling their rule.
* The `**` prefix on the glob is load-bearing: a root-anchored `.taskless/**`
* was measured to miss a `.taskless/` nested inside a monorepo package, which
* is the same CLI-managed config one level down.
*
* **A `--globs` exclusion of `.git/`**, on the same terms. Measured against
* 0.41.0: ast-grep has no exclusion of its own for it, so `--no-ignore hidden`
* makes `.git/` reachable along with every other dot-directory, and a
* whole-project scan descended into `.git/objects`, `.git/logs` and
* `.git/hooks`. That is wasted work proportional to repository history on every
* run, and `.git/hooks/*` are real source files that match language rules never
* meant to lint VCS internals. `.gitignore` does not cover this — `.git/` is not
* in it — so the default hidden-directory skip was the only thing holding it
* back. Repeated `--globs` flags were measured to both apply, unlike Vale's
* `--glob` where the last one silently wins, so the two exclusions are separate
* flags rather than one brace pattern.
*
* Both exclusions are applied **only** for a whole-project walk, matching what
* the Vale runner does and for its reason: an explicit path is a request, and
* silently declining to check a file someone named would be worse than checking
* one they did not. "Whole project" is {@link isWholeProjectWalk} rather than
* `paths.length === 0` — see that function for why the difference is
* load-bearing rather than cosmetic.
*
* Measured interactions worth keeping in mind if this is ever changed:
* `--globs` survives `--no-ignore hidden` rather than being overridden by it,
* and neither flag touches rule discovery — `ruleDirs` reads the rules out of
* `.taskless/` by its own walk, so excluding that path from the *scan* does not
* stop the rules from loading, and a rule's `.tests/` directory is still
* skipped rather than parsed as a rule (see `RULE_TESTS_DIRECTORY` in
* `engines.ts`).
*/
export function sgWalkArgv(paths: string[]): string[] {
Comment thread
thecodedrift marked this conversation as resolved.
const exclude = isWholeProjectWalk(paths)
? EXCLUDED_DIRECTORIES.flatMap((directory) => [
"--globs",
`!**/${directory}/**`,
])
: [];
return ["--no-ignore", "hidden", ...exclude];
}

/** Run ast-grep scan and return parsed results */
export async function runAstGrepScan(
cwd: string,
Expand All @@ -141,6 +227,7 @@ export async function runAstGrepScan(
"--config",
options.configPath ?? ASSEMBLED_SG_CONFIG,
"--json=stream",
...sgWalkArgv(paths),
...(paths.length > 0 ? ["--", ...paths] : []),
];
const child = spawn(sgBinary, argv, {
Expand Down
10 changes: 9 additions & 1 deletion packages/cli/src/rules/vale/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { CheckResult } from "../../types/check";
import { ASSEMBLED_VALE_CONFIG } from "../engines";

import { buildPath } from "../scan";
import { isWholeProjectWalk } from "../walk-scope";
import { findValeBinary, valeUnavailableMessage } from "./binary";
import { asValeConfigError, toValeCheckResults, type ValeOutput } from "./map";

Expand Down Expand Up @@ -128,7 +129,14 @@ export async function runVale(
// reason this is easy to miss: it takes its targets from the config and is
// content with none, so the two engines disagree about what "no paths" means.
// `cwd` is the project root, so `.` is the whole project.
const wholeProject = paths.length === 0;
//
// `isWholeProjectWalk` rather than `paths.length === 0`: `check .` arrives
// here with `paths = ["."]`, which a length test reads as a user-named path
// and so skips the `.taskless/` exclusion below. Vale reads hidden
// directories by default, so that route reported prose findings inside
// `.taskless/` on any `check .`, independently of the ast-grep fix in this
// change. Same defect, same signal, one line apart.
const wholeProject = isWholeProjectWalk(paths);
const targets = wholeProject ? ["."] : paths;

// Walking the whole project reaches `.taskless/` too, and Vale has no reason
Expand Down
35 changes: 35 additions & 0 deletions packages/cli/src/rules/walk-scope.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* Whether a set of positional paths means "walk the whole project".
*
* Both static engines apply their CLI-managed-directory exclusions only on a
* whole-project walk, because an explicit path is a request: silently declining
* to check a file someone named would be worse than checking one they did not.
* That rule needs a correct answer to "did we choose this, or did the user?",
* and both engines were getting it from `paths.length === 0`.
*
* **That test is wrong for `.`, and the mistake is silent.** `filterExistingPaths`
* (`commands/check.ts`) normalizes a positional path resolving to cwd into the
* literal string `"."` rather than dropping back to an empty array, so
* `taskless check .` — a near-default invocation — arrives with `paths = ["."]`.
* Under a length test that reads as a user request and skips the exclusions,
* which is how `check .` came to report findings inside `.taskless/` while a
* bare `check` did not.
*
* A single explicit `.` is a request for the project, not for the CLI's own
* config inside it, so it is a whole-project walk. A path *under* `.taskless/`
* is still honored: that names the config directly.
*
* Shared rather than duplicated because the two engines diverging here is
* exactly the class of bug this fixes — one of them was already wrong in the
* same way.
*/
export function isWholeProjectWalk(paths: string[]): boolean {
if (paths.length === 0) return true;
return paths.length === 1 && CWD_ALIASES.has(paths[0] ?? "");
}

/**
* Spellings of "here" that `filterExistingPaths` can emit or a shell can pass.
* `"."` is what the normalizer produces; the others reach us straight from argv.
*/
const CWD_ALIASES = new Set([".", "./", ".\\"]);
Loading