GSOC-Week1-Module_A#920
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (5)
Summary by CodeRabbit
WalkthroughAdds harvester configuration schemas, YAML loading and semantic validation, package exports, repository config examples, ignore patterns, and unit tests with YAML fixtures for valid and invalid repository cases. ChangesHarvester Configuration System
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
|
ParthAggarwal16#3 (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: 2
🧹 Nitpick comments (2)
application/tests/harvester_test/test_config_loader.py (1)
43-45: ⚡ Quick winUse Path object for consistency.
Line 45 passes a string literal while all other tests use
Pathobjects fromFIXTURES_DIR. For consistency and clarity, consider usingFIXTURES_DIR / "does_not_exist.yaml"or another Path-based approach.♻️ Proposed fix for consistency
def test_missing_config_file(): - with pytest.raises(FileNotFoundError): - load_repo_config("does_not_exist.yaml") + config_path = FIXTURES_DIR / "does_not_exist.yaml" + with pytest.raises(FileNotFoundError): + load_repo_config(config_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/tests/harvester_test/test_config_loader.py` around lines 43 - 45, Replace the string literal path in test_missing_config_file with a Path-based fixture to match the rest of the tests: call load_repo_config with FIXTURES_DIR / "does_not_exist.yaml" instead of "does_not_exist.yaml" so the test uses a Path object (refer to test_missing_config_file, load_repo_config and FIXTURES_DIR).application/tests/harvester_test/test_repos_validator.py (1)
14-45: ⚡ Quick winConsider testing case-insensitive duplicate detection.
The validator uses
casefold()for case-insensitive comparison (per the relevant code snippet), but there's no test verifying that repository IDs like"ASVS"and"asvs"are correctly identified as duplicates. Adding such a test would verify this important edge case.🔤 Proposed case-insensitive duplicate test
Create a fixture
duplicate_repo_ids_case_insensitive.yaml:repositories: - id: ASVS type: github owner: OWASP repo: ASVS paths: include: - "**/*.md" chunking: strategy: markdown max_tokens: 1200 polling: mode: incremental interval_minutes: 60 - id: asvs type: github owner: OWASP repo: CheatSheetSeries paths: include: - "**/*.md" chunking: strategy: markdown max_tokens: 1200 polling: mode: incremental interval_minutes: 60Then add the test:
def test_duplicate_repository_ids_case_insensitive(): config_path = FIXTURES_DIR / "duplicate_repo_ids_case_insensitive.yaml" config = load_repo_config(config_path) with pytest.raises( RepositoryValidationError, match="Duplicate repository id", ): validate_repositories(config)🤖 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/test_repos_validator.py` around lines 14 - 45, Add a case-insensitive duplicate-id test: create the fixture duplicate_repo_ids_case_insensitive.yaml with two repositories whose ids differ only by case (e.g., "ASVS" and "asvs"), then add a test function test_duplicate_repository_ids_case_insensitive() that loads the fixture via load_repo_config and asserts validate_repositories(config) raises RepositoryValidationError with a "Duplicate repository id" match; this ensures the validate_repositories code path that uses casefold() is exercised.
🤖 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/test_repos_validator.py`:
- Around line 14-45: Add a new success-case test to
application/tests/harvester_test/test_repos_validator.py that ensures
validate_repositories(config) does not raise for a known-good config: create a
test function (e.g., test_validate_valid_repositories) that loads FIXTURES_DIR /
"valid_repos.yaml" via load_repo_config and calls validate_repositories(config)
(no exception assertion needed); reference existing helpers load_repo_config and
the function validate_repositories so the test mirrors the failure tests' style.
In `@application/utils/harvester/__init__.py`:
- Around line 17-27: The __all__ export list is unsorted; reorder the entries
alphabetically so it meets linting standards by sorting the names:
ChunkingConfig, ConfigLoaderError, PathRules, PollingConfig, RepositoryConfig,
RepositoriesValidationError (note actual names below), ReposFile,
load_repo_config, RepositoryValidationError, validate_repositories —
specifically sort the existing symbols ("ChunkingConfig", "ConfigLoaderError",
"PathRules", "PollingConfig", "RepositoryConfig", "ReposFile",
"load_repo_config", "RepositoryValidationError", "validate_repositories") into
ascending alphabetical order and replace the current __all__ definition with
that sorted list.
---
Nitpick comments:
In `@application/tests/harvester_test/test_config_loader.py`:
- Around line 43-45: Replace the string literal path in test_missing_config_file
with a Path-based fixture to match the rest of the tests: call load_repo_config
with FIXTURES_DIR / "does_not_exist.yaml" instead of "does_not_exist.yaml" so
the test uses a Path object (refer to test_missing_config_file, load_repo_config
and FIXTURES_DIR).
In `@application/tests/harvester_test/test_repos_validator.py`:
- Around line 14-45: Add a case-insensitive duplicate-id test: create the
fixture duplicate_repo_ids_case_insensitive.yaml with two repositories whose ids
differ only by case (e.g., "ASVS" and "asvs"), then add a test function
test_duplicate_repository_ids_case_insensitive() that loads the fixture via
load_repo_config and asserts validate_repositories(config) raises
RepositoryValidationError with a "Duplicate repository id" match; this ensures
the validate_repositories code path that uses casefold() is exercised.
🪄 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: 9f37ad90-960e-469e-bfbe-c6dda95e7b54
📒 Files selected for processing (21)
application/tests/harvester_test/__init__.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/test_config_loader.pyapplication/tests/harvester_test/test_repos_validator.pyapplication/utils/harvester/__init__.pyapplication/utils/harvester/config_loader.pyapplication/utils/harvester/exclude_patterns.txtapplication/utils/harvester/repos.yamlapplication/utils/harvester/repos_validator.pyapplication/utils/harvester/schemas.py
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh api repos/OWASP/OpenCRE/commits/0be6df87c7642908c76dc915c48b54c30227acff --jq '.commit.message + "\n---\n" + (.files // [] | map(" " + .filename + " [+" + (.additions|tostring) + "/-" + (.deletions|tostring) + "]\n patch: " + (.patch // "")) | join("\n"))'Length of output: 1054
Both look good. Thanks for the fix! (^‿^)🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Great job on the W1 config layer — the schema vs semantic validation split (config_loader.py / repos_validator.py) is clean, the Pydantic models are well-structured, and the fixture matrix covers the important failure modes. This is ready to merge once the items below are addressed to match repo conventions.
Requested changes
1. Make tests discoverable by make test (blocker)
CI runs:
python -m unittest discover -s application/tests -p "*_test.py"
Your tests are named test_config_loader.py / test_repos_validator.py and use pytest — they never run in CI today.
Fix:
- Rename to
config_loader_test.pyandrepos_validator_test.py(underapplication/tests/harvester_test/). - Convert to
unittest(mirrorapplication/tests/noise_filter/config_loader_test.py):unittest.TestCaseclasses,self.assertRaises/self.assertEqual, nopytest.raises. - Run locally:
make testand confirm both modules are executed (not justflask routes).
2. Cover the remaining fixtures
You already have fixtures without tests:
invalid_polling_interval.yaml(interval_minutes: 0) — add a test expectingConfigLoaderError(Pydanticgt=0oninterval_minutes).empty_include_paths.yaml(include: []) — add a test expectingConfigLoaderError(min_length=1onpaths.include).
3. Rebase onto main
The branch is ~33 commits behind main. Please rebase and push so CI runs fresh on current main.
Note for upcoming weeks (not blocking W1)
Module B’s ChangeRecord contract (merged in #913) expects:
source.repoas a combined slug, e.g."OWASP/ASVS"(not separateowner+repofields in the emitted record).- Chunk sizing in characters (
max_chars, default 4000 in the labeled-dataset builder), while this config usesmax_tokens.
Fine for W1 config-only work — please align chunking units and emission shape in A-W3 so harvest output matches the B contract without a surprise refactor.
Production
No concerns — this is additive and nothing is wired into web/worker/import yet.
Ping when the test/convention fixes are in and rebased; happy to approve and merge then.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
application/tests/harvester_test/config_loader_test.py (1)
12-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing tests for "empty owner", "invalid chunking strategy", "invalid polling mode" fixtures.
The PR description explicitly lists "Empty owner validation," "Invalid chunking strategy," and "Invalid polling mode" as covered validation cases, and corresponding fixtures (
empty_owner.yaml,invalid_chunking_strategy.yaml,invalid_polling_mode.yaml) exist in the fixtures directory, but no test methods in this file exercise them. This leaves those fixtures unused and the claimed coverage unverified.✅ Suggested additions
def test_empty_include_paths(self): config_path = FIXTURES_DIR / "empty_include_paths.yaml" with self.assertRaises(ConfigLoaderError): load_repo_config(config_path) + def test_empty_owner(self): + config_path = FIXTURES_DIR / "empty_owner.yaml" + + with self.assertRaises(ConfigLoaderError): + load_repo_config(config_path) + + def test_invalid_chunking_strategy(self): + config_path = FIXTURES_DIR / "invalid_chunking_strategy.yaml" + + with self.assertRaises(ConfigLoaderError): + load_repo_config(config_path) + + def test_invalid_polling_mode(self): + config_path = FIXTURES_DIR / "invalid_polling_mode.yaml" + + with self.assertRaises(ConfigLoaderError): + load_repo_config(config_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/tests/harvester_test/config_loader_test.py` around lines 12 - 59, Add missing coverage in ConfigLoaderTests for the unused fixtures empty_owner.yaml, invalid_chunking_strategy.yaml, and invalid_polling_mode.yaml by creating focused test methods that call load_repo_config and assert ConfigLoaderError is raised. Use the existing test pattern from test_missing_repository_id, test_invalid_chunk_size, and test_invalid_polling_interval so the new cases are consistent and the validation paths in load_repo_config are exercised.
🤖 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 12-59: Add missing coverage in ConfigLoaderTests for the unused
fixtures empty_owner.yaml, invalid_chunking_strategy.yaml, and
invalid_polling_mode.yaml by creating focused test methods that call
load_repo_config and assert ConfigLoaderError is raised. Use the existing test
pattern from test_missing_repository_id, test_invalid_chunk_size, and
test_invalid_polling_interval so the new cases are consistent and the validation
paths in load_repo_config are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 528a6b53-0a7f-464e-b990-c3f189002cc1
📒 Files selected for processing (2)
application/tests/harvester_test/config_loader_test.pyapplication/tests/harvester_test/repos_validator_test.py
|
yo @northdpole , sorry it took a bit longer than enxpected, but i think it should be good to go now, let me know if anything else is neccesery or i missed |
|
yeah mb, forgot about that one, should be done now |
|
@ParthAggarwal16 Good work , let @northdpole approve the PR , then we can merge |
3 participants
Summary
This PR adds the initial repository configuration loading and validation layer for the harvester pipeline.
Added
Validation Coverage
Config Loader Validation
Repository Semantic Validation
Test Plan
Executed:
bash python3 -m pytest application/tests/harvester_test -v python3 -m coverage report python3 -m black application/utils/harvester application/tests/harvester_test
All tests passing.
Notes for Reviewers
Validation is intentionally split into:
Repository uniqueness checks are case-insensitive.
exclude_patterns.txt is currently scaffolded for future integration into harvesting logic.
ParthAggarwal16#3 (comment)
this comment has some good coderabbit diagrams if you guys wanna have a look at it