diff --git a/.changeset/check-honors-gitignore.md b/.changeset/check-honors-gitignore.md new file mode 100644 index 00000000..484dfdac --- /dev/null +++ b/.changeset/check-honors-gitignore.md @@ -0,0 +1,47 @@ +--- +"@taskless/cli": patch +--- + +Keep a whole-project `check` out of the paths git ignores. + +`check` reported prose findings from inside gitignored directories. The case +that surfaced it was a git worktree at `worktrees//`, which is a complete +second checkout: every Vale rule fired again over another branch's documents, +including code an agent was mid-edit on. That makes the finding count move when +a worktree appears or disappears with nothing in the output explaining why, and +the general shape is the same for `dist/`, vendored trees, and local scratch +directories — a check reporting on files nobody maintains. + +Only one engine was wrong, which is why it was hard to attribute. ast-grep's +walker is the `ignore` crate and `sgWalkArgv` has always passed `--no-ignore +hidden` without `vcs`, so a bare scan already skipped `worktrees/`; measured +against the pinned 0.41.0, it skips a hidden-_and_-ignored `.turbo/` too. Vale +has no notion of a VCS and walked everything. So the two static engines +disagreed about which files the project contains, and only the prose findings +duplicated. On a fixture repository with a worktree present, a bare `check` +went from 6 findings to 4; the two that left were both Vale, both a second copy +of a finding already reported against the tracked file. + +The set comes from `git ls-files --others --ignored --exclude-standard +--directory -z`, which is the complement of the tracked-plus-untracked set the +question is usually phrased as. The complement is the one that scales: +`--directory` collapses a wholly-ignored directory to a single entry, so +`node_modules/` costs one line rather than forty thousand, and the result is +short enough to hand Vale as `--glob` exclusions without meeting `ARG_MAX`. No +new dependency — `.gitignore` is not one file or one syntax question once +nested ignore files, `.git/info/exclude`, a global `core.excludesFile` and +negation patterns are involved, and git already answers all of it in one call. + +The exclusion belongs to the walk `check` chose for itself. `check +worktrees/probe` names an ignored path deliberately and still checks it, on the +same terms as the existing `.taskless/` exclusion. A directory that is not a +git repository, or a host with no `git` on its `PATH`, gets an empty ignore set +and the walk that shipped before this change. Standing _inside_ an ignored +directory is treated as explicit too: git answers `./` there, meaning +"everything here", and honouring that would return an empty check with nothing +saying why. + +The converter skip notice no longer names files inside ignored paths. An +`.adoc` under `worktrees/` is not a file this run declined to convert; it is a +file this run was never going to open, and naming it would send the reader to +investigate a directory the exclusion is there to keep out. diff --git a/packages/cli/src/rules/git-ignored.ts b/packages/cli/src/rules/git-ignored.ts new file mode 100644 index 00000000..66ea1d5e --- /dev/null +++ b/packages/cli/src/rules/git-ignored.ts @@ -0,0 +1,156 @@ +import { execFile } from "node:child_process"; + +/** + * What git considers ignored, and how the engines are told to skip it. + * + * ## Why this exists + * + * A whole-project `check` reported findings from paths git ignores — build + * output, vendored trees, and (the case that surfaced it, taskless/cli#166) a + * git worktree at `worktrees//`, which is a complete second checkout. A + * worktree is the worst shape of the bug because it duplicates *every* finding + * and attributes each copy to a branch the user is not working on, so the + * finding count moves when a worktree appears or disappears with nothing in the + * output explaining why. + * + * ## The two engines do not need the same amount of help + * + * **ast-grep already honors `.gitignore` and always has.** Its walker is the + * `ignore` crate, and `sgWalkArgv` passes `--no-ignore hidden` *only* — + * deliberately not `vcs`, which is the value that would switch VCS ignore files + * off. Measured against the pinned 0.41.0 in a repository ignoring + * `worktrees/`: a bare scan reports nothing under `worktrees/probe/`, and it + * reports nothing under a hidden-*and*-ignored `packages/cli/.turbo/` either, + * which is the case that would expose `--no-ignore hidden` if that flag had + * quietly disabled more than hidden-file skipping. So nothing in `scan.ts` + * changes; `check-gitignore.test.ts` pins the behavior so a future flag edit + * cannot take it away silently. + * + * **Vale honors nothing.** It has no notion of a VCS, walks `.` as handed to + * it, and reads hidden directories by default — so every gitignored document in + * the tree was linted. That is the whole of taskless/cli#166, and this module + * is what closes it. + * + * ## Why `git ls-files` and not a `.gitignore` parser + * + * The styleguide's rule about not adding a dependency to answer a question the + * existing toolchain can answer applies squarely. `.gitignore` is not one file + * and not one syntax question: the real answer folds in nested `.gitignore`s at + * every level, `.git/info/exclude`, the user's global `core.excludesFile`, and + * negation patterns that re-include a path a parent excluded. A parsing library + * approximates that; git *is* it, it is already required for `check`'s other + * work (see `util/git-remote.ts`), and it answers in one call. + * + * The call is `--others --ignored --exclude-standard --directory`, which is the + * *complement* of the set the issue proposed (`--cached --others + * --exclude-standard`). Both describe the same partition; the complement is + * chosen because it is the one that scales. The positive set is every file in + * the project — thousands of paths that would have to reach Vale as positional + * arguments and would meet `ARG_MAX` on a large repository. The complement is + * short, because `--directory` collapses an entirely-ignored directory to a + * single entry: `node_modules/` is one line, not forty thousand, and this + * repository yields thirty entries in total. + */ + +/** + * Paths git ignores under `cwd`, relative to it, directories with a trailing + * `/`. + * + * `-z` rather than newline-delimited output: a filename may legally contain a + * newline, and without `-z` git renders such a path quoted and C-escaped, so a + * line-split would produce two entries neither of which names a real file. + * + * Every failure mode returns an empty list, and that is the required behavior + * rather than a swallowed error: a directory that is not a git repository has + * nothing ignored, and it is the same answer a host with no `git` on its PATH + * must get. The engines then walk exactly as they did before this module + * existed, which is the non-git fallback the issue asks for. + */ +export async function listGitIgnoredEntries(cwd: string): Promise { + const stdout = await new Promise((resolve) => { + execFile( + "git", + [ + "ls-files", + "--others", + "--ignored", + "--exclude-standard", + "--directory", + "-z", + ], + { cwd, maxBuffer: 16 * 1024 * 1024 }, + (error, output) => { + resolve(error ? "" : output); + } + ); + }); + + const entries = [ + ...new Set(stdout.split("\0").filter((entry) => entry !== "")), + ]; + + // A walk root that is itself ignored answers `./`, and that single entry + // means "everything here". Measured by running `check` from inside a + // gitignored `build/`: git reports `./` and nothing else. Excluding it would + // hand the user an empty result set for a directory they deliberately stood + // in, with nothing in the output explaining where their findings went — the + // same silent-count-change complaint the issue opened with, inverted. + // + // Standing in a directory is as explicit as naming it, so the answer is the + // one an explicit path gets: ignore nothing. This does not currently change + // behavior — Vale's glob does not match `README.md` against `./**` — but that + // is a property of Vale's matcher, not a decision anyone made, and the whole + // failure mode is invisible. Decided here instead. + return entries.some((entry) => ROOT_ENTRIES.has(entry)) ? [] : entries; +} + +/** How git can spell "the walk root itself" in a `--directory` listing. */ +const ROOT_ENTRIES = new Set(["./", "."]); + +/** + * Characters that would make an entry mean something other than itself once it + * is spliced into Vale's `--glob` alternation. + * + * A comma is the dangerous one: `buildValeGlob` joins patterns with `,` inside + * `!{…}`, so a filename containing a comma would split into two patterns, and + * both halves would be wrong. The rest are glob metacharacters that would turn + * a literal path into a matcher. + * + * An entry carrying any of them is left out of the exclusion rather than + * escaped. Vale's glob dialect is not ours to guess at, and the cost of leaving + * it out is that one pathologically-named ignored path is still linted — which + * is exactly the behavior that shipped before this module, so it is a gap + * rather than a regression. {@link isGitIgnoredPath} does not share the + * restriction, so such a path is still kept out of the skip notice. + */ +const GLOB_METACHARACTERS = /[*?[\]{},\\!]/; + +/** + * The ignored entries, rendered as patterns for Vale's `--glob`. + * + * A directory entry becomes `dir/**` rather than `dir/`, because Vale matches + * the pattern against files and never against the directory itself. A file + * entry is used verbatim: git already reports it as a path relative to the + * project root, and every pattern in the combined alternation is matched + * path-wise (see `buildValeGlob` in `formats.ts` for why that is a property of + * the whole expression rather than of one branch). + */ +export function gitIgnoredExclusionGlobs(entries: string[]): string[] { + return entries + .filter((entry) => !GLOB_METACHARACTERS.test(entry)) + .map((entry) => (entry.endsWith("/") ? `${entry}**` : entry)); +} + +/** + * Whether `path` — relative to the project root — falls inside `entries`. + * + * Plain string work, deliberately: the entries are literal paths from git, so + * answering this with a glob engine would reintroduce the metacharacter + * problem {@link gitIgnoredExclusionGlobs} has to duck. This is what keeps the + * converter skip notice from naming files Vale was never going to open. + */ +export function isGitIgnoredPath(path: string, entries: string[]): boolean { + return entries.some((entry) => + entry.endsWith("/") ? path.startsWith(entry) : path === entry + ); +} diff --git a/packages/cli/src/rules/scan.ts b/packages/cli/src/rules/scan.ts index 70f4cd8d..e06677aa 100644 --- a/packages/cli/src/rules/scan.ts +++ b/packages/cli/src/rules/scan.ts @@ -164,6 +164,18 @@ const EXCLUDED_DIRECTORIES = [TASKLESS_DIRECTORY, GIT_DIRECTORY] as const; * measured to pull `dist/` into the scan — a rule has no business reporting * findings in build output or vendored dependencies. * + * That omission is the whole of this engine's `.gitignore` handling, and it is + * load-bearing rather than incidental: ast-grep's walker is the `ignore` crate, + * so leaving `vcs` off is what keeps a bare scan out of `worktrees/`, `dist/` + * and vendored trees. Re-measured for taskless/cli#166 against 0.41.0 — a bare + * scan reports nothing under a gitignored `worktrees/probe/`, and nothing under + * a hidden-*and*-ignored `packages/cli/.turbo/` either, which is the case that + * would show up if `--no-ignore hidden` had ever disabled more than hidden-file + * skipping. Vale, which honors none of this, is handed the same set explicitly; + * see `rules/git-ignored.ts` for why the two engines need different amounts of + * help to agree. `test/check-gitignore.test.ts` pins both sides, so a later + * edit to this argv cannot quietly hand the ignored tree back to the scan. + * * **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 diff --git a/packages/cli/src/rules/vale/run.ts b/packages/cli/src/rules/vale/run.ts index b1e1bdc2..3add0592 100644 --- a/packages/cli/src/rules/vale/run.ts +++ b/packages/cli/src/rules/vale/run.ts @@ -5,6 +5,11 @@ import { StringDecoder } from "node:string_decoder"; import type { CheckResult } from "../../types/check"; import { ASSEMBLED_VALE_CONFIG } from "../engines"; +import { + gitIgnoredExclusionGlobs, + isGitIgnoredPath, + listGitIgnoredEntries, +} from "../git-ignored"; import { buildPath } from "../scan"; import { isWholeProjectWalk } from "../walk-scope"; import { findValeBinary, valeUnavailableMessage } from "./binary"; @@ -203,19 +208,54 @@ export async function runVale( // findings with it. Honouring the request would cost the user the rest of // their check, so the request is declined and reported instead. See // `formats.ts` for the tier table this is derived from. + // + // What git ignores, on a whole-project walk only — same terms as + // `.taskless/**` above, and for a sharper version of the same reason. Vale + // has no notion of a VCS: it walked into build output, vendored trees, and a + // git worktree at `worktrees//`, which is a complete second checkout, + // so every prose rule fired again against another branch's documents + // (taskless/cli#166). ast-grep needed no equivalent — its walker honors + // `.gitignore` already — which is precisely why the two engines disagreed + // about which files the project contains. Asked only when we chose `.` + // ourselves, so `check worktrees/probe` still checks what it was handed. + // + // Started together with the converter-dependent scan below, because neither + // answer feeds the other: the ignore list shapes the `--glob` argument, the + // scan shapes the skipped-files notice, and only the `.filter` further down + // ever brings the two together. Awaited in series they would charge every + // whole-project run a git subprocess and then a directory walk, back to + // back, before the Vale subprocess has even been spawned. + // + // The notice half of that pair is asked before the run rather than inferred + // from it. Vale never reports what its walker declined to open, so once the + // glob has done its job the skipped files are unrecoverable from the output, + // and a fix whose only visible effect is that some findings are quietly + // missing is the bug it replaced. + // + // The ignored paths are filtered back out for the same reason the notice + // exists at all: it must describe the run that happened. An `.adoc` inside + // `worktrees/` is not a file this run declined to convert, it is a file this + // run was never going to look at, and naming it would send the reader to + // investigate a directory the fix above deliberately excluded. + const [ignoredEntries, converterDependent] = await Promise.all([ + wholeProject ? listGitIgnoredEntries(options.cwd) : [], + findConverterDependentFiles(options.cwd, paths), + ]); + const exclude = [ - ...(wholeProject ? [`${TASKLESS_DIRECTORY}/**`] : []), + ...(wholeProject + ? [ + `${TASKLESS_DIRECTORY}/**`, + ...gitIgnoredExclusionGlobs(ignoredEntries), + ] + : []), ...converterExclusionGlobs(), ]; const globArgument = buildValeGlob(exclude); const globFlags = globArgument === undefined ? [] : [globArgument]; - // Asked before the run rather than inferred from it. Vale never reports what - // its walker declined to open, so once the glob has done its job the skipped - // files are unrecoverable from the output — and a fix whose only visible - // effect is that some findings are quietly missing is the bug it replaced. const skipped = skippedFilesNotice( - await findConverterDependentFiles(options.cwd, paths) + converterDependent.filter((file) => !isGitIgnoredPath(file, ignoredEntries)) ); // `--` separates flags from positional paths, so a path beginning with `-` diff --git a/packages/cli/test/check-gitignore.test.ts b/packages/cli/test/check-gitignore.test.ts new file mode 100644 index 00000000..8d0e6ba4 --- /dev/null +++ b/packages/cli/test/check-gitignore.test.ts @@ -0,0 +1,236 @@ +import { execFile } from "node:child_process"; +import { cp, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { promisify } from "node:util"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { + gitIgnoredExclusionGlobs, + isGitIgnoredPath, + listGitIgnoredEntries, +} from "../src/rules/git-ignored"; +import { findValeBinary } from "../src/rules/vale/binary"; + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); +const fixturesDirectory = resolve( + import.meta.dirname, + "fixtures/mixed-engines-project" +); + +/** + * A whole-project `check` must not report findings from paths git ignores. + * + * The reported case (taskless/cli#166) was a git worktree at + * `worktrees//`, which is a complete second checkout: every rule fires + * again over another branch's files, so the finding count moves when a worktree + * appears or disappears and nothing in the output says why. The general shape is + * the same for `dist/`, vendored trees, and local scratch directories. + * + * The two engines arrive at the same behavior by different routes and both are + * pinned here, deliberately. ast-grep honors `.gitignore` through its own + * walker and always did — the assertion exists so that an edit to `sgWalkArgv` + * (adding `--no-ignore vcs`, say) cannot take it away silently. Vale honors + * nothing and is handed git's answer as `--glob` exclusions. A test that only + * covered the engine that was broken would let the two drift apart again, which + * is the condition that made the bug hard to attribute in the first place. + * + * These spawn the built CLI over a real git repository. Neither half can be + * usefully faked: the question is what ast-grep's walker and Vale's walker + * actually do, and a mock would be asserting the mock. + */ + +/** Run the built CLI, tolerating a non-zero exit. */ +async function runCli( + arguments_: string[] +): Promise<{ stdout: string; exitCode: number }> { + try { + const { stdout } = await execFileAsync("node", [binPath, ...arguments_]); + return { stdout, exitCode: 0 }; + } catch (error) { + const failure = error as { stdout: string; code: number }; + return { stdout: failure.stdout ?? "", exitCode: failure.code }; + } +} + +interface CheckFinding { + source: string; + ruleId: string; + file: string; +} + +interface CheckOutput { + results: CheckFinding[]; +} + +async function checkedFiles( + project: string, + arguments_: string[] = [] +): Promise { + const { stdout } = await runCli([ + "check", + "-d", + project, + "--json", + ...arguments_, + ]); + return (JSON.parse(stdout.trim()) as CheckOutput).results; +} + +/** Findings whose file sits under `ignored/`, by engine. */ +function sourcesUnderIgnored(results: CheckFinding[]): Set { + return new Set( + results + .filter((finding) => finding.file.startsWith("ignored/")) + .map((finding) => finding.source) + ); +} + +/** Vale ships per-platform; an unsupported host has none. */ +const valeAvailable = findValeBinary().path !== undefined; + +describe("check over a project with a gitignored directory", () => { + let project: string; + + beforeEach(async () => { + project = await mkdtemp(join(tmpdir(), "taskless-gitignore-")); + await cp(fixturesDirectory, project, { recursive: true }); + // A duplicate of the fixture's two rule-tripping files, one directory + // deeper. This is the worktree case in miniature: the same documents, in a + // place git has been told not to track. + await mkdir(join(project, "ignored"), { recursive: true }); + await cp(join(project, "README.md"), join(project, "ignored/README.md")); + await cp(join(project, "sample.js"), join(project, "ignored/sample.js")); + }); + + afterEach(async () => { + await rm(project, { recursive: true, force: true }); + }); + + describe("in a git repository that ignores it", () => { + beforeEach(async () => { + await writeFile(join(project, ".gitignore"), "ignored/\n"); + await execFileAsync("git", ["init", "--quiet"], { cwd: project }); + }); + + it("reports nothing from the ignored directory on a bare walk", async () => { + const results = await checkedFiles(project); + + expect(sourcesUnderIgnored(results)).toEqual(new Set()); + // Not vacuous: the same rules must still fire on the tracked copies, or + // this would pass just as happily with the engines switched off. + const trackedSources = new Set(results.map((finding) => finding.source)); + expect(trackedSources).toContain("ast-grep"); + if (valeAvailable) expect(trackedSources).toContain("vale"); + }); + + it("checks the ignored directory when it is named explicitly", async () => { + // An explicitly named path is an instruction, not an accident. The ignore + // rule belongs to the walk we chose, not to the one the user asked for. + const results = await checkedFiles(project, ["ignored"]); + + const expected = new Set( + valeAvailable ? ["ast-grep", "vale"] : ["ast-grep"] + ); + expect(sourcesUnderIgnored(results)).toEqual(expected); + }); + }); + + describe("outside a git repository", () => { + it("falls back to the existing walk and checks everything", async () => { + // No `git init`, so nothing is ignored and nothing may be skipped. This + // is the failure mode the fix must not have: a `git` that errors out + // silently pruning the project down to nothing. + const results = await checkedFiles(project); + + const expected = new Set( + valeAvailable ? ["ast-grep", "vale"] : ["ast-grep"] + ); + expect(sourcesUnderIgnored(results)).toEqual(expected); + }); + }); +}); + +describe("listGitIgnoredEntries", () => { + let project: string; + + beforeEach(async () => { + project = await mkdtemp(join(tmpdir(), "taskless-lsfiles-")); + }); + + afterEach(async () => { + await rm(project, { recursive: true, force: true }); + }); + + it("collapses a wholly-ignored directory to one entry", async () => { + await writeFile(join(project, ".gitignore"), "ignored/\nnoise.log\n"); + await mkdir(join(project, "ignored/deep"), { recursive: true }); + await writeFile(join(project, "ignored/deep/a.md"), "a\n"); + await writeFile(join(project, "ignored/deep/b.md"), "b\n"); + await writeFile(join(project, "noise.log"), "noise\n"); + await execFileAsync("git", ["init", "--quiet"], { cwd: project }); + + const entries = await listGitIgnoredEntries(project); + + // The reason this is the complement of `--cached --others` and not the set + // itself: the directory is one entry however many files it holds. + expect(entries).toContain("ignored/"); + expect(entries).toContain("noise.log"); + expect(entries).not.toContain("ignored/deep/a.md"); + }); + + it("ignores nothing when the walk root is itself ignored", async () => { + // Standing inside a gitignored directory is as explicit as naming it. git + // answers `./` there — "everything here" — and honouring that would return + // an empty check with nothing saying why. + await writeFile(join(project, ".gitignore"), "build/\n"); + await mkdir(join(project, "build"), { recursive: true }); + await writeFile(join(project, "build/a.md"), "a\n"); + await execFileAsync("git", ["init", "--quiet"], { cwd: project }); + + expect(await listGitIgnoredEntries(join(project, "build"))).toEqual([]); + }); + + it("returns nothing outside a git repository", async () => { + await writeFile(join(project, "noise.log"), "noise\n"); + + expect(await listGitIgnoredEntries(project)).toEqual([]); + }); +}); + +describe("gitIgnoredExclusionGlobs", () => { + it("turns a directory entry into a subtree pattern and leaves files alone", () => { + expect(gitIgnoredExclusionGlobs(["worktrees/", "agents.lock"])).toEqual([ + "worktrees/**", + "agents.lock", + ]); + }); + + it("drops entries that would not survive the glob alternation", () => { + // A comma would split into two patterns inside `!{…}` and both halves + // would be wrong; the rest would stop being literal paths. + expect( + gitIgnoredExclusionGlobs(["a,b.md", "star*.md", "brace{x}.md", "fine.md"]) + ).toEqual(["fine.md"]); + }); +}); + +describe("isGitIgnoredPath", () => { + const entries = ["worktrees/", "a,b.md"]; + + it("matches a file under an ignored directory", () => { + expect(isGitIgnoredPath("worktrees/probe/README.md", entries)).toBe(true); + }); + + it("matches an ignored file the glob had to drop", () => { + // The skip notice has no alternation to survive, so a path excluded from + // the glob is still known to be ignored here. + expect(isGitIgnoredPath("a,b.md", entries)).toBe(true); + }); + + it("does not match a tracked path that merely shares a prefix", () => { + expect(isGitIgnoredPath("worktrees-notes.md", entries)).toBe(false); + }); +});