GSoC Module A : Week 2 (stacked on top of #920)#983
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:
Summary by CodeRabbit
WalkthroughChangesHarvester foundation
Estimated code review effort: 4 (Complex) | ~60 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)
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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application/utils/harvester/git_repository_client.py (2)
27-70: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftRace condition on concurrent
clone()/sync()calls still unresolved.Two workers can both observe a missing/invalid cache via
verify_repository_integrity()and then concurrently write to the samelocal_path, corrupting the worktree. This is the same TOCTOU concern raised in a previous review round (no per-cache lock, no clone-into-temp-then-atomic-rename), and it remains unaddressed in this version —clone()still checks integrity then mutatesself.local_pathdirectly with no synchronization.🤖 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 27 - 70, Protect the repository cache mutation in clone() and the corresponding sync() path with a per-cache lock acquired before verify_repository_integrity() and held through all clone/update operations. Ensure concurrent workers serialize access to the same local_path, and preserve the existing integrity check and clone behavior once the lock is held.
12-21: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winReject option-like branch names. In
application/utils/harvester/git_repository_client.py:12-21,RepositoryConfig.branchis only required to be non-empty, andfetch()passesself.branchdirectly togit fetch origin ...; a branch starting with-can be parsed as an option and break sync. Add the same leading--guard here or in config validation.🤖 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 12 - 21, Reject branch values beginning with “-” during Git repository configuration, preferably in the constructor or existing RepositoryConfig validation, while retaining the non-empty requirement. Ensure fetch() cannot receive an option-like self.branch value, and preserve valid branch handling.
🤖 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.
Outside diff comments:
In `@application/utils/harvester/git_repository_client.py`:
- Around line 27-70: Protect the repository cache mutation in clone() and the
corresponding sync() path with a per-cache lock acquired before
verify_repository_integrity() and held through all clone/update operations.
Ensure concurrent workers serialize access to the same local_path, and preserve
the existing integrity check and clone behavior once the lock is held.
- Around line 12-21: Reject branch values beginning with “-” during Git
repository configuration, preferably in the constructor or existing
RepositoryConfig validation, while retaining the non-empty requirement. Ensure
fetch() cannot receive an option-like self.branch value, and preserve valid
branch handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b0909ef8-3edd-4af6-963c-868575bc401b
📒 Files selected for processing (4)
application/tests/harvester_test/git_repository_client_test.pyapplication/tests/harvester_test/repository_cache_test.pyapplication/utils/harvester/git_repository_client.pyapplication/utils/harvester/repository_cache.py
🚧 Files skipped from review as they are similar to previous changes (2)
- application/utils/harvester/repository_cache.py
- application/tests/harvester_test/git_repository_client_test.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
application/tests/harvester_test/git_repository_client_integration_test.py (1)
42-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnsure temporary directory is cleaned up even if
setUpfails.Using
self.addCleanupimmediately after creating the temporary directory guarantees it will be cleaned up, even if an exception occurs later insetUp. IfsetUpfails halfway,tearDownis never executed, which can lead to leaked resources on disk.♻️ Proposed refactor
- def setUp(self): - self.tempdir = tempfile.TemporaryDirectory() - self.root = Path(self.tempdir.name) - - self.remote = self.root / "remote.git" - self.work = self.root / "work" - self.cache = self.root / "cache" - - git("init", "--bare", self.remote) - - git("clone", self.remote, self.work) - - git("config", "user.name", "Test User", cwd=self.work) - git("config", "user.email", "test@example.com", cwd=self.work) - git("checkout", "-b", "main", cwd=self.work) - - (self.work / "test.txt").write_text("v1") - - git("add", ".", cwd=self.work) - git("commit", "-m", "initial", cwd=self.work) - git("push", "origin", "main", cwd=self.work) - - def tearDown(self): - self.tempdir.cleanup() + def setUp(self): + self.tempdir = tempfile.TemporaryDirectory() + self.addCleanup(self.tempdir.cleanup) + self.root = Path(self.tempdir.name) + + self.remote = self.root / "remote.git" + self.work = self.root / "work" + self.cache = self.root / "cache" + + git("init", "--bare", self.remote) + + git("clone", self.remote, self.work) + + git("config", "user.name", "Test User", cwd=self.work) + git("config", "user.email", "test@example.com", cwd=self.work) + git("checkout", "-b", "main", cwd=self.work) + + (self.work / "test.txt").write_text("v1") + + git("add", ".", cwd=self.work) + git("commit", "-m", "initial", cwd=self.work) + git("push", "origin", "main", cwd=self.work)🤖 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_integration_test.py` around lines 42 - 65, Register self.tempdir.cleanup with self.addCleanup immediately after creating the TemporaryDirectory in setUp, and remove the corresponding cleanup from tearDown to avoid duplicate cleanup while ensuring failures during setup still release the 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.
Nitpick comments:
In `@application/tests/harvester_test/git_repository_client_integration_test.py`:
- Around line 42-65: Register self.tempdir.cleanup with self.addCleanup
immediately after creating the TemporaryDirectory in setUp, and remove the
corresponding cleanup from tearDown to avoid duplicate cleanup while ensuring
failures during setup still release the temporary directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6e09a21a-a75a-46d7-867d-4dca10f3ae6d
📒 Files selected for processing (2)
application/tests/harvester_test/git_repository_client_integration_test.pyapplication/utils/harvester/git_repository_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- application/utils/harvester/git_repository_client.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