GSoC Module A : Week 2 (stacked on top of #920)#983
Conversation
Summary by CodeRabbit
WalkthroughChangesHarvester foundation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)application/tests/harvester_test/fixtures/duplicate_repo_ids_case.yamlTraceback (most recent call last): application/tests/harvester_test/fixtures/duplicate_repositories_case.yamlTraceback (most recent call last): application/tests/harvester_test/fixtures/invalid_overlap_tokens.yamlTraceback (most recent call last):
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: 3
🧹 Nitpick comments (2)
application/tests/harvester_test/git_repository_client_test.py (1)
84-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the specific arguments passed to
subprocess.run.Currently, these tests only verify that
subprocess.runwas called, but they don't ensure the correctgitcommands and arguments were executed. Consider usingassert_called_once_withto validate the exact command lists and arguments, which strengthens the tests against regression.♻️ Proposed refactor for stronger assertions (example for fetch)
`@patch`("application.utils.harvester.git_repository_client.subprocess.run") def test_fetch_runs_git_command(self, mock_run): client = GitRepositoryClient( owner="OWASP", repository="ASVS", ) client.fetch() - mock_run.assert_called_once() + mock_run.assert_called_once_with( + [ + "git", + "-C", + str(client.local_path), + "fetch", + "--all", + ], + check=True, + capture_output=True, + text=True, + timeout=300, + )You can apply a similar change to the checkout, get_current_commit_sha, and clone tests.
🤖 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 84 - 135, Strengthen the subprocess assertions in test_fetch_runs_git_command, test_checkout_runs_git_command, test_get_current_commit_sha_runs_git_command, and test_clone_runs_git_command by replacing assert_called_once with assert_called_once_with. Verify each exact git command list and relevant subprocess arguments produced by GitRepositoryClient, while preserving the existing SHA assertion and clone integrity setup.application/utils/harvester/__init__.py (1)
21-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__alphabetically.The
__all__declaration is not sorted alphabetically, which triggers the RuffRUF022linter rule.♻️ Proposed fix
-__all__ = [ - "build_repository_cache_path", - "ChunkingConfig", - "ConfigLoaderError", - "GitRepositoryClient", - "PathRules", - "PollingConfig", - "RepositoryClient", - "RepositoryConfig", - "RepositoryValidationError", - "ReposFile", - "load_repo_config", - "validate_repositories", -] +__all__ = [ + "ChunkingConfig", + "ConfigLoaderError", + "GitRepositoryClient", + "PathRules", + "PollingConfig", + "ReposFile", + "RepositoryClient", + "RepositoryConfig", + "RepositoryValidationError", + "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 entries in the __all__ declaration alphabetically to satisfy Ruff RUF022, preserving all existing exports and their names.Source: Linters/SAST tools
🤖 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/repository_cache_test.py`:
- Around line 9-18: Update test_build_repository_cache_path to compare the
returned Path object directly against a Path constructed from the expected cache
path, removing the str(path) conversion while preserving the expected
OWASP/ASVS/main segments.
In `@application/utils/harvester/git_repository_client.py`:
- Around line 110-122: Update the git checkout command construction in the
subprocess.run call to insert the argument separator "--" immediately before
reference, ensuring Git treats reference strictly as a positional branch or
commit argument while preserving the existing checkout behavior.
In `@application/utils/harvester/repos.yaml`:
- Around line 15-17: Update the chunking strategy value from markdown to
markdown_heading at both application/utils/harvester/repos.yaml lines 15-17 and
lines 36-38, preserving the existing max_tokens settings.
---
Nitpick comments:
In `@application/tests/harvester_test/git_repository_client_test.py`:
- Around line 84-135: Strengthen the subprocess assertions in
test_fetch_runs_git_command, test_checkout_runs_git_command,
test_get_current_commit_sha_runs_git_command, and test_clone_runs_git_command by
replacing assert_called_once with assert_called_once_with. Verify each exact git
command list and relevant subprocess arguments produced by GitRepositoryClient,
while preserving the existing SHA assertion and clone integrity setup.
In `@application/utils/harvester/__init__.py`:
- Around line 21-34: Sort the entries in the __all__ declaration alphabetically
to satisfy Ruff RUF022, preserving all existing exports and their names.
🪄 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: 1f0179e7-b3c6-4978-ae71-719e09cda7e3
📒 Files selected for processing (26)
application/tests/harvester_test/__init__.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/config_loader.pyapplication/utils/harvester/exclude_patterns.txtapplication/utils/harvester/git_repository_client.pyapplication/utils/harvester/repos.yamlapplication/utils/harvester/repos_validator.pyapplication/utils/harvester/repository_cache.pyapplication/utils/harvester/repository_client.pyapplication/utils/harvester/schemas.py
|
this commit 1738cb4 |
| self.repository, | ||
| ) | ||
|
|
||
| if self.verify_repository_integrity(): |
There was a problem hiding this comment.
fetch() only updates remote-tracking refs; it does not update local HEAD or harvested files. Apply the fix in this PR by fetching the configured origin branch and then updating the managed worktree to the fetched ref (for example, a validated hard reset to origin/<branch> under the documented disposable-cache model). #986 does this downstream, but this PR must work independently. Add a temporary-repository integration test that syncs once, advances the remote, syncs again, and asserts both file contents and get_current_commit_sha() advanced.
| str(self.local_path), | ||
| "checkout", | ||
| "--", | ||
| reference, |
There was a problem hiding this comment.
Blocker: after git checkout, -- terminates option parsing and makes reference a pathspec, so checkout("main") attempts to restore a file named main instead of switching revisions. Apply the fix by first validating/rejecting option-like references, then invoking a revision checkout form such as git checkout <validated-reference> (or git switch for branches and detached checkout for commits). Update test_checkout_runs_git_command to assert the complete argument list; assert_called_once() cannot detect this regression.
| def repository_url(self) -> str: | ||
| return f"https://github.com/{self.owner}/{self.repository}.git" | ||
|
|
||
| def clone(self) -> None: |
There was a problem hiding this comment.
Clone/fetch operations for the same cache key are not serialized: two workers can both observe a missing/invalid cache and write the same worktree concurrently. Apply the fix by acquiring a per-cache inter-process lock before the integrity-check/clone-or-fetch sequence. For initial creation, clone into a unique temporary sibling directory, verify it, then atomically rename it into place and clean up failures. Add a concurrent-sync test, or—if the supported runtime is strictly single-worker—enforce that invariant in orchestration and document it explicitly here.
| repository: str, | ||
| branch: str = "main", | ||
| ) -> Path: | ||
| return CACHE_ROOT / owner.casefold() / repository.casefold() / branch.casefold() |
There was a problem hiding this comment.
This path construction has two correctness/safety problems: absolute components or .. can escape CACHE_ROOT, and branch.casefold() collides for case-sensitive Git branches such as Release and release. Apply the fix by validating owner/repository syntax, encoding or hashing the exact branch string, resolving both root and candidate, and rejecting any candidate not contained beneath the resolved root. Preserve branch case in the cache identity. Add traversal, absolute-path, and case-sensitive branch-isolation tests.
|
|
||
| def verify_repository_integrity(self) -> bool: | ||
| git_directory = self.local_path / ".git" | ||
|
|
There was a problem hiding this comment.
Checking only for a .git entry accepts corrupt repositories and caches belonging to a different origin. Apply the fix by running git -C <path> rev-parse --is-inside-work-tree, reading remote.origin.url, normalizing it, and comparing it with repository_url; also verify the configured branch/ref exists. Return false or raise a dedicated integrity error on mismatch, and define safe quarantine/reclone behavior only after cache-root containment is guaranteed. Add tests for a fake .git, wrong origin, missing branch, and interrupted clone.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
application/tests/harvester_test/config_loader_test.py (1)
98-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake production configuration test less brittle.
Asserting the exact length and specific array indices of the production
repos.yamlfile makes this test fragile. If a new repository is added to the configuration in the future, this test will fail. Consider checking for inclusion instead.♻️ Proposed fix for test assertions
- self.assertEqual(len(config.repositories), 2) - self.assertEqual(config.repositories[0].id, "owasp-asvs") - self.assertEqual(config.repositories[1].id, "owasp-cheatsheets") + self.assertGreaterEqual(len(config.repositories), 2) + repo_ids = {repo.id for repo in config.repositories} + self.assertIn("owasp-asvs", repo_ids) + self.assertIn("owasp-cheatsheets", repo_ids)🤖 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 98 - 100, Update the production configuration assertions in the config loader test to verify that repositories with IDs "owasp-asvs" and "owasp-cheatsheets" are included, without asserting the total repository count or relying on fixed array indices.
🤖 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.
Nitpick comments:
In `@application/tests/harvester_test/config_loader_test.py`:
- Around line 98-100: Update the production configuration assertions in the
config loader test to verify that repositories with IDs "owasp-asvs" and
"owasp-cheatsheets" are included, without asserting the total repository count
or relying on fixed array indices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 225bb24b-6824-4573-b8d4-ad7e6deb2330
📒 Files selected for processing (10)
application/tests/harvester_test/config_loader_test.pyapplication/tests/harvester_test/fixtures/duplicate_repo_ids_case.yamlapplication/tests/harvester_test/fixtures/duplicate_repositories_case.yamlapplication/tests/harvester_test/fixtures/invalid_overlap_tokens.yamlapplication/tests/harvester_test/fixtures/whitespace_branch.yamlapplication/tests/harvester_test/fixtures/whitespace_id.yamlapplication/tests/harvester_test/fixtures/whitespace_owner.yamlapplication/tests/harvester_test/fixtures/whitespace_repo.yamlapplication/tests/harvester_test/repos_validator_test.pyapplication/utils/harvester/schemas.py
🚧 Files skipped from review as they are similar to previous changes (2)
- application/tests/harvester_test/repos_validator_test.py
- application/utils/harvester/schemas.py
2 participants
Summary
(this PR is stacked on top of #920 )
This PR introduces the repository client abstraction and Git-backed repository management layer for the harvester pipeline.
It adds a reusable interface for repository operations, deterministic local repository caching, and a Git implementation capable of cloning, fetching, checking out revisions, and tracking the current commit SHA.
Note:
This is part of the Week 2 implementation and is intended to be reviewed after Week 1 (Repository Configuration), even though it targets main.
Added
Validation Coverage
Repository Cache
Git Repository Client
Test Plan
Executed:
python3 -m unittest application.tests.harvester_test.git_repository_client_test
python3 -m unittest application.tests.harvester_test.repository_cache_test
python3 -m unittest discover -s application/tests/harvester_test -p "*_test.py"
make test
All tests passing.
Notes for Reviewers