GSoC Module A : week 4 : feat(harvester): implement repository file filtering (stacked on top of #985)#986
GSoC Module A : week 4 : feat(harvester): implement repository file filtering (stacked on top of #985)#986ParthAggarwal16 wants to merge 23 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds harvester configuration schemas and validation, Git repository synchronization, change detection, checkpoint persistence, file filtering, metrics, repository configuration, and comprehensive unit tests with YAML fixtures. ChangesHarvester configuration and validation
Repository lifecycle and change tracking
File filtering and metrics
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
application/tests/harvester_test/file_filter_test.py (1)
25-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIsolate regex filtering in this test.
This test currently uses
.github/workflows/test.ymlto test the regex exclusion path. However, because.ymlis not an allowed extension, this file would be excluded by the extension check even if the regex missed it. To strictly verify that the regex logic works independently, consider using a file path that has an allowed extension (e.g.,.md) but is excluded by the regex.💡 Proposed adjustment
def test_regex_filtering(self): file_filter = FileFilter() result = file_filter.filter_files( [ - ".github/workflows/test.yml", + ".github/workflows/readme.md", "docs/setup.md", ] )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/tests/harvester_test/file_filter_test.py` around lines 25 - 38, Update test_regex_filtering in FileFilter so the regex-excluded fixture uses an allowed file extension, such as a .md path matching the exclusion pattern, while retaining a separate allowed .md path that should remain in the result. Ensure the assertion verifies exclusion by regex rather than by the extension filter.application/utils/harvester/file_filter.py (1)
31-35: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPre-compile the exclusion regexes.
filter_filesruns this check for every path, so compilingexclude_patternsonce in__init__avoids repeated regex parsing on the hot path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/harvester/file_filter.py` around lines 31 - 35, Pre-compile each exclusion pattern during __init__ and store the compiled regex objects on the harvester instance, then update is_excluded_by_pattern to use those compiled patterns without calling re.search for every path. Preserve the existing exclude_patterns defaults and matching behavior.application/tests/harvester_test/config_loader_test.py (1)
55-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test cases for the remaining configuration fixtures.
The
invalid_chunking_strategy.yamlandinvalid_polling_mode.yamlfixtures were added in this PR but are currently unused in the test suite. Consider adding tests to verify that these invalid enumerations are correctly caught and rejected by the schema validation.♻️ Proposed tests
def test_empty_include_paths(self): config_path = FIXTURES_DIR / "empty_include_paths.yaml" with self.assertRaises(ConfigLoaderError): load_repo_config(config_path) + + def test_invalid_chunking_strategy(self): + config_path = FIXTURES_DIR / "invalid_chunking_strategy.yaml" + + with self.assertRaisesRegex(ConfigLoaderError, "strategy"): + load_repo_config(config_path) + + def test_invalid_polling_mode(self): + config_path = FIXTURES_DIR / "invalid_polling_mode.yaml" + + with self.assertRaisesRegex(ConfigLoaderError, "mode"): + load_repo_config(config_path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/tests/harvester_test/config_loader_test.py` around lines 55 - 60, Add tests alongside test_empty_include_paths that load invalid_chunking_strategy.yaml and invalid_polling_mode.yaml through load_repo_config, asserting each raises ConfigLoaderError during schema validation. Use the existing FIXTURES_DIR pattern and preserve the current test structure.application/utils/harvester/__init__.py (1)
27-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__list.To maintain consistency and comply with
ruffrules (RUF022), it is recommended to keep the__all__list alphabetically sorted.♻️ Proposed fix
__all__ = [ - "build_repository_cache_path", "ChunkingConfig", "ConfigLoaderError", - "GitRepositoryClient", "FileFilter", - "FilteringMetricsCollector", "FilteringBenchmark", "FilteringBenchmarkResult", + "FilteringMetricsCollector", + "GitRepositoryClient", "PathRules", "PollingConfig", "RepositoryClient", "RepositoryConfig", "RepositoryValidationError", "ReposFile", + "build_repository_cache_path", "load_repo_config", "validate_repositories", ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/harvester/__init__.py` around lines 27 - 44, Sort the exported names in the __all__ list alphabetically to satisfy the RUF022 ordering rule, without changing the listed symbols or their exports.Source: Linters/SAST tools
application/tests/harvester_test/git_repository_client_test.py (1)
95-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert exact arguments in
subprocess.runmocks.Currently, tests only assert that
subprocess.runwas called. To prevent regressions where critical flags (like branch references or timeouts) are accidentally removed, consider asserting the exact list of arguments that are dispatched.💡 Example improvement
def test_checkout_runs_git_command(self, mock_run): client = GitRepositoryClient( owner="OWASP", repository="ASVS", ) client.checkout("main") - mock_run.assert_called_once() + mock_run.assert_called_once_with( + [ + "git", + "-C", + str(client.get_local_path()), + "checkout", + "--", + "main", + ], + check=True, + capture_output=True, + text=True, + timeout=300, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/tests/harvester_test/git_repository_client_test.py` around lines 95 - 104, Update test_checkout_runs_git_command for GitRepositoryClient.checkout to assert the exact arguments passed to the mocked subprocess.run, including the repository checkout command, branch reference, and any required execution options such as timeout or check settings, rather than only verifying that it was called.application/utils/harvester/git_repository_client.py (1)
42-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClear existing corrupted directories before cloning.
If the integrity check fails but
self.local_pathstill exists (e.g., from a previously aborted clone or incomplete cleanup), the subsequentgit clonecommand will fail because the target directory already exists and is not empty. Clear the directory beforehand to ensure the clone operation succeeds and the pipeline can auto-recover.♻️ Proposed fix
+ if self.local_path.exists(): + import shutil + shutil.rmtree(self.local_path) + self.local_path.parent.mkdir( parents=True, exist_ok=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/harvester/git_repository_client.py` around lines 42 - 46, Update the directory setup in the Git repository cloning flow around self.local_path to remove any existing corrupted or incomplete target directory before recreating it. Preserve parent-directory creation, then ensure self.local_path is clean and ready for git clone while retaining the existing behavior when it does not exist.application/utils/harvester/change_detector.py (1)
19-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider using
-zto safely handle filenames with spaces.By default,
git diffmay wrap filenames containing spaces or special characters in quotes. Passing the-zflag outputs NUL-terminated strings instead of newlines, which ensures parsing works reliably regardless of the repository's file naming patterns orcore.quotePathconfiguration.(Note: If you apply this refactor, remember to update the corresponding mocked output in
change_detector_test.pyto use\0instead of\n).♻️ Proposed fix
try: result = subprocess.run( [ "git", "-C", str(self.repository_client.get_local_path()), "diff", "--name-only", + "-z", commit_sha, "HEAD", ], capture_output=True, text=True, check=True, timeout=60, ) except subprocess.CalledProcessError as exc: logger.error("Git command failed: %s", exc.stderr) raise - files = [ - file_path for file_path in result.stdout.splitlines() if file_path.strip() - ] + files = [ + file_path for file_path in result.stdout.split("\0") if file_path.strip() + ] return sorted(set(files))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/harvester/change_detector.py` around lines 19 - 43, Update the git diff invocation in the change-detection method to include the `-z` option, then parse `result.stdout` using NUL delimiters while retaining filtering and deduplication. Update the corresponding mocked output in `change_detector_test.py` to use `\0` separators.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@application/tests/harvester_test/filtering_benchmark_test.py`:
- Around line 11-46: Update FilteringBenchmarkTests.test_filtering_benchmark to
instantiate FilteringBenchmark and invoke its run() method with the sample
files, then assert the returned metrics. Remove the direct
FileFilter.filter_files() call and manual FilteringMetrics construction so the
test exercises the actual benchmark implementation.
In `@application/utils/harvester/checkpoint_store.py`:
- Around line 55-64: Update the checkpoint-saving logic around
self.checkpoint_file and os.replace to generate a unique temporary filename per
save, using a UUID or process/thread-specific identifier instead of the
hardcoded ".tmp" suffix. Keep the existing JSON serialization and atomic
replacement behavior unchanged.
In `@application/utils/harvester/filtering_benchmark.py`:
- Around line 17-23: Remove the unused metrics parameter from
FilteringBenchmark.__init__ and delete the corresponding self.metrics
assignment. Update all call sites to construct FilteringBenchmark with only the
required file_filter argument, preserving any metrics computed dynamically by
run().
In `@application/utils/harvester/git_repository_client.py`:
- Around line 140-151: Update sync so that after verify_repository_integrity()
succeeds and fetch() completes, the local working tree and HEAD are reset to the
fetched remote branch before change detection runs. Preserve the existing clone
path for invalid repositories and use the repository’s configured branch and
remote-tracking reference.
---
Nitpick comments:
In `@application/tests/harvester_test/config_loader_test.py`:
- Around line 55-60: Add tests alongside test_empty_include_paths that load
invalid_chunking_strategy.yaml and invalid_polling_mode.yaml through
load_repo_config, asserting each raises ConfigLoaderError during schema
validation. Use the existing FIXTURES_DIR pattern and preserve the current test
structure.
In `@application/tests/harvester_test/file_filter_test.py`:
- Around line 25-38: Update test_regex_filtering in FileFilter so the
regex-excluded fixture uses an allowed file extension, such as a .md path
matching the exclusion pattern, while retaining a separate allowed .md path that
should remain in the result. Ensure the assertion verifies exclusion by regex
rather than by the extension filter.
In `@application/tests/harvester_test/git_repository_client_test.py`:
- Around line 95-104: Update test_checkout_runs_git_command for
GitRepositoryClient.checkout to assert the exact arguments passed to the mocked
subprocess.run, including the repository checkout command, branch reference, and
any required execution options such as timeout or check settings, rather than
only verifying that it was called.
In `@application/utils/harvester/__init__.py`:
- Around line 27-44: Sort the exported names in the __all__ list alphabetically
to satisfy the RUF022 ordering rule, without changing the listed symbols or
their exports.
In `@application/utils/harvester/change_detector.py`:
- Around line 19-43: Update the git diff invocation in the change-detection
method to include the `-z` option, then parse `result.stdout` using NUL
delimiters while retaining filtering and deduplication. Update the corresponding
mocked output in `change_detector_test.py` to use `\0` separators.
In `@application/utils/harvester/file_filter.py`:
- Around line 31-35: Pre-compile each exclusion pattern during __init__ and
store the compiled regex objects on the harvester instance, then update
is_excluded_by_pattern to use those compiled patterns without calling re.search
for every path. Preserve the existing exclude_patterns defaults and matching
behavior.
In `@application/utils/harvester/git_repository_client.py`:
- Around line 42-46: Update the directory setup in the Git repository cloning
flow around self.local_path to remove any existing corrupted or incomplete
target directory before recreating it. Preserve parent-directory creation, then
ensure self.local_path is clean and ready for git clone while retaining the
existing behavior when it does not exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 62a102b9-a5e0-4bd5-ae8d-34913933cba9
📒 Files selected for processing (37)
application/tests/harvester_test/__init__.pyapplication/tests/harvester_test/change_detector_test.pyapplication/tests/harvester_test/checkpoint_store_test.pyapplication/tests/harvester_test/config_loader_test.pyapplication/tests/harvester_test/file_filter_test.pyapplication/tests/harvester_test/filtering_benchmark_test.pyapplication/tests/harvester_test/filtering_metrics_test.pyapplication/tests/harvester_test/fixtures/duplicate_include_paths.yamlapplication/tests/harvester_test/fixtures/duplicate_repo_ids.yamlapplication/tests/harvester_test/fixtures/duplicate_repositories.yamlapplication/tests/harvester_test/fixtures/empty_include_paths.yamlapplication/tests/harvester_test/fixtures/empty_owner.yamlapplication/tests/harvester_test/fixtures/invalid_chunk_size.yamlapplication/tests/harvester_test/fixtures/invalid_chunking_strategy.yamlapplication/tests/harvester_test/fixtures/invalid_missing_id.yamlapplication/tests/harvester_test/fixtures/invalid_polling_interval.yamlapplication/tests/harvester_test/fixtures/invalid_polling_mode.yamlapplication/tests/harvester_test/fixtures/invalid_yaml.yamlapplication/tests/harvester_test/fixtures/valid_repos.yamlapplication/tests/harvester_test/git_repository_client_test.pyapplication/tests/harvester_test/repos_validator_test.pyapplication/tests/harvester_test/repository_cache_test.pyapplication/utils/harvester/__init__.pyapplication/utils/harvester/change_detector.pyapplication/utils/harvester/checkpoint_store.pyapplication/utils/harvester/config_loader.pyapplication/utils/harvester/exclude_patterns.txtapplication/utils/harvester/file_filter.pyapplication/utils/harvester/filtering_benchmark.pyapplication/utils/harvester/filtering_metrics.pyapplication/utils/harvester/git_repository_client.pyapplication/utils/harvester/models.pyapplication/utils/harvester/repos.yamlapplication/utils/harvester/repos_validator.pyapplication/utils/harvester/repository_cache.pyapplication/utils/harvester/repository_client.pyapplication/utils/harvester/schemas.py
| self.allowed_extensions = allowed_extensions or DEFAULT_ALLOWED_EXTENSIONS | ||
|
|
||
| def is_excluded_by_pattern(self, file_path: str) -> bool: | ||
| return any(re.search(pattern, file_path) for pattern in self.exclude_patterns) |
There was a problem hiding this comment.
Blocker: this API receives glob patterns but interprets them as regular expressions. PathRules.exclude, repos.yaml, and exclude_patterns.txt define values such as **/archive/**; feeding that value to re.search() can raise re.error, so the expected config-to-filter path is unusable. Apply the fix by making globs the single public contract: normalize each repository-relative path to POSIX separators, validate exclusion globs during configuration loading/construction, and match them with one glob-aware helper rather than re.search(). Wire repository.paths.exclude through that helper and add a regression test using the actual **/archive/** configuration. Do not maintain a second regex syntax for the same setting.
| ".adoc", | ||
| } | ||
|
|
||
| DEFAULT_EXCLUDE_PATTERNS = [ |
There was a problem hiding this comment.
These defaults duplicate—but do not match—the existing exclusion configuration. Root anchors miss nested directories, and the list omits configured paths such as archive, .cursor, and .claude. Apply the fix by defining one canonical immutable set of default globs, then combine it with each repository's configured exclusions through the same matching path. Ensure directory exclusions apply at repository root and any nesting depth. Add allowed-extension cases for .github/README.md, packages/site/node_modules/README.md, docs/archive/old.md, and .cursor/rules/project.md. The partial nested-pattern adjustment in #987 does not resolve the duplicated contract or all omitted paths.
| exclude_patterns: list[str] | None = None, | ||
| allowed_extensions: set[str] | None = None, | ||
| ): | ||
| self.exclude_patterns = exclude_patterns or DEFAULT_EXCLUDE_PATTERNS |
There was a problem hiding this comment.
Using or discards intentional empty overrides, while assigning the module-level list/set directly shares mutable state between instances. Apply the fix by distinguishing absence explicitly: use a copy of defaults only when the argument is None; otherwise copy the supplied collection even when it is empty. Do the same for allowed_extensions (for example, list(defaults) if exclude_patterns is None else list(exclude_patterns) and the equivalent set(...)). Add tests proving empty overrides remain empty and mutations in one FileFilter instance cannot affect another.
|
|
||
| result = file_filter.filter_files( | ||
| [ | ||
| ".github/workflows/test.yml", |
There was a problem hiding this comment.
This does not test exclusion matching because .yml is already rejected by the extension allowlist; the combined test has the same problem with .js. Replace excluded examples with allowed documentation extensions so removing exclusion matching would make the test fail, e.g. .github/workflows/README.md and node_modules/react/README.md. Add focused cases for nested paths, the real **/archive/** custom glob, invalid glob handling, explicit empty exclusions/extensions, and default-instance isolation. Keep extension and path-filter behavior in separate tests so each assertion has one reason to pass.
2 participants
Summary
(this PR is stacked on top of #985 )
This PR adds the file filtering layer for the harvester pipeline, ensuring that only relevant documentation files continue through the harvesting process.
Added
Filtering Behavior
Allowed extensions
.md.mdx.rst.txt.adocDefault excluded paths
.github/.git/node_modules/dist/build/coverage/vendor/package-lock.jsonyarn.lockpnpm-lock.yamlValidation Coverage
File Filtering
Metrics
Benchmark
Test Plan
Executed:
python3 -m unittest discover \ -s application/tests/harvester_test \ -p "*_test.py" make testAll tests passing.
Notes for Reviewers
FileFilterconstructor.