diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 50d8172..e3909c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,6 +33,9 @@ jobs: - name: Install dependencies run: uv sync --locked + # Runs everything, including the randomized gitignore-parity suite: Actions sets + # `CI`, which is what that suite gates itself on. Deliberately no marker flag here — + # a flag is a second place to keep in sync, and forgetting it fails silently green. - name: Run tests run: uv run pytest diff --git a/.gitignore b/.gitignore index 43ae0e2..117ea32 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ __pycache__/ *.py[cod] +.house-lint-cache/ diff --git a/CLAUDE.md b/CLAUDE.md index cc93368..a61a7d9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,10 +49,28 @@ signature: `(source, options, *, limit=None) -> list[CandidateFinding]`. - `HSL900` (suppression-pragma validation) can never be disabled or suppressed — it governs how every other rule's findings can be silenced (`ignore`, `ignore-next`, `ignore-file` pragmas). -- File discovery does **not** shell out to git or read nested `.gitignore` files — only the root - `.gitignore` plus configured excludes. `--no-gitignore` disables just that root file. +- File discovery does **not** shell out to git. It reads the root `.gitignore` plus every nested + `.gitignore` between the root and each file, reimplementing git's precedence on `pathspec`. + `--no-gitignore` disables that at every level. Because it is a reimplementation, changes to + `discovery.py`'s pattern handling belong in `tests/integration/test_gitignore_parity.py`, which + differentially checks discovery against real `git check-ignore` — adding a case there costs one + `Scenario` entry and needs no expected-value literal. Two divergences are known, both from + `pathspec` deciding directory-only patterns from pattern text rather than from a real `is_dir`: + one over-lints, one **under**-lints (hides findings). Do not assume the old "always errs toward + over-linting" guarantee — it was false and has been removed. See `docs/configuration.md` and + `design/research/2026-08-20-gitignore-style-exclusion-inclusion/`. +- An ignored directory is pruned rather than enumerated, so `files_skipped` counts one skip per + pruned directory, not one per file inside it. - Default scan roots are `src`, `tests`, `scripts`, `tools`, `examples`, configurable via `[tool.house-lint] include`. +- A scanned file is read **exactly once** per scan, by `SourceFile.load()`. The cache key is + derived from that same buffer (`SourceFile.content_bytes` → `hash_source_content`), which is + what stops an entry from ever describing content that was not scanned under that key. Adding a + second read of a scanned path reopens that window; + `test_each_scanned_file_is_read_exactly_once` is what catches it. +- The result cache is namespaced by `-`, so editing + rule code invalidates it without a version bump. house-lint writes a self-ignoring `.gitignore` + into its own default cache base only — never into a user-supplied `--cache-dir`. ## Conventions diff --git a/README.md b/README.md index 2d83844..6533a98 100644 --- a/README.md +++ b/README.md @@ -81,6 +81,21 @@ house-lint check --select HSL002,HSL103 --ignore HSL103 Each `--select` or `--ignore` occurrence accepts one comma-separated list. Selection is strict: unknown, duplicate, empty, and `HSL900` IDs are usage errors. +To add or remove rules without replacing the rest of your configured selection, use `extend-select`/`extend-ignore` (in `[tool.house-lint]` or as `--extend-select`/`--extend-ignore`) instead of `select`/`ignore`: + +```bash +house-lint check --extend-select HSL101 +``` + +`extend-select`/`extend-ignore` layer additively on top of the base selection (configured `select`/`ignore`, or a CLI `--select` override) regardless of where that base came from. A final CLI `--ignore` still always wins. + +To silence a rule only for files matching a glob, without touching the selection everywhere else, use `[tool.house-lint.per-file-ignores]`: + +```toml +[tool.house-lint.per-file-ignores] +"tests/**" = ["HSL002"] +``` + Read [configuration](docs/configuration.md) for discovery, precedence, validation, excludes, and token-family options. ## Paths, roots, and Git ignores @@ -93,7 +108,17 @@ house-lint check src/service.py tests Explicit paths are strict. Missing, out-of-root, and non-Python file arguments are errors; ignored or excluded explicit Python files are counted as skipped. `--root` fixes the project boundary and only considers `/pyproject.toml`. Without `--root`, discovery starts at the current directory. `--config` selects an exact configuration file; without `--root`, its parent becomes the root. -The linter loads only the selected root's `.gitignore`, plus built-in and configured excludes. It does not search nested `.gitignore` files or shell out to Git. Use `--no-gitignore` to disable only the root `.gitignore`. +The linter loads the selected root's `.gitignore` plus every nested `.gitignore` between the root and each discovered file, combined with git's own precedence (a closer `.gitignore` can override a farther one, including via negation), plus built-in and configured excludes. It does not shell out to Git. Use `--no-gitignore` to disable `.gitignore` handling at every level. + +## Caching + +`check` caches each file's result under `/.house-lint-cache/-/`, keyed by the file's content, its effective rule set for that file, and the running Python version — `ast.parse` accepts different grammar across the versions house-lint supports, so a cache shared between venvs must not replay one interpreter's parse result under another. A cache hit skips tokenization, parsing, and rule execution entirely for that file. Upgrading house-lint — or editing its rule code in a working checkout — starts from an empty cache automatically, because both the version and a fingerprint of house-lint's own sources are part of the cache path. Superseded directories are pruned rather than left to accumulate. + +`--no-cache` disables reading from the cache but still writes to it, keeping it warm for the next run. `--cache-dir` overrides where the cache lives (still namespaced underneath the path you give it). + +house-lint adds a self-ignoring `.gitignore` to its own default `.house-lint-cache/` directory so it stays invisible to `git status`. It never writes one into a directory you name with `--cache-dir` — that directory is yours. + +A cache failure never fails a scan, but it is never silent either: an unwritable directory, a full disk or a corrupted entry prints one `warning:` line to stderr the first time it happens in a run. Only that first failure is printed by default. A broken cache directory fails once per scanned file, so printing every one would bury the single fact worth reporting under thousands of near-identical lines; the remainder are shown under `--debug`. Findings and the exit code are unaffected. ## Suppressions diff --git a/design/research/2026-08-20-gitignore-style-exclusion-inclusion/research.md b/design/research/2026-08-20-gitignore-style-exclusion-inclusion/research.md new file mode 100644 index 0000000..f83c4e2 --- /dev/null +++ b/design/research/2026-08-20-gitignore-style-exclusion-inclusion/research.md @@ -0,0 +1,241 @@ +--- +topic: "gitignore-style exclusion/inclusion in linters and file-walking tools" +date: 2026-08-20 +status: Draft +--- + +# Prior Art: gitignore-style exclusion/inclusion + +## The Problem + +Any tool that walks a source tree has to decide which files to skip, and the de facto +contract is "behave like `.gitignore`." That contract is much harder than it looks: patterns +are relative to the `.gitignore` file that declares them, deeper files override shallower +ones, the last matching line wins within a tier, trailing slashes make a pattern +directory-only, and — the load-bearing rule — **a negation can never re-include a file whose +parent directory was excluded**, because git never lists an excluded directory in the first +place. + +That last rule is not a quirk of the pattern language. It is a consequence of *how* git +matches: directory-by-directory, during traversal, with the walker supplying ground truth for +"is this entry a directory." A matcher that flattens every pattern into one root-anchored set +and matches full path strings has no way to express "this directory was never entered," and so +cannot reproduce the rule. Tools discover this the hard way, repeatedly. + +## How We Do It Today + +house-lint is a **hybrid**: it prunes directories during `os.walk` (traversal model) but +decides membership by rewriting every nested pattern to be root-anchored and matching the full +relative path against one combined `pathspec.GitIgnoreSpec` (flattened model). Everything +except glob-to-regex compilation is hand-rolled in `_FileSelector`. `docs/configuration.md` +already documents one deliberate divergence (`!sub/` re-includes everything beneath it) and +attributes it precisely: `pathspec` "compiles every pattern as a prefix search and so cannot +distinguish 'this pattern matched this entry' from 'it matched an ancestor'." + +The two halves of the hybrid are where the bugs live — walk-time pruning and flattened +matching have to agree about ancestor exclusion, and `_combined_gitignore_spec`'s +short-circuit to `IGNORE_EVERYTHING` is the seam holding them together. + +## Patterns Found + +### Pattern 1: Per-directory matcher stack, evaluated during traversal + +**Used by**: git itself (`dir.c`, `git ls-files --exclude-per-directory`), ripgrep's `ignore` +crate, fd, ruff (transitively), `gitignorefile` (Python — claimed, unverified). + +**How it works**: The walker descends one level at a time. At each directory it loads (or +reuses a cached) matcher built from *that directory's own* `.gitignore` plus inherited parent +matchers, and asks "is this specific entry — file or directory — ignored, whitelisted, or +unmatched?" A directory that is excluded is never entered, so its contents are never asked +about. That is exactly why the man page says re-inclusion under an excluded parent is +impossible. The `ignore` crate's `matched_stripped` walks candidate matches **in reverse** +(last pattern wins) and accepts a directory-only glob as the deciding match only when the +caller has passed a real `is_dir` flag derived from `readdir`/`stat` — never inferred from +pattern text. + +**Strengths**: The only model that reproduces git's pruning behaviour and the non-reinclusion +rule, because traversal *is* the source of truth. Supports per-level precedence tiers +naturally. Matchers cache and reuse cleanly as the walker moves between siblings. + +**Weaknesses**: Couples matching to the walk — can't be bolted onto a flat file list after the +fact. More moving parts: matcher construction, caching, stack push/pop per directory boundary. +And it is not a correctness guarantee on its own (see ruff #17392 below). + +**Example**: https://github.com/BurntSushi/ripgrep/blob/master/crates/ignore/src/gitignore.rs + +### Pattern 2: Full-path matching against a flattened, root-anchored pattern set + +**Used by**: `pathspec`'s `GitIgnoreSpec`, house-lint today, and the common "read `.gitignore`, +build one `PathSpec`, filter a glob list" recipe. + +**How it works**: Every `.gitignore` in the tree is read once, its patterns rewritten to be +root-anchored, and all of them compiled into one ordered spec. Each candidate's full relative +path is matched against that spec with last-match-wins applied globally. + +**Strengths**: Simple, single-pass, easy to test in isolation. No coupling to walk order. Works +on an already-known flat file list. + +**Weaknesses**: Structurally cannot represent "this directory was pruned, so nothing under it +should be asked about." A negation matching a path *under* an excluded ancestor still flips the +outcome. Directory-vs-file classification is done by inspecting pattern text and priority +rather than asking the filesystem — the root cause `pathspec` #81 identifies. Also prone to +independent anchoring bugs (#93: `foo**/bar` matching `foobar`, bracket expressions). + +**Example**: https://github.com/cpburnz/python-pathspec/issues/81 + +### Pattern 3: Shell out to real git + +**Used by**: `git-check-ignore` (PyPI wrapper); available to any tool willing to take the +dependency. + +**How it works**: Batch candidate paths through `git check-ignore --stdin -v`, or take git's +own file list via `git ls-files --others --exclude-standard`. + +**Strengths**: Perfect fidelity by definition — it *is* git, including edge cases nobody has +found yet. + +**Weaknesses**: Requires `git` on `PATH` and the tree to be inside a repository; needs a +fallback outside one. Subprocess overhead versus in-process matching. Submodule and worktree +behaviour needs separate handling [no source found for submodule specifics]. + +**Example**: https://git-scm.com/docs/git-ls-files + +### Pattern 4: Tri-state match result carrying the winning glob + +**Used by**: the `ignore` crate (ripgrep, fd, ruff). + +**How it works**: Matching returns `None`, `Ignore(T)`, or `Whitelist(T)`, where `T` identifies +*which* glob won. Because the result carries the winning pattern, the caller can interrogate it +— e.g. `is_only_dir()` — before accepting the match, instead of the matcher collapsing +everything to a boolean. + +**Strengths**: Lets caller-side rules layer cleanly without re-deriving pattern metadata. +Composes with override/whitelist matchers via the same type. + +**Weaknesses**: More API surface; easy to accidentally collapse `Whitelist` into `Ignore`. + +**Example**: https://docs.rs/ignore/latest/ignore/enum.Match.html + +### Pattern 5: Bespoke, intentionally non-gitignore-compatible semantics + +**Used by**: ESLint flat config's `ignores`, `.dockerignore`, GitHub CODEOWNERS. + +**How it works**: Define a narrower matching contract "inspired by" gitignore but not +equivalent, and document the difference. ESLint's flat-config patterns anchor to the config +file's directory rather than matching at any depth. Real `.gitignore` parity, when wanted, is +delegated to a separate adapter (`eslint-config-flat-gitignore`, built on `node-ignore`). + +**Strengths**: Sidesteps the entire class of fidelity bugs by not claiming fidelity. Matching +engine only needs to be internally consistent and documented. + +**Weaknesses**: Surprises users who assume gitignore syntax transfers. Needs an adapter and a +second dependency for anyone who wants real parity. + +**Example**: https://eslint.org/docs/latest/use/configure/ignore + +## Anti-Patterns + +- **Classifying directory-vs-file patterns from pattern text instead of asking the + filesystem.** `pathspec` #81 traces its divergence to an internal priority scheme that ranks + "directory patterns" below "file patterns" purely from parsing the glob string, rather than + carrying an `is_only_dir()` flag and letting the caller — which knows from `readdir` — decide. + https://github.com/cpburnz/python-pathspec/issues/81 +- **Flattening all `.gitignore` files into one root-anchored set.** Structurally incapable of + representing the non-reinclusion rule, which is why that exact bug has been independently + rediscovered in at least three unrelated codebases (`pathspec` #81, `node-ignore`'s historical + `fstream-ignore` fix, `graphify` #882). https://git-scm.com/docs/gitignore +- **Assuming "uses gitignore syntax" means git parity.** For most tools it means only "globs, + one per line, `#` comments, maybe `!`" — not implicit depth-anchoring, per-directory + precedence stacking, or the non-reinclusion rule. + https://nesbitt.io/2026/02/12/the-many-flavors-of-ignore-files.html +- **Trusting the right architecture to be bug-free.** ruff, on the `ignore` crate lineage, still + has an open bug where `.gitignore` fidelity differs between `ruff check src/foo` and + `ruff check src/`. Differential testing against real `git check-ignore` remains necessary + regardless of architecture. https://github.com/astral-sh/ruff/issues/17392 + +## Relevance to Us + +Three findings land directly on house-lint's situation. + +**1. The documented divergence is not a house-lint quirk — it is the defining weakness of +Pattern 2.** `docs/configuration.md`'s explanation ("compiles every pattern as a prefix search +and so cannot distinguish 'this pattern matched this entry' from 'it matched an ancestor'") is +almost verbatim the root cause `pathspec` #81 identifies. The newly-found under-linting case is +the same family. Continuing to patch individual manifestations is treating symptoms of an +architectural choice. + +**2. house-lint is already halfway to Pattern 1, and the bugs live in the seam.** It already +prunes during `os.walk`, already passes an explicit `is_dir` into `_ignored` (which the survey +identifies as the correct discipline), and already has ancestor-exclusion short-circuiting. What +it does not have is *per-level* matching — it flattens, then compensates for the flattening with +`IGNORE_EVERYTHING`. That compensation is where the residual bug almost certainly lives. Moving +to per-level matchers would keep `pathspec` as the glob→regex compiler while removing the +rewrite-and-flatten step (`_prefix_pattern`, `_collapse_double_star_run`, +`_normalize_contents_glob`) that exists only to serve flattening. + +**3. ruff #17392 is a near-exact analogue of the `..` bug just fixed** — gitignore fidelity +differing between an explicit subpath and a directory root. Independent confirmation that the +explicit-path-vs-walk-root seam is a recurring hazard, and that the parity/fuzz suites are the +right investment regardless of which architecture wins. + +Two constraints narrow the options. `CLAUDE.md` records that file discovery deliberately does +**not** shell out to git, which rules out Pattern 3 without an explicit reversal of that +decision — and Pattern 3 would also break scanning outside a repository. And house-lint's whole +parity apparatus exists to claim git fidelity, so Pattern 5 (drop the claim) would mean deleting +the suites that make house-lint trustworthy here. + +## Recommendation + +**Do not attempt the Pattern 1 refactor inside this PR.** It is the right end state, but it +rewrites the core of `discovery.py`, and this branch is already 30+ commits deep with two +landed fixes and a red test. + +Sequence instead: + +1. **Now, in this PR**: `xfail` the residual with the minimised repro, and correct + `docs/configuration.md`'s guarantee. That doc currently claims the divergence "always errs + toward linting a file git would ignore, never toward silently skipping one." That claim is + false as of this finding, and it is the *only* stated justification for accepting Pattern 2. + Leaving it standing is the real defect. Also regenerate the divergence-rate table (the fuzz + numbers moved). +2. **File an issue** for the Pattern 1 migration, attaching this survey — specifically the + `matched_stripped` mechanism (reverse iteration, `!glob.is_only_dir() || is_dir`) as the + reference design, and the `Match` tri-state as the shape to port. +3. **Before committing to that migration**, spend an hour on `gitignorefile` — the survey + flags it as claiming per-directory traversal but could not verify its internals. If it is + genuinely Pattern 1, it may be a dependency swap rather than a rewrite. Verify by reading + its source and running house-lint's existing parity suite against it; the suites make that a + cheap experiment. + +The honest framing for the PR: house-lint's ignore engine is a Pattern 2 implementation with +Pattern 1 aspirations, the direction guarantee that made that acceptable has been falsified, and +the fix is architectural rather than another patch. + +## Sources + +Note: these URLs were not live-verified by me; they come from the research pass. + +### Reference implementations +- https://github.com/BurntSushi/ripgrep/blob/master/crates/ignore/src/gitignore.rs — the `matched_stripped` reverse-iteration + `is_only_dir` mechanism +- https://github.com/BurntSushi/ripgrep/tree/master/crates/ignore — crate extracted from ripgrep to isolate this complexity +- https://docs.rs/ignore/latest/ignore/enum.Match.html — tri-state match result +- https://docs.rs/ignore/latest/ignore/struct.WalkBuilder.html — precedence tiering by ignore-file type, then depth +- https://github.com/kaelzhang/node-ignore — JS implementation that had to fix the same non-reinclusion bug +- https://pypi.org/project/gitignorefile — Python, claims per-directory traversal (unverified) + +### Bug reports & experience +- https://github.com/cpburnz/python-pathspec/issues/81 — `GitIgnoreSpec` vs git, directory-pattern priority root cause +- https://github.com/cpburnz/python-pathspec/issues/93 — further pathspec divergences (`foo**/bar`, brackets) +- https://github.com/astral-sh/ruff/issues/17392 — gitignore fidelity differs by walk-root spelling +- https://github.com/safishamsi/graphify/issues/882 — third independent tool, same non-reinclusion bug +- https://github.com/BurntSushi/ripgrep/discussions/2824 — internal-slash anchoring rule + +### Documentation & standards +- https://git-scm.com/docs/gitignore — the non-reinclusion rule and its performance rationale +- https://git-scm.com/docs/git-ls-files — `--exclude-per-directory`, git's own traversal model +- https://pypi.org/project/pathspec/ — maintainers acknowledge git's semantics are edge-case-heavy +- https://eslint.org/docs/latest/use/configure/ignore — deliberate non-parity +- https://github.com/antfu/eslint-config-flat-gitignore — delegation adapter +- https://prettier.io/docs/ignore — simplified subset (root `.gitignore` only) +- https://nesbitt.io/2026/02/12/the-many-flavors-of-ignore-files.html — survey of ignore-file dialects +- https://waylonwalker.com/gitignore-python/ — the common naive Python recipe diff --git a/docs/configuration.md b/docs/configuration.md index 1111661..4d9326c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -8,9 +8,23 @@ include = ["src", "tests", "scripts", "tools", "examples"] exclude = [] select = ["HSL001", "HSL002", "HSL003", "HSL004"] ignore = [] +extend-select = [] +extend-ignore = [] ``` -`include` contains literal root-relative files or directories, not globs. An empty array intentionally selects no roots for a full scan. `exclude` uses root-relative Git-ignore-style patterns. Unknown keys, absolute paths, parent traversal, invalid patterns, duplicate IDs, and `HSL900` in `select` or `ignore` are configuration errors. +`include` contains literal root-relative files or directories, not globs. An empty array intentionally selects no roots for a full scan. `exclude` uses root-relative Git-ignore-style patterns. Unknown keys, absolute paths, parent traversal, invalid patterns, duplicate IDs, and `HSL900` in `select`, `ignore`, `extend-select`, `extend-ignore`, or `per-file-ignores` are configuration errors. Unlike the rest of this schema, `extend-select`/`extend-ignore`/`per-file-ignores` are hyphenated by design, matching Ruff's spelling for the same concepts. + +## Per-file rule overrides + +`[tool.house-lint.per-file-ignores]` maps root-relative Git-ignore-style glob patterns to rule IDs to drop for matching files, without changing the global selection for everything else: + +```toml +[tool.house-lint.per-file-ignores] +"tests/**" = ["HSL002"] +"legacy/*.py" = ["HSL001", "HSL003"] +``` + +Applied after the base selection and `extend-select`/`extend-ignore` resolve, per file: a rule dropped by `per-file-ignores` for a matching file is not detected for that file at all, so a `# house-lint: ignore[...]` pragma naming it there is flagged the same way as suppressing an already-disabled rule. `HSL900` can never appear in a `per-file-ignores` value. ## Discovery and precedence @@ -18,9 +32,52 @@ ignore = [] 2. `--config` selects an exact configuration. Without `--root`, its parent is the root; with `--root`, it must be inside the root. 3. With `--root` and no `--config`, only `/pyproject.toml` is considered. 4. Without either option, the command searches upward from the current directory for the nearest `pyproject.toml` containing `[tool.house-lint]`. If none exists, it uses the nearest ancestor containing `.git` or any `pyproject.toml`; otherwise it uses the current directory. -5. CLI `--select` replaces configured selection, then CLI `--ignore` subtracts IDs. `HSL900` is always added. +5. The base selection is configured `select` minus configured `ignore`, or a CLI `--select` wholesale override when given. +6. `extend-select`/`extend-ignore` (config and CLI, unioned together) layer additively on top of that base, regardless of whether the base came from config or `--select`. `extend-ignore` removes rules from the whole base, not just from `extend-select` — `select = ["HSL001"]` with `extend-ignore = ["HSL001"]` drops HSL001 entirely, it isn't limited to canceling out `extend-select` additions. +7. CLI `--ignore` is applied last and always wins over everything above. `HSL900` is always added. + +The root `.gitignore` and every nested `.gitignore` between the root and each discovered file are loaded and combined with git's own precedence — a closer `.gitignore` can override a farther one, including via negation (`!pattern`). Built-in excludes are `.git/`, `.venv/`, `.nox/`, `__pycache__/`, `site-packages/`, and `node_modules/`; configured excludes are added afterwards. `--no-gitignore` disables `.gitignore` handling at every level. + +An ignored directory is skipped without being enumerated, which is what keeps a large `.venv/` or `node_modules/` cheap to exclude. The reported `files skipped` count follows from that: one pruned directory counts as one skip, however many files it contains. + +Exclusion attaches to the directory, so a negation cannot re-include anything beneath one that is already excluded — `exclude = ["src/generated/", "!src/generated/foo.py"]` leaves `foo.py` excluded, matching git. This holds for built-in excludes, configured `exclude`, and `.gitignore` alike, and it holds however the file is reached: naming `src/generated/foo.py` on the command line skips it just as a full scan does. + +`per-file-ignores` patterns are matched against each file's resolved location under the root, not the spelling used to reach it. Naming `src/../tests/a.py`, or a path reached through a symlinked directory, matches the same patterns as `tests/a.py` would. Findings still report the path as typed. + +house-lint reimplements git's ignore rules on top of [`pathspec`](https://pypi.org/project/pathspec/) rather than shelling out to git. Two test suites check that reimplementation against real `git check-ignore`: `tests/integration/test_gitignore_parity.py` runs a curated table of pattern shapes, and `tests/integration/test_gitignore_fuzz.py` generates random combinations. The second runs on every CI run and skips locally unless `CI` is set, since it makes thousands of real `git check-ignore` calls; run it by hand with `CI=1 uv run pytest -s tests/integration/test_gitignore_fuzz.py` (`-s` prints the rates below). + +Two divergences are known, both in the same family: `pathspec` decides directory-only patterns by inspecting the pattern text rather than by being told whether the candidate is a directory, so it cannot distinguish "this pattern matched this entry" from "it matched an ancestor". Closing either would mean owning the pattern-to-regex compiler rather than delegating whole-path matching to `pathspec`. + +- **Over-linting.** A negated directory-only pattern (`!sub/`) re-includes everything beneath it, whereas git re-includes only the `sub` entry itself and re-evaluates each descendant against the remaining patterns. It changes the outcome only when such a negation sits under a broader ignore that also covers the descendants. +- **Under-linting.** A directory-only negation may fail to re-include a directory git descends into. `GitIgnoreSpec.from_lines(("**", "!**/")).match_file("src")` returns `True`, while git reports `.gitignore:2:!**/` re-including `src` and walks it. house-lint asks `pathspec` exactly that question when deciding whether to prune, so it prunes a subtree git walks and every file underneath vanishes from the scan. Passing `"src/"` does not change `pathspec`'s answer. + +An earlier version of this section claimed the divergence "always errs toward linting a file git would ignore, never toward silently skipping one, so it cannot hide a finding." **That is not true**, and it was the whole justification for accepting the current design. The second case above hides findings exactly the way that sentence promised it could not — it was found once the fuzz suite's corner pool learned to compose repeated `**` segments, which it had never done before. The direction guarantee is now a measured property with a recorded ceiling rather than an invariant: `test_gitignore_fuzz.py` still fails on any under-linting divergence *outside* the named class, and separately caps how much the named class may account for, so it cannot quietly widen. + +Over-linting is visible and silenced with one `exclude` entry; under-linting is indistinguishable from a clean run. That asymmetry is why the under-linting case is tracked as a defect to remove rather than a trade to keep. The rates below come from three declared pattern distributions: + +| `.gitignore` content | divergence rate | skips a file git lints | +|---|---|---| +| plain names and globs, no negation | 0.00% (0/1500) | never | +| the same, 5% of patterns negated | 0.33% (5/1500) | never | +| corner-hunting pool, 30% negated | 2.47% (37/1500) | 1 (known directory-negation defect) | + +A rate is meaningless without the distribution that produced it, which is why all three are declared in the test rather than summarised as one number. Regenerate them there and update this table in the same change. + +`design/research/2026-08-20-gitignore-style-exclusion-inclusion/` surveys how git, ripgrep's `ignore` crate, fd, and the Node ecosystem handle this. The short version: matching the full path against one flattened, root-anchored pattern set is structurally unable to express "this directory was never entered," which is the rule git's own documentation calls out ("It is not possible to re-include a file if a parent directory of that file is excluded"). The same defect has been independently rediscovered in `pathspec` ([#81](https://github.com/cpburnz/python-pathspec/issues/81)), `node-ignore`, and other tools. The architectural fix is a per-directory matcher stack evaluated during traversal, with the walker supplying `is_dir` — not another patch. + +## Caching + +There is no TOML key for caching — it's controlled entirely by CLI flags, since it's a run-to-run performance concern rather than a project convention. + +Each file's result is cached under `/.house-lint-cache/-/`, flat and keyed by two hashes: the file's raw content, and the file's *effective* rule set for that run (`select`/`ignore`/`extend-select`/`extend-ignore`/`per-file-ignores` and CLI overrides already resolved, plus all three `HSL101`/`HSL102`/`HSL103` option tables, whether or not each of those rules is currently enabled — simpler than tracking which options are actually load-bearing, at the cost of some extra cache invalidation when an unused rule's options change). The running interpreter's major/minor version is always folded in as well, since `ast.parse` accepts different grammar across the Python versions house-lint supports (`type Alias = int` is a `SyntaxError` before 3.12) — without it, a cache shared between venvs could replay a stale `SyntaxError`, or a stale success, that the interpreter actually running the scan would not produce. Switching interpreters is therefore a full cache miss by design. The file's own name is folded in too whenever an enabled `HSL101` token family scopes to `"filenames"`, since that's the one detector whose output depends on the filename rather than purely the content. house-lint is a single-file analyzer with no cross-file dependencies, so this flat scheme is sufficient — there is no dependency graph to invalidate. A cache hit skips tokenization, parsing, and rule execution for that file entirely. + +The directory name carries both house-lint's version and a hash of its own Python sources. The version alone would not be enough: it only moves when a release is cut, so editing a detector in a working checkout and re-running would replay the previous detector's results for every unchanged file. The source fingerprint is content-based, so a released install keeps exactly one cache directory across machines and fresh clones. + +Superseded directories house-lint created are pruned so they do not accumulate — but only by a run that actually writes a cache entry. A scan where every file is a cache hit deletes nothing, which keeps the sweep from removing a namespace that a concurrent house-lint of a different version is still writing to. + +house-lint writes a self-ignoring `.gitignore` into its own default `.house-lint-cache/` base so the cache stays invisible to `git status`. It never writes one into a `--cache-dir` you supply, since that directory may hold unrelated data — or be a project root, where a wildcard ignore would hide the whole project. -Only the root `.gitignore` is loaded. Built-in excludes are `.git/`, `.venv/`, `.nox/`, `__pycache__/`, `site-packages/`, and `node_modules/`; configured excludes are added afterwards. `--no-gitignore` disables only the root `.gitignore`. +`--no-cache` disables reading from the cache but still writes to it, keeping it warm for the next run — the same semantics as Ruff's `--no-cache`. `--cache-dir ` overrides the base directory (the namespace segment is still appended underneath it). ## Rule options diff --git a/pyproject.toml b/pyproject.toml index c6b4713..a4d91b3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -38,6 +38,8 @@ build-backend = "uv_build" [tool.pytest.ini_options] testpaths = ["tests"] +# No marker filtering: `pytest` runs the same set everywhere. The one suite too slow for a local +# run gates itself on the `CI` environment variable instead — see test_gitignore_fuzz.py. [tool.ruff] line-length = 100 diff --git a/src/house_lint/cache.py b/src/house_lint/cache.py new file mode 100644 index 0000000..b6c3ebd --- /dev/null +++ b/src/house_lint/cache.py @@ -0,0 +1,597 @@ +"""Flat, version-namespaced per-file result cache. + +House-lint is a single-file analyzer with no cross-file dependencies, so a flat cache keyed +by (file content hash, effective config hash) is semantically correct — unlike a dependency- +graph cache (e.g. mypy's `.mypy_cache`), there is no invalidation-graph to track. The cache +directory is namespaced by house-lint's own version, so an upgrade invalidates stale entries +automatically without an explicit migration step. + +Cache entries are addressed purely by content and config hashes, not by file path — two files +with identical content and an identical effective rule set produce the same entry. Cached +findings and errors are therefore stored without their `path` field; `read_cached_result` takes +the caller-supplied `relative_path` of the file actually being scanned and re-attaches it to +each reconstructed finding/error. +""" + +import hashlib +import json +import os +import shutil +import sys +from contextlib import suppress +from dataclasses import asdict, dataclass +from enum import Enum, auto +from functools import lru_cache +from pathlib import Path +from typing import Any, cast + +from house_lint import __version__ +from house_lint.config import HSL101Options, HSL102Options, HSL103Options +from house_lint.results import Finding, LintError +from house_lint.source import MAX_SOURCE_BYTES + +CACHE_DIRNAME = ".house-lint-cache" +_VERSION_DIR_MARKER = ".house-lint-version" + + +class CacheReporter: + """Where every best-effort cache failure in a single scan is reported. + + A cache failure must never fail the scan — but "never fail" and "never signal" are different + guarantees, and this module used to conflate them: every failure branch printed only under + `--debug`. The runs this tool is built for (CI, pre-commit) never pass `--debug`, so an + unwritable cache directory made every scan silently pay the full re-analysis cost with + nothing to explain why. + + The first failure of a run is therefore always visible. The rest are `--debug`-only: a broken + cache directory fails once per scanned file, and printing all of them by default would bury + the single fact worth reporting under thousands of near-identical lines. + + Held per scan rather than in module state so concurrent scans, and tests, cannot see each + other's "have I warned yet" flag. Routing every failure site through one object is also what + keeps the guarantee enforceable: adding a silent `except: pass` to this module now means + visibly bypassing this class rather than merely forgetting a `debug` check. + """ + + def __init__(self, *, debug: bool = False) -> None: + self.debug = debug + self._reported = False + + def failure(self, message: str) -> None: + """Report one failed cache operation. Loud the first time, `--debug`-only after that.""" + if not self._reported: + self._reported = True + print(f"warning: {message}", file=sys.stderr) + return + if self.debug: + print(f"debug: {message}", file=sys.stderr) + + +def default_cache_base(root: Path) -> Path: + """Default cache base directory: `/.house-lint-cache/` (before version-namespacing).""" + return root / CACHE_DIRNAME + + +def default_cache_base_is_safe(cache_dir: Path) -> bool: + """Whether house-lint may create and write its own default cache at `cache_dir`. + + The default cache lives inside the scanned project, so its path is controlled by whoever + wrote that project: a repository can ship `.house-lint-cache` as a symlink pointing anywhere, + and `prepare_cache_dir`'s `mkdir(parents=True, exist_ok=True)` follows it. house-lint would + then write its version marker, its cache entries, and a wildcard `.gitignore` into the + directory the link names — outside the checkout, at a location the repository chose. A plain + `house-lint check` on a freshly cloned repository must never do that, so a symlinked default + cache path disables caching for the run instead. + + Both levels are checked, because both are predictable to whoever writes the repository. The + version namespace is derived from house-lint's version and source fingerprint + (`versioned_cache_dir`), so a project can ship a perfectly real `.house-lint-cache/` whose + `-` *child* is the symlink and reach the same outcome. Checking only + the base leaves that second door open. + + Only the default cache is checked. A `--cache-dir` names a directory the user picked + deliberately, and house-lint neither self-ignores nor second-guesses it. + """ + return not cache_dir.is_symlink() and not cache_dir.parent.is_symlink() + + +@lru_cache(maxsize=1) +def code_identity() -> str: + """Fingerprint house-lint's own Python sources, for use in the cache namespace. + + `__version__` alone is not enough to invalidate results across a code change: it only moves + when a release is cut, so editing a detector in a working checkout and re-running replays + the previous detector's findings for every file whose content and config are unchanged. For + a linter, silently serving a stale "clean" result after a rule fix defeats the point. It + also matters across checkouts: two working copies at the same version sharing one + `--cache-dir` would otherwise trade results. + + Hashes file contents rather than mtimes so the value is stable across machines and fresh + clones — a released install therefore keeps exactly one cache directory. Read once per + process; on any read failure (a zipped or otherwise non-file distribution) this returns the + constant `"unknown"`, so the namespace falls back to `-unknown` and invalidation + reverts to tracking the version alone rather than failing the scan. + """ + package_root = Path(__file__).parent + digest = hashlib.sha256() + try: + for source in sorted(package_root.rglob("*.py")): + digest.update(source.relative_to(package_root).as_posix().encode("utf-8")) + digest.update(source.read_bytes()) + except OSError: + return "unknown" + return digest.hexdigest()[:16] + + +def versioned_cache_dir(base: Path) -> Path: + """Namespace a cache base directory by house-lint's version and source fingerprint. + + Applies uniformly to the default base and to a user-supplied `--cache-dir` override — the + override changes *where* the cache lives, not whether it's still safe across upgrades. Stale + namespaces do not accumulate: `_prune_stale_version_dirs` removes the siblings house-lint + itself created the next time an entry is written. + """ + return base / f"{__version__}-{code_identity()}" + + +def hash_source_content(content: bytes | None) -> str | None: + """Hash already-read source bytes for cache-key purposes, or None if they can't be cached. + + Takes the buffer rather than a path on purpose: the caller passes the very bytes the scan + analyzes (`SourceFile.content_bytes`), so an entry can only ever be keyed by the content its + findings were derived from. Re-reading the path here instead would reintroduce a window in + which the key and the findings describe different content. + + None means "don't cache this file this run", not a failure: an unreadable or non-regular + file has no bytes, and an oversized one is not worth an entry — `SourceFile` still reports a + proper `LintError` for both. + """ + if content is None or len(content) > MAX_SOURCE_BYTES: + return None + return hashlib.sha256(content).hexdigest() + + +def hash_effective_config( + enabled_rules: tuple[str, ...], + hsl101: HSL101Options, + hsl102: HSL102Options, + hsl103: HSL103Options, + *, + filename: str, + python_version: tuple[int, int] | None = None, +) -> str: + """Hash the config inputs that can change a file's scan outcome, given fixed content. + + `enabled_rules` is the per-file effective set (after `per-file-ignores`, `extend-select`, + etc. have already resolved it), not the raw configured selection. + + `filename` (the file's own basename, e.g. `path.name`) is folded in only when an enabled + HSL101 token family scopes to `"filenames"` — that's the one detector in this codebase whose + output depends on the file's name rather than purely its content, since it matches spec + tokens against the filename itself (see `_filename_candidates` in rules/spec_tokens.py). + Without this, two files with identical content but different names could otherwise collide + on the same cache entry and silently swap each other's filename-derived findings. + + `python_version` (major, minor) defaults to the running interpreter's `sys.version_info[:2]` + and is always folded into the hash. `SourceFile._analyze` parses source with `ast.parse`, + whose accepted grammar differs across the Python versions this project supports (e.g. `type + Alias = int` is a `SyntaxError` before 3.12) — without this, a cache shared across venvs of + different Python versions could replay a stale `SyntaxError` (or a stale success) that no + longer matches the interpreter actually running the scan. Accepting it as a parameter (rather + than reading `sys.version_info` internally) keeps this directly testable. + """ + payload: dict[str, object] = { + "enabled_rules": sorted(enabled_rules), + "hsl101": asdict(hsl101), + "hsl102": asdict(hsl102), + "hsl103": asdict(hsl103), + "python_version": list( + python_version if python_version is not None else sys.version_info[:2] + ), + } + if "HSL101" in enabled_rules and any("filenames" in family.scopes for family in hsl101.tokens): + payload["filename"] = filename + return hashlib.sha256(json.dumps(payload, sort_keys=True).encode("utf-8")).hexdigest() + + +@dataclass(frozen=True) +class CachedFileResult: + """A cacheable per-file scan outcome — everything `FileScanResult` carries except `stop`. + + `stop` (the process-boundary internal-error signal) is deliberately excluded: internal + errors are non-deterministic failures, not something a re-run with the same content and + config should replay from cache. + """ + + findings: tuple[Finding, ...] = () + errors: tuple[LintError, ...] = () + suppressed_count: int = 0 + files_scanned: int = 0 + + +def _entry_path(cache_dir: Path, content_hash: str, config_hash: str) -> Path: + return cache_dir / f"{content_hash}-{config_hash}.json" + + +def _require_text( + data: Any, required: tuple[str, ...], optional: tuple[str, ...] = () +) -> dict[str, Any]: + """Reject a payload whose text fields are not strings, before it reaches a dataclass. + + `Finding`/`LintError` are plain dataclasses: they validate their location fields (via + `results._validate_location`) but accept any type for the rest. A corrupted-but-valid-JSON + entry carrying, say, an integer `message` therefore constructs fine and only fails later, + when `ScanResult.to_dict()` sorts findings and hits `int < str`. That happens while the + result is being rendered — outside `check()`'s exception boundary — so the command crashes + with a traceback instead of treating the entry as the documented cache miss. + + Returns the payload so callers can validate and unpack in one expression. + """ + if not isinstance(data, dict): + raise TypeError("cache entry item must be an object") + payload = cast(dict[str, Any], data) + for name in required: + if not isinstance(payload.get(name), str): + raise TypeError(f"{name} must be a string") + for name in optional: + value = payload.get(name) + if value is not None and not isinstance(value, str): + raise TypeError(f"{name} must be a string or null") + return payload + + +def _finding_to_payload(finding: Finding) -> dict[str, Any]: + data = finding.to_dict() + del data["path"] + return data + + +def _finding_from_payload(data: dict[str, Any], *, path: str) -> Finding: + return Finding(path=path, **_require_text(data, ("rule_id", "message"))) + + +def _error_to_payload(err: LintError) -> dict[str, Any]: + data = err.to_dict() + del data["path"] + return data + + +def _error_from_payload(data: dict[str, Any], *, path: str) -> LintError: + return LintError( + path=path, + **_require_text(data, ("code", "kind", "phase", "operation", "message"), ("rule_id",)), + ) + + +def read_cached_result( + cache_dir: Path, + content_hash: str, + config_hash: str, + *, + relative_path: str, + reporter: CacheReporter, +) -> CachedFileResult | None: + """Return the cached result for this (content, config) pair, or None on a miss. + + A missing entry (the common case — nothing has cached this file/config pair yet) is a + silent miss. An entry that exists but can't be read or parsed is also treated as a miss — + a stale or corrupted cache entry must never fail a scan, only fall back to re-analyzing — + but that case is unusual enough to go through `reporter`, which makes the run's first such + failure visible without `--debug`. + """ + path = _entry_path(cache_dir, content_hash, config_hash) + try: + raw = path.read_text(encoding="utf-8") + except FileNotFoundError: + return None + except UnicodeDecodeError as exc: + # Corruption (or a foreign write) can leave bytes that are not valid UTF-8. That raises + # here, at decode time, rather than in the parse block below — and `UnicodeDecodeError` + # is a `ValueError`, so the `OSError` handler does not catch it either. Without this + # branch the exception escapes `_scan` entirely, aborting the run with an internal error + # instead of the cache miss this function promises. + reporter.failure(f"cache entry for {relative_path} is corrupted: {exc}") + return None + except OSError as exc: + reporter.failure(f"cache read failed for {relative_path}: {exc}") + return None + try: + payload = json.loads(raw) + suppressed_count = payload["suppressed_count"] + files_scanned = payload["files_scanned"] + # Dataclasses don't enforce annotations at runtime, so a corrupted-but-valid-JSON entry + # (e.g. `"suppressed_count": "1"`) would otherwise construct successfully here and only + # fail later when the caller accumulates it (`suppressed_count += cached.suppressed_count` + # in `cli.py`), turning what should be a graceful cache miss into an internal-error exit. + # Negative values are the same class of corruption reaching the same accumulation: no + # real scan produces one, and `"files_scanned": -5` would silently lower the run's + # reported totals rather than degrading to a miss. + if ( + not isinstance(suppressed_count, int) + or isinstance(suppressed_count, bool) + or suppressed_count < 0 + ): + raise TypeError("suppressed_count must be a non-negative int") + if ( + not isinstance(files_scanned, int) + or isinstance(files_scanned, bool) + or files_scanned < 0 + ): + raise TypeError("files_scanned must be a non-negative int") + return CachedFileResult( + findings=tuple( + _finding_from_payload(item, path=relative_path) for item in payload["findings"] + ), + errors=tuple( + _error_from_payload(item, path=relative_path) for item in payload["errors"] + ), + suppressed_count=suppressed_count, + files_scanned=files_scanned, + ) + except (ValueError, KeyError, TypeError) as exc: + reporter.failure(f"cache entry for {relative_path} is corrupted: {exc}") + return None + + +def _write_marker_if_absent( + marker: Path, content: str, *, reporter: CacheReporter, description: str +) -> None: + """Create `marker` with `content` unless it already exists, best-effort. + + Both of house-lint's cache markers are write-once and must never fail a scan, so they share + this shape. `description` names the marker in the failure line, which is the only part a + reader of stderr needs to tell the two apart. + + Created with `O_CREAT | O_EXCL`, not an `exists()` test followed by `write_text`. The two are + not equivalent for a path the scanned project controls: `exists()` follows symlinks, so a + *dangling* symlink at the marker path reports false and the subsequent write follows the link + and creates the file it names. `default_cache_base_is_safe` does not close this — it only + rejects a symlinked base, and a real `.house-lint-cache/` directory holding a dangling + `.gitignore` symlink passes it. A plain `house-lint check` on a freshly cloned repository + would then write `*` to a path that repository chose, anywhere on the filesystem. `O_EXCL` + fails with `EEXIST` on a symlink whether or not its target exists, which is exactly the + "create only if nothing is here" test this needs, in one unraceable syscall. + """ + # Split across two `try` blocks rather than one: only the open can raise `FileExistsError` + # for the reason this function cares about, and merging them would let a write-time + # `FileExistsError` take the silent "already there, nothing to do" path. + try: + descriptor = os.open(marker, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o644) + except FileExistsError: + # Already present — including as a symlink, which is the case `exists()` missed. Either + # way this marker is not ours to write, and "unless it already exists" is satisfied. + return + except OSError as exc: + reporter.failure(f"cache {description} marker write failed: {exc}") + return + try: + with os.fdopen(descriptor, "w", encoding="utf-8") as handle: + handle.write(content) + except OSError as exc: + reporter.failure(f"cache {description} marker write failed: {exc}") + + +def _write_self_ignore_marker(base: Path, *, reporter: CacheReporter) -> None: + """Write a `.gitignore` containing `*` into the cache *base* directory, once. + + Mirrors how pytest/mypy self-ignore their own cache directories: a downstream project that + runs house-lint gets an untracked, `git status`-invisible `.house-lint-cache/` without having + to add it to their own `.gitignore` by hand. Written at `base` (the unversioned + `.house-lint-cache/` directory), not the version-namespaced subdirectory, since that's the + path a `git status` in the scanned repo would actually flag. Best-effort: a failed marker + write must never fail the scan. + + Only ever called for house-lint's own default base (see `prepare_cache_dir`). A + `--cache-dir` names a directory the user already owns — writing `*` into it would change + Git's behaviour for every unrelated sibling, and pointing it at a project root would hide + the entire project from `git status`. + """ + _write_marker_if_absent( + base / ".gitignore", "*\n", reporter=reporter, description="self-ignore" + ) + + +def _write_version_dir_marker(cache_dir: Path, *, reporter: CacheReporter) -> None: + """Mark `cache_dir` as a house-lint-owned version directory, best-effort. + + `_prune_stale_version_dirs` only deletes sibling directories carrying this marker — without + it, a `--cache-dir` pointed at a pre-existing shared directory (e.g. `~/.cache`) would have + every unrelated sibling directory recursively deleted on the first cache write, since nothing + would distinguish "an old house-lint version directory" from "someone else's data." + """ + _write_marker_if_absent( + cache_dir / _VERSION_DIR_MARKER, "", reporter=reporter, description="version-dir" + ) + + +def _prune_stale_version_dirs(cache_dir: Path, *, reporter: CacheReporter) -> None: + """Remove sibling version directories under `cache_dir`'s base, best-effort. + + `versioned_cache_dir` namespaces the cache so an upgrade (or a source change) invalidates + stale entries, but nothing else ever deletes the superseded directory — left alone, those + namespaces accumulate under `/` forever. Reached only via `prune_stale_cache_dirs`, + which a scan calls once and only after it has actually written an entry, so a run of pure + cache hits never deletes anything. A *concurrent* process actively writing under a different + namespace during an overlapping run can still have its directory removed by this call — + best-effort here means "safe to fail," not "race-free." + + Only siblings carrying `_VERSION_DIR_MARKER` are eligible — a directory without it was never + created by house-lint's own versioned-cache writes, so it's left untouched regardless of how + it got there (a pre-existing directory under a shared `--cache-dir`, something else entirely). + """ + base = cache_dir.parent + try: + siblings = [child for child in base.iterdir() if child.is_dir() and child != cache_dir] + except OSError as exc: + reporter.failure(f"cache prune could not list {base}: {exc}") + return + for sibling in siblings: + marker = sibling / _VERSION_DIR_MARKER + # `is_file()` follows symlinks, so a marker that is merely a link to some regular file + # would satisfy it — turning "house-lint created this" into a claim any directory can + # make, with `shutil.rmtree` on the other side of it. `_write_marker_if_absent` creates + # the marker with `O_EXCL`, which can never produce a symlink, so demanding a real file + # here rejects nothing house-lint itself wrote. + if marker.is_symlink() or not marker.is_file(): + continue + try: + shutil.rmtree(sibling) + except OSError as exc: + reporter.failure(f"cache prune of stale version dir failed: {exc}") + + +def prepare_cache_dir(cache_dir: Path, *, self_ignore: bool, reporter: CacheReporter) -> None: + """Create and mark the cache directory, once per scan. + + This is per-run bookkeeping, not per-entry: creating the directory, marking it as + house-lint-owned, and (for house-lint's own default base only) dropping the self-ignore + marker. Doing it inside `write_cached_result` meant three extra filesystem calls for every + single file scanned, which is real overhead in the one code path whose entire purpose is to + be faster. + + Deliberately does *not* prune — see `prune_stale_cache_dirs`, which must stay tied to an + actual write. `self_ignore` must be true only when `cache_dir` sits beneath house-lint's own + default `.house-lint-cache/` base, never for a user-supplied `--cache-dir`. Best-effort + throughout: a failure here costs caching, never the scan. + """ + try: + cache_dir.mkdir(parents=True, exist_ok=True) + except OSError as exc: + reporter.failure(f"cache directory could not be created: {exc}") + return + _write_version_dir_marker(cache_dir, reporter=reporter) + if self_ignore: + _write_self_ignore_marker(cache_dir.parent, reporter=reporter) + + +def prune_stale_cache_dirs(cache_dir: Path, *, reporter: CacheReporter) -> None: + """Sweep superseded namespaces, once per scan and only after a real write has happened. + + Kept separate from `prepare_cache_dir` so that a run which writes nothing — every file a + cache hit — never deletes anything. That matters because the deletion is not race-free: a + concurrent house-lint process on a different version or build, sharing a `--cache-dir`, can + have its in-progress namespace removed. Tying the sweep to "this run actually wrote an + entry" keeps that window as narrow as it was before the per-run bookkeeping was hoisted out + of `write_cached_result`. + """ + _prune_stale_version_dirs(cache_dir, reporter=reporter) + + +class _WriteOutcome(Enum): + """Why one atomic entry write ended the way it did — specifically, whether retrying helps.""" + + WRITTEN = auto() + DIRECTORY_MISSING = auto() + FAILED = auto() + + +def _create_temp_exclusively(temporary: Path) -> int: + """Open `temporary` for writing, creating it and never following a symlink. + + `Path.write_text()` follows a symlink sitting at this path and truncates whatever it names. + The path is `-.json..tmp` — two hashes a scanned repository can compute + and a PID it can guess — so a repository that pre-creates the link gets an arbitrary file + overwritten with cache JSON, before `os.replace()` ever runs. `O_EXCL` is the same answer + `_write_marker_if_absent` already uses one function up: it fails with `EEXIST` on a symlink + whether or not the target exists, and on a hard link too, in one unraceable syscall. + + The one cost of `O_EXCL` is that a *real* leftover temp file blocks the write, and PIDs are + reused — a run killed mid-write would otherwise make that entry permanently unwritable and + silently disable caching for one file forever. Unlinking and retrying once fixes that without + reopening the hole: the retry is still `O_EXCL`, so it can only create, never write through + something another process put there in between. If the retry also loses, the caller reports a + failed write and the scan carries on uncached. + """ + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY + try: + return os.open(temporary, flags, 0o644) + except FileExistsError: + temporary.unlink(missing_ok=True) + return os.open(temporary, flags, 0o644) + + +def _write_entry(path: Path, payload: dict[str, Any], *, reporter: CacheReporter) -> _WriteOutcome: + """Write one entry atomically (temp file + `os.replace`). + + Atomicity means an interrupted process, or two concurrent house-lint runs writing the same + entry, can never leave a partially-written file in place of a real one. The temp file is + named per-PID, so a failure part-way through would otherwise strand a file no later run could + recognise or clean up — hence the unlink on the error path. + """ + temporary = path.with_name(f"{path.name}.{os.getpid()}.tmp") + try: + with os.fdopen(_create_temp_exclusively(temporary), "w", encoding="utf-8") as handle: + handle.write(json.dumps(payload)) + os.replace(temporary, path) + except FileNotFoundError as exc: + reporter.failure(f"cache write failed, directory is gone: {exc}") + with suppress(OSError): + temporary.unlink(missing_ok=True) + return _WriteOutcome.DIRECTORY_MISSING + except OSError as exc: + reporter.failure(f"cache write failed: {exc}") + with suppress(OSError): + temporary.unlink(missing_ok=True) + return _WriteOutcome.FAILED + return _WriteOutcome.WRITTEN + + +def write_cached_result( + cache_dir: Path, + content_hash: str, + config_hash: str, + result: CachedFileResult, + *, + self_ignore: bool, + reporter: CacheReporter, +) -> bool: + """Write a cache entry, best-effort. A failed write must never fail the scan itself — but it + is reported through `reporter`, so "why isn't caching working" is answerable from a plain + `house-lint check` and diagnosable in full under `--debug`. + + Returns whether the entry was durably persisted. Callers need that distinction rather than + "a write was attempted": `prune_stale_cache_dirs` is gated on this run having contributed a + real entry, and a run whose every write fails must not delete another process's namespace + while contributing nothing of its own. + + Assumes `prepare_cache_dir` has already run for this `cache_dir` — and restores that + precondition once if it has been undone mid-run. A concurrent house-lint process on a + different namespace, sharing the same `--cache-dir`, can `rmtree` this one out from under an + in-progress scan (see `_prune_stale_version_dirs`). Since `prepare_cache_dir` runs once per + scan and is never retried, one raced prune would otherwise cost every remaining write in the + run. Re-preparing and retrying bounds the damage to the single entry in flight. `self_ignore` + is what that re-preparation needs, and must carry the same value the scan's own + `prepare_cache_dir` call used. + + Only a vanished directory is retried. A permissions or out-of-space failure will not fix + itself between two adjacent calls, so retrying it would just pay twice for the same failure. + """ + path = _entry_path(cache_dir, content_hash, config_hash) + payload = { + "findings": [_finding_to_payload(finding) for finding in result.findings], + "errors": [_error_to_payload(err) for err in result.errors], + "suppressed_count": result.suppressed_count, + "files_scanned": result.files_scanned, + } + outcome = _write_entry(path, payload, reporter=reporter) + if outcome is not _WriteOutcome.DIRECTORY_MISSING: + return outcome is _WriteOutcome.WRITTEN + prepare_cache_dir(cache_dir, self_ignore=self_ignore, reporter=reporter) + return _write_entry(path, payload, reporter=reporter) is _WriteOutcome.WRITTEN + + +__all__ = [ + "CACHE_DIRNAME", + "CacheReporter", + "CachedFileResult", + "code_identity", + "default_cache_base", + "default_cache_base_is_safe", + "hash_effective_config", + "hash_source_content", + "prepare_cache_dir", + "prune_stale_cache_dirs", + "read_cached_result", + "versioned_cache_dir", + "write_cached_result", +] diff --git a/src/house_lint/cli.py b/src/house_lint/cli.py index b39126b..17ba450 100644 --- a/src/house_lint/cli.py +++ b/src/house_lint/cli.py @@ -6,11 +6,26 @@ from cyclopts import App, CycloptsError +from house_lint.cache import ( + CachedFileResult, + CacheReporter, + default_cache_base, + default_cache_base_is_safe, + hash_effective_config, + hash_source_content, + prepare_cache_dir, + prune_stale_cache_dirs, + read_cached_result, + versioned_cache_dir, + write_cached_result, +) from house_lint.config import ( ConfigError, LintConfig, + compile_per_file_ignores, default_config, load_config, + per_file_enabled_rules, selected_detector_inputs, ) from house_lint.discovery import DiscoveryError, discover_files, resolve_project @@ -30,7 +45,8 @@ internal_error, ) from house_lint.rule_catalog import rule_ids, rule_metadata -from house_lint.scanner import scan_file +from house_lint.scanner import FileScanResult, open_source, scan_source +from house_lint.source import SourceFile app = App(name="house-lint", help="Opinionated Python house-style linter.") @@ -97,6 +113,57 @@ def _requested_format(arguments: list[str]) -> str: return "text" +def _cache_keys( + source: SourceFile, config: LintConfig, file_enabled_rules: tuple[str, ...] +) -> tuple[str, str] | tuple[None, None]: + """Compute this file's (content_hash, config_hash) cache key, or (None, None) if the file + can't be safely hashed — that just means this file's result is never cached, not a failure. + + The content hash comes from `source.content_bytes`: the single buffer the detectors will + analyze. Deriving the key from the same bytes as the findings is what makes a cache entry + honest — it cannot describe content that was never scanned under that key. + """ + content_hash = hash_source_content(source.content_bytes) + if content_hash is None: + return None, None + config_hash = hash_effective_config( + file_enabled_rules, config.hsl101, config.hsl102, config.hsl103, filename=source.path.name + ) + return content_hash, config_hash + + +def _persist_cache_entry( + cache_dir: Path, + content_hash: str, + config_hash: str, + file_result: FileScanResult, + *, + self_ignore: bool, + reporter: CacheReporter, +) -> bool: + """Persist one scanned file's result. Returns whether it was durably written. + + Called only for a file that already has a cache key, so the returned `False` carries one + meaning — the write failed — and is safe to latch a circuit breaker on. An earlier version + of this helper also returned `False` for "this file isn't cacheable", which would have let a + single unhashable file disable caching for the rest of the run; `_scan` screens that case out + before calling here, and the two must not be recombined. + """ + return write_cached_result( + cache_dir, + content_hash, + config_hash, + CachedFileResult( + file_result.findings, + file_result.errors, + file_result.suppressed_count, + file_result.files_scanned, + ), + self_ignore=self_ignore, + reporter=reporter, + ) + + def _scan( paths: tuple[Path, ...], *, @@ -104,6 +171,9 @@ def _scan( config_path: Path | None, config: LintConfig, use_gitignore: bool, + read_cache: bool, + cache_dir: Path, + cache_self_ignore: bool, debug: bool, ) -> ScanResult: """Run the complete file pipeline, retaining all completed-file results.""" @@ -125,17 +195,118 @@ def _scan( errors=(exc.error,), ) + cache_reporter = CacheReporter(debug=debug) + # Named for what it checks, not for whether caching happens: `read_cache` (--no-cache) and a + # mid-run write failure below also govern that. `cache_self_ignore` is true exactly when + # `cache_dir` sits under house-lint's own default base (see `check`), which is the only base + # whose path the scanned project itself controls and so the only one worth vetting. + cache_base_is_safe = not cache_self_ignore or default_cache_base_is_safe(cache_dir) + if discovered.files: + # Both branches run once per scan, and only when there is something to scan — so a run + # that discovers nothing neither creates a cache directory in the project nor reports on + # one it was never going to use. + if cache_base_is_safe: + prepare_cache_dir(cache_dir, self_ignore=cache_self_ignore, reporter=cache_reporter) + else: + cache_reporter.failure( + f"caching disabled: the default cache directory {cache_dir} or its parent " + f"is a symlink" + ) + findings: list[Finding] = [] errors = list(discovered.errors) detector_inputs = selected_detector_inputs(config) + compiled_per_file_ignores = compile_per_file_ignores(config.per_file_ignores) suppressed_count = 0 files_scanned = 0 + wrote_cache_entry = False + cache_writes_failed = False for path in discovered.files: - file_result = scan_file( + relative = path.relative_to(root).as_posix() + # Pattern matching runs on the file's resolved location, reporting on the spelling the + # user typed. `discovered.files` preserves the argument as given, so an accepted path + # containing `..` — `src/../tests/a.py`, with both directories present — reaches + # `relative` as `src/../tests/a.py`, which a configured `"tests/**"` never matches: + # house-lint then runs a rule the config disabled for everything under `tests/`. + # + # Resolved, not collapsed lexically. A purely lexical `..` collapse is only correct when + # no traversed component is a symlink: with `link/ -> x/y/`, the OS reads `link/../foo.py` + # as `x/foo.py` while the lexical form reads `foo.py`, so a pattern written for the + # file's real location silently stops matching. Resolving cannot drift from the file + # actually opened, and it is already the identity discovery itself uses — `selected` is + # keyed by resolved path, which is what makes a symlink and its target deduplicate. A + # per-file-ignore therefore follows the file, not the spelling that reached it. + match_relative = discovered.resolved_paths[path].relative_to(root).as_posix() + file_enabled_rules = config.enabled_rules + file_detector_inputs = detector_inputs + if compiled_per_file_ignores: + file_enabled_rules = per_file_enabled_rules( + config.enabled_rules, compiled_per_file_ignores, match_relative + ) + if file_enabled_rules != config.enabled_rules: + # Suppression handling only flags pragmas naming a disabled rule (see + # apply_suppressions/_collect_claims in suppressions.py) — it does not filter + # candidate findings by enabled_rules. detector_inputs must be recomputed here + # so a per-file-ignored rule's detector never runs for this file; skipping this + # recompute would let its findings leak through unfiltered. + file_detector_inputs = selected_detector_inputs( + config, enabled_rules=file_enabled_rules + ) + # The file is read exactly once per scan, here. Everything below — the cache key, the + # detectors, and the entry written back — derives from that one buffer, so a cache entry + # can never describe bytes that were not the ones scanned. + loaded = open_source( + # Indexed, not `.get()`: `DiscoveryResult.files` is built from `resolved_paths`' + # own keys, so a miss means that invariant broke. Falling back to `None` there would + # silently re-resolve the path and reopen the symlink-retarget window this threading + # exists to close, so a `KeyError` is the wanted outcome. path, root=root, - enabled_rules=config.enabled_rules, - detector_inputs=detector_inputs, + resolved_path=discovered.resolved_paths[path], + debug=debug, + ) + if isinstance(loaded, FileScanResult): + # open_source only returns a result for a process-boundary abort, which is fatal to + # the run and never cached. + errors.extend(loaded.errors) + files_scanned += loaded.files_scanned + break + source = loaded + # Computed unconditionally (even under --no-cache) because a write still happens on a + # miss regardless of read_cache — --no-cache only disables the read below, not the + # write further down, so the hashes are needed either way. + content_hash, config_hash = _cache_keys(source, config, file_enabled_rules) + cached = ( + read_cached_result( + cache_dir, + content_hash, + config_hash, + relative_path=relative, + reporter=cache_reporter, + ) + if ( + cache_base_is_safe + and read_cache + and content_hash is not None + and config_hash is not None + ) + else None + ) + # A cached *error* is not replayable under `--debug`: the traceback is printed by + # `scan_source`, which a hit skips, so the first debug run showed the exception and every + # identical one after it showed only the structured line. Re-scanning those files keeps + # `--debug` output independent of cache state, at the cost of re-analyzing the few files + # that failed — clean files still hit the cache, so the diagnostic mode stays fast. + if cached is not None and not (debug and cached.errors): + findings.extend(cached.findings) + errors.extend(cached.errors) + suppressed_count += cached.suppressed_count + files_scanned += cached.files_scanned + continue + file_result = scan_source( + source, + enabled_rules=file_enabled_rules, + detector_inputs=file_detector_inputs, debug=debug, ) findings.extend(file_result.findings) @@ -143,7 +314,43 @@ def _scan( suppressed_count += file_result.suppressed_count files_scanned += file_result.files_scanned if file_result.stop: + # A stop result is a non-deterministic process-boundary failure, not a reproducible + # scan outcome, so it must never be replayed from cache on a later hit. break + if content_hash is None or config_hash is None: + # Unhashable file (non-regular, unreadable, or oversized). Never cached, and not a + # cache failure — it must not trip the circuit breaker below. + continue + if not cache_base_is_safe: + # Decided once, before the loop, and already reported there. + continue + if cache_writes_failed: + # A cache directory that rejected one write rejects every later one for the rest of + # this process (unwritable, full, or removed mid-run by a concurrent prune). Retrying + # per file would mean up to MAX_DISCOVERED_FILES pointless attempts and one + # near-duplicate --debug line each, burying the single fact worth reporting. + continue + if _persist_cache_entry( + cache_dir, + content_hash, + config_hash, + file_result, + self_ignore=cache_self_ignore, + reporter=cache_reporter, + ): + wrote_cache_entry = True + else: + cache_writes_failed = True + # Through the reporter rather than a bare print, so every cache diagnostic in the + # pipeline goes through one object. The failing write above has already reported, + # so this lands on the `--debug`-only branch — which is where a follow-on + # "and here is what that failure means for the rest of the run" line belongs. + cache_reporter.failure( + f"cache writes disabled for the rest of this run after the first failure " + f"(at {relative})" + ) + if wrote_cache_entry: + prune_stale_cache_dirs(cache_dir, reporter=cache_reporter) return ScanResult( root, config_path, @@ -165,7 +372,11 @@ def check( format: str = "text", select: list[str] | None = None, ignore: list[str] | None = None, + extend_select: list[str] | None = None, + extend_ignore: list[str] | None = None, no_gitignore: bool = False, + no_cache: bool = False, + cache_dir: Path | None = None, debug: bool = False, ) -> int: """Scan configured roots or explicit Python paths.""" @@ -175,6 +386,8 @@ def check( return 2 cli_select = _flatten_ids(select) cli_ignore = _flatten_ids(ignore) + cli_extend_select = _flatten_ids(extend_select) + cli_extend_ignore = _flatten_ids(extend_ignore) resolved_root: Path | None = None resolved_config: Path | None = None try: @@ -187,20 +400,36 @@ def check( resolved_root = resolution.root resolved_config = resolution.config lint_config = ( - default_config(cli_select=cli_select, cli_ignore=cli_ignore) + default_config( + cli_select=cli_select, + cli_ignore=cli_ignore, + cli_extend_select=cli_extend_select, + cli_extend_ignore=cli_extend_ignore, + ) if resolution.config is None else load_config( resolution.config, cli_select=cli_select, cli_ignore=cli_ignore, + cli_extend_select=cli_extend_select, + cli_extend_ignore=cli_extend_ignore, ) ) + cache_base = ( + cache_dir.expanduser().resolve() + if cache_dir is not None + else default_cache_base(resolution.root) + ) + resolved_cache_dir = versioned_cache_dir(cache_base) result = _scan( tuple(paths or ()), root=resolution.root, config_path=resolution.config, config=lint_config, use_gitignore=not no_gitignore, + read_cache=not no_cache, + cache_dir=resolved_cache_dir, + cache_self_ignore=cache_dir is None, debug=debug, ) except ConfigError as exc: diff --git a/src/house_lint/config.py b/src/house_lint/config.py index 8dbfa05..4e2d3bb 100644 --- a/src/house_lint/config.py +++ b/src/house_lint/config.py @@ -2,9 +2,10 @@ import re import tomllib -from collections.abc import Iterable -from dataclasses import dataclass +from collections.abc import Iterable, Mapping +from dataclasses import dataclass, field from pathlib import Path +from types import MappingProxyType from typing import Any, cast from pathspec import GitIgnoreSpec @@ -65,14 +66,23 @@ class LintConfig: hsl101: HSL101Options = HSL101Options() hsl102: HSL102Options = HSL102Options() hsl103: HSL103Options = HSL103Options() + per_file_ignores: Mapping[str, tuple[str, ...]] = field( + default_factory=lambda: MappingProxyType({}) + ) DetectorOptions = HSL101Options | HSL102Options | HSL103Options | None DetectorInput = tuple[str, DetectorOptions] -def selected_detector_inputs(config: LintConfig) -> tuple[DetectorInput, ...]: - """Return enabled ordinary rules with their already-validated options.""" +def selected_detector_inputs( + config: LintConfig, *, enabled_rules: tuple[str, ...] | None = None +) -> tuple[DetectorInput, ...]: + """Return enabled ordinary rules with their already-validated options. + + `enabled_rules` overrides `config.enabled_rules` when given, for callers that resolved a + per-file effective rule set (see `per_file_enabled_rules`) rather than the global one. + """ options: dict[str, DetectorOptions] = { "HSL001": None, "HSL002": None, @@ -82,16 +92,55 @@ def selected_detector_inputs(config: LintConfig) -> tuple[DetectorInput, ...]: "HSL102": config.hsl102, "HSL103": config.hsl103, } + rules = config.enabled_rules if enabled_rules is None else enabled_rules + return tuple((rule_id, options[rule_id]) for rule_id in rules if rule_id in options) + + +CompiledPerFileIgnores = tuple[tuple[GitIgnoreSpec, frozenset[str]], ...] + + +def compile_per_file_ignores( + per_file_ignores: Mapping[str, tuple[str, ...]], +) -> CompiledPerFileIgnores: + """Precompile per-file-ignores glob patterns once, for repeated per-file matching.""" return tuple( - (rule_id, options[rule_id]) for rule_id in config.enabled_rules if rule_id in options + (GitIgnoreSpec.from_lines((pattern,)), frozenset(rule_ids)) + for pattern, rule_ids in per_file_ignores.items() ) +def per_file_enabled_rules( + base_enabled_rules: tuple[str, ...], + compiled_per_file_ignores: CompiledPerFileIgnores, + relative_path: str, +) -> tuple[str, ...]: + """Return `base_enabled_rules` minus any rules whose Git-ignore-style glob pattern matches + this root-relative file path.""" + ignored: set[str] = set() + for spec, rule_ids in compiled_per_file_ignores: + if spec.match_file(relative_path): + ignored |= rule_ids + if not ignored: + return base_enabled_rules + return tuple(rule_id for rule_id in base_enabled_rules if rule_id not in ignored) + + def default_config( - *, cli_select: Iterable[str] | None = None, cli_ignore: Iterable[str] | None = None + *, + cli_select: Iterable[str] | None = None, + cli_ignore: Iterable[str] | None = None, + cli_extend_select: Iterable[str] | None = None, + cli_extend_ignore: Iterable[str] | None = None, ) -> LintConfig: """Build built-in configuration with the same CLI selection semantics as TOML.""" - enabled_rules = _effective_rule_selection(DEFAULT_SELECT, (), cli_select, cli_ignore) + enabled_rules = _effective_rule_selection( + DEFAULT_SELECT, + (), + cli_select, + cli_ignore, + cli_extend_select=cli_extend_select, + cli_extend_ignore=cli_extend_ignore, + ) if "HSL101" in enabled_rules: raise ConfigError("HSL101 requires tokens when selected") return LintConfig(enabled_rules=enabled_rules) @@ -130,8 +179,27 @@ def _effective_rule_selection( configured_ignore: Iterable[str], cli_select: Iterable[str] | None, cli_ignore: Iterable[str] | None, + *, + configured_extend_select: Iterable[str] = (), + configured_extend_ignore: Iterable[str] = (), + cli_extend_select: Iterable[str] | None = None, + cli_extend_ignore: Iterable[str] | None = None, ) -> tuple[str, ...]: - """Apply the one selection precedence algorithm shared by defaults and TOML.""" + """Apply the one selection precedence algorithm shared by defaults and TOML. + + Order: + 1. `select`/`ignore` establish the base set — or a CLI `--select` gives a wholesale + override, replacing configured `select`/`ignore` entirely rather than adding to them. + 2. `extend-select`/`extend-ignore` layer on top of that base *regardless of its source*. + Config and CLI variants of each are merged together (concatenated, not one overriding + the other) before being applied, since — unlike `select` vs. `--select` — neither is + meant to replace the other. + 3. `extend-ignore` is subtractive against the *whole* pool from steps 1-2, not just + against `extend-select`'s own additions — `select = ["HSL001"]` combined with + `extend-ignore = ["HSL001"]` drops HSL001 entirely, the same as if it had never been + selected. + 4. CLI `--ignore` is applied last and always wins over everything above. + """ configured = _ids(list(configured_select), "select") configured_ignored = _ids(list(configured_ignore), "ignore") selected = ( @@ -139,8 +207,15 @@ def _effective_rule_selection( if cli_select is not None else tuple(rule_id for rule_id in configured if rule_id not in configured_ignored) ) + extend_selected = _ids(list(configured_extend_select), "extend-select") + _ids( + list(cli_extend_select or ()), "--extend-select" + ) + extend_ignored = _ids(list(configured_extend_ignore), "extend-ignore") + _ids( + list(cli_extend_ignore or ()), "--extend-ignore" + ) + extended = (set(selected) | set(extend_selected)) - set(extend_ignored) cli_ignored = _ids(list(cli_ignore or ()), "--ignore") - return tuple(sorted(set(selected) - set(cli_ignored))) + ("HSL900",) + return tuple(sorted(extended - set(cli_ignored))) + ("HSL900",) def _validate_include(values: tuple[str, ...]) -> tuple[str, ...]: @@ -153,16 +228,35 @@ def _validate_include(values: tuple[str, ...]) -> tuple[str, ...]: return values -def _validate_exclude(values: tuple[str, ...]) -> tuple[str, ...]: +def _validate_git_ignore_patterns(values: tuple[str, ...], name: str) -> tuple[str, ...]: + """Root-relative, syntactically valid Git-ignore-style patterns, shared by `exclude` and + `per-file-ignores` — the two config keys that accept this pattern style.""" if any(Path(value).is_absolute() or ".." in Path(value).parts for value in values): - raise ConfigError("exclude patterns must be root-relative") + raise ConfigError(f"{name} patterns must be root-relative") try: GitIgnoreSpec.from_lines(values) except (TypeError, ValueError, re.error) as exc: - raise ConfigError(f"exclude contains invalid Git-ignore patterns: {exc}") from exc + raise ConfigError(f"{name} contains invalid Git-ignore patterns: {exc}") from exc return values +def _validate_exclude(values: tuple[str, ...]) -> tuple[str, ...]: + return _validate_git_ignore_patterns(values, "exclude") + + +def _per_file_ignores(raw: Any) -> Mapping[str, tuple[str, ...]]: + table = _table(raw, "per-file-ignores") + result: dict[str, tuple[str, ...]] = {} + for pattern, value in table.items(): + if not pattern: + raise ConfigError("per-file-ignores keys must be non-empty Git-ignore-style patterns") + if pattern.startswith("!"): + raise ConfigError("per-file-ignores keys must not be negated patterns") + _validate_git_ignore_patterns((pattern,), "per-file-ignores") + result[pattern] = _ids(value, f"per-file-ignores.{pattern!r}") + return MappingProxyType(result) + + def _strict_keys(table: dict[str, Any], allowed: set[str], name: str) -> None: unknown = set(table) - allowed if unknown: @@ -294,23 +388,55 @@ def get_house_lint_table(document: dict[str, Any]) -> dict[str, Any] | None: def load_config( - path: Path, *, cli_select: Iterable[str] | None = None, cli_ignore: Iterable[str] | None = None + path: Path, + *, + cli_select: Iterable[str] | None = None, + cli_ignore: Iterable[str] | None = None, + cli_extend_select: Iterable[str] | None = None, + cli_extend_ignore: Iterable[str] | None = None, ) -> LintConfig: """Load and validate one TOML configuration file.""" document = load_toml(path) house = get_house_lint_table(document) if house is None: raise ConfigError("config lacks [tool.house-lint]") - _strict_keys(house, {"include", "exclude", "select", "ignore", "rules"}, "tool.house-lint") + _strict_keys( + house, + { + "include", + "exclude", + "select", + "ignore", + "extend-select", + "extend-ignore", + "per-file-ignores", + "rules", + }, + "tool.house-lint", + ) include = _validate_include(_strings(house.get("include", list(DEFAULT_INCLUDE)), "include")) exclude = _validate_exclude(_strings(house.get("exclude", []), "exclude")) + # Every raw TOML value goes through `_strings` here rather than reaching + # `_effective_rule_selection` as-is: that function converts with `list(...)` before validating, + # which turns `select = 5` into a `TypeError` (an internal-error exit, not the documented + # config-error exit) and silently splits `select = "HSL001"` into single characters, reported + # as an unknown rule ID instead of "must be an array". Its other callers pass tuples built in + # this module, so the conversion stays where it is and the untrusted edge is checked here. enabled = _effective_rule_selection( - house.get("select", list(DEFAULT_SELECT)), - house.get("ignore", []), + _strings(house.get("select", list(DEFAULT_SELECT)), "select"), + _strings(house.get("ignore", []), "ignore"), cli_select, cli_ignore, + configured_extend_select=_strings(house.get("extend-select", []), "extend-select"), + configured_extend_ignore=_strings(house.get("extend-ignore", []), "extend-ignore"), + cli_extend_select=cli_extend_select, + cli_extend_ignore=cli_extend_ignore, ) + per_file_ignores = _per_file_ignores(house.get("per-file-ignores", {})) options = _rule_options(house) if "HSL101" in enabled and not options[0].tokens: raise ConfigError("HSL101 requires tokens when selected") - return LintConfig(include, exclude, tuple(sorted(enabled)), *options) + # `enabled` is already sorted with the always-on rule appended (see + # `_effective_rule_selection`); re-sorting here would only differ from `default_config`'s + # handling of the same value if an always-on rule ID ever stopped sorting last. + return LintConfig(include, exclude, enabled, *options, per_file_ignores) diff --git a/src/house_lint/discovery.py b/src/house_lint/discovery.py index 1a4dc2b..d84ac1f 100644 --- a/src/house_lint/discovery.py +++ b/src/house_lint/discovery.py @@ -2,6 +2,7 @@ import os import re +from collections.abc import Callable, Mapping from dataclasses import dataclass, field from pathlib import Path @@ -11,7 +12,14 @@ from house_lint.results import LintError BUILTIN_EXCLUDES = (".git/", ".venv/", ".nox/", "__pycache__/", "site-packages/", "node_modules/") +# Stands in for "an ancestor of this directory is excluded", where nothing beneath it can be +# re-included — see `_FileSelector._combined_gitignore_spec`. +IGNORE_EVERYTHING = ("**",) MAX_DISCOVERED_FILES = 100_000 +_GITIGNORE_METACHARS = re.compile(r"([!#*?\[\]\\])") +_CONTENTS_GLOB = re.compile(r"(? None: @dataclass(frozen=True) class DiscoveryResult: + """Selected files, plus the resolved target discovery actually validated for each. + + `files` holds unresolved paths, because those are what the user named and what findings are + reported against. `resolved_paths` maps each of them to the `resolve()` result that passed the + containment check, so the scan can read *that* target rather than resolving the symlink a + second time and possibly following it somewhere else — see `SourceFile.__init__`. + """ + files: tuple[Path, ...] files_skipped: int = 0 errors: tuple[LintError, ...] = () + resolved_paths: Mapping[Path, Path] = field(default_factory=lambda: dict[Path, Path]()) @dataclass(frozen=True) @@ -59,39 +76,209 @@ def _gitignore_error(operation: str, message: str) -> LintError: ) +def _load_gitignore_lines(path: Path, on_error: Callable[[str, str], None]) -> tuple[str, ...]: + """Read and validate a `.gitignore` file's pattern lines, reporting stat/read/parse failures. + + Returns raw lines rather than a parsed spec: nested `.gitignore` files get their lines + rewritten (see `_prefix_pattern`) and combined with their ancestors' before the final parse, + so negation in a closer `.gitignore` can override a less-specific ignore the way git itself + resolves precedence. Parsing here first — then discarding the result — exists purely for + error attribution: without it, a bad line surviving into the merged multi-file list would + only be blamed on the directory being combined, not on the specific `.gitignore` at fault. + """ + try: + # Checked before `is_file()`, which follows symlinks: git does not read a symlinked + # `.gitignore` at all, so following one would apply patterns git never applies. With + # `src/.gitignore -> patterns` containing `*.py`, discovery would skip `src/a.py` while + # `git check-ignore` still reports it as included. + if path.is_symlink(): + return () + is_file = path.is_file() + except OSError as exc: + on_error("stat", str(exc)) + return () + if not is_file: + return () + try: + lines = tuple(path.read_text(encoding="utf-8").splitlines()) + except (OSError, UnicodeDecodeError) as exc: + on_error("read", str(exc)) + return () + try: + GitIgnoreSpec.from_lines(lines) + except (TypeError, ValueError, re.error) as exc: + on_error("parse", str(exc)) + return () + return lines + + +def _escape_gitignore_literal(segment: str) -> str: + """Escape characters gitwildmatch treats as pattern syntax within a literal path segment. + + `_prefix_pattern` embeds real directory names into a pattern string that gets re-parsed by + `GitIgnoreSpec`. Without this, a directory literally named e.g. "sub[1]" or "!important" + would have its `[`/`]`/`!` read back as wildcard or negation syntax instead of literal + characters, silently changing which files the rewritten pattern matches. + """ + return _GITIGNORE_METACHARS.sub(r"\\\1", segment) + + +def _strip_unescaped_trailing_whitespace(text: str) -> str: + """Trim trailing spaces/tabs, except one quoted by a backslash escape. + + Mirrors gitwildmatch's own trailing-whitespace rule ("trailing spaces are ignored unless + they are quoted with backslash") so a nested pattern's rewrite doesn't discard whitespace + that `GitIgnoreSpec` would otherwise treat as significant. + + What decides the question is the *parity* of the backslash run before the whitespace, not + whether a single backslash sits there: backslashes quote each other pairwise, so an even + run leaves the space unquoted and git strips it. `a\\\\ ` (two backslashes, one space) is + the case that separates the two readings — git reduces it to `a\\\\`, which names `a\\`, + while treating the lone preceding backslash as an escape keeps the space and names `a\\ ` + instead. Checked against real `git check-ignore`; see the parity suite. + """ + end = len(text) + while end > 0 and text[end - 1] in " \t": + backslashes = 0 + index = end - 2 + while index >= 0 and text[index] == "\\": + backslashes += 1 + index -= 1 + if backslashes % 2 == 1: + break + end -= 1 + return text[:end] + + +def _collapse_double_star_run(core: str) -> str: + """Reduce every run of consecutive `**` segments in `core` to a single `**`. + + git reads a run of `**` segments as one: `**/**/` ignores exactly what `**/` ignores, and + `**/**/b.py` matches exactly what `**/b.py` matches (checked against real `git check-ignore`; + see the collapse family in the parity suite). Collapsing here means the branches below only + ever see the canonical one-segment spelling, so the `core == "**"` case covers the whole + family rather than the single spelling someone happened to write down. + + Without this, a repeated form reached the generic slash-containing branch instead — `**/**/` + became `/**/**/`, which `GitIgnoreSpec` matches against an immediate regular file + (`/a.py`) that git leaves alone, silently hiding it from the linter. + `_normalize_contents_glob` cannot repair that downstream: it deliberately skips a `/**` + preceded by another `*`. + """ + return _DOUBLE_STAR_RUN.sub("**", core) + + +def _normalize_contents_glob(pattern: str) -> str: + """Rewrite a trailing `/**` so it cannot match the directory whose contents it names. + + git reads `build/**` as "everything inside build" and never matches `build` itself; + `GitIgnoreSpec` matches the directory too. The difference is invisible until a later + negation re-includes something underneath, because house-lint prunes `build/` at walk time + and then never consults the negation inside it — git, by contrast, still descends. Spelling + the pattern `build/**/*` means the same thing to both, since `**` matches zero or more + directories but the trailing `*` still demands a path component. + + Runs on every pattern that reaches a spec (see `_spec_for_lines` and `_patterns`), so it + also sees `_prefix_pattern`'s output. The lookbehind matches `**` specifically rather than a + single `*`, so it skips only `a/**/**` — where rewriting has no evidence behind it — while + still rewriting `a/*/**`, an ordinary pattern whose preceding segment just happens to end in + a star. `_prefix_pattern` handles a bare `**` line itself rather than emitting + `/**/**` and relying on this to clean it up: the two `**` fixes are deliberately + split that way, and neither subsumes the other. + """ + if not pattern or pattern.startswith("#"): + return pattern + return _CONTENTS_GLOB.sub(r"/**/*\1", pattern) + + +def _prefix_pattern(prefix: str, line: str) -> str: + """Rewrite a gitignore pattern owned by `prefix` into an equivalent root-anchored pattern. + + `prefix` is the pattern's owning directory, relative to root, posix-style, no trailing slash, + with each path segment already escaped via `_escape_gitignore_literal` (e.g. "src/sub"). + Mirrors git's own per-directory pattern semantics: a pattern with no other slash matches at + any depth under its directory (`_prefix_pattern("src", "foo.py") == "src/**/foo.py"`, which + `GitIgnoreSpec` matches against both "src/foo.py" and "src/sub/foo.py" — "**" matches zero or + more directories), one with an embedded (or leading) slash is anchored to that directory, and + a leading "!" negates independent of anchoring. + + Only *unescaped trailing* whitespace is insignificant per gitwildmatch — a leading space is + part of the pattern (matches a filename that itself starts with a space), and "#"/"!" only + carry their special meaning as the pattern's literal first character. Blindly stripping the + whole line (as an earlier version of this function did) silently dropped a leading space from + the matched filename and could misidentify a leading-whitespace-prefixed "#"/"!" as + comment/negation syntax that real gitignore parsing (verified against `GitIgnoreSpec` directly) + does not treat as such — so only `.strip()`'s result is used to test for an all-whitespace + (blank) line; the pattern body itself is built from the unstripped `line`. + """ + if not line.strip(): + return line + if line.startswith("#"): + return line + negated = line.startswith("!") + body = _strip_unescaped_trailing_whitespace(line[1:] if negated else line) + if body in ("", "/"): + # A bare "/" (or an empty pattern after stripping "!") has no defined gitignore meaning; + # treat it as inert rather than accidentally suppressing the whole owning directory. + return line + has_trailing_slash = body.endswith("/") and body != "/" + core = _collapse_double_star_run(body[:-1] if has_trailing_slash else body) + if core.startswith("/"): + anchored_core = f"{prefix}/{core[1:]}" + elif "/" in core: + anchored_core = f"{prefix}/{core}" + elif core == "**": + # `**` is the one no-slash pattern the general expansion below gets wrong: it would + # produce `/**/**`, which `GitIgnoreSpec` matches against `` itself and + # — in the directory-only `**/` form — against an immediate regular file + # (`/a.py`) that git leaves alone. Naming an explicit segment (`*`) after the + # `**` keeps "at any depth" while still requiring a path component to be there. + # `_normalize_contents_glob` cannot repair this downstream: it deliberately skips a + # `/**` preceded by another `*`, so the bad form has to not be produced here. + anchored_core = f"{prefix}/**/*" + else: + anchored_core = f"{prefix}/**/{core}" + anchored = anchored_core + ("/" if has_trailing_slash else "") + return f"!{anchored}" if negated else anchored + + def _patterns( root: Path, excludes: tuple[str, ...], use_gitignore: bool -) -> tuple[GitIgnoreSpec, GitIgnoreSpec, GitIgnoreSpec, tuple[LintError, ...]]: - exclude_spec = GitIgnoreSpec.from_lines(excludes) - gitignore_spec = GitIgnoreSpec.from_lines(()) +) -> tuple[GitIgnoreSpec, GitIgnoreSpec, tuple[str, ...], tuple[LintError, ...]]: errors: list[LintError] = [] + root_gitignore_lines: tuple[str, ...] = () if use_gitignore: - ignore = root / ".gitignore" - try: - is_file = ignore.is_file() - except OSError as exc: - errors.append(_gitignore_error("stat", str(exc))) - is_file = False - if is_file: - try: - gitignore_spec = GitIgnoreSpec.from_lines( - ignore.read_text(encoding="utf-8").splitlines() - ) - except (OSError, UnicodeDecodeError) as exc: - errors.append(_gitignore_error("read", str(exc))) - except (TypeError, ValueError, re.error) as exc: - errors.append(_gitignore_error("parse", str(exc))) + + def on_error(operation: str, message: str) -> None: + errors.append(_gitignore_error(operation, message)) + + root_gitignore_lines = _load_gitignore_lines(root / ".gitignore", on_error) return ( GitIgnoreSpec.from_lines(BUILTIN_EXCLUDES), - exclude_spec, - gitignore_spec, + GitIgnoreSpec.from_lines(_normalize_contents_glob(value) for value in excludes), + root_gitignore_lines, tuple(errors), ) -def _ignored(root: Path, path: Path, *specs: GitIgnoreSpec) -> bool: +def _ignored(root: Path, path: Path, *specs: GitIgnoreSpec, is_dir: bool) -> bool: + """Match `path`, relative to `root`, against each spec. + + All specs here are root-anchored, including the combined gitignore-hierarchy spec built by + `_FileSelector._combined_gitignore_spec` — nested `.gitignore` patterns are rewritten to be + root-anchored before that spec is built, so no directory-relative matching is needed here. + + `is_dir` selects which single form the path is matched in: git classifies a path once, as + either a file or a directory, and then applies last-matching-line-wins within that one + classification. Probing both forms and OR-ing them (as an earlier version did) breaks that: + an ignore matching the directory form survives a negation that only matches the file form, + so `["cache", "!cache/"]` wrongly excluded `cache/`, and a directory-only pattern like + `b.py/` wrongly matched the regular file `b.py`. Callers always already know which kind of + path they hold, so this is a parameter rather than another `stat` call. + """ relative = path.relative_to(root).as_posix() - return any(spec.match_file(relative) or spec.match_file(f"{relative}/") for spec in specs) + probe = f"{relative}/" if is_dir else relative + return any(spec.match_file(probe) for spec in specs) @dataclass @@ -99,11 +286,25 @@ class _FileSelector: root: Path builtin_spec: GitIgnoreSpec exclude_spec: GitIgnoreSpec - gitignore_spec: GitIgnoreSpec + root_gitignore_lines: tuple[str, ...] errors: list[LintError] + use_gitignore: bool = True selected: dict[Path, Path] = field(default_factory=lambda: dict[Path, Path]()) files_skipped: int = 0 limit_reached: bool = False + own_gitignore_lines_cache: dict[Path, tuple[str, ...]] = field( + default_factory=lambda: dict[Path, tuple[str, ...]]() + ) + combined_gitignore_spec_cache: dict[Path, GitIgnoreSpec] = field( + default_factory=lambda: dict[Path, GitIgnoreSpec]() + ) + spec_by_lines_cache: dict[tuple[str, ...], GitIgnoreSpec] = field( + default_factory=lambda: dict[tuple[str, ...], GitIgnoreSpec]() + ) + excluded_ancestor_cache: dict[Path, bool] = field(default_factory=lambda: dict[Path, bool]()) + reported_spec_failures: set[tuple[tuple[str, ...], Path]] = field( + default_factory=lambda: set[tuple[tuple[str, ...], Path]]() + ) def select(self, requested: tuple[Path, ...], *, explicit_paths: bool) -> None: seen_arguments: set[Path] = set() @@ -117,11 +318,27 @@ def select(self, requested: tuple[Path, ...], *, explicit_paths: bool) -> None: self._consider(argument, explicit_paths=explicit_paths) def result(self) -> DiscoveryResult: + # `selected` is keyed by resolved path so a symlink and its target deduplicate; the scan + # needs the reverse direction, from the path it reports to the target it may read. + resolved_paths = {path: resolved for resolved, path in self.selected.items()} return DiscoveryResult( - tuple(sorted(self.selected.values())), self.files_skipped, tuple(self.errors) + tuple(sorted(resolved_paths)), self.files_skipped, tuple(self.errors), resolved_paths ) - def _consider(self, path: Path, *, explicit_paths: bool) -> None: + def _consider( + self, + path: Path, + *, + explicit_paths: bool, + combined_gitignore_spec: GitIgnoreSpec | None = None, + ) -> None: + """Evaluate `path` for selection. + + `combined_gitignore_spec` is precomputed once per directory by `_walk` and passed in for + every file it discovers there, avoiding a redundant per-file rebuild. Callers outside a + walk (`select`'s top-level `include`/`explicit` entries) leave it `None` and it's built + lazily below, once, for that single path. + """ if self.limit_reached: return try: @@ -163,7 +380,35 @@ def _consider(self, path: Path, *, explicit_paths: bool) -> None: self.files_skipped += 1 return if is_dir: - self._walk(path) + # Walked in resolved form. Both ancestor walks key off `relative_to(root)`, which + # keeps a `..` as a literal part, so `src/../tests` would enumerate `src` as an + # ancestor of `tests` and apply its `.gitignore` — skipping files that `check tests` + # selects. The resolved directory is the one whose ignore-file ancestors actually + # govern it, and it is already the containment-checked path. This is the rule + # `per-file-ignores` documents (`docs/configuration.md`): match the resolved + # location, not the spelling used to reach it. + # + # A discovery root reached from `include` or an explicit argument is the one + # directory `_traversable_dirs` never sees, because `_walk` starts *inside* it. Left + # unchecked, an ignored root's files were only excluded when the patterns happened + # to match the files as well — so `["src/", "!*.py"]` re-included every Python file + # under an ignored `src/`, which git never does. Matched against the spec for the + # parent, deliberately excluding this directory's own `.gitignore`, for the same + # reason `_traversable_dirs` does. + if resolved != self.root and ( + self._has_excluded_ancestor(resolved.parent) + or _ignored( + self.root, + resolved, + self.builtin_spec, + self.exclude_spec, + self._combined_gitignore_spec(resolved.parent), + is_dir=True, + ) + ): + self.files_skipped += 1 + return + self._walk(resolved) return if not is_file or path.suffix != ".py": if explicit_paths: @@ -174,7 +419,21 @@ def _consider(self, path: Path, *, explicit_paths: bool) -> None: ) self.files_skipped += 1 return - if _ignored(self.root, path, self.builtin_spec, self.exclude_spec, self.gitignore_spec): + # Resolved, for the same reason the directory branch above is: `relative_to(root)` keeps + # a `..` as a literal part, so `src/../tests/a.py` would walk `src` as an ancestor and + # apply its `.gitignore` to a file under `tests`. The two branches have to agree — + # fixing only one left `check src/../tests` and `check src/../tests/a.py` disagreeing + # with each other as well as with git. `path` stays the reported spelling. + if self._has_excluded_ancestor(resolved.parent) or _ignored( + self.root, + resolved, + self.builtin_spec, + self.exclude_spec, + combined_gitignore_spec + if combined_gitignore_spec is not None + else self._combined_gitignore_spec(resolved.parent), + is_dir=False, + ): self.files_skipped += 1 return if resolved in self.selected: @@ -204,14 +463,190 @@ def onerror(err: OSError) -> None: if self.limit_reached: break current_path = Path(current) - dirs[:] = self._traversable_dirs(current_path, dirs) + combined_spec = self._combined_gitignore_spec(current_path) + dirs[:] = self._traversable_dirs(current_path, dirs, combined_spec) for name in sorted(names): - self._consider(current_path / name, explicit_paths=False) + self._consider( + current_path / name, + explicit_paths=False, + combined_gitignore_spec=combined_spec, + ) if self.limit_reached: break - def _traversable_dirs(self, current_path: Path, dirs: list[str]) -> list[str]: - """Return child directory names with symlinks excluded, recording why each was dropped.""" + def _ancestor_chain(self, directory: Path) -> list[Path]: + """Every directory from the one just below root down to `directory`, root-first. + + Both ancestor walks below need this same sequence, and both need it to stop cleanly for a + `directory` outside root — `relative_to` raises there, and the empty chain is the honest + answer: nothing between the two to check. + """ + try: + relative_parts = directory.relative_to(self.root).parts + except ValueError: + return [] + chain: list[Path] = [] + current = self.root + for part in relative_parts: + current = current / part + chain.append(current) + return chain + + def _has_excluded_ancestor(self, directory: Path) -> bool: + """Whether any directory between root and `directory` (inclusive) is itself excluded by + `builtin_spec` or `exclude_spec`. + + The gitignore side of this already exists, inside `_combined_gitignore_spec`, for exactly + the reason spelled out there: git attributes the exclusion to the directory, so a + negation can never re-include a file whose parent directory is excluded. Configured + `exclude` accepts the same Git-ignore syntax, negations included, and `docs/configuration.md` + documents it as following git's semantics — but it is a static root-anchored spec matched + only against the path in hand, so it had no equivalent. + + The gap showed up only on explicit paths. With `exclude = ["src/generated/", + "!src/generated/foo.py"]` a normal walk prunes `generated` at `_traversable_dirs` and + never reaches the negation, but `house-lint check src/generated/foo.py` goes straight to + the file-level match, where the negation is the last matching line and wins — so the same + file was skipped by a full scan and linted when named. Built-in excludes are folded in + here too: they are directory patterns of the same shape (`.git/`, `.venv/`), and a + configured negation must not resurrect a file out of one either. + + Cached per directory, like the combined gitignore spec, so the walk pays O(depth) once + per directory rather than once per file. + """ + if directory in self.excluded_ancestor_cache: + return self.excluded_ancestor_cache[directory] + excluded = any( + _ignored(self.root, ancestor, self.builtin_spec, self.exclude_spec, is_dir=True) + for ancestor in self._ancestor_chain(directory) + ) + self.excluded_ancestor_cache[directory] = excluded + return excluded + + def _combined_gitignore_spec(self, directory: Path) -> GitIgnoreSpec: + """Root-anchored spec combining the root `.gitignore` with every nested `.gitignore` + between root and `directory`, ordered root-to-leaf (least to most specific) so + `GitIgnoreSpec`'s own last-matching-line-wins semantics reproduce git's + closest-directory-and-latest-line-wins precedence — including cross-level negation. + + Rebuilds the ancestor chain on every call rather than maintaining a push/pop stack synced + to `os.walk`'s traversal order: `os.walk` backtracks between sibling subtrees with no + explicit "leaving a directory" signal, so a manual stack would need the same + relative-path bookkeeping this does anyway. `_own_gitignore_lines` memoizes each + directory's own `.gitignore` read, and the combined spec itself is cached per directory, + so the rebuild only repeats cheap dict lookups, not I/O or reparsing. A sibling + directory with no `.gitignore` of its own accumulates the exact same line tuple as its + parent, so `spec_by_lines_cache` is keyed on the accumulated lines themselves (not the + directory) to skip `GitIgnoreSpec.from_lines` entirely on that repeat, while + `combined_gitignore_spec_cache` still keeps the per-directory lookup itself O(1). + + Checks each ancestor against the lines accumulated from *its* ancestors before reading + that ancestor's own `.gitignore` and folding its patterns in. Real git never reads ignore + files inside a directory it doesn't descend into, so once an ancestor is already excluded, + a nested negation further down must not be allowed to resurrect it. Normal tree walks + never hit this — `_traversable_dirs` already prunes an ignored directory before this + method is ever called for anything beneath it — but an *explicit* path (`house-lint check + src/ignored/foo.py`) reaches straight in here without going through that walk-time pruning. + + An excluded ancestor therefore returns a match-everything spec rather than the patterns + accumulated so far. Merely stopping the walk is not enough: the accumulated lines can + themselves contain the resurrecting negation, since git allows `src/generated/` and + `!src/generated/foo.py` to sit in the *same* file. Returning those lines would let the + negation win for an explicit `src/generated/foo.py`, which git reports as ignored — it + attributes the exclusion to the directory, and a negation can never re-include a file + whose parent directory is excluded. + """ + if directory in self.combined_gitignore_spec_cache: + return self.combined_gitignore_spec_cache[directory] + if not self.use_gitignore: + # Short-circuit before any nested `.gitignore` is even read, not just before the + # result is used — `--no-gitignore` should skip that filesystem I/O entirely. + spec = GitIgnoreSpec.from_lines(()) + self.combined_gitignore_spec_cache[directory] = spec + return spec + lines: list[str] = list(self.root_gitignore_lines) + for current in self._ancestor_chain(directory): + if lines and _ignored( + self.root, + current, + self._spec_for_lines(tuple(lines), current), + is_dir=True, + ): + excluded = self._spec_for_lines(IGNORE_EVERYTHING, directory) + self.combined_gitignore_spec_cache[directory] = excluded + return excluded + prefix = "/".join( + _escape_gitignore_literal(segment) + for segment in current.relative_to(self.root).parts + ) + lines.extend( + _prefix_pattern(prefix, line) for line in self._own_gitignore_lines(current) + ) + spec = self._spec_for_lines(tuple(lines), directory) + self.combined_gitignore_spec_cache[directory] = spec + return spec + + def _spec_for_lines(self, lines: tuple[str, ...], directory: Path) -> GitIgnoreSpec: + """Build (or reuse) the `GitIgnoreSpec` for an accumulated line tuple. + + `directory` is used only for error attribution if the lines fail to parse; it is not + part of the cache key, since two directories that accumulate the same lines (e.g. a + directory with no `.gitignore` of its own repeating its parent's accumulated lines) share + one parsed spec. + """ + cached_spec = self.spec_by_lines_cache.get(lines) + if cached_spec is not None: + return cached_spec + try: + spec = GitIgnoreSpec.from_lines(_normalize_contents_glob(line) for line in lines) + except (TypeError, ValueError, re.error) as exc: + # Each source's own lines are already validated in `_load_gitignore_lines`, but + # `_prefix_pattern`'s rewrite of them is not independently re-validated — a valid + # original line could in principle become invalid once prefixed, so this stays live. + # + # Reported once per (lines, directory) pair rather than once per call. A failing + # ancestor's lines are re-walked by `_combined_gitignore_spec` for every directory + # beneath it, so without this the same failure is appended once per descendant — + # hundreds of identical entries in a large tree, all attributed to the one ancestor. + # Keyed on the pair, not the lines alone, so a second directory whose accumulated + # lines fail the same way is still reported against its own path. + if (lines, directory) not in self.reported_spec_failures: + self.reported_spec_failures.add((lines, directory)) + self.errors.append(self._error(directory, "traversal", "combine", str(exc))) + return GitIgnoreSpec.from_lines(()) + self.spec_by_lines_cache[lines] = spec + return spec + + def _own_gitignore_lines(self, directory: Path) -> tuple[str, ...]: + if directory in self.own_gitignore_lines_cache: + return self.own_gitignore_lines_cache[directory] + ignore = directory / ".gitignore" + + def on_error(operation: str, message: str) -> None: + self.errors.append(self._error(ignore, "traversal", operation, message)) + + lines = _load_gitignore_lines(ignore, on_error) + self.own_gitignore_lines_cache[directory] = lines + return lines + + def _traversable_dirs( + self, current_path: Path, dirs: list[str], combined_gitignore_spec: GitIgnoreSpec + ) -> list[str]: + """Return child directory names to descend into, recording why each was dropped. + + Drops symlinked directories (never traversed) and directories already excluded by + `combined_gitignore_spec` — the spec accumulated from root down to `current_path` + (the parent), deliberately *not* including the child's own, not-yet-read + `.gitignore`. Checking a child against its own nested `.gitignore` before deciding + whether to descend into it would let a negation inside that file "resurrect" files + that should stay excluded because the directory itself is ignored — real git never + reads ignore files inside a directory it never descends into. Skipping the + directory here means `_own_gitignore_lines`/`_combined_gitignore_spec` are simply + never called for it, so its nested `.gitignore` (if any) is never read at all. + `combined_gitignore_spec` already folds in `use_gitignore` (it's an empty spec when + disabled), and `builtin_spec`/`exclude_spec` inside `_ignored` apply unconditionally, + matching how file-level ignoring already treats those two specs. + """ kept: list[str] = [] for item in sorted(dirs): child = current_path / item @@ -225,6 +660,16 @@ def _traversable_dirs(self, current_path: Path, dirs: list[str]) -> list[str]: self._error(child, "traversal", "walk", "directory symlink is not traversed") ) continue + if _ignored( + self.root, + child, + self.builtin_spec, + self.exclude_spec, + combined_gitignore_spec, + is_dir=True, + ): + self.files_skipped += 1 + continue kept.append(item) return kept @@ -266,7 +711,7 @@ def discover_files( ) -> DiscoveryResult: """Discover qualifying files, or raise for strict explicit path failures.""" root = root.expanduser().resolve() - builtin_spec, exclude_spec, gitignore_spec, pattern_errors = _patterns( + builtin_spec, exclude_spec, root_gitignore_lines, pattern_errors = _patterns( root, excludes, use_gitignore ) requested = explicit or tuple(root / item for item in include) @@ -274,8 +719,9 @@ def discover_files( root, builtin_spec, exclude_spec, - gitignore_spec, + root_gitignore_lines, list(pattern_errors), + use_gitignore=use_gitignore, ) selector.select(requested, explicit_paths=bool(explicit)) return selector.result() diff --git a/src/house_lint/scanner.py b/src/house_lint/scanner.py index 60cd612..5b1e423 100644 --- a/src/house_lint/scanner.py +++ b/src/house_lint/scanner.py @@ -32,42 +32,69 @@ class FileScanResult: stop: bool = False -def scan_file( - path: Path, +def open_source( + path: Path, *, root: Path, resolved_path: Path | None = None, debug: bool +) -> SourceFile | FileScanResult: + """Construct a `SourceFile` and perform its one read of the file's bytes. + + Split out from `scan_source` so the caller can compute a cache key from the bytes this read + produced and skip scanning on a hit, without any second read of the path. + + `resolved_path` carries discovery's `resolve()` result forward so a symlink is resolved once + for the whole pipeline; see `SourceFile.__init__`. + + A `FileScanResult` comes back only for a process-boundary failure (`stop=True`). Ordinary + source errors — path escape, non-regular file, oversize, undecodable — stay on the returned + `SourceFile`, which `scan_source` turns into findings-level errors. + """ + source: SourceFile | None = None + try: + source = SourceFile(path, root, resolved_path=resolved_path) + source.load() + return source + except Exception: # noqa: BLE001 - this is the process-boundary internal-error path. + error_path = source.relative_path if source is not None else _fallback_path(path, root) + if debug: + traceback.print_exc(file=sys.stderr) + return FileScanResult( + errors=(internal_error("analysis", "source-load", path=error_path),), stop=True + ) + + +def _fallback_path(path: Path, root: Path) -> str: + """Best-effort reporting path for a file whose `SourceFile` never finished constructing.""" + try: + return path.absolute().relative_to(root.absolute()).as_posix() + except ValueError: + return path.name + + +def scan_source( + source: SourceFile, *, - root: Path, enabled_rules: tuple[str, ...], detector_inputs: tuple[DetectorInput, ...], debug: bool, ) -> FileScanResult: - """Scan one selected file after resolving source-load failures.""" - source = _load_source(path, root=root, debug=debug) - if isinstance(source, FileScanResult): - return source - return _scan_ready_source( - source, enabled_rules=enabled_rules, detector_inputs=detector_inputs, debug=debug - ) - - -def _load_source(path: Path, *, root: Path, debug: bool) -> SourceFile | FileScanResult: - """Load a source file or convert source-load failures into a scan result.""" - source: SourceFile | None = None + """Scan an already-loaded source after resolving source-load failures.""" try: - source = SourceFile(path, root) if source.error is not None: if debug and source.debug_exception is not None: traceback.print_exception(source.debug_exception, file=sys.stderr) return FileScanResult(errors=(source.error,)) - return source except Exception: # noqa: BLE001 - this is the process-boundary internal-error path. - error_path = ( - source.relative_path if source is not None else path.relative_to(root).as_posix() - ) if debug: traceback.print_exc(file=sys.stderr) return FileScanResult( - errors=(internal_error("analysis", "source-load", path=error_path),), stop=True + # "source-analyze", not "source-load": `open_source` already completed the read and + # owns the "source-load" label. Reaching `source.error` runs `_analyze()`, so a + # failure here is tokenize/parse, not I/O. + errors=(internal_error("analysis", "source-analyze", path=source.relative_path),), + stop=True, ) + return _scan_ready_source( + source, enabled_rules=enabled_rules, detector_inputs=detector_inputs, debug=debug + ) def _scan_ready_source( diff --git a/src/house_lint/source.py b/src/house_lint/source.py index 71d2280..2354bdb 100644 --- a/src/house_lint/source.py +++ b/src/house_lint/source.py @@ -15,17 +15,52 @@ Token: TypeAlias = tokenize.TokenInfo +def read_regular_file_bytes(path: Path, *, max_bytes: int) -> bytes | None: + """Read up to `max_bytes` + 1 from a regular file via a nonblocking descriptor. + + The nonblocking descriptor prevents a raced FIFO from stalling the read. Returns None if + the path isn't a regular file; raises OSError for other failures (missing file, permission + denied, etc.) so callers can decide how to report them — `SourceFile` turns both cases into + a `LintError`, while cache-key hashing just treats either as an uncacheable file. + + `O_NOFOLLOW` narrows the window between discovery resolving a path and the scan opening it. + Callers pass an already-fully-resolved path, so its final component is by construction not a + symlink — unless it was replaced with one after discovery approved it, which is exactly the + case that must not be read. Refusing to follow it turns that race into an ordinary read + error rather than an out-of-root read. This does not close the window on the *directory* + components of the path, which would need an `openat`-based descent from the root. + """ + descriptor = os.open( + path, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0) | getattr(os, "O_NOFOLLOW", 0) + ) + with os.fdopen(descriptor, "rb") as handle: + if not stat.S_ISREG(os.fstat(handle.fileno()).st_mode): + return None + return handle.read(max_bytes + 1) + + class SourceFile: - """Load one Python file, failing closed before any rule can inspect it.""" + """Load one Python file, failing closed before any rule can inspect it. + + This is the only place a scanned file's bytes are read. `content_bytes` exposes that single + buffer so the cache key can be derived from exactly the content the detectors analyze — see + `cli._scan`. + """ - def __init__(self, path: Path, root: Path) -> None: + def __init__(self, path: Path, root: Path, *, resolved_path: Path | None = None) -> None: + # `resolved_path` is discovery's own `resolve()` result, threaded through rather than + # recomputed. Resolving a second time here would reopen a window in which a symlink + # retargeted after the containment check sends the read somewhere discovery never + # approved. Containment is still re-checked below against whichever path is used, so + # passing one in cannot widen what this class will read. self.path = path.absolute() - self.resolved_path = path.resolve() + self.resolved_path = path.resolve() if resolved_path is None else resolved_path self.root = root.resolve() self._error: LintError | None = None self._debug_exception: BaseException | None = None self._loaded = False self._analyzed = False + self._source_bytes: bytes | None = None self._text: str | None = None self._lines: list[str] | None = None self._tokens: tuple[Token, ...] = () @@ -49,21 +84,20 @@ def __init__(self, path: Path, root: Path) -> None: ) return - def _load(self) -> None: + def load(self) -> None: + """Read this file's bytes, once. Idempotent; failures become `error`, never exceptions.""" if self._loaded or self._error is not None: self._loaded = True return self._loaded = True try: - # A nonblocking descriptor prevents a raced FIFO from stalling the scan. - descriptor = os.open(self.resolved_path, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0)) - with os.fdopen(descriptor, "rb") as handle: - if not stat.S_ISREG(os.fstat(handle.fileno()).st_mode): - self._error = self._make_error( - "path-error", "path", "source-check", "regular-file", "not a regular file" - ) - return - source_bytes = handle.read(MAX_SOURCE_BYTES + 1) + source_bytes = read_regular_file_bytes(self.resolved_path, max_bytes=MAX_SOURCE_BYTES) + self._source_bytes = source_bytes + if source_bytes is None: + self._error = self._make_error( + "path-error", "path", "source-check", "regular-file", "not a regular file" + ) + return if len(source_bytes) > MAX_SOURCE_BYTES: self._error = self._make_error( "source-too-large", @@ -108,7 +142,7 @@ def _load(self) -> None: def _analyze(self) -> None: if self._analyzed: return - self._load() + self.load() if self._error is not None: self._analyzed = True return @@ -173,6 +207,17 @@ def _make_error( message, ) + @property + def content_bytes(self) -> bytes | None: + """The raw bytes this file was read as, or None if it could not be read at all. + + None covers both "not a regular file" and a failed read; an oversized or undecodable + file still reports the bytes that were read. Callers that need a cache key apply their + own cacheability policy to this (`cache.hash_source_content`) rather than re-reading. + """ + self.load() + return self._source_bytes + @property def error(self) -> LintError | None: self._analyze() @@ -258,4 +303,4 @@ def _node_key(node: ast.stmt) -> tuple[int, int, int, int]: ) -__all__ = ["MAX_SOURCE_BYTES", "SourceFile"] +__all__ = ["MAX_SOURCE_BYTES", "SourceFile", "read_regular_file_bytes"] diff --git a/tests/integration/_git_harness.py b/tests/integration/_git_harness.py new file mode 100644 index 0000000..e9d1d42 --- /dev/null +++ b/tests/integration/_git_harness.py @@ -0,0 +1,90 @@ +"""Shared harness for the two differential tests that compare discovery against real git. + +`test_gitignore_parity.py` (curated table) and `test_gitignore_fuzz.py` (randomized) both need a +throwaway repository and a way to ask `git check-ignore` what it would skip. Those two suites +exist to catch house-lint drifting from git; keeping one copy of the harness stops the harness +itself from drifting between them — the same failure mode one level up. + +Not a `conftest.py`: these are plain helpers called from module-level functions, not fixtures +injected into test signatures. `tests/integration/` has no `__init__.py`, so pytest's default +prepend import mode puts it on `sys.path` and both modules can import this one by name. +""" + +import os +import subprocess +from pathlib import Path + +import pytest + +# A git call that hangs (a credential helper waiting on stdin, a pager, a corrupt config) would +# otherwise be bounded only by CI's job-level timeout, which kills the whole matrix leg without +# saying which test stalled. +GIT_TIMEOUT_SECONDS = 30 + + +# Variables that override `cwd` when locating the repository. An inherited value would point +# `git init` and `git check-ignore` at a different repository than the one built for the +# scenario, so the comparison would measure the wrong tree and still exit 0. +_REPOSITORY_POINTING_VARIABLES = ("GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE") + + +def git_env() -> dict[str, str]: + """Neutralise every ignore source outside the repository under test. + + Dropping the repository-pointing variables matters as much as the config ones: passing + `cwd=root` is not enough on its own, since any of the three override it. These suites exist + to catch house-lint drifting from git, so a harness that can silently compare against + somebody else's repository defeats the only thing they are for. + """ + inherited = { + key: value for key, value in os.environ.items() if key not in _REPOSITORY_POINTING_VARIABLES + } + return inherited | { + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_SYSTEM": os.devnull, + "HOME": os.devnull, + } + + +def init_repository(root: Path) -> None: + """Create a repository at `root` with every out-of-tree ignore source disabled.""" + for command in (["git", "init", "-q", "."], ["git", "config", "core.excludesFile", ""]): + # Not `check=True`: `CalledProcessError`'s message carries only the command and exit + # status, and pytest never prints the captured stderr hanging off the exception. A CI + # failure here would read "returned non-zero exit status 128" and say nothing about why. + completed = subprocess.run( + command, + cwd=root, + check=False, + capture_output=True, + text=True, + env=git_env(), + timeout=GIT_TIMEOUT_SECONDS, + ) + if completed.returncode != 0: + pytest.fail(f"{' '.join(command)} failed: {completed.stderr}") + + +def git_ignored(root: Path, relatives: tuple[str, ...]) -> set[str]: + """Return which of `relatives` git itself would ignore. + + NUL-separated (`-z`) in both directions. Without it git applies its C-style quoting to any + path it considers unusual — a filename containing a backslash comes back as + `"src/dir\\\\/b.py"`, quotes and doubled escapes included — and the comparison then fails on + the encoding rather than on the ignore decision. `-z` turns quoting off entirely, so + newline-free paths (which every scenario uses) round-trip byte for byte. + """ + completed = subprocess.run( + ["git", "check-ignore", "-z", "--stdin"], + cwd=root, + input="\0".join(relatives), + capture_output=True, + text=True, + env=git_env(), + check=False, + timeout=GIT_TIMEOUT_SECONDS, + ) + # `check-ignore` exits 1 when nothing matches, which is not a failure for us. + if completed.returncode not in (0, 1): + pytest.fail(f"git check-ignore failed: {completed.stderr}") + return {entry for entry in completed.stdout.split("\0") if entry} diff --git a/tests/integration/test_cli.py b/tests/integration/test_cli.py index dd88349..898cd41 100644 --- a/tests/integration/test_cli.py +++ b/tests/integration/test_cli.py @@ -8,12 +8,18 @@ import pytest from house_lint import cli, scanner +from house_lint import source as source_module from house_lint.analysis import MAX_CANDIDATES_PER_FILE def _run( - root: Path, *args: str, module: bool = False, prelude: str | None = None + root: Path, + *args: str, + module: bool = False, + prelude: str | None = None, + pythonpath: Path | None = None, ) -> subprocess.CompletedProcess[str]: + """Run house-lint in a subprocess against this checkout, or against `pythonpath` instead.""" command = ( [sys.executable, "-c", prelude] if prelude is not None @@ -21,7 +27,8 @@ def _run( if module else [str(shutil.which("house-lint"))] ) - environment = os.environ | {"PYTHONPATH": str(Path(__file__).parents[2] / "src")} + source = pythonpath if pythonpath is not None else Path(__file__).parents[2] / "src" + environment = os.environ | {"PYTHONPATH": str(source)} return subprocess.run( command + list(args), cwd=root, env=environment, text=True, capture_output=True, check=False ) @@ -119,9 +126,495 @@ def test_check_selects_repeatable_comma_separated_rule_ids(repository: Path) -> assert [finding["rule_id"] for finding in result["findings"]] == ["HSL002"] +def test_cli_extend_select_adds_a_rule_without_dropping_configured_select( + repository: Path, +) -> None: + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + (repository / "pyproject.toml").write_text('[tool.house-lint]\nselect = ["HSL002"]\n') + + completed = _run( + repository, + "check", + "--root", + str(repository), + "--format", + "json", + "--extend-select", + "HSL001", + ) + + result = json.loads(completed.stdout) + assert completed.returncode == 1 + assert result["enabled_rules"] == ["HSL001", "HSL002", "HSL900"] + + +def test_cli_extend_ignore_subtracts_from_extend_select(repository: Path) -> None: + (repository / "pyproject.toml").write_text('[tool.house-lint]\nselect = ["HSL001"]\n') + + completed = _run( + repository, + "check", + "--root", + str(repository), + "--format", + "json", + "--extend-select", + "HSL002", + "--extend-ignore", + "HSL002", + ) + + result = json.loads(completed.stdout) + assert completed.returncode == 0 + assert result["enabled_rules"] == ["HSL001", "HSL900"] + + +def test_per_file_ignores_silences_a_rule_only_for_matching_files(repository: Path) -> None: + (repository / "tests").mkdir() + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + (repository / "tests" / "test_finding.py").write_text("def example():\n import module\n") + (repository / "pyproject.toml").write_text( + '[tool.house-lint]\nselect = ["HSL002"]\n' + '[tool.house-lint.per-file-ignores]\n"tests/**" = ["HSL002"]\n' + ) + + completed = _run(repository, "check", "--root", str(repository), "--format", "json") + + result = json.loads(completed.stdout) + assert completed.returncode == 1 + assert result["enabled_rules"] == ["HSL002", "HSL900"] + assert [finding["path"] for finding in result["findings"]] == ["src/finding.py"] + + +def test_per_file_ignores_match_a_path_spelled_with_parent_traversal(repository: Path) -> None: + """An explicit path keeps the spelling the user typed all the way to pattern matching, so + `src/../tests/x.py` was matched literally and `"tests/**"` missed it — running a rule the + config disabled for everything under `tests/`.""" + (repository / "tests").mkdir() + (repository / "tests" / "test_finding.py").write_text("def example():\n import module\n") + (repository / "pyproject.toml").write_text( + '[tool.house-lint]\nselect = ["HSL002"]\n' + '[tool.house-lint.per-file-ignores]\n"tests/**" = ["HSL002"]\n' + ) + + completed = _run( + repository, + "check", + "--root", + str(repository), + "--format", + "json", + "src/../tests/test_finding.py", + ) + + result = json.loads(completed.stdout) + assert completed.returncode == 0 + assert result["findings"] == [] + + +def test_per_file_ignores_follow_a_symlinked_component_rather_than_the_spelling( + repository: Path, +) -> None: + """A lexical `..` collapse is only correct when nothing traversed is a symlink. With + `link/ -> src/nested/`, the OS reads `link/../finding.py` as `src/finding.py` while the + lexical form reads `finding.py` — so a pattern written for the file's real location would + stop matching the file house-lint actually opens.""" + (repository / "src" / "nested").mkdir(parents=True, exist_ok=True) + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + (repository / "link").symlink_to(repository / "src" / "nested") + (repository / "pyproject.toml").write_text( + '[tool.house-lint]\nselect = ["HSL002"]\n' + '[tool.house-lint.per-file-ignores]\n"src/**" = ["HSL002"]\n' + ) + + completed = _run( + repository, "check", "--root", str(repository), "--format", "json", "link/../finding.py" + ) + + result = json.loads(completed.stdout) + assert completed.returncode == 0 + assert result["findings"] == [] + + +def test_per_file_ignores_flags_a_pragma_naming_a_rule_disabled_for_that_file( + repository: Path, +) -> None: + (repository / "tests").mkdir() + (repository / "tests" / "test_finding.py").write_text( + "def example():\n import module # house-lint: ignore[HSL002] - stale suppression\n" + ) + (repository / "pyproject.toml").write_text( + '[tool.house-lint]\nselect = ["HSL002"]\n' + '[tool.house-lint.per-file-ignores]\n"tests/**" = ["HSL002"]\n' + ) + + completed = _run(repository, "check", "--root", str(repository), "--format", "json") + + result = json.loads(completed.stdout) + assert completed.returncode == 1 + assert [(finding["rule_id"], finding["message"]) for finding in result["findings"]] == [ + ("HSL900", "unused suppression for disabled rule HSL002") + ] + + +def test_cache_is_populated_and_reused_across_runs(repository: Path) -> None: + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + + first = _run( + repository, "check", "--root", str(repository), "--select", "HSL002", "--format", "json" + ) + assert first.returncode == 1 + + # repository also contains a clean src/clean.py with no findings, so it gets its own + # (empty) cache entry — select finding.py's entry specifically by its non-empty content. + entries = list((repository / ".house-lint-cache").rglob("*.json")) + assert entries + entry = next(e for e in entries if json.loads(e.read_text())["findings"]) + + # Poison the cache entry directly (bypassing the real scan) to prove a normal run reads + # it back rather than re-deriving the same result independently. + poisoned = { + "findings": [ + { + "rule_id": "HSL002", + "line": 1, + "column": 1, + "end_line": 1, + "end_column": 2, + "message": "poisoned cache entry", + } + ], + "errors": [], + "suppressed_count": 0, + "files_scanned": 1, + } + entry.write_text(json.dumps(poisoned)) + + second = _run( + repository, "check", "--root", str(repository), "--select", "HSL002", "--format", "json" + ) + second_result = json.loads(second.stdout) + assert [finding["message"] for finding in second_result["findings"]] == ["poisoned cache entry"] + + # --no-cache must ignore the poisoned entry (real scan runs) but still overwrite it + # afterward, keeping the cache warm for the next normal run. + third = _run( + repository, + "check", + "--root", + str(repository), + "--select", + "HSL002", + "--no-cache", + "--format", + "json", + ) + third_result = json.loads(third.stdout) + assert [finding["message"] for finding in third_result["findings"]] == [ + "import inside function body" + ] + assert json.loads(entry.read_text())["findings"][0]["message"] == "import inside function body" + + +def test_cache_dir_flag_overrides_the_default_location(repository: Path) -> None: + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + custom_cache = repository.parent / "custom-cache" + + completed = _run( + repository, + "check", + "--root", + str(repository), + "--select", + "HSL002", + "--cache-dir", + str(custom_cache), + "--format", + "json", + ) + + assert completed.returncode == 1 + assert not (repository / ".house-lint-cache").exists() + assert list(custom_cache.rglob("*.json")) + + +def test_cache_dir_never_writes_a_gitignore_into_the_directory_it_is_given( + repository: Path, +) -> None: + """`--cache-dir` names a directory the user already owns, so house-lint must not drop a + wildcard `.gitignore` into it. Pointed at the project root, that one file would hide the + entire project from `git status`.""" + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + + completed = _run( + repository, + "check", + "--root", + str(repository), + "--select", + "HSL002", + "--cache-dir", + str(repository), + "--format", + "json", + ) + + assert completed.returncode == 1 + assert not (repository / ".gitignore").exists() + assert (repository / "src" / "finding.py").exists() + + +def test_cache_is_invalidated_when_rule_code_changes_without_a_version_bump( + repository: Path, tmp_path: Path +) -> None: + """`__version__` only moves at release time, so it cannot be the sole invalidation signal: + editing a detector in a working checkout and re-running would otherwise replay the previous + detector's findings for every file whose content and config are unchanged.""" + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + package_source = Path(__file__).parents[2] / "src" + patched_source = tmp_path / "patched-src" + shutil.copytree(package_source, patched_source) + + def namespaces() -> set[str]: + return { + entry.name for entry in (repository / ".house-lint-cache").iterdir() if entry.is_dir() + } + + arguments = ("check", "--root", str(repository), "--select", "HSL002", "--format", "json") + first = _run(repository, *arguments) + assert first.returncode == 1 + assert json.loads(first.stdout)["findings"] + first_namespaces = namespaces() + assert len(first_namespaces) == 1 + + detector = patched_source / "house_lint" / "rules" / "lazy_imports.py" + original = detector.read_text() + assert "def detect(" in original + detector.write_text(original.replace("def detect(", "def detect( # patched\n", 1)) + + second = _run(repository, *arguments, pythonpath=patched_source) + + # The edit is behaviour-preserving, so the findings must match — what must differ is the + # cache namespace, proving the second run could not have replayed the first's entry. The + # superseded namespace is pruned rather than left to accumulate, so exactly one remains. + assert second.returncode == 1 + assert json.loads(second.stdout)["findings"] == json.loads(first.stdout)["findings"] + second_namespaces = namespaces() + assert len(second_namespaces) == 1 + assert second_namespaces != first_namespaces + + +def test_a_corrupted_cache_field_degrades_to_a_miss_instead_of_crashing(repository: Path) -> None: + """Rendering happens outside `check()`'s exception boundary, so a cached value that only + breaks at sort time (`int < str` in `ScanResult.to_dict()`) surfaced as a traceback with no + output at all, rather than the documented cache miss.""" + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + arguments = ("check", "--root", str(repository), "--select", "HSL002", "--format", "json") + first = _run(repository, *arguments) + assert first.returncode == 1 + expected = json.loads(first.stdout)["findings"] + + entry = next( + candidate + for candidate in (repository / ".house-lint-cache").rglob("*.json") + if json.loads(candidate.read_text())["findings"] + ) + payload = json.loads(entry.read_text()) + poisoned = dict(payload["findings"][0]) + poisoned["message"] = 12345 + payload["findings"] = [payload["findings"][0], poisoned] + entry.write_text(json.dumps(payload)) + + second = _run(repository, *arguments) + + payload = json.loads(second.stdout) + assert second.returncode == 1 + assert "Traceback" not in second.stderr + assert payload["errors"] == [], "a corrupted cache entry must never become a scan error" + # Re-scanned from source, so the real finding comes back and the poison is discarded. + assert payload["findings"] == expected + + +def test_a_symlinked_default_cache_directory_disables_caching( + repository: Path, tmp_path_factory: pytest.TempPathFactory +) -> None: + """A cloned repository controls the path `/.house-lint-cache` resolves to. Pointed at a + directory outside the checkout, `mkdir(parents=True, exist_ok=True)` follows the link and + house-lint's version marker, entries, and wildcard `.gitignore` all land there. The scan must + still succeed, and must leave the linked directory exactly as it found it.""" + outside = tmp_path_factory.mktemp("outside") + (repository / ".house-lint-cache").symlink_to(outside, target_is_directory=True) + + result = _run(repository, "check", "--root", str(repository), "--format", "json") + + assert result.returncode == 0 + assert "Traceback" not in result.stderr + assert "caching disabled" in result.stderr + assert json.loads(result.stdout)["errors"] == [] + assert list(outside.iterdir()) == [] + + +def test_a_symlinked_cache_dir_override_is_still_honoured( + repository: Path, tmp_path_factory: pytest.TempPathFactory +) -> None: + """The symlink refusal above is scoped to the default base. `--cache-dir` names a directory + the user chose, so house-lint neither self-ignores it nor second-guesses how they linked it.""" + target = tmp_path_factory.mktemp("target") + link = tmp_path_factory.mktemp("links") / "cache" + link.symlink_to(target, target_is_directory=True) + + result = _run( + repository, "check", "--root", str(repository), "--cache-dir", str(link), "--format", "json" + ) + + assert result.returncode == 0 + assert result.stderr == "" + assert list(target.iterdir()) != [] + assert not (target / ".gitignore").exists() + + +def test_a_run_of_pure_cache_hits_does_not_prune_another_namespace(repository: Path) -> None: + """Pruning is not race-free: a concurrent house-lint on a different version or build sharing + a `--cache-dir` can have its in-progress namespace deleted. Tying the sweep to "this run + actually wrote an entry" keeps that window narrow — a scan that writes nothing must not + delete anything.""" + arguments = ("check", "--root", str(repository), "--select", "HSL002", "--format", "json") + assert _run(repository, *arguments).returncode == 0 + + foreign = repository / ".house-lint-cache" / "9.9.9-ffffffffffffffff" + foreign.mkdir(parents=True) + (foreign / ".house-lint-version").write_text("") + (foreign / "in-progress.json").write_text("{}") + + second = _run(repository, *arguments) + + assert second.returncode == 0 + assert json.loads(second.stdout)["files_scanned"] == 1 + assert foreign.exists(), "a zero-write run must not prune another namespace" + + # A run that does write an entry still sweeps it, so namespaces cannot accumulate. + (repository / "src" / "new.py").write_text("value = 2\n") + assert _run(repository, *arguments).returncode == 0 + assert not foreign.exists() + + +def test_an_unusable_cache_dir_warns_once_without_debug_and_still_scans(repository: Path) -> None: + """The runs this tool is built for — CI, pre-commit — never pass `--debug`. Without a + default-visible line, an unwritable cache directory means every scan silently pays the full + re-analysis cost with nothing to explain why. The scan itself must be unaffected.""" + blocked = repository / "blocked" + blocked.write_text("not a directory") + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + + result = _run( + repository, + "check", + "--root", + str(repository), + "--select", + "HSL002", + "--cache-dir", + str(blocked / "cache"), + "--format", + "json", + ) + + assert result.returncode == 1, result.stderr + warnings = [line for line in result.stderr.splitlines() if line.startswith("warning: ")] + assert len(warnings) == 1, result.stderr + assert "cache" in warnings[0] + payload = json.loads(result.stdout) + assert [finding["rule_id"] for finding in payload["findings"]] == ["HSL002"] + assert payload["errors"] == [], "a cache failure must never become a scan error" + + +def test_cache_hit_never_scans_the_source( + repository: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + + first_code = cli.check(root=repository, select=["HSL002"], format="json") + first_output = capsys.readouterr().out + assert first_code == 1 + + calls: list[object] = [] + + def record_call(*args: object, **kwargs: object) -> None: + calls.append(args) + raise AssertionError("scan_source must not be called on a cache hit") + + monkeypatch.setattr(cli, "scan_source", record_call) + + second_code = cli.check(root=repository, select=["HSL002"], format="json") + second_output = capsys.readouterr().out + + assert calls == [], "scan_source must not be called on a cache hit" + assert second_code == first_code + assert json.loads(second_output) == json.loads(first_output) + + +@pytest.mark.parametrize("cache_state", ["cold", "warm"]) +def test_each_scanned_file_is_read_exactly_once( + repository: Path, monkeypatch: pytest.MonkeyPatch, cache_state: str +) -> None: + # A cache entry is a promise that this exact content produced these exact findings. That + # promise only holds because the cache key and the findings come from the same buffer: the + # file is read once, and everything downstream derives from it. A second read of the same + # path would reopen a TOCTOU window in which an edit landing between reads lets the entry be + # written under a key describing content that was never scanned. Counting reads is what + # guards the structure — the race is unobservable once it is gone, but a regression would + # show up here as a count above one. The counter intercepts `source.read_regular_file_bytes`, + # which is the only file-reading entry point on the scan path (`cli` and `cache` no longer + # import one); a re-read added through some other API would need its own guard. + (repository / "src" / "finding.py").write_text("def example():\n import module\n") + if cache_state == "warm": + assert cli.check(root=repository, select=["HSL002"], format="json") == 1 + + real_read = source_module.read_regular_file_bytes + reads: dict[str, int] = {} + + def counting_read(path: Path, *, max_bytes: int) -> bytes | None: + reads[path.name] = reads.get(path.name, 0) + 1 + return real_read(path, max_bytes=max_bytes) + + monkeypatch.setattr(source_module, "read_regular_file_bytes", counting_read) + + assert cli.check(root=repository, select=["HSL002"], format="json") == 1 + + assert reads, "the scan must have read at least one file" + assert reads["finding.py"] == 1 + assert set(reads.values()) == {1}, f"every file must be read exactly once, got {reads}" + + +def test_cache_does_not_cross_contaminate_hsl101_filename_findings_between_same_content_files( + repository: Path, +) -> None: + # Both files have identical content, so a cache key that ignores the filename would let + # whichever file is scanned first "poison" the entry the other one reads back. + (repository / "src" / "TASK123.py").write_text("x = 1\n") + (repository / "src" / "plain.py").write_text("x = 1\n") + (repository / "pyproject.toml").write_text( + '[tool.house-lint]\nselect = ["HSL101"]\n' + "[[tool.house-lint.rules.HSL101.tokens]]\n" + 'prefixes = ["TASK"]\nscopes = ["filenames"]\n' + ) + + completed = _run(repository, "check", "--root", str(repository), "--format", "json") + + result = json.loads(completed.stdout) + assert completed.returncode == 1 + assert [finding["path"] for finding in result["findings"]] == ["src/TASK123.py"] + + @pytest.mark.parametrize( ("option", "value"), - [("--select", "HSL001,"), ("--select", "HSL001,,HSL002"), ("--ignore", " ")], + [ + ("--select", "HSL001,"), + ("--select", "HSL001,,HSL002"), + ("--ignore", " "), + ("--extend-select", "HSL001,"), + ("--extend-ignore", " "), + ], ) def test_empty_cli_rule_id_elements_are_usage_errors( repository: Path, option: str, value: str @@ -202,6 +695,26 @@ def test_debug_operational_details_stay_on_stderr_for_json_output(repository: Pa assert "def broken()" in completed.stderr +def test_debug_tracebacks_survive_a_warm_cache(repository: Path) -> None: + """`--debug` output must not depend on whether a previous run cached the error. + + The traceback is printed by `scan_source`, which a cache hit skips entirely — so the first + `--debug` run showed the exception type and the offending source, and every identical run + after it showed only the one-line structured error. Someone reaching for `--debug` to + diagnose a parse failure would get less information the second time they asked, with nothing + on screen to explain why. + """ + (repository / "src" / "broken.py").write_text("def broken()\n pass\n") + + cold = _run(repository, "check", "--root", str(repository), "--debug") + warm = _run(repository, "check", "--root", str(repository), "--debug") + + for completed in (cold, warm): + assert completed.returncode == 3 + assert "SyntaxError:" in completed.stderr + assert "def broken()" in completed.stderr + + def test_invalid_check_format_writes_only_a_usage_diagnostic_to_stderr(repository: Path) -> None: completed = _run(repository, "check", "--root", str(repository), "--format", "xml") @@ -631,7 +1144,7 @@ def test_source_construction_failure_preserves_completed_results( second.write_text("value = 1\n") source_file = scanner.SourceFile - def fail_second(path: Path, root: Path) -> scanner.SourceFile: + def fail_second(path: Path, root: Path, **kwargs: object) -> scanner.SourceFile: if path == second: raise RuntimeError("simulated construction failure") return source_file(path, root) diff --git a/tests/integration/test_gitignore_fuzz.py b/tests/integration/test_gitignore_fuzz.py new file mode 100644 index 0000000..c2e3e18 --- /dev/null +++ b/tests/integration/test_gitignore_fuzz.py @@ -0,0 +1,308 @@ +"""Randomized differential parity between house-lint's discovery and real `git check-ignore`. + +`test_gitignore_parity.py` pins a curated table of pattern shapes someone thought to write down. +This generates combinations nobody thought of, and measures how often they disagree with git. + +Two things come out of it. The divergence *direction* is the hard guarantee, asserted for every +distribution below: a disagreement may leave house-lint linting a file git would ignore, but must +never leave it skipping a file git would lint. Over-linting is visible and fixable in one +`exclude` line; under-linting is silent, and a linter that silently skips a file has failed at its +only job. The divergence *rate* is the softer one — a tripwire under a documented ceiling, so a +`pathspec` bump or an edit to `_prefix_pattern`/`_normalize_contents_glob` shows up as a failure +rather than as drift nobody measured. + +The rate is meaningless without the distribution that produced it, which is why three are +declared rather than one. `docs/configuration.md` quotes these; regenerate with +`CI=1 uv run pytest -s tests/integration/test_gitignore_fuzz.py` and update both together. The +no-negation case is the load-bearing one: the known divergence cannot occur without a negation, +and that is what pins it. + +Thousands of real `git check-ignore` calls is worth the wait in CI but not on every local +`pytest`, so this runs when `CI` is set and skips otherwise — no marker to select and no flag to +remember, in either direction. Every CI provider sets `CI`, GitHub Actions included, so the +workflow needs no configuration for this and cannot silently stop running it by drifting out of +sync with a flag. Locally, prefix any invocation with `CI=1`. + +One repository is initialised per distribution and its `.gitignore` files are rewritten per trial +— `git check-ignore` needs a repository, not a commit, so per-trial `git init` would be overhead. +""" + +import os +import random +import shutil +from dataclasses import dataclass +from pathlib import Path + +import pytest +from _git_harness import git_ignored, init_repository + +from house_lint.discovery import discover_files + +pytestmark = [ + pytest.mark.skipif( + not os.environ.get("CI"), reason="randomized suite; set CI=1 to run it locally" + ), + pytest.mark.skipif(shutil.which("git") is None, reason="git is not installed"), +] + +TRIALS = 1500 +# Fixed so a reported rate is reproducible and a regression is bisectable. Change it only to +# widen coverage deliberately, never to make a failing run pass. +SEED = 20260820 +# How many under-linting divergences the known directory-negation defect is allowed to account +# for in the adversarial distribution. A ceiling rather than a blanket exemption: the class is +# tolerated because `pathspec` cannot currently decide it (see +# `_is_known_directory_negation_defect`), but it must not silently grow. Regenerate alongside the +# rates in `docs/configuration.md`. +MAX_KNOWN_DIRECTORY_NEGATION_DIVERGENCES = 1 + +# A tree deep and wide enough that anchoring, directory-only patterns, and negation inside a +# nested directory all have somewhere to bite, but small enough that a trial stays cheap. +TREE = ( + "src/a.py", + "src/b.py", + "src/sub/a.py", + "src/sub/b.py", + "src/sub/deep/a.py", + "src/other/a.py", + "src/other/deep/b.py", +) +IGNORE_OWNERS = ("", "src", "src/sub") + +# Plain names and globs, the shape an actual project's .gitignore is made of. +ORDINARY_BODIES = ( + "*.py", + "*.pyc", + "a.py", + "b.py", + "build", + "dist", + "sub", + "other", + "deep", + "src/other", + "/a.py", + "sub/", + "other/", + "build/", +) +# The curated table's shapes recombined freely: anchored and unanchored, directory-only and not, +# with and without `**`. Deliberately unrepresentative — this is the corner-hunting pool. +# +# `**/**` and `**/**/` earn their place separately from the single-`**` entries above them. Every +# other body here contains at most one `**`, so no combination this file generated ever reached +# `_prefix_pattern`'s two-`**` path — a gap that hid a real under-linting bug through several +# rounds of review until someone read the rewrite by hand. Composing a token with itself is the +# cheap generalisation of "one of these", and the class it covers is exactly the one the +# generator was blind to. +CORNER_BODIES = ( + *ORDINARY_BODIES, + "**", + "**/", + "**/a.py", + "**/**", + "**/**/", + "sub/**", + "deep/**/", + "/sub", + "a.py/", + "sub/a.py", + "src/sub", + "deep/", +) + + +@dataclass(frozen=True) +class Distribution: + """One way of generating `.gitignore` content, and the divergence rate it is allowed.""" + + name: str + negation_rate: float + bodies: tuple[str, ...] + max_divergence_rate: float + + +DISTRIBUTIONS = ( + # The known divergence requires a negation to re-include something under a broader ignore. + # With no negation in play there is nothing to diverge about, so this ceiling is exactly zero + # — the strongest of the three, and the one that would catch a genuinely new class of bug. + Distribution("no-negation", 0.0, ORDINARY_BODIES, 0.0), + Distribution("typical", 0.05, ORDINARY_BODIES, 0.01), + Distribution("adversarial", 0.30, CORNER_BODIES, 0.04), +) + + +@dataclass(frozen=True) +class Divergence: + """One disagreement with git, kept with enough context to reproduce it by hand.""" + + ignores: dict[str, tuple[str, ...]] + skipped_by_git_only: frozenset[str] + skipped_by_house_lint_only: frozenset[str] + + def render(self) -> str: + rules = "; ".join( + f"{owner or ''}/.gitignore={list(lines)}" for owner, lines in self.ignores.items() + ) + return ( + f"{rules} -> house-lint wrongly skips {sorted(self.skipped_by_house_lint_only)}, " + f"wrongly lints {sorted(self.skipped_by_git_only)}" + ) + + +def _is_known_directory_negation_defect(divergence: Divergence) -> bool: + """Whether this under-linting divergence is the known `pathspec` directory-negation defect. + + `pathspec` will not let a directory-only negation win for a directory path: + `GitIgnoreSpec.from_lines(("**", "!**/")).match_file("src")` returns True, while git reports + `.gitignore:2:!**/` re-including `src` and descends into it. house-lint asks `pathspec` that + exact question when deciding whether to prune a directory, so it prunes a subtree git walks + — and every file underneath vanishes from the scan. + + The defect is one level below house-lint. Passing a trailing-slash candidate (`"src/"`) + does not change `pathspec`'s answer, so there is no shape of question house-lint can ask + that gets the right verdict; deciding it means owning the matcher rather than delegating + whole-path matching (see `design/research/2026-08-20-gitignore-style-exclusion-inclusion/`). + + Recognised by the ingredient that makes the verdict `pathspec`'s to get wrong: a negated + directory-only pattern somewhere in the rule set. Deliberately narrow — an under-linting + divergence *without* one is a genuinely new bug and still fails the suite. This is the only + reason a wrongly-skipped file is tolerated anywhere in this file, and it is capped by + `MAX_KNOWN_DIRECTORY_NEGATION_DIVERGENCES` so the class cannot quietly widen. + """ + return any( + line.startswith("!") and line.endswith("/") + for lines in divergence.ignores.values() + for line in lines + ) + + +def _build_tree(root: Path) -> None: + for relative in TREE: + path = root / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("x = 1\n") + + +def _random_rules(rng: random.Random, distribution: Distribution) -> dict[str, tuple[str, ...]]: + """Pick one to three `.gitignore` files, each with one to four patterns.""" + owners = rng.sample(IGNORE_OWNERS, k=rng.randint(1, len(IGNORE_OWNERS))) + rules: dict[str, tuple[str, ...]] = {} + for owner in owners: + bodies = rng.choices(distribution.bodies, k=rng.randint(1, 4)) + rules[owner] = tuple( + f"!{body}" if rng.random() < distribution.negation_rate else body for body in bodies + ) + return rules + + +def _write_rules(root: Path, rules: dict[str, tuple[str, ...]]) -> None: + for owner in IGNORE_OWNERS: + path = (root / owner / ".gitignore") if owner else (root / ".gitignore") + if owner in rules: + path.write_text("\n".join(rules[owner]) + "\n") + else: + path.unlink(missing_ok=True) + + +def _house_lint_skipped(root: Path) -> set[str]: + result = discover_files(root, include=("src",)) + selected = {path.relative_to(root.resolve()).as_posix() for path in result.files} + return {relative for relative in TREE if relative not in selected} + + +@pytest.fixture(scope="module", params=DISTRIBUTIONS, ids=lambda item: item.name) +def trial_run( + request: pytest.FixtureRequest, tmp_path_factory: pytest.TempPathFactory +) -> tuple[Distribution, tuple[Divergence, ...]]: + """Run every trial for one distribution; both assertions below read the same result set.""" + distribution: Distribution = request.param + root = tmp_path_factory.mktemp(f"fuzz-{distribution.name}") + _build_tree(root) + init_repository(root) + rng = random.Random(SEED) + + found: list[Divergence] = [] + for _ in range(TRIALS): + rules = _random_rules(rng, distribution) + _write_rules(root, rules) + git_skipped = git_ignored(root, TREE) + house_lint_skipped = _house_lint_skipped(root) + if house_lint_skipped != git_skipped: + found.append( + Divergence( + rules, + frozenset(git_skipped - house_lint_skipped), + frozenset(house_lint_skipped - git_skipped), + ) + ) + # Producing this number is the harness's job, so it is reported rather than only asserted on: + # `CI=1 uv run pytest -s ` is how docs/configuration.md's figures get regenerated. + print( + f"\n[gitignore-fuzz] {distribution.name}: {len(found)}/{TRIALS} diverge " + f"({len(found) / TRIALS:.2%}), ceiling {distribution.max_divergence_rate:.0%}" + ) + return distribution, tuple(found) + + +def test_no_divergence_ever_skips_a_file_git_would_lint( + trial_run: tuple[Distribution, tuple[Divergence, ...]], +) -> None: + """The safety property, and the only one here that is not merely a quality target. + + A file house-lint skips is a file it reports nothing about, and `0 findings` is + indistinguishable from a clean run. A file it lints that git ignores is at worst noise the + user can see and silence. It is the property the surveyed alternatives (`igittigitt`, + `dulwich.ignore`) fail. + + This used to read "every known divergence errs the second way." That is no longer true: + `_is_known_directory_negation_defect` documents one class that errs the *first* way, found + once the corner pool learned to compose repeated `**` segments. The assertion is narrowed + to that named class rather than dropped — an under-linting divergence outside it is still a + hard failure, which is the whole point of running this at all. + """ + _, divergences = trial_run + unsafe = [item for item in divergences if item.skipped_by_house_lint_only] + unexplained = [item for item in unsafe if not _is_known_directory_negation_defect(item)] + + assert not unexplained, ( + "discovery skipped files git would lint, outside the known directory-negation defect:\n" + + "\n".join(item.render() for item in unexplained[:10]) + ) + + +def test_the_known_directory_negation_defect_does_not_widen( + trial_run: tuple[Distribution, tuple[Divergence, ...]], +) -> None: + """Caps what the one tolerated under-linting class is allowed to account for. + + Without a cap, `_is_known_directory_negation_defect` would be an open-ended licence to skip + files: any future regression involving a negated directory-only pattern would be absorbed + silently. Pinning the count means a change that widens the defect fails here even though the + safety test still passes. + """ + distribution, divergences = trial_run + unsafe = [item for item in divergences if item.skipped_by_house_lint_only] + + assert len(unsafe) <= MAX_KNOWN_DIRECTORY_NEGATION_DIVERGENCES, ( + f"{distribution.name}: {len(unsafe)} under-linting divergences attributed to the known " + f"directory-negation defect, above the recorded ceiling of " + f"{MAX_KNOWN_DIRECTORY_NEGATION_DIVERGENCES}. Either a change widened the defect, or the " + f"ceiling needs regenerating alongside docs/configuration.md. Examples:\n" + + "\n".join(item.render() for item in unsafe[:10]) + ) + + +def test_divergence_rate_stays_within_its_documented_ceiling( + trial_run: tuple[Distribution, tuple[Divergence, ...]], +) -> None: + """Backs the rates `docs/configuration.md` quotes. Update both together, never one alone.""" + distribution, divergences = trial_run + rate = len(divergences) / TRIALS + + assert rate <= distribution.max_divergence_rate, ( + f"{distribution.name}: {len(divergences)}/{TRIALS} ({rate:.2%}) of generated combinations " + f"diverge from git, above the {distribution.max_divergence_rate:.0%} ceiling recorded here " + f"and in docs/configuration.md. Examples:\n" + + "\n".join(item.render() for item in divergences[:10]) + ) diff --git a/tests/integration/test_gitignore_parity.py b/tests/integration/test_gitignore_parity.py new file mode 100644 index 0000000..436d26d --- /dev/null +++ b/tests/integration/test_gitignore_parity.py @@ -0,0 +1,393 @@ +"""Differential parity between house-lint's discovery and real `git check-ignore`. + +house-lint reimplements git's ignore semantics on top of `pathspec` rather than shelling out to +git, so every rule it reimplements is a chance to drift. The unit tests in +`tests/unit/test_discovery.py` pin the behaviour house-lint *intends*; these pin it against the +only authority that matters, by building a real repository and asking git itself. + +Each scenario declares the `.gitignore` files to write and the Python files to create, and the +test asserts that the set of files house-lint skips is exactly the set git ignores. A scenario +needs no expected-value literal — git supplies it — so adding a regression case costs one table +entry. + +Skipped wholesale when git is unavailable. Git config is neutralised (`GIT_CONFIG_GLOBAL`, +`GIT_CONFIG_SYSTEM`, `core.excludesFile`) so a developer's own global ignore rules can never +change the outcome. +""" + +import shutil +from dataclasses import dataclass, field +from pathlib import Path + +import pytest +from _git_harness import git_ignored, init_repository + +from house_lint.discovery import DiscoveryResult, discover_files + +pytestmark = pytest.mark.skipif(shutil.which("git") is None, reason="git is not installed") + +PY_CONTENT = "x = 1\n" + + +@dataclass(frozen=True) +class Scenario: + """One parity case. + + `ignores` maps an owning directory (root-relative posix, `""` for the repository root) to + that directory's `.gitignore` lines. `files` are the Python files to create. `include` is + the discovery root set, mirroring `[tool.house-lint] include`. + + `symlinked_ignores` has the same shape as `ignores`, but writes the lines to a sibling file + and leaves `.gitignore` as a symlink to it. Git does not follow a symlinked ignore file, so + these scenarios pin that discovery does not either. + """ + + name: str + ignores: dict[str, list[str]] + files: tuple[str, ...] + include: tuple[str, ...] = ("src",) + symlinked_ignores: dict[str, list[str]] = field(default_factory=dict[str, list[str]]) + + +SCENARIOS = ( + # --- The nested-`**/` family. `**/` is directory-only: git ignores every directory below + # its owner but leaves an immediate regular file alone. + Scenario( + "nested '**/' spares an immediate regular file", + {"src": ["**/"]}, + ("src/a.py", "src/sub/b.py"), + ), + Scenario( + "root '**/' ignores every nested directory", + {"": ["**/"]}, + ("src/a.py", "src/sub/b.py"), + ), + Scenario( + "nested '**' without a trailing slash covers files too", + {"src": ["**"]}, + ("src/a.py", "src/sub/b.py"), + ), + # git collapses a run of consecutive `**` segments into one, so each of these means exactly + # what its single-`**` counterpart above means. house-lint rewrites nested patterns rather + # than handing them to git, and only the one-segment spelling used to be recognised — the + # repeated form fell through to the generic slash-containing branch and produced a pattern + # that swallowed the immediate file this family exists to spare. + Scenario( + "nested '**/**/' collapses to '**/' and spares an immediate regular file", + {"src": ["**/**/"]}, + ("src/a.py", "src/sub/b.py"), + ), + Scenario( + "nested '**/**' collapses to '**' and covers files too", + {"src": ["**/**"]}, + ("src/a.py", "src/sub/b.py"), + ), + Scenario( + "a longer '**' run collapses the same way", + {"src": ["**/**/**/"]}, + ("src/a.py", "src/sub/b.py"), + ), + Scenario( + "'**/**/' collapses to '**/'", + {"src": ["**/**/b.py"]}, + ("src/a.py", "src/sub/b.py"), + ), + # --- Directory-only patterns must not match a same-named regular file, and a directory-form + # negation must be able to cancel an earlier file-form match (last matching line wins). + Scenario( + "directory-only pattern does not match a same-named .py file", + {"": ["b.py/"]}, + ("src/a.py", "src/b.py"), + ), + Scenario( + "directory-form negation cancels an earlier unanchored ignore", + {"": ["cache", "!cache/"]}, + ("src/a.py", "src/cache/c.py"), + ), + Scenario( + "nested directory-form negation cancels its own earlier ignore", + {"src": ["sub/", "!sub/"]}, + ("src/a.py", "src/sub/b.py"), + ), + # --- A file inside an ignored directory can never be re-included by a later negation, + # including when the ignored directory is the discovery root itself. + Scenario( + "negation cannot resurrect a file from an ignored discovery root", + {"": ["src/", "!*.py"]}, + ("src/a.py", "src/sub/b.py"), + ), + Scenario( + "negation cannot resurrect a file from an ignored child directory", + {"": ["gen/", "!*.py"]}, + ("src/a.py", "src/gen/g.py"), + ), + Scenario( + "negation beside its own directory exclusion in one file cannot resurrect", + {"": ["src/generated/", "!src/generated/foo.py"]}, + ("src/generated/foo.py", "src/other.py"), + ), + Scenario( + "negation cannot resurrect a file from a '**/'-ignored discovery root", + {"": ["**/", "!b.py"]}, + ("src/a.py", "src/b.py", "src/sub/b.py"), + ), + # --- A trailing `/**` names a directory's contents, so a negation can still re-include + # something underneath it. The star in `a/*/**`'s middle segment must not exempt it, and + # `a/**/` composes the embedded-slash and directory-only forms in one pattern. + Scenario( + "contents glob after a single-star segment still allows a negation underneath", + {"": ["src/a/*/**", "!src/a/sub/keep.py"]}, + ("src/a/sub/keep.py", "src/a/sub/drop.py"), + ), + Scenario( + "contents glob allows a negation underneath", + {"": ["src/gen/**", "!src/gen/keep.py"]}, + ("src/gen/keep.py", "src/gen/drop.py"), + ), + Scenario( + "embedded slash combined with the directory-only contents glob", + {"src": ["a/**/"]}, + ("src/a/y.py", "src/a/sub/x.py", "src/b.py"), + ), + # --- Ordinary per-directory semantics that must keep working. + Scenario( + "nested pattern without a slash matches at any depth below its owner", + {"src": ["a.py"]}, + ("src/a.py", "src/b.py", "src/sub/a.py"), + ), + Scenario( + "nested leading-slash pattern is anchored to its own directory", + {"src": ["/a.py"]}, + ("src/a.py", "src/sub/a.py"), + ), + Scenario( + "closer .gitignore negation overrides a farther ignore", + {"": ["*.py"], "src": ["!a.py"]}, + ("src/a.py", "src/sub/b.py"), + ), + Scenario( + "wildcard-everything with re-included files", + {"src": ["*", "!keep.py", "!.gitignore"]}, + ("src/keep.py", "src/drop.py"), + ), + Scenario( + "trailing whitespace is insignificant unless backslash-escaped", + {"src": ["a.py "]}, + ("src/a.py", "src/b.py"), + ), + # Backslashes quote each other pairwise, so it is the parity of the run before the space + # that decides whether the space survives — not merely whether a backslash precedes it. + # An even run leaves the space unquoted and git strips it; an odd run escapes it. + Scenario( + "an even backslash run leaves trailing whitespace unquoted", + {"src": ["dir\\\\ "]}, + ("src/a.py", "src/dir\\/b.py"), + ), + Scenario( + "an odd backslash run quotes trailing whitespace", + {"src": ["dir\\ "]}, + ("src/a.py", "src/dir /b.py"), + ), + Scenario( + "directory names containing glob metacharacters stay literal", + {"src": ["other/"]}, + ("src/s[1]/a.py", "src/other/a.py"), + ), + Scenario( + "deeply nested .gitignore files compose", + {"": ["*.py"], "src": ["!*.py"], "src/sub": ["b.py"]}, + ("src/a.py", "src/sub/a.py", "src/sub/b.py"), + ), + # --- Symlinked ignore files. Git reads `.gitignore` with `lstat` and skips it when it is a + # symlink; `Path.is_file()` follows one, so discovery would apply patterns git never applies. + Scenario( + "a symlinked nested .gitignore is not read", + {}, + ("src/a.py", "src/sub/b.py"), + symlinked_ignores={"src": ["*.py"]}, + ), + Scenario( + "a symlinked root .gitignore is not read", + {}, + ("src/a.py",), + symlinked_ignores={"": ["*.py"]}, + ), + Scenario( + "a symlinked nested .gitignore cannot negate a real ancestor ignore", + {"": ["*.py"]}, + ("src/a.py",), + symlinked_ignores={"src": ["!*.py"]}, + ), +) + + +def _build(root: Path, scenario: Scenario) -> None: + for relative in scenario.files: + path = root / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(PY_CONTENT) + for owner, lines in scenario.ignores.items(): + path = (root / owner / ".gitignore") if owner else (root / ".gitignore") + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("\n".join(lines) + "\n") + for owner, lines in scenario.symlinked_ignores.items(): + directory = (root / owner) if owner else root + directory.mkdir(parents=True, exist_ok=True) + target = directory / "ignore-patterns" + target.write_text("\n".join(lines) + "\n") + (directory / ".gitignore").symlink_to(target.name) + + +@pytest.mark.parametrize("scenario", SCENARIOS, ids=lambda item: item.name) +def test_discovery_matches_git_check_ignore(scenario: Scenario, tmp_path: Path) -> None: + _build(tmp_path, scenario) + init_repository(tmp_path) + + result = discover_files(tmp_path, include=scenario.include) + selected = {path.relative_to(tmp_path.resolve()).as_posix() for path in result.files} + house_lint_skipped = {relative for relative in scenario.files if relative not in selected} + ignored_by_git = git_ignored(tmp_path, scenario.files) + + assert result.errors == () + assert house_lint_skipped == ignored_by_git + + +@pytest.mark.parametrize("scenario", SCENARIOS, ids=lambda item: item.name) +def test_explicit_paths_match_git_check_ignore(scenario: Scenario, tmp_path: Path) -> None: + """The same table, but reaching each file directly instead of walking to it. + + An explicit path skips `_traversable_dirs` entirely and leans on + `_combined_gitignore_spec` alone, so walk-time pruning cannot mask a wrong answer here. + That makes this the stricter half of the pair: `house-lint check src/generated/foo.py` has + to reach the same verdict git does with no directory traversal to help it. + """ + _build(tmp_path, scenario) + init_repository(tmp_path) + + result = discover_files(tmp_path, explicit=tuple(tmp_path / item for item in scenario.files)) + selected = {path.relative_to(tmp_path.resolve()).as_posix() for path in result.files} + house_lint_skipped = {relative for relative in scenario.files if relative not in selected} + + assert result.errors == () + assert house_lint_skipped == git_ignored(tmp_path, scenario.files) + + +@pytest.mark.xfail( + strict=True, + reason=( + "Known pathspec/git divergence: a negated directory-only pattern ('!sub/') re-includes " + "everything beneath it in pathspec, while git re-includes only the 'sub' entry itself " + "and re-evaluates each descendant. Closing this would mean matching path components " + "against git's precedence by hand instead of delegating whole-path matching to " + "pathspec. It errs toward linting a file git would ignore, never toward skipping one, " + "so it cannot hide a finding. Strict xfail: if this starts passing, the limitation is " + "gone and the note in docs/configuration.md should go with it." + ), +) +def test_negated_directory_pattern_does_not_re_include_nested_directories(tmp_path: Path) -> None: + scenario = Scenario( + "negated directory pattern re-includes only the directory itself", + {"src": ["**/", "!sub/"]}, + ("src/a.py", "src/sub/a.py", "src/sub/deep/a.py"), + ) + _build(tmp_path, scenario) + init_repository(tmp_path) + + result = discover_files(tmp_path, include=scenario.include) + selected = {path.relative_to(tmp_path.resolve()).as_posix() for path in result.files} + house_lint_skipped = {relative for relative in scenario.files if relative not in selected} + + assert house_lint_skipped == git_ignored(tmp_path, scenario.files) + + +@pytest.mark.xfail( + strict=True, + reason=( + "Known pathspec/git divergence, same directory-negation family as the test above but " + "pointing the other way — and this one *under*-lints. pathspec will not let a " + "directory-only negation win for a directory path: " + "GitIgnoreSpec.from_lines(('**', '!**/')).match_file('src') returns True, while git " + "reports '.gitignore:2:!**/ src' re-including the directory and descends into it. " + "house-lint asks pathspec exactly that when deciding whether to prune, so it prunes a " + "subtree git walks and every file underneath vanishes from the scan. Passing 'src/' " + "does not change pathspec's answer, so no shape of question fixes it here; deciding it " + "means owning the matcher (see design/research/" + "2026-08-20-gitignore-style-exclusion-inclusion/). Strict xfail: if this starts " + "passing, the limitation is gone and docs/configuration.md should say so." + ), +) +def test_negated_directory_pattern_re_includes_a_directory_git_descends_into( + tmp_path: Path, +) -> None: + scenario = Scenario( + "directory-only negation re-includes the directory git walks", + {"": ["**", "!**/"], "src": ["!**"]}, + ("src/a.py", "src/sub/b.py"), + ) + _build(tmp_path, scenario) + init_repository(tmp_path) + + result = discover_files(tmp_path, include=scenario.include) + selected = {path.relative_to(tmp_path.resolve()).as_posix() for path in result.files} + house_lint_skipped = {relative for relative in scenario.files if relative not in selected} + + assert house_lint_skipped == git_ignored(tmp_path, scenario.files) + + +@pytest.mark.parametrize("scenario", SCENARIOS, ids=lambda item: item.name) +def test_explicit_directory_arguments_match_git_check_ignore( + scenario: Scenario, tmp_path: Path +) -> None: + """The same table again, reached by naming a directory explicitly. + + `house-lint check src/` takes a third route: `_consider`'s directory branch, which is the + one place neither the include-root walk nor an explicit *file* exercises. That branch is + exactly what the excluded-ancestor fix had to patch, so leaving it to hand-written unit + tests would reproduce the blind spot this harness exists to close. + """ + _build(tmp_path, scenario) + init_repository(tmp_path) + + result = discover_files(tmp_path, explicit=tuple(tmp_path / item for item in scenario.include)) + selected = {path.relative_to(tmp_path.resolve()).as_posix() for path in result.files} + house_lint_skipped = {relative for relative in scenario.files if relative not in selected} + + assert result.errors == () + assert house_lint_skipped == git_ignored(tmp_path, scenario.files) + + +@pytest.mark.parametrize("broken", ["skips-everything", "skips-nothing"]) +def test_harness_detects_a_real_divergence( + broken: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Guard the guard: prove the comparison actually fails when discovery disagrees with git. + + A parity suite that could not fail would be worse than no suite, because it reads as + evidence. Confirming a case where the two agree does not establish that — it exercises the + same concordant path every scenario already does. So this substitutes a deliberately broken + `discover_files` in both directions (selecting nothing, and selecting everything) and + asserts the comparison raises. Both directions matter: a stub that skipped everything would + satisfy a suite whose scenarios all expect skips, and one that skipped nothing would satisfy + a suite whose scenarios all expect selections. + """ + files = ("src/a.py", "src/b.py") + _build( + tmp_path, + Scenario("guard", {"": ["a.py"]}, files), + ) + init_repository(tmp_path) + + ignored_by_git = git_ignored(tmp_path, files) + assert ignored_by_git == {"src/a.py"}, "fixture must produce a genuine mix of ignored and not" + + def broken_discovery(root: Path, **_: object) -> DiscoveryResult: + if broken == "skips-everything": + return DiscoveryResult(()) + return DiscoveryResult(tuple(sorted(root.resolve() / item for item in files))) + + # `tests` is not an importable package, so patch this module's own globals — which is what + # `test_discovery_matches_git_check_ignore` resolves `discover_files` through. + monkeypatch.setitem(globals(), "discover_files", broken_discovery) + + with pytest.raises(AssertionError): + test_discovery_matches_git_check_ignore(Scenario("guard", {"": ["a.py"]}, files), tmp_path) diff --git a/tests/unit/test_cache.py b/tests/unit/test_cache.py new file mode 100644 index 0000000..4aecc9b --- /dev/null +++ b/tests/unit/test_cache.py @@ -0,0 +1,811 @@ +import json +import os +import shutil +import sys +from pathlib import Path + +import pytest + +from house_lint import __version__ +from house_lint.cache import ( + CachedFileResult, + CacheReporter, + code_identity, + default_cache_base, + default_cache_base_is_safe, + hash_effective_config, + hash_source_content, + prepare_cache_dir, + prune_stale_cache_dirs, + read_cached_result, + versioned_cache_dir, + write_cached_result, +) +from house_lint.config import HSL101Options, HSL102Options, HSL103Options, TokenFamily +from house_lint.results import Finding, LintError + +# Well-formed cache payloads, as `_finding_to_payload`/`_error_to_payload` write them (the +# `path` field is stripped on write and re-attached by the reader). +_FINDING = { + "rule_id": "HSL002", + "line": 1, + "column": 5, + "end_line": 1, + "end_column": 20, + "message": "import inside function", +} +_ERROR = { + "code": "read-error", + "kind": "read", + "line": None, + "column": None, + "end_line": None, + "end_column": None, + "phase": "read", + "operation": "bounded-read", + "rule_id": None, + "message": "could not read", +} + + +def test_hash_source_content_is_stable_and_changes_with_content() -> None: + first = hash_source_content(b"x = 1\n") + assert first is not None + assert hash_source_content(b"x = 1\n") == first + assert hash_source_content(b"x = 2\n") != first + + +def test_hash_source_content_is_none_without_bytes() -> None: + # None is what `SourceFile.content_bytes` reports for a file it could not read at all — + # missing, non-regular, or permission-denied. Such a file is simply never cached. + assert hash_source_content(None) is None + + +def test_hash_source_content_is_none_when_oversized(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr("house_lint.cache.MAX_SOURCE_BYTES", 2) + + assert hash_source_content(b"x = 1\n") is None + + +def test_hash_effective_config_is_order_independent_and_content_sensitive() -> None: + hsl101, hsl102, hsl103 = HSL101Options(), HSL102Options(), HSL103Options() + + # Explicit parameters rather than `**kwargs`: a typo'd keyword would be silently ignored by + # `kwargs.get`, leaving the final assertion comparing two identical hashes and passing while + # testing nothing. + def h( + enabled_rules: tuple[str, ...], + *, + options101: HSL101Options = hsl101, + options102: HSL102Options = hsl102, + options103: HSL103Options = hsl103, + ) -> str: + return hash_effective_config( + enabled_rules, options101, options102, options103, filename="a.py" + ) + + assert h(("HSL001", "HSL002")) == h(("HSL002", "HSL001")) + assert h(("HSL001",)) != h(("HSL001", "HSL002")) + assert h(("HSL102",), options102=HSL102Options(max_lines=100)) != h(("HSL102",)) + + +def test_hash_effective_config_changes_with_python_version() -> None: + hsl101, hsl102, hsl103 = HSL101Options(), HSL102Options(), HSL103Options() + + def h(python_version: tuple[int, int]) -> str: + return hash_effective_config( + ("HSL001",), hsl101, hsl102, hsl103, filename="a.py", python_version=python_version + ) + + assert h((3, 11)) != h((3, 12)) + assert h((3, 12)) == h((3, 12)) + + +def test_hash_effective_config_defaults_python_version_to_the_running_interpreter() -> None: + hsl101, hsl102, hsl103 = HSL101Options(), HSL102Options(), HSL103Options() + + default = hash_effective_config(("HSL001",), hsl101, hsl102, hsl103, filename="a.py") + explicit = hash_effective_config( + ("HSL001",), + hsl101, + hsl102, + hsl103, + filename="a.py", + python_version=tuple(sys.version_info[:2]), + ) + + assert default == explicit + + +def test_hash_effective_config_includes_filename_only_when_hsl101_scopes_to_filenames() -> None: + scoped = HSL101Options( + tokens=(TokenFamily(prefixes=("TASK",), scopes=("filenames",)),), + ) + unscoped = HSL101Options( + tokens=(TokenFamily(prefixes=("TASK",), scopes=("comments",)),), + ) + other_rule_options = (HSL102Options(), HSL103Options()) + + assert hash_effective_config( + ("HSL101",), scoped, *other_rule_options, filename="a.py" + ) != hash_effective_config(("HSL101",), scoped, *other_rule_options, filename="b.py") + assert hash_effective_config( + ("HSL101",), unscoped, *other_rule_options, filename="a.py" + ) == hash_effective_config(("HSL101",), unscoped, *other_rule_options, filename="b.py") + # HSL101 configured with a filenames-scoped family, but not currently enabled: filename + # cannot affect output for this run, so it must not affect the hash either. + assert hash_effective_config( + (), scoped, *other_rule_options, filename="a.py" + ) == hash_effective_config((), scoped, *other_rule_options, filename="b.py") + + +def test_default_cache_base_and_versioned_cache_dir(tmp_path: Path) -> None: + base = default_cache_base(tmp_path) + assert base == tmp_path / ".house-lint-cache" + assert versioned_cache_dir(base) == base / f"{__version__}-{code_identity()}" + + +def test_write_then_read_round_trips_and_reattaches_the_caller_supplied_path( + tmp_path: Path, +) -> None: + cache_dir = tmp_path / "cache" + prepare_cache_dir(cache_dir, self_ignore=False, reporter=CacheReporter()) + result = CachedFileResult( + findings=(Finding("HSL002", "wrong/path.py", 1, 5, 1, 20, "import inside function"),), + errors=( + LintError( + "read-error", + "read", + "wrong/path.py", + None, + None, + None, + None, + "read", + "bounded-read", + None, + "could not read", + ), + ), + suppressed_count=1, + files_scanned=1, + ) + + write_cached_result( + cache_dir, + "content-hash", + "config-hash", + result, + self_ignore=False, + reporter=CacheReporter(), + ) + cached = read_cached_result( + cache_dir, + "content-hash", + "config-hash", + relative_path="actual/path.py", + reporter=CacheReporter(), + ) + + assert cached is not None + assert cached.findings == ( + Finding("HSL002", "actual/path.py", 1, 5, 1, 20, "import inside function"), + ) + assert cached.errors == ( + LintError( + "read-error", + "read", + "actual/path.py", + None, + None, + None, + None, + "read", + "bounded-read", + None, + "could not read", + ), + ) + assert cached.suppressed_count == 1 + assert cached.files_scanned == 1 + + +def test_read_cached_result_is_a_miss_when_no_entry_exists(tmp_path: Path) -> None: + cache_dir = tmp_path / "cache" + assert ( + read_cached_result(cache_dir, "x", "y", relative_path="a.py", reporter=CacheReporter()) + is None + ) + + +def test_read_cached_result_is_a_miss_on_corrupted_entries(tmp_path: Path) -> None: + cache_dir = tmp_path / "cache" + cache_dir.mkdir(parents=True) + (cache_dir / "content-hash-config-hash.json").write_text("not valid json {{{") + + assert ( + read_cached_result( + cache_dir, "content-hash", "config-hash", relative_path="a.py", reporter=CacheReporter() + ) + is None + ) + + +def test_read_cached_result_is_a_miss_when_a_required_field_is_missing(tmp_path: Path) -> None: + cache_dir = tmp_path / "cache" + cache_dir.mkdir(parents=True) + (cache_dir / "content-hash-config-hash.json").write_text('{"findings": []}') + + assert ( + read_cached_result( + cache_dir, "content-hash", "config-hash", relative_path="a.py", reporter=CacheReporter() + ) + is None + ) + + +def test_read_cached_result_is_a_miss_when_a_scalar_field_has_the_wrong_type( + tmp_path: Path, +) -> None: + """A corrupted-but-valid-JSON entry (e.g. `suppressed_count` as a string) must be treated + as a cache miss here, not accepted and left to raise `TypeError` later when the caller + accumulates it (`suppressed_count += cached.suppressed_count` in `cli.py`).""" + cache_dir = tmp_path / "cache" + cache_dir.mkdir(parents=True) + payload = '{"findings": [], "errors": [], "suppressed_count": "1", "files_scanned": 1}' + (cache_dir / "content-hash-config-hash.json").write_text(payload) + + assert ( + read_cached_result( + cache_dir, "content-hash", "config-hash", relative_path="a.py", reporter=CacheReporter() + ) + is None + ) + + +@pytest.mark.parametrize( + ("label", "payload"), + [ + ("negative suppressed_count", '{"suppressed_count": -7, "files_scanned": 1}'), + ("negative files_scanned", '{"suppressed_count": 0, "files_scanned": -1}'), + ], +) +def test_read_cached_result_is_a_miss_when_a_count_is_negative( + label: str, payload: str, tmp_path: Path +) -> None: + """Negative counts reach the same accumulation the type checks above exist to protect. No + real scan produces one — suppression counts are nonnegative and `files_scanned` is 0 or 1 — + so a negative value is corruption, and accepting it would silently lower the run's totals.""" + cache_dir = tmp_path / "cache" + cache_dir.mkdir(parents=True) + entry = json.loads(payload) | {"findings": [], "errors": []} + (cache_dir / "content-hash-config-hash.json").write_text(json.dumps(entry)) + + assert ( + read_cached_result( + cache_dir, "content-hash", "config-hash", relative_path="a.py", reporter=CacheReporter() + ) + is None + ) + + +def test_read_cached_result_is_a_miss_when_the_entry_is_not_utf8(tmp_path: Path) -> None: + """Invalid UTF-8 raises `UnicodeDecodeError` at `read_text`, before the parse block. It is a + `ValueError`, so the `OSError` handler does not catch it either — left unhandled it escapes + `_scan` and aborts the run with an internal error instead of degrading to a miss.""" + cache_dir = tmp_path / "cache" + cache_dir.mkdir(parents=True) + (cache_dir / "content-hash-config-hash.json").write_bytes(b"\xff\xfe not utf-8") + + assert ( + read_cached_result( + cache_dir, "content-hash", "config-hash", relative_path="a.py", reporter=CacheReporter() + ) + is None + ) + + +@pytest.mark.parametrize( + ("label", "findings", "errors"), + [ + ("finding message is not a string", [{**_FINDING, "message": 12345}], []), + ("finding rule_id is not a string", [{**_FINDING, "rule_id": 2}], []), + ("finding is not an object", ["not an object"], []), + ("error message is not a string", [], [{**_ERROR, "message": []}]), + ("error kind is not a string", [], [{**_ERROR, "kind": 7}]), + ("error rule_id is neither string nor null", [], [{**_ERROR, "rule_id": 3}]), + ("error is not an object", [], [42]), + # `_finding_from_payload`/`_error_from_payload` splat the whole payload into the + # dataclass constructor, so an extra key raises `TypeError: unexpected keyword argument`. + # Pinned here so a later change to those constructor calls cannot quietly turn a + # corrupted entry into an uncaught crash. + ("finding carries an unexpected key", [{**_FINDING, "stop": True}], []), + ("error carries an unexpected key", [], [{**_ERROR, "path": "leaked.py"}]), + ], +) +def test_read_cached_result_is_a_miss_when_a_text_field_has_the_wrong_type( + label: str, findings: list[object], errors: list[object], tmp_path: Path +) -> None: + """`Finding`/`LintError` validate their location fields but accept any type elsewhere, so a + corrupted-but-valid-JSON entry would construct fine and only blow up later — `int < str` + while `ScanResult.to_dict()` sorts findings, during rendering, outside `check()`'s exception + boundary. That surfaces as a traceback rather than the documented cache miss.""" + cache_dir = tmp_path / "cache" + cache_dir.mkdir(parents=True) + payload = { + "findings": findings, + "errors": errors, + "suppressed_count": 0, + "files_scanned": 1, + } + (cache_dir / "content-hash-config-hash.json").write_text(json.dumps(payload)) + + assert ( + read_cached_result( + cache_dir, "content-hash", "config-hash", relative_path="a.py", reporter=CacheReporter() + ) + is None + ), label + + +def test_read_cached_result_accepts_a_well_formed_entry_with_a_null_error_rule_id( + tmp_path: Path, +) -> None: + """The nullable text fields must stay nullable — the validation above rejects wrong types, + not legitimately absent values.""" + cache_dir = tmp_path / "cache" + cache_dir.mkdir(parents=True) + payload = { + "findings": [_FINDING], + "errors": [_ERROR], + "suppressed_count": 0, + "files_scanned": 1, + } + (cache_dir / "content-hash-config-hash.json").write_text(json.dumps(payload)) + + cached = read_cached_result( + cache_dir, "content-hash", "config-hash", relative_path="a.py", reporter=CacheReporter() + ) + + assert cached is not None + assert cached.findings[0].message == "import inside function" + assert cached.errors[0].rule_id is None + + +def test_corrupted_entry_is_reported_without_debug( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + cache_dir = tmp_path / "cache" + cache_dir.mkdir(parents=True) + (cache_dir / "content-hash-config-hash.json").write_text("not valid json {{{") + + read_cached_result( + cache_dir, "content-hash", "config-hash", relative_path="a.py", reporter=CacheReporter() + ) + + captured = capsys.readouterr().err + assert captured.startswith("warning: ") + assert "a.py" in captured + + +def test_write_failure_is_reported_without_debug( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + blocked = tmp_path / "blocked" + blocked.write_text("not a directory") + + write_cached_result( + blocked / "cache", + "x", + "y", + CachedFileResult(), + self_ignore=False, + reporter=CacheReporter(), + ) + + assert capsys.readouterr().err.startswith("warning: ") + + +def test_reporter_warns_once_then_falls_back_to_debug_only( + capsys: pytest.CaptureFixture[str], +) -> None: + """A broken cache directory fails once per scanned file. Reporting every one by default would + bury the single fact worth reporting, so only the first failure of a run is unconditional.""" + quiet = CacheReporter() + quiet.failure("first failure") + quiet.failure("second failure") + + captured = capsys.readouterr().err + assert captured == "warning: first failure\n" + + verbose = CacheReporter(debug=True) + verbose.failure("first failure") + verbose.failure("second failure") + + assert capsys.readouterr().err == "warning: first failure\ndebug: second failure\n" + + +def test_write_cached_result_is_best_effort_and_does_not_raise_on_a_bad_directory( + tmp_path: Path, +) -> None: + # cache_dir collides with an existing file, so mkdir(parents=True) must fail silently. + blocked = tmp_path / "blocked" + blocked.write_text("not a directory") + result = CachedFileResult(findings=(), errors=(), suppressed_count=0, files_scanned=1) + + write_cached_result( + blocked / "cache", "x", "y", result, self_ignore=False, reporter=CacheReporter() + ) # must not raise + + +def test_write_cached_result_writes_atomically_and_leaves_no_temp_file(tmp_path: Path) -> None: + cache_dir = tmp_path / "cache" + prepare_cache_dir(cache_dir, self_ignore=False, reporter=CacheReporter()) + result = CachedFileResult() + + write_cached_result( + cache_dir, + "content-hash", + "config-hash", + result, + self_ignore=False, + reporter=CacheReporter(), + ) + + entries = {entry.name for entry in cache_dir.iterdir()} + assert entries == {"content-hash-config-hash.json", ".house-lint-version"} + assert not any(name.endswith(".tmp") for name in entries) + + +def test_write_cached_result_removes_its_temp_file_when_the_atomic_replace_fails( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A stranded `..tmp` file is unrecognisable to every later run, so nothing would ever + clean it up — repeated interrupted or failing writes would accumulate forever.""" + cache_dir = tmp_path / "cache" + prepare_cache_dir(cache_dir, self_ignore=False, reporter=CacheReporter()) + + def failing_replace(source: object, target: object) -> None: + raise OSError("no space left on device") + + monkeypatch.setattr(os, "replace", failing_replace) + write_cached_result( + cache_dir, + "content-hash", + "config-hash", + CachedFileResult(), + self_ignore=False, + reporter=CacheReporter(), + ) + + assert [entry.name for entry in cache_dir.iterdir()] == [".house-lint-version"] + + +def test_write_cached_result_recreates_a_namespace_pruned_out_from_under_it( + tmp_path: Path, +) -> None: + """`prepare_cache_dir` runs once per scan and is never retried, so a concurrent house-lint + process pruning this namespace mid-run would otherwise cost every remaining write. The write + restores the directory once and retries, bounding the loss to the entry in flight.""" + cache_dir = tmp_path / ".house-lint-cache" / "1.0.0" + prepare_cache_dir(cache_dir, self_ignore=True, reporter=CacheReporter()) + + shutil.rmtree(cache_dir) # stands in for a sibling process's prune + + assert write_cached_result( + cache_dir, + "content-hash", + "config-hash", + CachedFileResult(files_scanned=1), + self_ignore=True, + reporter=CacheReporter(), + ) + assert (cache_dir / "content-hash-config-hash.json").is_file() + assert (cache_dir / ".house-lint-version").is_file(), "the namespace marker must be restored" + + +def test_write_cached_result_does_not_retry_a_failure_that_cannot_resolve_itself( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Only a vanished directory is worth retrying. A permissions or out-of-space failure will + not fix itself between two adjacent calls, so retrying would just pay twice.""" + cache_dir = tmp_path / "cache" + prepare_cache_dir(cache_dir, self_ignore=False, reporter=CacheReporter()) + attempts = 0 + + def failing_replace(source: object, target: object) -> None: + nonlocal attempts + attempts += 1 + raise OSError("no space left on device") + + monkeypatch.setattr(os, "replace", failing_replace) + + assert not write_cached_result( + cache_dir, + "content-hash", + "config-hash", + CachedFileResult(), + self_ignore=False, + reporter=CacheReporter(), + ) + assert attempts == 1 + + +def test_prepare_cache_dir_creates_self_ignoring_gitignore_for_the_default_base( + tmp_path: Path, +) -> None: + base = tmp_path / ".house-lint-cache" + + prepare_cache_dir(base / "1.2.3", self_ignore=True, reporter=CacheReporter()) + + assert (base / ".gitignore").read_text(encoding="utf-8") == "*\n" + + +def test_prepare_cache_dir_never_writes_a_gitignore_into_a_user_supplied_cache_dir( + tmp_path: Path, +) -> None: + """`--cache-dir` names a directory the user already owns. Writing `*` into it would change + Git's behaviour for every unrelated sibling — and pointing it at a project root would hide + the whole project from `git status`.""" + base = tmp_path / "shared-cache" + (base / "someone-elses-data").mkdir(parents=True) + + prepare_cache_dir(base / "1.2.3", self_ignore=False, reporter=CacheReporter()) + + assert not (base / ".gitignore").exists() + assert (base / "someone-elses-data").exists() + + +def test_prepare_cache_dir_does_not_overwrite_an_existing_gitignore_marker( + tmp_path: Path, +) -> None: + base = tmp_path / ".house-lint-cache" + base.mkdir(parents=True) + (base / ".gitignore").write_text("custom content\n", encoding="utf-8") + + prepare_cache_dir(base / "1.2.3", self_ignore=True, reporter=CacheReporter()) + + assert (base / ".gitignore").read_text(encoding="utf-8") == "custom content\n" + + +def test_prepare_cache_dir_does_not_follow_a_dangling_self_ignore_marker_symlink( + tmp_path: Path, +) -> None: + """`default_cache_base_is_safe` rejects a symlinked *base*, but a real cache directory can + still hold a dangling `.gitignore` symlink. `exists()` reports false for one, so an + `exists()`-then-write would follow the link and create `*` at the target the scanned + repository named — outside the project.""" + outside = tmp_path / "outside" / "attacker-chosen" + outside.parent.mkdir(parents=True) + base = tmp_path / ".house-lint-cache" + base.mkdir(parents=True) + (base / ".gitignore").symlink_to(outside) + + prepare_cache_dir(base / "1.2.3", self_ignore=True, reporter=CacheReporter()) + + assert not outside.exists() + + +def test_prepare_cache_dir_does_not_follow_a_dangling_version_marker_symlink( + tmp_path: Path, +) -> None: + """The version-dir marker shares `_write_marker_if_absent` with the self-ignore marker, so + it shares the symlink exposure — and its directory is created before the marker is written, + leaving the same window.""" + outside = tmp_path / "outside" / "attacker-chosen" + outside.parent.mkdir(parents=True) + cache_dir = tmp_path / ".house-lint-cache" / "1.2.3" + cache_dir.mkdir(parents=True) + (cache_dir / ".house-lint-version").symlink_to(outside) + + prepare_cache_dir(cache_dir, self_ignore=False, reporter=CacheReporter()) + + assert not outside.exists() + + +def test_prune_stale_cache_dirs_removes_superseded_sibling_namespaces(tmp_path: Path) -> None: + base = tmp_path / ".house-lint-cache" + old_version_dir = base / "0.9.0-aaaaaaaaaaaaaaaa" + old_version_dir.mkdir(parents=True) + (old_version_dir / "stale-entry.json").write_text("{}") + (old_version_dir / ".house-lint-version").write_text("") + current_version_dir = base / "1.0.0-bbbbbbbbbbbbbbbb" + + prepare_cache_dir(current_version_dir, self_ignore=True, reporter=CacheReporter()) + prune_stale_cache_dirs(current_version_dir, reporter=CacheReporter()) + + assert not old_version_dir.exists() + assert current_version_dir.exists() + + +def test_prepare_cache_dir_does_not_prune_on_its_own(tmp_path: Path) -> None: + """Pruning is not race-free, so it stays tied to an actual write rather than running for + every scan. `prepare_cache_dir` must leave a sibling namespace alone.""" + base = tmp_path / ".house-lint-cache" + old_version_dir = base / "0.9.0-aaaaaaaaaaaaaaaa" + old_version_dir.mkdir(parents=True) + (old_version_dir / ".house-lint-version").write_text("") + + prepare_cache_dir(base / "1.0.0-bbbbbbbbbbbbbbbb", self_ignore=True, reporter=CacheReporter()) + + assert old_version_dir.exists() + + +def test_prune_stale_cache_dirs_does_not_prune_directories_without_the_version_marker( + tmp_path: Path, +) -> None: + """A `--cache-dir` pointed at a pre-existing shared directory (e.g. `~/.cache`) must never + have its unrelated sibling directories swept up as "stale house-lint versions" — only + directories house-lint itself created (marked via `.house-lint-version`) are eligible.""" + base = tmp_path / ".cache" + unrelated_dir = base / "some-other-tool" + unrelated_dir.mkdir(parents=True) + (unrelated_dir / "important-data.txt").write_text("do not delete") + + prepare_cache_dir(base / "1.0.0", self_ignore=False, reporter=CacheReporter()) + prune_stale_cache_dirs(base / "1.0.0", reporter=CacheReporter()) + + assert unrelated_dir.exists() + assert (unrelated_dir / "important-data.txt").exists() + + +def test_prune_stale_cache_dirs_does_not_accept_a_symlinked_version_marker( + tmp_path: Path, +) -> None: + """`is_file()` follows symlinks, so a marker that is a link to *any* regular file passes it. + + That turns the ownership check into something an unrelated directory can satisfy by + accident or on purpose, and the consequence is `shutil.rmtree` on data house-lint never + created. The marker is written with `O_EXCL` precisely so it cannot be a symlink when + house-lint made it; reading it back has to demand the same thing, or the guarantee only + holds on the write side. + """ + base = tmp_path / ".cache" + unrelated_dir = base / "some-other-tool" + unrelated_dir.mkdir(parents=True) + (unrelated_dir / "important-data.txt").write_text("do not delete") + decoy_target = tmp_path / "any-regular-file" + decoy_target.write_text("\n") + (unrelated_dir / ".house-lint-version").symlink_to(decoy_target) + + prepare_cache_dir(base / "1.0.0", self_ignore=False, reporter=CacheReporter()) + prune_stale_cache_dirs(base / "1.0.0", reporter=CacheReporter()) + + assert unrelated_dir.exists() + assert (unrelated_dir / "important-data.txt").exists() + + +def test_prepare_cache_dir_leaves_the_current_namespace_untouched(tmp_path: Path) -> None: + base = tmp_path / ".house-lint-cache" + current_version_dir = base / "1.0.0" + current_version_dir.mkdir(parents=True) + (current_version_dir / "existing-entry.json").write_text("{}") + + prepare_cache_dir(current_version_dir, self_ignore=True, reporter=CacheReporter()) + write_cached_result( + current_version_dir, + "content-hash", + "config-hash", + CachedFileResult(), + # Matches the `prepare_cache_dir` call above, as `write_cached_result` documents: it + # re-invokes `prepare_cache_dir` with this value on the vanished-directory retry path. + self_ignore=True, + reporter=CacheReporter(), + ) + + assert (current_version_dir / "existing-entry.json").exists() + assert (current_version_dir / "content-hash-config-hash.json").exists() + + +def test_prepare_cache_dir_is_best_effort_on_an_unusable_directory(tmp_path: Path) -> None: + blocked = tmp_path / "blocked" + blocked.write_text("not a directory") + + prepare_cache_dir( + blocked / "cache", self_ignore=True, reporter=CacheReporter() + ) # must not raise + + +def test_default_cache_base_is_unsafe_when_it_is_a_symlink(tmp_path: Path) -> None: + """A repository can ship `.house-lint-cache` as a symlink. `mkdir(exist_ok=True)` follows it, + so without this check a plain `house-lint check` on a fresh clone would write its entries and + a wildcard `.gitignore` into whatever directory outside the checkout the link names.""" + outside = tmp_path / "outside" + outside.mkdir() + root = tmp_path / "repository" + root.mkdir() + base = default_cache_base(root) + base.symlink_to(outside, target_is_directory=True) + + assert not default_cache_base_is_safe(versioned_cache_dir(base)) + assert default_cache_base_is_safe(versioned_cache_dir(default_cache_base(outside))) + + +def test_default_cache_dir_is_unsafe_when_the_version_namespace_is_a_symlink( + tmp_path: Path, +) -> None: + """Checking only the base leaves the predictable `-` child open. + + A repository can ship a perfectly real `.house-lint-cache/` directory whose *child* is the + symlink — the namespace name is derived from house-lint's own version and source hash, so it + is predictable to anyone who knows which release will run. `mkdir(parents=True, + exist_ok=True)` then succeeds against the link's target and entries land outside the + checkout, which is the exact outcome the base check exists to prevent. + """ + outside = tmp_path / "outside" + outside.mkdir() + root = tmp_path / "repository" + root.mkdir() + base = default_cache_base(root) + base.mkdir() + cache_dir = versioned_cache_dir(base) + cache_dir.symlink_to(outside, target_is_directory=True) + + assert not default_cache_base_is_safe(cache_dir) + + +def test_cache_entry_write_does_not_follow_a_symlink_at_the_temporary_path( + tmp_path: Path, +) -> None: + """The temp path is `..tmp` — derived from two hashes and a PID, all knowable. + + `Path.write_text()` follows a symlink sitting there and overwrites whatever it names before + `os.replace()` ever runs, so a repository that pre-creates one gets an arbitrary file + clobbered with cache JSON. `_write_marker_if_absent` already established `O_EXCL` as the + answer for this in the same module; the entry write has to use it too. + """ + cache_dir = tmp_path / "cache" + cache_dir.mkdir() + victim = tmp_path / "victim.txt" + victim.write_text("ORIGINAL CONTENT\n", encoding="utf-8") + content_hash, config_hash = "a" * 16, "b" * 16 + entry = cache_dir / f"{content_hash}-{config_hash}.json" + entry.with_name(f"{entry.name}.{os.getpid()}.tmp").symlink_to(victim) + + write_cached_result( + cache_dir, + content_hash, + config_hash, + CachedFileResult(findings=(), errors=(), suppressed_count=0, files_scanned=1), + self_ignore=False, + reporter=CacheReporter(), + ) + + assert victim.read_text(encoding="utf-8") == "ORIGINAL CONTENT\n" + + +def test_cache_entry_write_recovers_from_a_stale_temporary_file(tmp_path: Path) -> None: + """A crashed run can leave a real `..tmp` behind, and PIDs are reused. + + `O_EXCL` refuses to open it, so without an unlink-and-retry that entry would be permanently + unwritable — a leftover from an unrelated crash would silently disable caching for one file + forever. Unlinking is safe precisely because the retry is still `O_EXCL`: it can only ever + create, never write through something another process put there. + """ + cache_dir = tmp_path / "cache" + cache_dir.mkdir() + content_hash, config_hash = "c" * 16, "d" * 16 + entry = cache_dir / f"{content_hash}-{config_hash}.json" + entry.with_name(f"{entry.name}.{os.getpid()}.tmp").write_text("stale\n", encoding="utf-8") + + written = write_cached_result( + cache_dir, + content_hash, + config_hash, + CachedFileResult(findings=(), errors=(), suppressed_count=0, files_scanned=1), + self_ignore=False, + reporter=CacheReporter(), + ) + + assert written + assert json.loads(entry.read_text(encoding="utf-8"))["files_scanned"] == 1 + + +def test_code_identity_is_stable_within_a_process_and_shapes_the_cache_namespace( + tmp_path: Path, +) -> None: + identity = code_identity() + + assert identity == code_identity() + assert identity and identity != "unknown" + assert versioned_cache_dir(tmp_path).name == f"{__version__}-{identity}" diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 02ff3a4..5cf77de 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -2,7 +2,14 @@ import pytest -from house_lint.config import ConfigError, default_config, get_house_lint_table, load_config +from house_lint.config import ( + ConfigError, + compile_per_file_ignores, + default_config, + get_house_lint_table, + load_config, + per_file_enabled_rules, +) def test_defaults_and_cli_selection_precedence(tmp_path: Path) -> None: @@ -17,6 +24,180 @@ def test_defaults_and_cli_selection_precedence(tmp_path: Path) -> None: assert config.include == ("src", "tests", "scripts", "tools", "examples") +def test_extend_select_adds_to_configured_select_without_replacing_it(tmp_path: Path) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text('[tool.house-lint]\nselect = ["HSL001"]\nextend-select = ["HSL002"]\n') + + config = load_config(config_path) + + assert config.enabled_rules == ("HSL001", "HSL002", "HSL900") + + +def test_cli_extend_select_adds_one_rule_without_losing_the_rest_of_select( + tmp_path: Path, +) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text('[tool.house-lint]\nselect = ["HSL001", "HSL002"]\n') + + config = load_config(config_path, cli_extend_select=("HSL003",)) + + assert config.enabled_rules == ("HSL001", "HSL002", "HSL003", "HSL900") + + +def test_extend_select_also_layers_on_top_of_a_cli_select_override(tmp_path: Path) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text('[tool.house-lint]\nselect = ["HSL001"]\nextend-select = ["HSL002"]\n') + + config = load_config(config_path, cli_select=("HSL003",)) + + assert config.enabled_rules == ("HSL002", "HSL003", "HSL900") + + +def test_extend_ignore_subtracts_from_extend_select_and_configured_select( + tmp_path: Path, +) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text( + '[tool.house-lint]\nselect = ["HSL001", "HSL002"]\n' + 'extend-select = ["HSL003"]\nextend-ignore = ["HSL002", "HSL003"]\n' + ) + + config = load_config(config_path) + + assert config.enabled_rules == ("HSL001", "HSL900") + + +def test_cli_ignore_always_wins_over_extend_select(tmp_path: Path) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text('[tool.house-lint]\nselect = ["HSL001"]\n') + + config = load_config(config_path, cli_extend_select=("HSL002",), cli_ignore=("HSL002",)) + + assert config.enabled_rules == ("HSL001", "HSL900") + + +def test_default_config_extend_select_layers_on_top_of_default_select() -> None: + assert default_config(cli_extend_select=("HSL102",)).enabled_rules == ( + "HSL001", + "HSL002", + "HSL003", + "HSL004", + "HSL102", + "HSL900", + ) + + +def test_default_config_extend_select_still_enforces_hsl101_token_requirement() -> None: + with pytest.raises(ConfigError, match="HSL101 requires tokens"): + default_config(cli_extend_select=("HSL101",)) + + +def test_extend_select_and_extend_ignore_reject_duplicate_and_always_on_ids( + tmp_path: Path, +) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text('[tool.house-lint]\nextend-select = ["HSL001", "HSL001"]\n') + with pytest.raises(ConfigError, match="duplicate rule IDs"): + load_config(config_path) + + config_path.write_text('[tool.house-lint]\nextend-ignore = ["HSL900"]\n') + with pytest.raises(ConfigError, match="unknown or forbidden rule ID"): + load_config(config_path) + + +@pytest.mark.parametrize("key", ["select", "ignore", "extend-select", "extend-ignore"]) +@pytest.mark.parametrize("value", ["5", '"HSL001"']) +def test_selection_keys_reject_a_non_array_value_as_a_config_error( + key: str, value: str, tmp_path: Path +) -> None: + """`_effective_rule_selection` converts with `list(...)` before validating, so a raw TOML + value reaching it unchecked turns `select = 5` into a `TypeError` — an internal-error exit + rather than the documented config-error one — and splits `select = "HSL001"` into single + characters, reported as an unknown rule ID instead of a type problem.""" + config_path = tmp_path / "pyproject.toml" + config_path.write_text(f"[tool.house-lint]\n{key} = {value}\n") + + with pytest.raises(ConfigError, match=f"{key} must be an array"): + load_config(config_path) + + +def test_per_file_ignores_removes_rules_only_for_matching_files(tmp_path: Path) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text( + '[tool.house-lint]\nselect = ["HSL001", "HSL002"]\n' + '[tool.house-lint.per-file-ignores]\n"tests/**" = ["HSL002"]\n' + ) + + config = load_config(config_path) + + assert config.per_file_ignores == {"tests/**": ("HSL002",)} + compiled = compile_per_file_ignores(config.per_file_ignores) + assert per_file_enabled_rules(config.enabled_rules, compiled, "tests/test_foo.py") == ( + "HSL001", + "HSL900", + ) + assert per_file_enabled_rules(config.enabled_rules, compiled, "src/foo.py") == ( + "HSL001", + "HSL002", + "HSL900", + ) + + +def test_per_file_ignores_cannot_target_hsl900_or_repeat_a_rule(tmp_path: Path) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text('[tool.house-lint.per-file-ignores]\n"tests/**" = ["HSL900"]\n') + with pytest.raises(ConfigError, match="unknown or forbidden rule ID"): + load_config(config_path) + + config_path.write_text( + '[tool.house-lint.per-file-ignores]\n"tests/**" = ["HSL001", "HSL001"]\n' + ) + with pytest.raises(ConfigError, match="duplicate rule IDs"): + load_config(config_path) + + +@pytest.mark.parametrize( + "table", + [ + '[tool.house-lint.per-file-ignores]\n"../outside" = ["HSL001"]\n', + '[tool.house-lint.per-file-ignores]\n"/absolute" = ["HSL001"]\n', + '[tool.house-lint.per-file-ignores]\n"" = ["HSL001"]\n', + ], +) +def test_per_file_ignores_rejects_non_root_relative_or_empty_patterns( + tmp_path: Path, table: str +) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text(table) + with pytest.raises(ConfigError, match="root-relative|non-empty"): + load_config(config_path) + + +def test_per_file_ignores_rejects_negated_patterns(tmp_path: Path) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text('[tool.house-lint.per-file-ignores]\n"!tests/**" = ["HSL001"]\n') + with pytest.raises(ConfigError, match="must not be negated patterns"): + load_config(config_path) + + +def test_per_file_ignores_rejects_non_array_values(tmp_path: Path) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text('[tool.house-lint.per-file-ignores]\n"tests/**" = "HSL001"\n') + with pytest.raises(ConfigError, match="must be an array"): + load_config(config_path) + + +def test_per_file_ignores_rejects_invalid_gitignore_pattern(tmp_path: Path) -> None: + config_path = tmp_path / "pyproject.toml" + config_path.write_text('[tool.house-lint.per-file-ignores]\n"\\\\" = ["HSL001"]\n') + with pytest.raises(ConfigError, match="invalid Git-ignore"): + load_config(config_path) + + +def test_default_config_has_no_per_file_ignores() -> None: + assert default_config().per_file_ignores == {} + + def test_default_config_uses_the_shared_selection_precedence() -> None: assert default_config( cli_select=("HSL002", "HSL003"), cli_ignore=("HSL003",) diff --git a/tests/unit/test_discovery.py b/tests/unit/test_discovery.py index 2691e4c..55c54ac 100644 --- a/tests/unit/test_discovery.py +++ b/tests/unit/test_discovery.py @@ -133,18 +133,593 @@ def fail_gitignore(lines: Iterable[str]) -> discovery.GitIgnoreSpec: assert result.errors[0].operation == "parse" -def test_nested_gitignore_is_not_loaded(tmp_path: Path) -> None: +def test_nested_gitignore_is_applied(tmp_path: Path) -> None: + source = tmp_path / "src" + source.mkdir() + (source / ".gitignore").write_text("ignored.py\n") + (source / "ignored.py").write_text("x = 1\n") + kept = source / "kept.py" + kept.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (kept,) + # 2 skips: ignored.py (matched) + the .gitignore file itself (non-.py) + assert result.files_skipped == 2 + + +def test_nested_gitignore_patterns_are_relative_to_their_own_directory(tmp_path: Path) -> None: + source = tmp_path / "src" + sub = source / "sub" + sub.mkdir(parents=True) + # "ignored.py" without a leading slash matches at any depth under src/, + # including inside src/sub/ — same semantics as a root .gitignore. + (source / ".gitignore").write_text("ignored.py\n") + nested_ignored = sub / "ignored.py" + nested_ignored.write_text("x = 1\n") + kept = sub / "kept.py" + kept.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (kept,) + # 2 skips: ignored.py (matched) + the .gitignore file itself (non-.py) + assert result.files_skipped == 2 + + +def test_nested_gitignore_pattern_preserves_a_significant_leading_space(tmp_path: Path) -> None: + # A leading space is part of the pattern per gitwildmatch (verified directly against + # `GitIgnoreSpec`) -- it must match a file whose name itself starts with a space, not the + # same name with the space stripped. + source = tmp_path / "src" + source.mkdir() + (source / ".gitignore").write_text(" ignored.py\n") + space_prefixed = source / " ignored.py" + space_prefixed.write_text("x = 1\n") + kept = source / "ignored.py" + kept.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert kept in result.files + assert space_prefixed not in result.files + + +def test_nested_gitignore_pattern_preserves_an_escaped_trailing_space(tmp_path: Path) -> None: + # Trailing whitespace is insignificant per gitwildmatch *unless* escaped with a backslash, + # in which case it's part of the pattern -- verified directly against `GitIgnoreSpec`. + source = tmp_path / "src" + source.mkdir() + (source / ".gitignore").write_text("ignored.py\\ \n") + space_suffixed = source / "ignored.py " + space_suffixed.write_text("x = 1\n") + kept = source / "ignored.py" + kept.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert kept in result.files + assert space_suffixed not in result.files + + +def test_nested_gitignore_leading_slash_anchors_to_its_own_directory(tmp_path: Path) -> None: + source = tmp_path / "src" + sub = source / "sub" + sub.mkdir(parents=True) + # A leading slash anchors the pattern to the directory that owns the .gitignore, so it + # must match src/ignored.py but not src/sub/ignored.py. + (source / ".gitignore").write_text("/ignored.py\n") + anchored_ignored = source / "ignored.py" + anchored_ignored.write_text("x = 1\n") + not_anchored = sub / "ignored.py" + not_anchored.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert anchored_ignored not in result.files + assert not_anchored in result.files + assert result.files == (not_anchored,) + + +def test_nested_gitignore_trailing_slash_directory_pattern_matches_at_any_depth( + tmp_path: Path, +) -> None: + source = tmp_path / "src" + sub = source / "sub" + sub.mkdir(parents=True) + # A trailing-slash directory pattern with no other slash matches "build/" at any depth + # under its owning directory, same as the no-slash file case above. + (source / ".gitignore").write_text("build/\n") + direct_build = source / "build" / "direct.py" + direct_build.parent.mkdir(parents=True) + direct_build.write_text("x = 1\n") + nested_build = sub / "build" / "nested.py" + nested_build.parent.mkdir(parents=True) + nested_build.write_text("x = 1\n") + kept = sub / "kept.py" + kept.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert direct_build not in result.files + assert nested_build not in result.files + assert result.files == (kept,) + + +def test_multi_level_nested_gitignore_files_all_apply(tmp_path: Path) -> None: + source = tmp_path / "src" + sub = source / "sub" + sub.mkdir(parents=True) + (source / ".gitignore").write_text("from_src.py\n") + (sub / ".gitignore").write_text("from_sub.py\n") + (sub / "from_src.py").write_text("x = 1\n") + (sub / "from_sub.py").write_text("x = 1\n") + kept = sub / "kept.py" + kept.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (kept,) + # 4 skips: from_src.py + from_sub.py (matched) + the two .gitignore files (non-.py) + assert result.files_skipped == 4 + + +def test_nested_gitignore_negation_overrides_root_gitignore(tmp_path: Path) -> None: + (tmp_path / ".gitignore").write_text("*.py\n") + source = tmp_path / "src" + source.mkdir() + (source / ".gitignore").write_text("!important.py\n") + important = source / "important.py" + important.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (important,) + + +def test_closer_nested_gitignore_negation_overrides_a_farther_one(tmp_path: Path) -> None: + source = tmp_path / "src" + sub = source / "sub" + sub.mkdir(parents=True) + (source / ".gitignore").write_text("*.py\n") + (sub / ".gitignore").write_text("!keep.py\n") + keep = sub / "keep.py" + keep.write_text("x = 1\n") + other = sub / "other.py" + other.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (keep,) + assert other not in result.files + + +@pytest.mark.parametrize( + ("line", "expected"), + [ + # `**` (either form) must not expand to `/**/**`, which GitIgnoreSpec matches + # against the prefix directory itself and, in the `**/` form, against an immediate + # regular file that git leaves alone. + ("**/", "src/**/*/"), + ("**", "src/**/*"), + # Everything else keeps the documented per-directory semantics. + ("a.py", "src/**/a.py"), + ("!a.py", "!src/**/a.py"), + ("/a.py", "src/a.py"), + ("!/a.py", "!src/a.py"), + ("sub/", "src/**/sub/"), + ("sub/x.py", "src/sub/x.py"), + ], +) +def test_prefix_pattern_rewrites_nested_patterns_to_root_anchored_equivalents( + line: str, expected: str +) -> None: + assert discovery._prefix_pattern("src", line) == expected + + +@pytest.mark.parametrize( + ("pattern", "expected"), + [ + # A trailing `/**` names a directory's contents, never the directory itself. + ("build/**", "build/**/*"), + ("build/**/", "build/**/*/"), + ("!build/**", "!build/**/*"), + # A preceding segment ending in a single `*` is an ordinary pattern and must still be + # rewritten — only a literal `**` before the trailing `/**` is left alone. + ("a/*/**", "a/*/**/*"), + ("packages/*/dist/**", "packages/*/dist/**/*"), + # Already-explicit and unrelated patterns are left exactly as written. + ("build/**/*", "build/**/*"), + ("a/**/b.py", "a/**/b.py"), + ("**", "**"), + ("a/**/**", "a/**/**"), + ("# build/**", "# build/**"), + ("", ""), + ], +) +def test_normalize_contents_glob_only_rewrites_a_trailing_contents_glob( + pattern: str, expected: str +) -> None: + assert discovery._normalize_contents_glob(pattern) == expected + + +def test_ignored_directory_include_root_is_skipped_without_being_walked(tmp_path: Path) -> None: + # `_walk` starts *inside* an include root, so the root itself is the one directory + # `_traversable_dirs` never evaluates. A negation must not resurrect its files. + (tmp_path / ".gitignore").write_text("src/\n!*.py\n") + (tmp_path / "src").mkdir() + (tmp_path / "src" / "a.py").write_text("x = 1\n") + (tmp_path / "tools").mkdir() + kept = tmp_path / "tools" / "t.py" + kept.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src", "tools")) + + assert result.files == (kept,) + assert result.errors == () + + +def test_pruned_directory_counts_as_one_skip_not_one_per_contained_file(tmp_path: Path) -> None: + # Ignored directories are pruned rather than enumerated, which is what makes skipping a + # large `.venv`/`node_modules` cheap. The reported count follows that: one pruned + # directory contributes one skip regardless of how many files it holds. Pinned here so + # the number cannot drift silently the way it did when pruning was introduced. + (tmp_path / ".gitignore").write_text("gen/\n") + (tmp_path / "src" / "gen" / "deep").mkdir(parents=True) + (tmp_path / "src" / "a.py").write_text("x = 1\n") + for relative in ("src/gen/g1.py", "src/gen/g2.py", "src/gen/deep/g3.py"): + (tmp_path / relative).write_text("x = 1\n") + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (tmp_path / "src" / "a.py",) + assert result.files_skipped == 1 + + +def test_directory_names_with_gitignore_metacharacters_are_treated_as_literal( + tmp_path: Path, +) -> None: + bracketed = tmp_path / "sub[1]" + bracketed.mkdir() + (bracketed / ".gitignore").write_text("secret.py\n") + secret = bracketed / "secret.py" + secret.write_text("x = 1\n") + # A sibling whose name resembles what the (buggy) unescaped bracket pattern would + # actually match, proving the fix isn't just "nothing matches anymore". + lookalike = tmp_path / "sub1" + lookalike.mkdir() + (lookalike / "secret.py").write_text("x = 1\n") + + result = discover_files(tmp_path, include=("sub[1]", "sub1")) + + assert result.files == (lookalike / "secret.py",) + assert secret not in result.files + + +def test_directory_name_starting_with_bang_is_not_read_as_negation(tmp_path: Path) -> None: + source = tmp_path / "!important" + source.mkdir() + (source / ".gitignore").write_text("secret.py\n") + secret = source / "secret.py" + secret.write_text("x = 1\n") + # A control file the .gitignore does not name — without it, `result.files == ()` would + # also pass if the whole "!important" directory were (wrongly) skipped outright, which + # wouldn't prove the directory's own .gitignore was correctly read and applied to just + # the one file it names. + kept = source / "kept.py" + kept.write_text("x = 1\n") + + result = discover_files(tmp_path, include=("!important",)) + + assert result.files == (kept,) + assert secret not in result.files + + +def test_gitignored_directory_is_never_descended_so_nested_negation_cannot_resurrect_files( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Real git never reads .gitignore files inside a directory it never descends into, so a + # negation nested inside an excluded directory must not "resurrect" files under it. + source = tmp_path / "src" + source.mkdir() + (tmp_path / ".gitignore").write_text("src/generated/\n") + generated = source / "generated" + generated.mkdir() + nested_ignore = generated / ".gitignore" + nested_ignore.write_text("!foo.py\n") + foo = generated / "foo.py" + foo.write_text("x = 1\n") + kept = source / "kept.py" + kept.write_text("x = 1\n") + read_text = Path.read_text + read_calls: list[Path] = [] + + def spy_read_text(self: Path, *, encoding: str) -> str: + read_calls.append(self) + return read_text(self, encoding=encoding) + + monkeypatch.setattr(Path, "read_text", spy_read_text) + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (kept,) + assert foo not in result.files + assert result.files_skipped == 1 + # The nested .gitignore was never even read, proving we didn't descend into `generated/` + # rather than descending and merely discarding its (negating) effect afterward. + assert nested_ignore not in read_calls + + +def test_explicit_path_inside_ignored_directory_cannot_be_resurrected_by_nested_negation( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Same scenario as the walked-directory version above, but reached via an explicit path + # rather than a directory scan. Explicit paths skip `_traversable_dirs`'s walk-time pruning + # and go straight to `_combined_gitignore_spec`, which must independently refuse to read a + # nested .gitignore that lives inside an already-ignored ancestor. + source = tmp_path / "src" + source.mkdir() + (tmp_path / ".gitignore").write_text("src/generated/\n") + generated = source / "generated" + generated.mkdir() + nested_ignore = generated / ".gitignore" + nested_ignore.write_text("!foo.py\n") + foo = generated / "foo.py" + foo.write_text("x = 1\n") + read_text = Path.read_text + read_calls: list[Path] = [] + + def spy_read_text(self: Path, *, encoding: str) -> str: + read_calls.append(self) + return read_text(self, encoding=encoding) + + monkeypatch.setattr(Path, "read_text", spy_read_text) + + result = discover_files(tmp_path, explicit=(foo,)) + + assert result.files == () + assert result.files_skipped == 1 + assert nested_ignore not in read_calls + + +def test_explicit_path_inside_excluded_directory_cannot_be_resurrected_by_a_negated_exclude( + tmp_path: Path, +) -> None: + # The `exclude`-config counterpart of the .gitignore case above. A walk prunes `generated` + # and never reaches the negation, so the two entry points disagreed: a full scan skipped the + # file and naming it explicitly linted it. + generated = tmp_path / "src" / "generated" + generated.mkdir(parents=True) + foo = generated / "foo.py" + foo.write_text("x = 1\n") + excludes = ("src/generated/", "!src/generated/foo.py") + + explicit = discover_files(tmp_path, explicit=(foo,), excludes=excludes, use_gitignore=False) + walked = discover_files(tmp_path, include=("src",), excludes=excludes, use_gitignore=False) + + assert explicit.files == () + assert explicit.files_skipped == 1 + assert walked.files == () + + +def test_explicit_directory_below_an_excluded_directory_cannot_be_resurrected( + tmp_path: Path, +) -> None: + # The directory-branch counterpart. A bare `src/generated/` already matches everything + # beneath it, so the ancestor check only earns its keep once a negation re-includes the + # subdirectory: last-matching-line-wins then hands back a directory git considers excluded. + nested = tmp_path / "src" / "generated" / "nested" + nested.mkdir(parents=True) + (nested / "a.py").write_text("x = 1\n") + + result = discover_files( + tmp_path, + explicit=(nested,), + excludes=("src/generated/", "!src/generated/nested/"), + use_gitignore=False, + ) + + assert result.files == () + + +def test_a_failing_combined_spec_is_reported_once_per_directory( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`_combined_gitignore_spec` re-walks every ancestor for each directory below it, so a line + tuple that fails to parse is re-submitted with the same attribution once per descendant. + Left undeduplicated, one bad pattern produces an entry per directory in the subtree — all + naming the same offending directory — instead of the one per directory the docstring + promises.""" + (tmp_path / ".gitignore").write_text("boom.py\n") + for name in ("sub1", "sub2", "sub3"): + (tmp_path / "src" / name).mkdir(parents=True) + (tmp_path / "src" / name / "a.py").write_text("x = 1\n") + # Patched here rather than at `GitIgnoreSpec.from_lines`: `_load_gitignore_lines` validates a + # source's own raw lines, while only `_spec_for_lines` normalizes them. Failing inside the + # normalize step is therefore the one seam that reproduces "valid on its own, invalid once + # combined" — the case the combine-time handler exists for. + normalize = discovery._normalize_contents_glob # pyright: ignore[reportPrivateUsage] + + def exploding(line: str) -> str: + if line == "boom.py": + raise ValueError("bad pattern") + return normalize(line) + + monkeypatch.setattr(discovery, "_normalize_contents_glob", exploding) + + result = discover_files(tmp_path, include=("src",)) + + combine_errors = [error for error in result.errors if error.operation == "combine"] + assert combine_errors, "the combine-time parse failure must still be reported" + attributions = [error.path for error in combine_errors] + assert len(attributions) == len(set(attributions)), attributions + + +def test_directories_with_identical_accumulated_gitignore_lines_reuse_the_same_spec( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # sibling1/ and sibling2/ each have no .gitignore of their own, so both accumulate the same + # (root-only) line tuple. Parsing should happen once, not once per directory. + (tmp_path / ".gitignore").write_text("*.log\n") + source = tmp_path / "src" + sibling1 = source / "sibling1" + sibling2 = source / "sibling2" + sibling1.mkdir(parents=True) + sibling2.mkdir(parents=True) + (sibling1 / "a.py").write_text("x = 1\n") + (sibling2 / "b.py").write_text("x = 1\n") + from_lines = discovery.GitIgnoreSpec.from_lines + parse_call_count = 0 + + def counting_from_lines(lines: Iterable[str]) -> discovery.GitIgnoreSpec: + nonlocal parse_call_count + parse_call_count += 1 + return from_lines(lines) + + monkeypatch.setattr(discovery.GitIgnoreSpec, "from_lines", counting_from_lines) + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (sibling1 / "a.py", sibling2 / "b.py") + # One parse for BUILTIN_EXCLUDES, one for excludes=(), one to validate the root .gitignore's + # own lines in `_load_gitignore_lines`, and one to build the combined spec for the + # (root-only) accumulated lines shared by src/, sibling1/, and sibling2/ — the sibling + # directories reuse that fourth parse's result via `spec_by_lines_cache` instead of + # triggering a fifth and sixth call. + assert parse_call_count == 4 + + +def test_no_gitignore_disables_nested_gitignore_too(tmp_path: Path) -> None: source = tmp_path / "src" source.mkdir() (source / ".gitignore").write_text("ignored.py\n") ignored = source / "ignored.py" ignored.write_text("x = 1\n") - result = discover_files(tmp_path, include=("src",)) + result = discover_files(tmp_path, include=("src",), use_gitignore=False) assert result.files == (ignored,) +def test_nested_gitignore_applies_when_explicit_path_starts_below_it(tmp_path: Path) -> None: + source = tmp_path / "src" + source.mkdir() + (source / ".gitignore").write_text("ignored.py\n") + ignored = source / "ignored.py" + ignored.write_text("x = 1\n") + + result = discover_files(tmp_path, explicit=(ignored,)) + + assert result.files == () + assert result.files_skipped == 1 + + +def test_explicit_directory_spelled_through_dotdot_ignores_the_traversed_sibling( + tmp_path: Path, +) -> None: + """A `..` in an explicit directory must not make the directory it steps out of an ancestor. + + `src/../tests` names `tests`, whose only ignore-file ancestor is the root — `src` is not + above the resolved target and its `.gitignore` has no say. Matching the unresolved spelling + walked `src` as an ancestor and applied its patterns, so `check src/../tests` silently + skipped files that `check tests` selects. Mirrors the rule `per-file-ignores` already + follows (`docs/configuration.md`): match the resolved location, not the spelling used to + reach it. + """ + source = tmp_path / "src" + source.mkdir() + (source / ".gitignore").write_text("*.py\n") + tests = tmp_path / "tests" + tests.mkdir() + kept = tests / "a.py" + kept.write_text("x = 1\n") + + direct = discover_files(tmp_path, explicit=(tests,)) + through_dotdot = discover_files(tmp_path, explicit=(source / ".." / "tests",)) + + assert direct.files == (kept,) + assert through_dotdot.files == (kept,) + assert through_dotdot.files_skipped == 0 + + +def test_explicit_file_spelled_through_dotdot_ignores_the_traversed_sibling( + tmp_path: Path, +) -> None: + """The same rule as the directory case, on the branch an explicit *file* takes. + + Fixing only the directory branch left this one walking the unresolved spelling, so + `check src/../tests/a.py` still applied `src/.gitignore` to a file under `tests`. The two + branches have to agree: whichever way a path is named, its ignore ancestry is decided by + where it resolves to. + """ + source = tmp_path / "src" + source.mkdir() + (source / ".gitignore").write_text("*.py\n") + tests = tmp_path / "tests" + tests.mkdir() + kept = tests / "a.py" + kept.write_text("x = 1\n") + + direct = discover_files(tmp_path, explicit=(kept,)) + through_dotdot = discover_files(tmp_path, explicit=(source / ".." / "tests" / "a.py",)) + + assert direct.files == (kept,) + assert through_dotdot.files_skipped == 0 + assert [path.resolve() for path in through_dotdot.files] == [kept] + + +def test_invalid_nested_gitignore_pattern_reports_a_structured_error( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + source = tmp_path / "src" + source.mkdir() + kept = source / "kept.py" + kept.write_text("x = 1\n") + (source / ".gitignore").write_text("ignored/\n") + from_lines = discovery.GitIgnoreSpec.from_lines + + def fail_gitignore(lines: Iterable[str]) -> discovery.GitIgnoreSpec: + values = list(lines) + if values == ["ignored/"]: + raise ValueError("invalid pattern") + return from_lines(values) + + monkeypatch.setattr(discovery.GitIgnoreSpec, "from_lines", fail_gitignore) + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (kept,) + assert result.errors[0].kind == "traversal" + assert result.errors[0].path == "src/.gitignore" + assert result.errors[0].operation == "parse" + + +def test_unreadable_nested_gitignore_reports_an_error_and_keeps_reachable_files( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + source = tmp_path / "src" + source.mkdir() + kept = source / "kept.py" + kept.write_text("x = 1\n") + ignore = source / ".gitignore" + ignore.write_text("ignored/\n") + read_text = Path.read_text + + def fail_read_text(self: Path, *, encoding: str) -> str: + if self == ignore: + raise OSError("permission denied") + return read_text(self, encoding=encoding) + + monkeypatch.setattr(Path, "read_text", fail_read_text) + + result = discover_files(tmp_path, include=("src",)) + + assert result.files == (kept,) + assert result.errors[0].kind == "traversal" + assert result.errors[0].path == "src/.gitignore" + assert result.errors[0].operation == "read" + + def test_unreadable_root_gitignore_reports_an_error_and_keeps_reachable_files( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -229,6 +804,32 @@ def test_direct_symlink_file_is_safe_only_when_target_is_in_root(tmp_path: Path) outside.unlink() +def test_result_maps_each_file_to_the_resolved_target_it_validated(tmp_path: Path) -> None: + """The scan reads `resolved_paths[file]`, not its own fresh `resolve()`, so containment is + checked and the read is performed against the same target — see `SourceFile.__init__`.""" + source = tmp_path / "src" + source.mkdir() + plain = source / "a.py" + plain.write_text("value = 1\n") + link = source / "link.py" + link.symlink_to(plain) + + result = discover_files(tmp_path, explicit=(plain, link)) + + assert set(result.resolved_paths) == set(result.files) + for reported, resolved in result.resolved_paths.items(): + assert resolved == reported.resolve() + + # `selected` is keyed by resolved path, so passing both keeps `plain` and drops `link` as a + # duplicate — leaving every surviving entry with `resolved == reported` and the mapping's + # whole reason to exist unexercised. Reaching the symlink alone is the only case where the + # reported path and the validated target actually differ. + link_only = discover_files(tmp_path, explicit=(link,)) + + assert link_only.files == (link,) + assert link_only.resolved_paths == {link: plain} + + def test_walked_file_symlinks_are_not_selected(tmp_path: Path) -> None: source = tmp_path / "src" source.mkdir() diff --git a/tests/unit/test_source.py b/tests/unit/test_source.py index bd8a74f..eaec8c6 100644 --- a/tests/unit/test_source.py +++ b/tests/unit/test_source.py @@ -80,6 +80,7 @@ def test_non_regular_and_oversized_files_are_rejected_and_not_clean(tmp_path): non_regular = SourceFile(directory, tmp_path) assert non_regular.error is not None assert non_regular.error.kind == "path" + assert non_regular.content_bytes is None assert_not_clean(tmp_path, non_regular) oversized = tmp_path / "large.py" @@ -88,6 +89,9 @@ def test_non_regular_and_oversized_files_are_rejected_and_not_clean(tmp_path): assert too_large.error is not None assert too_large.error.kind == "budget" assert too_large.error.code == "source-too-large" + # The bytes are still reported even though the file is too large to analyze; it is + # `hash_source_content` that declines to key a cache entry on them. + assert too_large.content_bytes is not None assert_not_clean(tmp_path, too_large) @@ -137,6 +141,56 @@ def test_symlink_escaping_root_is_a_structured_path_error(tmp_path): assert_not_clean(root, source) +def test_source_reads_the_resolved_target_it_was_given_not_a_fresh_one(tmp_path): + """Discovery resolves a symlink to check containment, then the scan reads it. Resolving a + second time here would let a retarget landing in between send the read to a file discovery + never approved, so the resolved target is threaded through instead of recomputed.""" + root = tmp_path / "root" + root.mkdir() + approved = root / "approved.py" + approved.write_text("approved = 1\n") + swapped = root / "swapped.py" + swapped.write_text("swapped = 1\n") + link = root / "link.py" + link.symlink_to(approved) + + resolved = link.resolve() # what discovery would have validated + link.unlink() + link.symlink_to(swapped) # the retarget, landing before the read + + source = SourceFile(link, root, resolved_path=resolved) + + assert source.error is None + assert source.text == "approved = 1\n" + # Without the threaded path the same construction follows the retargeted link instead. + assert SourceFile(link, root).text == "swapped = 1\n" + + +def test_a_resolved_path_replaced_by_a_symlink_is_refused_rather_than_followed(tmp_path): + """Discovery now resolves the whole selection before the scan begins, so the gap between a + path being approved and being opened spans the run rather than a single file. A resolved + path's final component is by construction not a symlink; if it is one by the time the scan + opens it, it was swapped afterwards and must not be read. `O_NOFOLLOW` turns that into an + ordinary read error instead of a read outside the root.""" + root = tmp_path / "root" + root.mkdir() + approved = root / "approved.py" + approved.write_text("approved = 1\n") + outside = tmp_path / "outside.py" + outside.write_text("outside = 1\n") + + resolved = approved.resolve() # what discovery validated + approved.unlink() + approved.symlink_to(outside) # swapped after approval, before the read + + source = SourceFile(approved, root, resolved_path=resolved) + + assert source.error is not None + assert source.error.code == "read-error" + with pytest.raises(RuntimeError, match="source is unavailable"): + _ = source.text + + def test_escaped_symlink_source_cannot_be_read(tmp_path): root = tmp_path / "root" root.mkdir()