GSoC Module A: week 5 : feat(harvester): add git diff retrieval pipeline (stacked on top of #986)#987
Open
ParthAggarwal16 wants to merge 28 commits into
Open
GSoC Module A: week 5 : feat(harvester): add git diff retrieval pipeline (stacked on top of #986)#987ParthAggarwal16 wants to merge 28 commits into
ParthAggarwal16 wants to merge 28 commits into
Conversation
ParthAggarwal16
requested review from
Pa04rth,
northdpole and
robvanderveer
as code owners
July 18, 2026 16:30
Contributor
WalkthroughAdds a harvester utility foundation covering typed configuration, repository caching and synchronization, checkpointing, change detection, diff processing, file filtering, metrics, production configuration, and comprehensive tests. ChangesHarvester foundation
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
application/utils/harvester/config_loader.py (1)
18-23: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle file access errors more robustly.
Using
is_file()followed byopen()is susceptible to a time-of-check to time-of-use (TOCTOU) race condition. More importantly, if the file exists but the application lacks read permissions,open()will raise aPermissionError(anOSError), which isn't caught here and will bubble up unhandled, bypassing your custom configuration exceptions.Consider catching
OSErrordirectly when opening the file to handle both existence and permission errors uniformly.♻️ Proposed refactor
- if not config_path.is_file(): - raise ConfigFileNotFoundError(f"Configuration file not found: {config_path}") - try: with config_path.open("r", encoding="utf-8") as file: raw_config = yaml.safe_load(file) + except OSError as exc: + raise ConfigFileNotFoundError(f"Cannot access configuration file at {config_path}") from exc except yaml.YAMLError as exc:🤖 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/config_loader.py` around lines 18 - 23, Update the configuration file loading in the config loader to catch OSError directly around config_path.open, converting missing, permission, and other file access failures into the established custom configuration exception. Remove reliance on the preceding is_file check so the open operation is authoritative and avoids the TOCTOU race.application/utils/harvester/__init__.py (1)
29-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__list.To improve maintainability and address the static analysis warning, sort the items in the
__all__list alphabetically.♻️ Proposed refactor
__all__ = [ - "build_repository_cache_path", "ChunkingConfig", "ConfigLoaderError", "DiffRetriever", "FileFilter", "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 29 - 47, Sort the __all__ entries in application/utils/harvester/__init__.py alphabetically, preserving every existing export and its spelling.Source: Linters/SAST tools
application/utils/harvester/file_filter.py (1)
25-40: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize regex compilation and extension matching for performance.
When filtering a large number of files during harvesting, computing regexes on the fly and using a generator for
str.endswithcan become a significant bottleneck.Consider the following optimizations:
- Compile the regex patterns once in
__init__.- Convert
allowed_extensionsto a tuple in__init__and pass it directly tostr.endswith. This string method natively checks against a tuple in C, making it much faster than a generator expression.⚡ Proposed performance improvements
class FileFilter: def __init__( self, exclude_patterns: list[str] | None = None, allowed_extensions: set[str] | None = None, ): self.exclude_patterns = exclude_patterns or DEFAULT_EXCLUDE_PATTERNS self.allowed_extensions = allowed_extensions or DEFAULT_ALLOWED_EXTENSIONS + self._compiled_patterns = [re.compile(p) for p in self.exclude_patterns] + self._allowed_ext_tuple = tuple(self.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) + return any(pattern.search(file_path) for pattern in self._compiled_patterns) def is_allowed_extension(self, file_path: str) -> bool: - return any( - file_path.endswith(extension) for extension in self.allowed_extensions - ) + return file_path.endswith(self._allowed_ext_tuple)🤖 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 25 - 40, Update FileFilter.__init__ to precompile each exclude pattern with re.compile and store allowed_extensions as a tuple, preserving the configured defaults. Change is_excluded_by_pattern to use the compiled pattern objects and change is_allowed_extension to pass the tuple directly to file_path.endswith instead of iterating with any.application/utils/harvester/diff_normalizer.py (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused import.
The
repository_clientmodule is imported but never used in this file.♻️ Proposed fix
-from application.utils.harvester import repository_client🤖 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/diff_normalizer.py` at line 3, Remove the unused repository_client import from diff_normalizer.py, leaving the remaining imports and implementation unchanged.application/tests/harvester_test/diff_retriever_test.py (1)
13-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate test mocks to align with the proposed byte-handling fix.
If you adopt the recommended change to handle
subprocess.runoutput as bytes inDiffRetriever, you will need to update these mocks to return byte strings (b"...") and remove thetext=Trueassertion.♻️ Proposed test update
def test_get_diff(self, mock_run): mock_run.return_value = MagicMock( - stdout="diff --git a/README.md b/README.md\n", + stdout=b"diff --git a/README.md b/README.md\n", ) client = MagicMock() client.get_local_path.return_value = "/tmp/repo" retriever = DiffRetriever(client) diff = retriever.get_diff( "abc123", "def456", ) self.assertEqual( diff, "diff --git a/README.md b/README.md\n", ) mock_run.assert_called_once_with( [ "git", "-C", "/tmp/repo", "diff", "abc123", "def456", ], capture_output=True, - text=True, check=True, timeout=300, ) `@patch`("application.utils.harvester.diff_retriever.subprocess.run") def test_large_diff_raises(self, mock_run): mock_run.return_value = MagicMock( - stdout="A" * (51 * 1024 * 1024), + stdout=b"A" * (51 * 1024 * 1024), )🤖 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/diff_retriever_test.py` around lines 13 - 52, Update the DiffRetriever test mocks in test_get_diff and test_large_diff_raises to return byte strings, matching the byte-oriented subprocess output handling. Remove text=True from the expected subprocess.run arguments while preserving the existing diff content and size assertions.application/utils/harvester/change_detector.py (1)
21-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
-zwithgit difffor robust filename parsing.By default,
git diff --name-onlyquotes filenames that contain special characters or spaces (e.g.,"path/to/file name.md"). Using the-zflag ensures that filenames are separated by null bytes without quoting, making parsing more robust.♻️ Proposed refactor
[ "git", "-C", str(self.repository_client.get_local_path()), "diff", + "-z", "--name-only", 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() + file_path for file_path in result.stdout.split("\0") if file_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/change_detector.py` around lines 21 - 41, Update the Git diff invocation in the change-detection method to include the -z option, then parse result.stdout using null-byte separators instead of splitlines while retaining filtering of empty paths. Keep the existing CalledProcessError logging and re-raising behavior unchanged.
🤖 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/diff_pipeline_test.py`:
- Around line 20-30: Call GitRepositoryClient.sync() immediately after
constructing client and before starting the benchmark timer in the diff pipeline
test, ensuring retriever.get_diff() runs against a locally available repository.
In `@application/utils/harvester/checkpoint_store.py`:
- Around line 13-22: Update CheckpointStore.load to remove the separate
checkpoint_file.exists() check and perform read_text directly inside a try
block; catch FileNotFoundError and return None, preserving the existing JSON
parsing behavior when the file is available.
- Around line 36-44: Update CheckpointStore.save to remove the exists-then-read
sequence: attempt read_text directly and catch FileNotFoundError, preserving an
empty data dictionary when the checkpoint file is absent. Protect the complete
read, modification, and replacement/write operation with a file lock shared by
concurrent workers, using the project’s established locking mechanism if
available.
In `@application/utils/harvester/diff_parser.py`:
- Around line 47-57: Update the diff-line header checks in the parser’s
surrounding function to skip only exact `+++` and `---` file-header lines,
including their expected delimiter or path format, rather than any line sharing
those prefixes. Preserve added and removed code whose diff payload begins with
additional `+` or `-` characters, and keep the existing hunk-header and line
collection behavior unchanged.
In `@application/utils/harvester/diff_retriever.py`:
- Around line 47-76: Update the subprocess.run call in the diff retrieval method
to capture raw bytes by removing text mode, then measure the byte length
directly on result.stdout before decoding. Decode the output as UTF-8 with
errors="replace", and retain the existing size-limit validation and return
behavior using the safely decoded diff.
In `@application/utils/harvester/file_filter.py`:
- Around line 11-22: Update DEFAULT_EXCLUDE_PATTERNS so directory exclusions for
.github, .git, node_modules, dist, build, coverage, and vendor match at the
repository root or after any path separator by using (?:^|/); remove the
redundant .* prefixes from the package-lock.json, yarn.lock, and pnpm-lock.yaml
patterns while preserving their re.search matching behavior.
---
Nitpick comments:
In `@application/tests/harvester_test/diff_retriever_test.py`:
- Around line 13-52: Update the DiffRetriever test mocks in test_get_diff and
test_large_diff_raises to return byte strings, matching the byte-oriented
subprocess output handling. Remove text=True from the expected subprocess.run
arguments while preserving the existing diff content and size assertions.
In `@application/utils/harvester/__init__.py`:
- Around line 29-47: Sort the __all__ entries in
application/utils/harvester/__init__.py alphabetically, preserving every
existing export and its spelling.
In `@application/utils/harvester/change_detector.py`:
- Around line 21-41: Update the Git diff invocation in the change-detection
method to include the -z option, then parse result.stdout using null-byte
separators instead of splitlines while retaining filtering of empty paths. Keep
the existing CalledProcessError logging and re-raising behavior unchanged.
In `@application/utils/harvester/config_loader.py`:
- Around line 18-23: Update the configuration file loading in the config loader
to catch OSError directly around config_path.open, converting missing,
permission, and other file access failures into the established custom
configuration exception. Remove reliance on the preceding is_file check so the
open operation is authoritative and avoids the TOCTOU race.
In `@application/utils/harvester/diff_normalizer.py`:
- Line 3: Remove the unused repository_client import from diff_normalizer.py,
leaving the remaining imports and implementation unchanged.
In `@application/utils/harvester/file_filter.py`:
- Around line 25-40: Update FileFilter.__init__ to precompile each exclude
pattern with re.compile and store allowed_extensions as a tuple, preserving the
configured defaults. Change is_excluded_by_pattern to use the compiled pattern
objects and change is_allowed_extension to pass the tuple directly to
file_path.endswith instead of iterating with any.
🪄 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: 4a212f95-eb0a-4747-a72b-a6c38a884f69
📒 Files selected for processing (45)
.gitignoreapplication/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/diff_normalizer_test.pyapplication/tests/harvester_test/diff_parser_test.pyapplication/tests/harvester_test/diff_pipeline_test.pyapplication/tests/harvester_test/diff_retriever_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/diff_normalizer.pyapplication/utils/harvester/diff_parser.pyapplication/utils/harvester/diff_retriever.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
Comment on lines
+47
to
+57
| if line.startswith("+++"): | ||
| continue | ||
|
|
||
| if line.startswith("---"): | ||
| continue | ||
|
|
||
| if line.startswith("@@"): | ||
| continue | ||
|
|
||
| if line.startswith("+") and not line.startswith("+++"): | ||
| added_lines.append(line[1:]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix parsing bug that silently drops added lines.
Checking line.startswith("+++") correctly skips the +++ b/file diff header, but it also incorrectly skips any newly added code that starts with ++ (e.g., ++i; or markdown ++ text), which appear as +++i; in the diff. The same applies to ---.
Match the exact header prefixes to safely ignore headers while preserving valid added code.
🛠️ Proposed fix
- if line.startswith("+++"):
+ if line.startswith("+++ b/") or line.startswith("+++ /dev/null"):
continue
- if line.startswith("---"):
+ if line.startswith("--- a/") or line.startswith("--- /dev/null"):
continue
if line.startswith("@@"):
continue
- if line.startswith("+") and not line.startswith("+++"):
+ if line.startswith("+"):
added_lines.append(line[1:])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if line.startswith("+++"): | |
| continue | |
| if line.startswith("---"): | |
| continue | |
| if line.startswith("@@"): | |
| continue | |
| if line.startswith("+") and not line.startswith("+++"): | |
| added_lines.append(line[1:]) | |
| if line.startswith("+++ b/") or line.startswith("+++ /dev/null"): | |
| continue | |
| if line.startswith("--- a/") or line.startswith("--- /dev/null"): | |
| continue | |
| if line.startswith("@@"): | |
| continue | |
| if line.startswith("+"): | |
| added_lines.append(line[1:]) |
🤖 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/diff_parser.py` around lines 47 - 57, Update the
diff-line header checks in the parser’s surrounding function to skip only exact
`+++` and `---` file-header lines, including their expected delimiter or path
format, rather than any line sharing those prefixes. Preserve added and removed
code whose diff payload begins with additional `+` or `-` characters, and keep
the existing hunk-header and line collection behavior unchanged.
Contributor
Author
|
Updated the benchmark to compare HEAD -1 against HEAD instead of fixed commit SHAs so it remains valid as the upstream repository evolves. |
ParthAggarwal16
force-pushed
the
week_5-clean
branch
from
July 18, 2026 19:04
d4bca65 to
8dea1c7
Compare
Contributor
Author
|
hey guys, if you are reviewing this, pls hold for a bit, i will update on slack regarding the failing CI test, been doing this for a while now, so i will tackle this tomorrow, need some sleep right now :( |
This was referenced Jul 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
(this PR is stacked on top of #986 )
This PR implements the initial diff processing pipeline for the harvester, introducing the components required to retrieve, parse, and normalize repository diffs before downstream document processing.
Changes
Diff Retrieval
DiffRetrieverfor retrieving git diffs between commits.Diff Parsing
DiffParserto convert unified git diffs into structuredDiffBlockobjects.Diff Normalization
DiffNormalizerto normalize extracted diff content.Models
DiffBlockas the intermediate representation for parsed diff content.Exports
DiffRetrieverthrough the harvester package.Tests
Added unit tests covering:
Why
These components establish the foundation of the incremental harvesting pipeline. Instead of processing entire repositories, the harvester can now retrieve repository diffs, extract newly added content, normalize it into a consistent format, and prepare it for subsequent embedding and indexing stages.
Testing
This diagram combines the independently implemented modules into the expected polling flow. The orchestration itself is not yet present in a production entry point.
smoke tests:
