From 61f9f74140515bdab8d8673da32cbee5ab7d1114 Mon Sep 17 00:00:00 2001 From: Chintan Gohil Date: Fri, 3 Apr 2026 00:12:45 +0530 Subject: [PATCH 1/3] fix: extend default ignore patterns and fix nested path matching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `_should_ignore` function only used `fnmatch.fnmatch()` which matches from the start of the path. This means nested dependency directories like `packages/app/node_modules/react/index.js` were NOT ignored — only top-level `node_modules/` was caught. This is a problem in monorepos, workspaces, and any project with nested dependency directories. The graph would parse thousands of third-party files, inflating build time and polluting blast radius queries. Fix: check if any path segment matches the pattern prefix (e.g., if "node_modules" appears anywhere in the path parts). Also extends DEFAULT_IGNORE_PATTERNS to cover common frameworks: - PHP/Laravel: vendor/, storage/, bootstrap/cache/, public/build/ - Ruby/Rails: vendor/bundle/, .bundle/ - Java/Kotlin: .gradle/, *.jar - .NET/C#: bin/, obj/, packages/ - Dart/Flutter: .dart_tool/, .pub-cache/ - General: coverage/, .cache/, tmp/, .nuxt/ --- code_review_graph/incremental.py | 58 +++++++++++++++++++++++++++++--- tests/test_incremental.py | 33 ++++++++++++++++++ 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/code_review_graph/incremental.py b/code_review_graph/incremental.py index af72d29a..673d73b4 100644 --- a/code_review_graph/incremental.py +++ b/code_review_graph/incremental.py @@ -24,26 +24,56 @@ # Default ignore patterns (in addition to .gitignore) DEFAULT_IGNORE_PATTERNS = [ ".code-review-graph/**", - "node_modules/**", ".git/**", + # JavaScript / TypeScript / Node + "node_modules/**", + ".next/**", + ".nuxt/**", + # Python "__pycache__/**", "*.pyc", ".venv/**", "venv/**", + # PHP / Laravel / Composer + "vendor/**", + "storage/**", + "bootstrap/cache/**", + "public/build/**", + # Ruby / Rails + "vendor/bundle/**", + ".bundle/**", + # Java / Kotlin / Gradle / Maven + ".gradle/**", + "*.jar", + # .NET / C# + "bin/**", + "obj/**", + "packages/**", + # Rust + "target/**", + # Dart / Flutter + ".dart_tool/**", + ".pub-cache/**", + # Build outputs "dist/**", "build/**", - ".next/**", - "target/**", + "coverage/**", + # Minified / generated "*.min.js", "*.min.css", "*.map", "*.lock", "package-lock.json", "yarn.lock", + # Database files "*.db", "*.sqlite", "*.db-journal", "*.db-wal", + # Misc + ".cache/**", + ".tmp/**", + "tmp/**", ] @@ -116,8 +146,26 @@ def _load_ignore_patterns(repo_root: Path) -> list[str]: def _should_ignore(path: str, patterns: list[str]) -> bool: - """Check if a path matches any ignore pattern.""" - return any(fnmatch.fnmatch(path, p) for p in patterns) + """Check if a path matches any ignore pattern. + + For ** patterns like 'node_modules/**', matches any path segment — not just + the root. This ensures nested dependency directories (e.g., + 'packages/app/node_modules/react/index.js') are correctly ignored in + monorepos and workspaces. + """ + from pathlib import PurePosixPath + + pp = PurePosixPath(path) + for p in patterns: + if "**" in p: + prefix = p.split("/**")[0] + if any(part == prefix for part in pp.parts) or fnmatch.fnmatch(path, p): + return True + elif fnmatch.fnmatch(path, p) or any( + fnmatch.fnmatch(part, p) for part in pp.parts + ): + return True + return False def _is_binary(path: Path) -> bool: diff --git a/tests/test_incremental.py b/tests/test_incremental.py index ec3184fa..e0113f05 100644 --- a/tests/test_incremental.py +++ b/tests/test_incremental.py @@ -101,6 +101,39 @@ def test_should_ignore_matches(self): assert _should_ignore(".git/HEAD", patterns) assert not _should_ignore("src/main.py", patterns) + def test_should_ignore_nested_paths(self): + """Nested dependency dirs (monorepos/workspaces) must be ignored.""" + patterns = ["node_modules/**", "vendor/**", "storage/**"] + # Nested node_modules (monorepo workspace) + assert _should_ignore("packages/app/node_modules/react/index.js", patterns) + # Nested vendor (PHP monorepo) + assert _should_ignore("services/api/vendor/guzzlehttp/Client.php", patterns) + # Nested storage + assert _should_ignore("app/storage/logs/laravel.log", patterns) + # Source files must not be affected + assert not _should_ignore("packages/app/src/main.ts", patterns) + assert not _should_ignore("src/vendors/custom.php", patterns) + + def test_should_ignore_framework_patterns(self): + """Framework-specific dirs from DEFAULT_IGNORE_PATTERNS.""" + from code_review_graph.incremental import DEFAULT_IGNORE_PATTERNS + + patterns = DEFAULT_IGNORE_PATTERNS + # PHP / Laravel + assert _should_ignore("vendor/laravel/framework/src/Collection.php", patterns) + assert _should_ignore("storage/logs/laravel.log", patterns) + assert _should_ignore("bootstrap/cache/packages.php", patterns) + # .NET + assert _should_ignore("bin/Debug/net8.0/app.dll", patterns) + assert _should_ignore("obj/Release/app.assets.cache", patterns) + # Dart / Flutter + assert _should_ignore(".dart_tool/package_config.json", patterns) + # Java / Gradle + assert _should_ignore(".gradle/caches/transforms-3/file.jar", patterns) + # Source files untouched + assert not _should_ignore("src/app/page.tsx", patterns) + assert not _should_ignore("app/Http/Controllers/UserController.php", patterns) + class TestIsBinary: def test_text_file_is_not_binary(self, tmp_path): From b7c279306cddaad50c86c9a3a4ffcde2c6c8c1c8 Mon Sep 17 00:00:00 2001 From: Chintan Gohil Date: Thu, 9 Apr 2026 18:08:26 +0530 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20safe-anywhere=20vs=20root-relative=20patterns?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses @tirth8205's review concerns: 1. `packages/**` in defaults breaks monorepos (npm workspaces, Lerna, Turborepo all use `packages/` for source code). Removed from defaults; .NET users can add via `.code-review-graphignore`. 2. `PurePosixPath.parts` matching anywhere was too aggressive — `packages/app/src/main.ts` would match `packages/**`. Split patterns into two explicit styles: - `**/name/**` — matches `name` as any path segment (safe-anywhere). Used only for dirs that are NEVER source code: node_modules, __pycache__, .venv, venv, vendor, .bundle, .gradle, .dart_tool, .pub-cache, .cache. - `name/**` — matches only at repo root. Used for dirs that may be valid source names: bin/, obj/, build/, dist/, target/, .next/, .nuxt/, storage/, bootstrap/cache/, public/build/, coverage/, tmp/, .tmp/. 3. Moved `from pathlib import PurePosixPath` to module level (it was inside the hot-path `_should_ignore` function). Multi-segment root prefixes like `bootstrap/cache/**` are also supported correctly. Test coverage added for: - Safe-anywhere matching nested dependency dirs - Root-relative patterns NOT matching nested `name/` dirs - Multi-segment prefix matching - Monorepo source code (packages/app/src/) preserved --- code_review_graph/incremental.py | 106 +++++++++++++++++-------------- tests/test_incremental.py | 60 +++++++++++------ 2 files changed, 101 insertions(+), 65 deletions(-) diff --git a/code_review_graph/incremental.py b/code_review_graph/incremental.py index 673d73b4..6d10ad75 100644 --- a/code_review_graph/incremental.py +++ b/code_review_graph/incremental.py @@ -13,7 +13,7 @@ import re import subprocess import time -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Optional from .graph import GraphStore @@ -22,58 +22,62 @@ logger = logging.getLogger(__name__) # Default ignore patterns (in addition to .gitignore) +# +# Two pattern styles: +# 1. `**/name/**` — matches `name` as any path segment (safe-anywhere). +# Use only for directories that are NEVER legitimate source code names +# (node_modules, __pycache__, .venv, vendor, .gradle, .dart_tool, etc.). +# 2. `name/**` — matches only at repo root. +# Use for directories that MAY be valid source names in some projects +# (packages/, bin/, build/, dist/, storage/, obj/). DEFAULT_IGNORE_PATTERNS = [ + # Tool-owned (always safe anywhere) ".code-review-graph/**", ".git/**", - # JavaScript / TypeScript / Node - "node_modules/**", + # Dependency directories — never source code, safe to match anywhere + "**/node_modules/**", + "**/__pycache__/**", + "**/.venv/**", + "**/venv/**", + "**/vendor/**", # Composer (PHP), Go modules, Ruby/Rails + "**/.bundle/**", # Ruby Bundler + "**/.gradle/**", # Gradle cache + "**/.dart_tool/**", # Dart/Flutter + "**/.pub-cache/**", # Dart/Flutter + "**/.cache/**", + # Framework build/output dirs — at repo root only to avoid matching + # legit source dirs (e.g. src/build/, packages/, etc.) ".next/**", ".nuxt/**", - # Python - "__pycache__/**", - "*.pyc", - ".venv/**", - "venv/**", - # PHP / Laravel / Composer - "vendor/**", - "storage/**", - "bootstrap/cache/**", - "public/build/**", - # Ruby / Rails - "vendor/bundle/**", - ".bundle/**", - # Java / Kotlin / Gradle / Maven - ".gradle/**", - "*.jar", - # .NET / C# - "bin/**", - "obj/**", - "packages/**", - # Rust - "target/**", - # Dart / Flutter - ".dart_tool/**", - ".pub-cache/**", - # Build outputs "dist/**", "build/**", + "target/**", # Rust (also used by Maven/SBT) + "bin/**", # .NET (root only; src/bin/ not ignored) + "obj/**", # .NET + # NOTE: `packages/**` (.NET NuGet) is NOT included because it conflicts + # with npm/Lerna/Turborepo monorepos where packages/ holds source code. + # .NET users should add it via .code-review-graphignore. + "storage/**", # Laravel + "bootstrap/cache/**", # Laravel + "public/build/**", # Laravel Mix/Vite output "coverage/**", - # Minified / generated + ".tmp/**", + "tmp/**", + # Python cache file + "*.pyc", + # Minified / generated single files "*.min.js", "*.min.css", "*.map", "*.lock", "package-lock.json", "yarn.lock", + "*.jar", # Java compiled # Database files "*.db", "*.sqlite", "*.db-journal", "*.db-wal", - # Misc - ".cache/**", - ".tmp/**", - "tmp/**", ] @@ -148,22 +152,30 @@ def _load_ignore_patterns(repo_root: Path) -> list[str]: def _should_ignore(path: str, patterns: list[str]) -> bool: """Check if a path matches any ignore pattern. - For ** patterns like 'node_modules/**', matches any path segment — not just - the root. This ensures nested dependency directories (e.g., - 'packages/app/node_modules/react/index.js') are correctly ignored in - monorepos and workspaces. + Pattern semantics: + - ``**/name/**`` — matches ``name`` as any path segment (safe-anywhere). + Use for dirs that are never valid source code (node_modules, vendor). + - ``name/**`` — matches only at the repo root (first segment is ``name``). + Use for dirs that may be valid source names in some projects (packages, + bin, build). + - ``*.ext`` and other non-``**`` patterns fall back to ``fnmatch``. """ - from pathlib import PurePosixPath - - pp = PurePosixPath(path) + parts = PurePosixPath(path).parts for p in patterns: - if "**" in p: - prefix = p.split("/**")[0] - if any(part == prefix for part in pp.parts) or fnmatch.fnmatch(path, p): + # Safe-anywhere: **/name/** + if p.startswith("**/") and p.endswith("/**"): + segment = p[3:-3] + if segment in parts: + return True + # Root-relative: name/** — matches only if first segment is `name` + elif p.endswith("/**"): + prefix = p[:-3] + # Support multi-segment prefixes like "bootstrap/cache" + prefix_parts = prefix.split("/") + if len(parts) >= len(prefix_parts) and tuple(parts[: len(prefix_parts)]) == tuple(prefix_parts): return True - elif fnmatch.fnmatch(path, p) or any( - fnmatch.fnmatch(part, p) for part in pp.parts - ): + # Plain glob (e.g. *.pyc, *.min.js) + elif fnmatch.fnmatch(path, p): return True return False diff --git a/tests/test_incremental.py b/tests/test_incremental.py index e0113f05..4addb5de 100644 --- a/tests/test_incremental.py +++ b/tests/test_incremental.py @@ -80,9 +80,9 @@ def test_cleans_legacy_side_files(self, tmp_path): class TestIgnorePatterns: def test_default_patterns_loaded(self, tmp_path): patterns = _load_ignore_patterns(tmp_path) - assert "node_modules/**" in patterns + assert "**/node_modules/**" in patterns assert ".git/**" in patterns - assert "__pycache__/**" in patterns + assert "**/__pycache__/**" in patterns def test_custom_ignore_file(self, tmp_path): ignore = tmp_path / ".code-review-graphignore" @@ -95,43 +95,67 @@ def test_custom_ignore_file(self, tmp_path): assert "" not in patterns def test_should_ignore_matches(self): - patterns = ["node_modules/**", "*.pyc", ".git/**"] + patterns = ["**/node_modules/**", "*.pyc", ".git/**"] assert _should_ignore("node_modules/foo/bar.js", patterns) assert _should_ignore("test.pyc", patterns) assert _should_ignore(".git/HEAD", patterns) assert not _should_ignore("src/main.py", patterns) - def test_should_ignore_nested_paths(self): - """Nested dependency dirs (monorepos/workspaces) must be ignored.""" - patterns = ["node_modules/**", "vendor/**", "storage/**"] - # Nested node_modules (monorepo workspace) + def test_safe_anywhere_matches_nested_paths(self): + """**/name/** patterns match nested dependency dirs (monorepos).""" + patterns = ["**/node_modules/**", "**/vendor/**", "**/__pycache__/**"] + # Nested node_modules (npm workspaces, Lerna, Turborepo) assert _should_ignore("packages/app/node_modules/react/index.js", patterns) # Nested vendor (PHP monorepo) assert _should_ignore("services/api/vendor/guzzlehttp/Client.php", patterns) - # Nested storage - assert _should_ignore("app/storage/logs/laravel.log", patterns) - # Source files must not be affected + # Nested __pycache__ + assert _should_ignore("src/utils/__pycache__/helpers.cpython-311.pyc", patterns) + # Actual source code must not be affected assert not _should_ignore("packages/app/src/main.ts", patterns) assert not _should_ignore("src/vendors/custom.php", patterns) + def test_root_relative_patterns_dont_match_nested(self): + """name/** patterns should only match at root, not nested `name/` dirs.""" + patterns = ["packages/**", "bin/**", "build/**"] + # Root-level matches + assert _should_ignore("packages/nuget-cache/lib.dll", patterns) + assert _should_ignore("bin/Debug/net8.0/app.dll", patterns) + assert _should_ignore("build/output.txt", patterns) + # Nested `packages/` in monorepo must NOT match + assert not _should_ignore("apps/web/packages/src/main.ts", patterns) + assert not _should_ignore("services/api/bin/helper.sh", patterns) + assert not _should_ignore("docs/build/page.md", patterns) + + def test_multi_segment_root_prefix(self): + """Multi-segment patterns like `bootstrap/cache/**` match only at root.""" + patterns = ["bootstrap/cache/**"] + assert _should_ignore("bootstrap/cache/packages.php", patterns) + assert not _should_ignore("src/bootstrap/cache/file.php", patterns) + def test_should_ignore_framework_patterns(self): """Framework-specific dirs from DEFAULT_IGNORE_PATTERNS.""" from code_review_graph.incremental import DEFAULT_IGNORE_PATTERNS patterns = DEFAULT_IGNORE_PATTERNS - # PHP / Laravel + # Safe-anywhere: dependency dirs never used as source assert _should_ignore("vendor/laravel/framework/src/Collection.php", patterns) + assert _should_ignore("services/api/vendor/pkg/file.go", patterns) # nested + assert _should_ignore(".dart_tool/package_config.json", patterns) + assert _should_ignore(".gradle/caches/transforms-3/file.jar", patterns) + assert _should_ignore("node_modules/react/index.js", patterns) + assert _should_ignore("apps/api/node_modules/foo.js", patterns) # nested + # Root-only: Laravel framework dirs assert _should_ignore("storage/logs/laravel.log", patterns) assert _should_ignore("bootstrap/cache/packages.php", patterns) - # .NET + assert _should_ignore("public/build/assets/app.js", patterns) + # Root-only: .NET assert _should_ignore("bin/Debug/net8.0/app.dll", patterns) assert _should_ignore("obj/Release/app.assets.cache", patterns) - # Dart / Flutter - assert _should_ignore(".dart_tool/package_config.json", patterns) - # Java / Gradle - assert _should_ignore(".gradle/caches/transforms-3/file.jar", patterns) - # Source files untouched - assert not _should_ignore("src/app/page.tsx", patterns) + # Monorepo-safe: `packages/` is NOT in defaults (npm/Lerna/Turborepo + # use it for source code; .NET users must add it to .code-review-graphignore) + assert not _should_ignore("packages/app/src/main.ts", patterns) + assert not _should_ignore("packages/Newtonsoft.Json.13.0.1/lib.dll", patterns) + assert not _should_ignore("apps/web/src/main.tsx", patterns) assert not _should_ignore("app/Http/Controllers/UserController.php", patterns) From a6635cdd0b252462f4e40591279649ef8b492982 Mon Sep 17 00:00:00 2001 From: Chintan Gohil Date: Sun, 12 Apr 2026 18:39:43 +0530 Subject: [PATCH 3/3] style: wrap long line in _should_ignore to stay under 100-char limit --- code_review_graph/incremental.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/code_review_graph/incremental.py b/code_review_graph/incremental.py index 6d10ad75..c4169f68 100644 --- a/code_review_graph/incremental.py +++ b/code_review_graph/incremental.py @@ -172,7 +172,10 @@ def _should_ignore(path: str, patterns: list[str]) -> bool: prefix = p[:-3] # Support multi-segment prefixes like "bootstrap/cache" prefix_parts = prefix.split("/") - if len(parts) >= len(prefix_parts) and tuple(parts[: len(prefix_parts)]) == tuple(prefix_parts): + if ( + len(parts) >= len(prefix_parts) + and parts[: len(prefix_parts)] == tuple(prefix_parts) + ): return True # Plain glob (e.g. *.pyc, *.min.js) elif fnmatch.fnmatch(path, p):