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:
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 |
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
|
damn, this was much tougher than i expected, but i think i have reached a stage where i can say its good to go from my side guys ᕙ(⇀‸↼‶)ᕗ |
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Re-reviewed. Prior blockers on this tip look fixed (fetch+reset --hard, checkout without --, cache-path validation, lock around sync, real integrity checks). Review threads resolved.
Not merging yet: after #920 landed this branch conflicts with main. Please rebase and force-push yourself — we will not push to your fork.
git fetch upstream main # or origin, whichever tracks OWASP/OpenCRE
git checkout week_2-clean
git rebase upstream/main
# week-1 commits should drop as already upstream; resolve only if needed
git push --force-with-lease origin week_2-cleanPing when green/conflict-free and I’ll rebase-merge.
Non-blocking for a later PR: also reject branch.startswith("-") in GitRepositoryClient.__init__ (checkout already guards refs).
|
Status: code approved; blocked only by conflicts with |
a1c590f to
adf6065
Compare
|
hey @northdpole rebased and conflicts are gone, this is good to go |
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