GSoC Module A : Week 3: feat(harvester): implement incremental change detection (stacked on #983)#985
GSoC Module A : Week 3: feat(harvester): implement incremental change detection (stacked on #983)#985ParthAggarwal16 wants to merge 21 commits into
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds the harvester configuration schema and validation stack, Git repository caching and synchronization utilities, incremental change detection, checkpoint persistence, repository defaults, exclusion patterns, and comprehensive unit-test fixtures. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
application/utils/harvester/__init__.py (1)
21-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__list.The static analysis tool Ruff flags that the
__all__list is not sorted. Using a standard ASCII sort (uppercase before lowercase) resolves this warning and follows standard Python conventions.♻️ Proposed fix
__all__ = [ - "build_repository_cache_path", "ChunkingConfig", "ConfigLoaderError", "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 21 - 34, Sort the __all__ entries in application/utils/harvester/__init__.py using standard ASCII ordering, placing uppercase names before lowercase names, without changing the exported symbols.Source: Linters/SAST tools
application/tests/harvester_test/git_repository_client_test.py (2)
32-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock the filesystem to prevent flaky tests.
This test asserts that the local repository path does not exist. However, if a developer runs this test locally after having previously run a harvest or sync, the
.harvester_cachedirectory might actually exist on their machine, causing the test to fail.Consider mocking
Path.existsto fully isolate the test from the local filesystem state.♻️ Proposed refactor
- def test_repository_exists_locally_false(self): + `@patch`("pathlib.Path.exists") + def test_repository_exists_locally_false(self, mock_exists): + mock_exists.return_value = False client = GitRepositoryClient( owner="OWASP", repository="ASVS", ) self.assertFalse(client.exists_locally())🤖 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 32 - 38, Update test_repository_exists_locally_false to mock pathlib.Path.exists so it deterministically reports that the repository path is absent, then assert GitRepositoryClient.exists_locally() returns False without consulting the developer’s filesystem.
27-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
.as_posix()to prevent cross-platform test failures.Comparing stringified
Pathobjects to a UNIX-style string path (.harvester_cache/owasp/asvs/main) can cause test failures on Windows, wherestr(Path)uses backslashes (\). Using.as_posix()ensures the output uses forward slashes on all platforms.♻️ Proposed refactor
- self.assertEqual( - str(client.get_local_path()), - ".harvester_cache/owasp/asvs/main", - ) + self.assertEqual( + client.get_local_path().as_posix(), + ".harvester_cache/owasp/asvs/main", + )🤖 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 27 - 30, Update the path assertion for client.get_local_path() to call .as_posix() before comparing with the expected UNIX-style path, preserving the existing expected value and test behavior across platforms.application/utils/harvester/change_detector.py (2)
47-60: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider returning commits in chronological order.
By default,
git logoutputs commits in reverse chronological order (newest first). If the orchestrator uses this list to process commits sequentially or incrementally replay history, it typically requires chronological order (oldest first).You can pass the
--reverseflag togit logto achieve this.💡 Proposed change
result = subprocess.run( [ "git", "-C", str(self.repository_client.get_local_path()), "log", + "--reverse", "--format=%H", f"{commit_sha}..HEAD", ], capture_output=True, text=True, check=True, timeout=60, )🤖 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 47 - 60, Update the git log invocation in the change-detection method to include the --reverse option, ensuring commits between commit_sha and HEAD are returned in chronological order while preserving the existing output and filtering behavior.
20-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
-zfor robust file path parsing.Git may escape or quote filenames containing special characters or newlines by default (e.g.,
"path/to/file with spaces.md"). Passing-ztogit diffproduces null-terminated output, which guarantees accurate parsing of all file names without interference from Git's quotation rules.♻️ Proposed refactor
result = subprocess.run( [ "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, )If you apply this, update the parsing logic on line 40 to split by
\0:files = [ file_path for file_path in result.stdout.split("\0") if file_path.strip() ]🤖 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 20 - 34, Update the git diff invocation in the change-detection flow to include the null-delimited output option, then update the parsing logic that consumes result.stdout to split on "\0" and ignore empty entries. Preserve accurate filenames, including spaces, quotes, and newlines, through the existing file-processing path.
🤖 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/checkpoint_store.py`:
- Around line 35-54: Update the checkpoint_store save method to serialize the
modified data into a temporary file in the checkpoint file’s directory, then
atomically replace the existing checkpoint file with it. Preserve the current
JSON content and encoding, and ensure temporary-file cleanup on write or
replacement failure.
---
Nitpick comments:
In `@application/tests/harvester_test/git_repository_client_test.py`:
- Around line 32-38: Update test_repository_exists_locally_false to mock
pathlib.Path.exists so it deterministically reports that the repository path is
absent, then assert GitRepositoryClient.exists_locally() returns False without
consulting the developer’s filesystem.
- Around line 27-30: Update the path assertion for client.get_local_path() to
call .as_posix() before comparing with the expected UNIX-style path, preserving
the existing expected value and test behavior across platforms.
In `@application/utils/harvester/__init__.py`:
- Around line 21-34: Sort the __all__ entries in
application/utils/harvester/__init__.py using standard ASCII ordering, placing
uppercase names before lowercase names, without changing the exported symbols.
In `@application/utils/harvester/change_detector.py`:
- Around line 47-60: Update the git log invocation in the change-detection
method to include the --reverse option, ensuring commits between commit_sha and
HEAD are returned in chronological order while preserving the existing output
and filtering behavior.
- Around line 20-34: Update the git diff invocation in the change-detection flow
to include the null-delimited output option, then update the parsing logic that
consumes result.stdout to split on "\0" and ignore empty entries. Preserve
accurate filenames, including spaces, quotes, and newlines, through the existing
file-processing path.
🪄 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: 45a49c90-33e4-4f20-9cac-25cd04822648
📒 Files selected for processing (31)
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/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/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
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Week 3 review — required before approval
This layer establishes durable incremental state, so checkpoint persistence should use Postgres now rather than introducing a JSON runtime contract that must be replaced later. Full multi-worker concurrency can be deferred, provided this version explicitly enforces a single harvester worker with no overlapping runs.
Required in this PR:
- replace JSON checkpoint persistence with the minimal Postgres model/migration described inline;
- enforce the single-worker/no-overlap execution constraint;
- replay commits oldest-first;
- use validated immutable base/target SHAs instead of mutable
HEAD; - add Postgres checkpoint tests and a deterministic temporary-Git integration test.
Please open separate follow-up tickets for multi-worker lease/CAS fencing, deletion/rename semantics, NUL-safe filenames, force-push/history-rewrite recovery, and broader failure/concurrency coverage. Every follow-up ticket should carry both the Module B and V1 labels so these are tracked as required next steps.
Verdict: changes are required before approval, but full concurrency support is not required in this PR.
| from .models import RepositoryCheckpoint | ||
|
|
||
|
|
||
| class CheckpointStore: |
There was a problem hiding this comment.
Blocker: replace the JSON runtime store with Postgres in this PR. Add a harvester_checkpoint SQLAlchemy model and an Alembic migration based on the current migration head after rebasing. The minimum schema should contain: immutable repository_id as the primary key, canonical source identity (provider/owner/repository/exact branch, with a unique canonical source key), nullable last_processed_commit, and timezone-aware created_at/updated_at. Replace this Path-based API with database load(repository_id) and transactional upsert behavior using the application SQLAlchemy session. Do not fall back to JSON on a database error; fail closed and leave the checkpoint unchanged. Full lease/CAS columns may be added in the labelled V1 follow-up, but this PR must explicitly enforce single-worker/no-overlap execution until then.
| "diff", | ||
| "--name-only", | ||
| commit_sha, | ||
| "HEAD", |
There was a problem hiding this comment.
Do not diff against mutable HEAD. Change the API to receive both a validated base SHA from the checkpoint and an immutable target SHA captured after synchronization, then invoke git diff <base> <target>. Validate each value as a full commit object before use (for example with rev-parse --verify --end-of-options <sha>^{commit}) and pass only the resolved SHA to subsequent Git commands. Add a temporary local-repository test where the branch advances after target capture and prove the detected range remains stable. Force-push/history-rewrite reconciliation can be handled in a Module B + V1 follow-up ticket, but this API should not bake in HEAD.
| str(self.repository_client.get_local_path()), | ||
| "log", | ||
| "--format=%H", | ||
| f"{commit_sha}..HEAD", |
There was a problem hiding this comment.
Git log is newest-first by default, which is unsafe for sequential replay and checkpoint progression. Add --reverse and use the explicit validated <base>..<target> range rather than <base>..HEAD, making the method contract oldest-first. Update the unit test to assert the complete Git argument list, and add a deterministic temporary-Git test with at least three commits that verifies the returned order.
|
|
||
| try: | ||
| store = CheckpointStore( | ||
| tmp_dir / "checkpoints.json", |
There was a problem hiding this comment.
These filesystem/JSON tests should be replaced with Postgres-backed persistence tests using the repository's application/SQLAlchemy test setup. Cover: create/load, update/upsert, two repository rows remaining isolated, duplicate canonical source identity rejection, immutable repository identity, null initial checkpoint, transaction rollback leaving the previous checkpoint intact, and database failure with no JSON fallback. Also add a migration check that upgrades a clean Postgres database and verifies the table, primary key, unique identity constraint, and timestamps. Multi-worker contention tests belong in the follow-up ticket labelled Module B and V1.
2 participants
Summary
(this PR is stacked on top of #983 )
This PR introduces change detection and checkpoint persistence for the harvester pipeline.
It adds Git-based change detection utilities for discovering modified files and commits since a checkpoint, along with a lightweight checkpoint store for persisting repository processing state across pipeline runs.
Note: This PR is part of the Week 3 implementation and is intended to be reviewed after Week 2 (Repository Client), even though it targets
main.Added
ChangeDetectorfor Git-based incremental change detectionCheckpointStoreRepositoryCheckpointmodelRepositoryChangeSetmodelValidation Coverage
Change Detection
Checkpoint Store
Test Plan
Executed:
All tests passing.
Notes for Reviewers
ChangeDetectoris intentionally separated from the repository client so repository operations and incremental change detection remain independent concerns.1: High-Level Architecture (HLA)