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 18 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe PR adds database-backed ingest and checkpoint persistence, refactors Git repository synchronization and commit change detection, introduces file filtering and metrics utilities, exports harvester APIs, and adds unit and integration coverage. Ingest persistence
Harvester processing
Estimated code review effort: 4 (Complex) | ~60 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.
There was a problem hiding this comment.
i looked into invalid glob handling, i tried using malformed patterns such as [ expecting pathspec to reject them, but pathspec.PathSpec.from_lines() accepts them without raising an exception, so I couldn’t find a practical invalid glob case to test against, is there a specific pattern you had in mind?
|
Status: not mergeable (no push from us). Open blockers still apply:
Also: this branch is on the stale pre-#985 week-3 tip. Rebase onto the fixed #985 tip after that lands, then address filter blockers. |
|
Still blocked: no new commits since the filter blockers, and branch still conflicts / sits on the old week-3 tip. After #985 is fixed and merged, please rebase `week_4-clean` onto that tip and address the open `file_filter` threads. |
10b8f2f to
a13641a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
application/utils/harvester/__init__.py (1)
27-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSort
__all__to satisfy Ruff RUF022.Reorder the exports using isort-style sorting so
make lintpasses.As per coding guidelines,
**/*.{py,ts,tsx,js,jsx}changes must passmake lint.🤖 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, Reorder the entries in the __all__ list using isort-style alphabetical sorting to satisfy Ruff RUF022, without changing the exported symbols.Sources: Coding guidelines, Linters/SAST tools
application/utils/harvester/git_repository_client.py (2)
54-98: 🎯 Functional Correctness | 🟠 MajorSubprocess calls only catch
CalledProcessError, notTimeoutExpired.Every git subprocess call in this file sets
timeout=300but only wraps the call inexcept subprocess.CalledProcessError. If a timeout fires,subprocess.TimeoutExpiredpropagates uncaught. This is especially problematic foris_valid_repository()/verify_repository_integrity()(lines 227-285), whose callers (e.g.sync()at line 196) expect a plainbool— an uncaught timeout there crashes the sync flow instead of falling back toclone().This is the same pattern as
change_detector.py's subprocess calls; see the consolidated comment for the combined fix.Also applies to: 100-146, 147-180, 201-225, 227-285
🤖 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 54 - 98, Handle subprocess.TimeoutExpired alongside CalledProcessError for every timed git invocation in _clone_atomically, the fetch/sync paths, is_valid_repository, and verify_repository_integrity. Log timeout failures consistently, and ensure is_valid_repository and verify_repository_integrity return False on timeout so sync() can follow its existing fallback flow rather than propagating the exception.
16-33: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winOwner/repository validation is bypassed when
local_pathis supplied explicitly.
_VALID_COMPONENTvalidation forowner/repositoryonly happens as a side effect of callingbuild_repository_cache_path(line 32), which is skipped whenever a caller supplieslocal_pathdirectly (line 30-31). This means arbitrary, unsanitizedowner/repositorystrings can flow intorepository_url(line 37) and into git subprocess arguments elsewhere, with no defense-in-depth if a future caller passes attacker-influenced values alongside an explicitlocal_path.🔒️ Proposed fix
+from .repository_cache import _VALID_COMPONENT # or expose a validate_repository_identity() helper + class GitRepositoryClient(RepositoryClient): def __init__( self, owner: str, repository: str, branch: str = "main", local_path: Path | None = None, ) -> None: if branch.startswith("-"): raise ValueError("Invalid git branch") + if not _VALID_COMPONENT.fullmatch(owner): + raise ValueError(f"Invalid repository owner: {owner}") + if not _VALID_COMPONENT.fullmatch(repository): + raise ValueError(f"Invalid repository name: {repository}") self.owner = owner self.repository = repository self.branch = branch🤖 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 16 - 33, Validate owner and repository with the existing _VALID_COMPONENT checks at the start of GitRepositoryClient.__init__, before the local_path conditional, so validation occurs whether or not a cache path is supplied. Preserve the existing branch validation and build_repository_cache_path behavior for callers without an explicit local_path.
🧹 Nitpick comments (3)
application/utils/harvester/checkpoint_store.py (1)
70-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve exception chain when converting
IntegrityErrortoValueError.Static analysis flags the bare
raise ValueError(...)here as losing the original exception context.🧹 Proposed fix
except IntegrityError: session.rollback() - raise ValueError("duplicate canonical source identity") + raise ValueError("duplicate canonical source identity") from excAs per static analysis hints,
ruffflags: "Within anexceptclause, raise exceptions withraise ... from errorraise ... from None."🤖 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/checkpoint_store.py` around lines 70 - 78, Update the IntegrityError handler in the checkpoint store commit flow to raise the duplicate-identity ValueError explicitly from the caught IntegrityError, preserving the exception chain; keep the rollback and generic exception re-raise behavior unchanged.Source: Linters/SAST tools
application/utils/harvester/repository_cache.py (1)
12-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEmpty
branchcollapses into the parent path, unlikeowner/repository.
owner/repositorymust match_VALID_COMPONENT(non-empty, restricted charset), butbranchis only checked against{".", ".."}. An empty string passesquote("", safe="")unchanged (""), and joining aPathwith""adds no new segment, socandidatecollapses toCACHE_ROOT/owner/repo— colliding with any other branch's expectations of a per-branch cache directory. Consider validatingbranchagainst the same/similar pattern asowner/repositorybefore quoting.🐛 Proposed fix
- if branch in {".", ".."}: - raise ValueError("Invalid branch name") + if not branch or branch in {".", ".."}: + raise ValueError("Invalid branch name")🤖 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/repository_cache.py` around lines 12 - 29, Update build_repository_cache_path to validate branch as a non-empty, restricted path component before calling quote, using _VALID_COMPONENT or an equivalent branch-specific pattern. Preserve the existing rejection of "." and "..", and ensure every accepted branch produces its own cache path segment.application/tests/harvester_test/git_repository_client_test.py (1)
55-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests write real files under the project's default cache path.
test_sync_clones_when_repository_missing,test_sync_fetches_when_repository_exists, andtest_clone_runs_git_commandall constructGitRepositoryClientwithout an explicitlocal_path, sosync()/clone()create real directories/lock files under.harvester_cache/owasp/asvs/mainrelative to the test runner's CWD (repository_lock'smkdir, and intest_clone_runs_git_command,_clone_atomically's realmkdtemp/os.replace). Other tests in this file (e.g. lines 35-53) correctly isolate viatempfile.TemporaryDirectory()+ explicitlocal_path; these three should do the same to avoid leaving stray artifacts and cross-test/CI pollution.Also applies to: 74-105, 156-169
🤖 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 55 - 71, Update test_sync_clones_when_repository_missing, test_sync_fetches_when_repository_exists, and test_clone_runs_git_command to create a tempfile.TemporaryDirectory and pass its path as local_path when constructing GitRepositoryClient. Keep the existing assertions and mocked behavior unchanged while ensuring all repository, lock, and temporary clone artifacts stay within the isolated temporary directory.
🤖 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/database/db.py`:
- Around line 410-470: Wrap the commit operations in
create_artifact_ingest_event and create_ingest_chunk with the file’s existing
rollback-on-failure pattern: catch commit exceptions, call
sqla.session.rollback(), then re-raise the original exception. Preserve the
current add, commit, and return behavior on success, and ensure both
unique-constraint paths leave the shared session usable.
- Around line 349-361: Update _normalize_utc_datetime so naive datetime values
are handled explicitly rather than returned unchanged: reject them or
consistently assign UTC according to the project’s established convention.
Preserve conversion of timezone-aware values through timezone.utc and leave
non-datetime values unchanged, ensuring observed_at/created_at always satisfy
the UTC invariant.
In `@application/tests/harvester_test/git_repository_client_test.py`:
- Around line 73-105: Update test_sync_fetches_when_repository_exists to stop
patching client.fetch, allowing the real fetch implementation to invoke the
mocked subprocess.run and record the expected git reset command. Keep
verify_repository_integrity mocked and retain the existing mock_run assertions.
In `@application/utils/harvester/change_detector.py`:
- Around line 13-34: The subprocess calls in _resolve_commit,
get_modified_files_since, and get_commits_since must handle
subprocess.TimeoutExpired in addition to CalledProcessError. Catch the timeout
exception for each 60-second Git invocation, log the failure consistently with
the existing error handling, and re-raise it.
In `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py`:
- Around line 1-7: Update the migration header docstring’s Revises value to
match the actual down_revision value "c7d8e9f0a1b2", keeping the revision ID and
other metadata unchanged.
---
Outside diff comments:
In `@application/utils/harvester/__init__.py`:
- Around line 27-44: Reorder the entries in the __all__ list using isort-style
alphabetical sorting to satisfy Ruff RUF022, without changing the exported
symbols.
In `@application/utils/harvester/git_repository_client.py`:
- Around line 54-98: Handle subprocess.TimeoutExpired alongside
CalledProcessError for every timed git invocation in _clone_atomically, the
fetch/sync paths, is_valid_repository, and verify_repository_integrity. Log
timeout failures consistently, and ensure is_valid_repository and
verify_repository_integrity return False on timeout so sync() can follow its
existing fallback flow rather than propagating the exception.
- Around line 16-33: Validate owner and repository with the existing
_VALID_COMPONENT checks at the start of GitRepositoryClient.__init__, before the
local_path conditional, so validation occurs whether or not a cache path is
supplied. Preserve the existing branch validation and
build_repository_cache_path behavior for callers without an explicit local_path.
---
Nitpick comments:
In `@application/tests/harvester_test/git_repository_client_test.py`:
- Around line 55-71: Update test_sync_clones_when_repository_missing,
test_sync_fetches_when_repository_exists, and test_clone_runs_git_command to
create a tempfile.TemporaryDirectory and pass its path as local_path when
constructing GitRepositoryClient. Keep the existing assertions and mocked
behavior unchanged while ensuring all repository, lock, and temporary clone
artifacts stay within the isolated temporary directory.
In `@application/utils/harvester/checkpoint_store.py`:
- Around line 70-78: Update the IntegrityError handler in the checkpoint store
commit flow to raise the duplicate-identity ValueError explicitly from the
caught IntegrityError, preserving the exception chain; keep the rollback and
generic exception re-raise behavior unchanged.
In `@application/utils/harvester/repository_cache.py`:
- Around line 12-29: Update build_repository_cache_path to validate branch as a
non-empty, restricted path component before calling quote, using
_VALID_COMPONENT or an equivalent branch-specific pattern. Preserve the existing
rejection of "." and "..", and ensure every accepted branch produces its own
cache path segment.
🪄 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 Plus
Run ID: a81c928d-c502-48a8-a65f-3daa5e1d8afe
📒 Files selected for processing (21)
application/database/db.pyapplication/tests/harvester_test/change_detector_test.pyapplication/tests/harvester_test/checkpoint_store_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/git_repository_client_integration_test.pyapplication/tests/harvester_test/git_repository_client_test.pyapplication/tests/import_run_test.pyapplication/utils/harvester/__init__.pyapplication/utils/harvester/change_detector.pyapplication/utils/harvester/checkpoint_store.pyapplication/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/repository_cache.pyapplication/utils/harvester/repository_lock.pymigrations/versions/6a9d0d62ef41_add_harvester_checkpoint_table.pymigrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py
🚧 Files skipped from review as they are similar to previous changes (7)
- application/tests/harvester_test/filtering_benchmark_test.py
- application/utils/harvester/filtering_benchmark.py
- application/utils/harvester/models.py
- application/tests/harvester_test/filtering_metrics_test.py
- application/tests/harvester_test/change_detector_test.py
- application/utils/harvester/filtering_metrics.py
- application/tests/harvester_test/file_filter_test.py
| def _serialize_json_value(value: Any) -> str: | ||
| return flask_json.dumps(value) | ||
|
|
||
|
|
||
| def _normalize_utc_datetime(value: Any) -> Any: | ||
| from datetime import datetime, timezone | ||
|
|
||
| if isinstance(value, datetime): | ||
| if value.tzinfo is None: | ||
| return value | ||
| return value.astimezone(timezone.utc) | ||
| return value | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
_normalize_utc_datetime silently skips normalization for naive input.
Aware datetimes get converted to UTC, but a naive datetime is returned unchanged rather than being rejected or assumed-UTC. If a caller ever passes a naive, non-UTC datetime to observed_at/created_at, it will be persisted as-is, silently breaking the "always UTC" invariant the helper is meant to guarantee (and undermining comparisons against the tz-aware timestamps elsewhere, e.g. HarvesterCheckpoint).
🩹 Proposed fix: make naive input explicit
def _normalize_utc_datetime(value: Any) -> Any:
- from datetime import datetime, timezone
-
if isinstance(value, datetime):
if value.tzinfo is None:
- return value
+ raise ValueError("naive datetime is ambiguous; pass a timezone-aware value")
return value.astimezone(timezone.utc)
return value🤖 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/database/db.py` around lines 349 - 361, Update
_normalize_utc_datetime so naive datetime values are handled explicitly rather
than returned unchanged: reject them or consistently assign UTC according to the
project’s established convention. Preserve conversion of timezone-aware values
through timezone.utc and leave non-datetime values unchanged, ensuring
observed_at/created_at always satisfy the UTC invariant.
| def create_artifact_ingest_event( | ||
| *, | ||
| run_id: str, | ||
| artifact_id: str, | ||
| harvest_mode: str, | ||
| event_type: str, | ||
| source_json: Any, | ||
| locator_json: Any, | ||
| artifact_json: Any, | ||
| harvest_json: Any, | ||
| observed_at: Any, | ||
| ) -> ArtifactIngestEvent: | ||
| from datetime import datetime, timezone | ||
|
|
||
| observed_at = _normalize_utc_datetime(observed_at) | ||
|
|
||
| event = ArtifactIngestEvent( | ||
| id=generate_uuid(), | ||
| run_id=run_id, | ||
| artifact_id=artifact_id, | ||
| harvest_mode=harvest_mode, | ||
| event_type=event_type, | ||
| source_json=_serialize_json_value(source_json), | ||
| locator_json=_serialize_json_value(locator_json), | ||
| artifact_json=_serialize_json_value(artifact_json), | ||
| harvest_json=_serialize_json_value(harvest_json), | ||
| observed_at=observed_at, | ||
| created_at=_normalize_utc_datetime(datetime.now(timezone.utc)), | ||
| ) | ||
| sqla.session.add(event) | ||
| sqla.session.commit() | ||
| return event | ||
|
|
||
|
|
||
| def create_ingest_chunk( | ||
| *, | ||
| artifact_event_id: str, | ||
| chunk_id: str, | ||
| text: str, | ||
| char_count: int, | ||
| span_json: Any, | ||
| delta_json: Optional[Any] = None, | ||
| ) -> IngestChunk: | ||
| from datetime import datetime, timezone | ||
|
|
||
| chunk = IngestChunk( | ||
| id=generate_uuid(), | ||
| artifact_event_id=artifact_event_id, | ||
| chunk_id=chunk_id, | ||
| text=text, | ||
| char_count=char_count, | ||
| span_json=_serialize_json_value(span_json), | ||
| delta_json=( | ||
| _serialize_json_value(delta_json) if delta_json is not None else None | ||
| ), | ||
| created_at=_normalize_utc_datetime(datetime.now(timezone.utc)), | ||
| ) | ||
| sqla.session.add(chunk) | ||
| sqla.session.commit() | ||
| return chunk | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded session.commit() risks leaving the session in an aborted state on constraint violation.
Both create_artifact_ingest_event and create_ingest_chunk call sqla.session.add(...) then sqla.session.commit() with no try/except. Both new tables enforce unique constraints (uq_artifact_ingest_event_run_artifact, uq_ingest_chunk_artifact_chunk) that a harvester retry after a transient failure (or overlapping backfills) could violate. An uncaught IntegrityError here will abort the DB transaction and leave the shared session unusable for any subsequent operation in the same request/scope until it's rolled back — a real cascading-failure risk.
This is inconsistent with the rollback discipline already used elsewhere in this file for comparable writes (e.g. CheckpointStore.save(), add_node()), which wrap commit() in try/except and roll back on failure.
🛡️ Proposed fix: rollback on commit failure
sqla.session.add(event)
- sqla.session.commit()
+ try:
+ sqla.session.commit()
+ except Exception:
+ sqla.session.rollback()
+ raise
return eventApply the same pattern to create_ingest_chunk.
Consider also adding a regression test for the duplicate-insert path (mirroring checkpoint_store_test.py's test_duplicate_canonical_source_identity_rejected) once this is fixed.
🤖 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/database/db.py` around lines 410 - 470, Wrap the commit
operations in create_artifact_ingest_event and create_ingest_chunk with the
file’s existing rollback-on-failure pattern: catch commit exceptions, call
sqla.session.rollback(), then re-raise the original exception. Preserve the
current add, commit, and return behavior on success, and ensure both
unique-constraint paths leave the shared session usable.
| @@ -88,6 +89,21 @@ def test_sync_fetches_when_repository_exists(self): | |||
|
|
|||
| mock_fetch.assert_called_once() | |||
|
|
|||
| mock_run.assert_called_once_with( | |||
| [ | |||
| "git", | |||
| "-C", | |||
| str(client.get_local_path()), | |||
| "reset", | |||
| "--hard", | |||
| "origin/main", | |||
| ], | |||
| check=True, | |||
| capture_output=True, | |||
| text=True, | |||
| timeout=300, | |||
| ) | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -i 'git_repository_client_test\.py|git_repository_client\.py' .
echo "== test file excerpt =="
test_file=$(fd -i 'git_repository_client_test.py' . | head -n1 || true)
if [ -n "${test_file:-}" ]; then
wc -l "$test_file"
sed -n '1,140p' "$test_file" | cat -n
fi
echo "== source symbols =="
source_file=$(fd -i 'git_repository_client.py' . | head -n1 || true)
if [ -n "${source_file:-}" ]; then
wc -l "$source_file"
ast-grep outline "$source_file" 2>/dev/null || true
sed -n '1,260p' "$source_file" | cat -n
fi
echo "== behavioral probe =="
python3 - <<'PY'
from unittest.mock import Mock, patch
class Client:
local_path = "/"
def get_local_path(self):
return self.local_path
def verify_repository_integrity(self):
raise NotImplementedError
def fetch(self):
raise NotImplementedError
# in real code this has subprocess.run(..., ["reset","--hard","origin/main"])
def sync(self):
if not self.verify_repository_integrity():
print("clone")
else:
self.fetch()
client = Client()
try:
with patch.object(client, 'verify_repository_integrity', return_value=True), \
patch.object(client, 'fetch') as mock_fetch:
client.sync()
mock_fetch.assert_called_once()
print("patch.object fetch assertion passed")
except Exception as e:
print("patch.object fetch assertion failed:", type(e).__name__, str(e))
subprocess_calls = []
def recording_run(*a, **kw): subprocess_calls.append((a, kw)); return Mock()
with patch('builtins.__import__') as fake_import:
pass
PYRepository: OWASP/OpenCRE
Length of output: 14314
Don’t mock fetch in the sync-exists test.
fetch contains the real subprocess.run(... ["reset", "--hard", ...]) call, so mocking it prevents mock_run from recording that git command. In this path mock_run records 0 calls instead of the 1 that the assertion expects.
🤖 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
73 - 105, Update test_sync_fetches_when_repository_exists to stop patching
client.fetch, allowing the real fetch implementation to invoke the mocked
subprocess.run and record the expected git reset command. Keep
verify_repository_integrity mocked and retain the existing mock_run assertions.
| def _resolve_commit(self, commit_sha: str) -> str: | ||
| try: | ||
| result = subprocess.run( | ||
| [ | ||
| "git", | ||
| "-C", | ||
| str(self.repository_client.get_local_path()), | ||
| "rev-parse", | ||
| "--verify", | ||
| "--end-of-options", | ||
| f"{commit_sha}^{{commit}}", | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| timeout=60, | ||
| ) | ||
| except subprocess.CalledProcessError as exc: | ||
| logger.error("Git command failed: %s", exc.stderr) | ||
| raise | ||
|
|
||
| return result.stdout.strip() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Same missing TimeoutExpired handling as git_repository_client.py.
All three subprocess calls here (_resolve_commit, get_modified_files_since, get_commits_since) set timeout=60 but only catch subprocess.CalledProcessError; a timeout raises subprocess.TimeoutExpired uncaught. See the consolidated comment for the combined fix across both files.
Also applies to: 36-72, 74-107
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 14-28: Command coming from incoming request
Context: subprocess.run(
[
"git",
"-C",
str(self.repository_client.get_local_path()),
"rev-parse",
"--verify",
"--end-of-options",
f"{commit_sha}^{{commit}}",
],
capture_output=True,
text=True,
check=True,
timeout=60,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.0)
[error] 15-15: subprocess call: check for execution of untrusted input
(S603)
[error] 16-24: Starting a process with a partial executable path
(S607)
🤖 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 13 - 34, The
subprocess calls in _resolve_commit, get_modified_files_since, and
get_commits_since must handle subprocess.TimeoutExpired in addition to
CalledProcessError. Catch the timeout exception for each 60-second Git
invocation, log the failure consistently with the existing error handling, and
re-raise it.
| """add artifact ingest event and chunk tables | ||
|
|
||
| Revision ID: 9f1a2b3c4d5e | ||
| Revises: e1f2a3b4c5d6 | ||
| Create Date: 2026-07-23 | ||
|
|
||
| """ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Docstring Revises: doesn't match the actual down_revision.
The header comment says Revises: e1f2a3b4c5d6, but the code sets down_revision = "c7d8e9f0a1b2". Alembic uses the code value (which appears correct, matching the embedding-vec migration referenced in db.py), but the stale docstring will mislead anyone reading the migration chain — likely a leftover from rebasing onto a different base tip.
📝 Proposed fix
"""add artifact ingest event and chunk tables
Revision ID: 9f1a2b3c4d5e
-Revises: e1f2a3b4c5d6
+Revises: c7d8e9f0a1b2
Create Date: 2026-07-23
"""Also applies to: 13-16
🤖 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 `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py` around
lines 1 - 7, Update the migration header docstring’s Revises value to match the
actual down_revision value "c7d8e9f0a1b2", keeping the revision ID and other
metadata unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/harvester/file_filter.py`:
- Around line 37-41: Update the allowed_extensions initialization in the
relevant harvester filter class to reject empty extension strings while copying
configured values, preventing an empty suffix from matching every file; preserve
the default-extension behavior when no configuration is supplied, and add a
regression test covering allowed_extensions={""}.
🪄 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 Plus
Run ID: 60f0873d-e01f-4904-b91d-990d20f349bf
📒 Files selected for processing (3)
application/tests/harvester_test/file_filter_test.pyapplication/utils/harvester/exclude_patterns.txtapplication/utils/harvester/file_filter.py
|
hey, i think i have addressed everything, good to go from my side, let me know if you need any further improvments |
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.