GSoC Module A: week 5 : feat(harvester): add git diff retrieval pipeline (stacked on top of #986) - #987
GSoC Module A: week 5 : feat(harvester): add git diff retrieval pipeline (stacked on top of #986)#987ParthAggarwal16 wants to merge 27 commits into
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
WalkthroughAdds harvester runtime and public exports, database-backed checkpoints and ingest records, Git change and diff processing, configurable file filtering, cache and locking updates, migrations, dependencies, and extensive unit, integration, and benchmark tests. ChangesHarvester pipeline
Estimated code review effort: 5 (Critical) | ~120 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: 6
🧹 Nitpick comments (6)
application/utils/harvester/config_loader.py (1)
18-23: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle file access errors more robustly.
Using
is_file()followed byopen()is susceptible to a time-of-check to time-of-use (TOCTOU) race condition. More importantly, if the file exists but the application lacks read permissions,open()will raise aPermissionError(anOSError), which isn't caught here and will bubble up unhandled, bypassing your custom configuration exceptions.Consider catching
OSErrordirectly when opening the file to handle both existence and permission errors uniformly.♻️ Proposed refactor
- if not config_path.is_file(): - raise ConfigFileNotFoundError(f"Configuration file not found: {config_path}") - try: with config_path.open("r", encoding="utf-8") as file: raw_config = yaml.safe_load(file) + except OSError as exc: + raise ConfigFileNotFoundError(f"Cannot access configuration file at {config_path}") from exc except yaml.YAMLError as exc:🤖 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/config_loader.py` around lines 18 - 23, Update the configuration file loading in the config loader to catch OSError directly around config_path.open, converting missing, permission, and other file access failures into the established custom configuration exception. Remove reliance on the preceding is_file check so the open operation is authoritative and avoids the TOCTOU race.application/utils/harvester/__init__.py (1)
29-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__list.To improve maintainability and address the static analysis warning, sort the items in the
__all__list alphabetically.♻️ Proposed refactor
__all__ = [ - "build_repository_cache_path", "ChunkingConfig", "ConfigLoaderError", "DiffRetriever", "FileFilter", "FilteringBenchmark", "FilteringBenchmarkResult", "FilteringMetricsCollector", "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 29 - 47, Sort the __all__ entries in application/utils/harvester/__init__.py alphabetically, preserving every existing export and its spelling.Source: Linters/SAST tools
application/utils/harvester/file_filter.py (1)
25-40: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize regex compilation and extension matching for performance.
When filtering a large number of files during harvesting, computing regexes on the fly and using a generator for
str.endswithcan become a significant bottleneck.Consider the following optimizations:
- Compile the regex patterns once in
__init__.- Convert
allowed_extensionsto a tuple in__init__and pass it directly tostr.endswith. This string method natively checks against a tuple in C, making it much faster than a generator expression.⚡ Proposed performance improvements
class FileFilter: def __init__( self, exclude_patterns: list[str] | None = None, allowed_extensions: set[str] | None = None, ): self.exclude_patterns = exclude_patterns or DEFAULT_EXCLUDE_PATTERNS self.allowed_extensions = allowed_extensions or DEFAULT_ALLOWED_EXTENSIONS + self._compiled_patterns = [re.compile(p) for p in self.exclude_patterns] + self._allowed_ext_tuple = tuple(self.allowed_extensions) def is_excluded_by_pattern(self, file_path: str) -> bool: - return any(re.search(pattern, file_path) for pattern in self.exclude_patterns) + return any(pattern.search(file_path) for pattern in self._compiled_patterns) def is_allowed_extension(self, file_path: str) -> bool: - return any( - file_path.endswith(extension) for extension in self.allowed_extensions - ) + return file_path.endswith(self._allowed_ext_tuple)🤖 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/file_filter.py` around lines 25 - 40, Update FileFilter.__init__ to precompile each exclude pattern with re.compile and store allowed_extensions as a tuple, preserving the configured defaults. Change is_excluded_by_pattern to use the compiled pattern objects and change is_allowed_extension to pass the tuple directly to file_path.endswith instead of iterating with any.application/utils/harvester/diff_normalizer.py (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused import.
The
repository_clientmodule is imported but never used in this file.♻️ Proposed fix
-from application.utils.harvester import repository_client🤖 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/diff_normalizer.py` at line 3, Remove the unused repository_client import from diff_normalizer.py, leaving the remaining imports and implementation unchanged.application/tests/harvester_test/diff_retriever_test.py (1)
13-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate test mocks to align with the proposed byte-handling fix.
If you adopt the recommended change to handle
subprocess.runoutput as bytes inDiffRetriever, you will need to update these mocks to return byte strings (b"...") and remove thetext=Trueassertion.♻️ Proposed test update
def test_get_diff(self, mock_run): mock_run.return_value = MagicMock( - stdout="diff --git a/README.md b/README.md\n", + stdout=b"diff --git a/README.md b/README.md\n", ) client = MagicMock() client.get_local_path.return_value = "/tmp/repo" retriever = DiffRetriever(client) diff = retriever.get_diff( "abc123", "def456", ) self.assertEqual( diff, "diff --git a/README.md b/README.md\n", ) mock_run.assert_called_once_with( [ "git", "-C", "/tmp/repo", "diff", "abc123", "def456", ], capture_output=True, - text=True, check=True, timeout=300, ) `@patch`("application.utils.harvester.diff_retriever.subprocess.run") def test_large_diff_raises(self, mock_run): mock_run.return_value = MagicMock( - stdout="A" * (51 * 1024 * 1024), + stdout=b"A" * (51 * 1024 * 1024), )🤖 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/diff_retriever_test.py` around lines 13 - 52, Update the DiffRetriever test mocks in test_get_diff and test_large_diff_raises to return byte strings, matching the byte-oriented subprocess output handling. Remove text=True from the expected subprocess.run arguments while preserving the existing diff content and size assertions.application/utils/harvester/change_detector.py (1)
21-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
-zwithgit difffor robust filename parsing.By default,
git diff --name-onlyquotes filenames that contain special characters or spaces (e.g.,"path/to/file name.md"). Using the-zflag ensures that filenames are separated by null bytes without quoting, making parsing more robust.♻️ Proposed refactor
[ "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, ) except subprocess.CalledProcessError as exc: logger.error("Git command failed: %s", exc.stderr) raise files = [ - file_path for file_path in result.stdout.splitlines() if file_path.strip() + file_path for file_path in result.stdout.split("\0") if file_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/utils/harvester/change_detector.py` around lines 21 - 41, Update the Git diff invocation in the change-detection method to include the -z option, then parse result.stdout using null-byte separators instead of splitlines while retaining filtering of empty paths. Keep the existing CalledProcessError logging and re-raising behavior unchanged.
🤖 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/diff_pipeline_test.py`:
- Around line 20-30: Call GitRepositoryClient.sync() immediately after
constructing client and before starting the benchmark timer in the diff pipeline
test, ensuring retriever.get_diff() runs against a locally available repository.
In `@application/utils/harvester/checkpoint_store.py`:
- Around line 13-22: Update CheckpointStore.load to remove the separate
checkpoint_file.exists() check and perform read_text directly inside a try
block; catch FileNotFoundError and return None, preserving the existing JSON
parsing behavior when the file is available.
- Around line 36-44: Update CheckpointStore.save to remove the exists-then-read
sequence: attempt read_text directly and catch FileNotFoundError, preserving an
empty data dictionary when the checkpoint file is absent. Protect the complete
read, modification, and replacement/write operation with a file lock shared by
concurrent workers, using the project’s established locking mechanism if
available.
In `@application/utils/harvester/diff_parser.py`:
- Around line 47-57: Update the diff-line header checks in the parser’s
surrounding function to skip only exact `+++` and `---` file-header lines,
including their expected delimiter or path format, rather than any line sharing
those prefixes. Preserve added and removed code whose diff payload begins with
additional `+` or `-` characters, and keep the existing hunk-header and line
collection behavior unchanged.
In `@application/utils/harvester/diff_retriever.py`:
- Around line 47-76: Update the subprocess.run call in the diff retrieval method
to capture raw bytes by removing text mode, then measure the byte length
directly on result.stdout before decoding. Decode the output as UTF-8 with
errors="replace", and retain the existing size-limit validation and return
behavior using the safely decoded diff.
In `@application/utils/harvester/file_filter.py`:
- Around line 11-22: Update DEFAULT_EXCLUDE_PATTERNS so directory exclusions for
.github, .git, node_modules, dist, build, coverage, and vendor match at the
repository root or after any path separator by using (?:^|/); remove the
redundant .* prefixes from the package-lock.json, yarn.lock, and pnpm-lock.yaml
patterns while preserving their re.search matching behavior.
---
Nitpick comments:
In `@application/tests/harvester_test/diff_retriever_test.py`:
- Around line 13-52: Update the DiffRetriever test mocks in test_get_diff and
test_large_diff_raises to return byte strings, matching the byte-oriented
subprocess output handling. Remove text=True from the expected subprocess.run
arguments while preserving the existing diff content and size assertions.
In `@application/utils/harvester/__init__.py`:
- Around line 29-47: Sort the __all__ entries in
application/utils/harvester/__init__.py alphabetically, preserving every
existing export and its spelling.
In `@application/utils/harvester/change_detector.py`:
- Around line 21-41: Update the Git diff invocation in the change-detection
method to include the -z option, then parse result.stdout using null-byte
separators instead of splitlines while retaining filtering of empty paths. Keep
the existing CalledProcessError logging and re-raising behavior unchanged.
In `@application/utils/harvester/config_loader.py`:
- Around line 18-23: Update the configuration file loading in the config loader
to catch OSError directly around config_path.open, converting missing,
permission, and other file access failures into the established custom
configuration exception. Remove reliance on the preceding is_file check so the
open operation is authoritative and avoids the TOCTOU race.
In `@application/utils/harvester/diff_normalizer.py`:
- Line 3: Remove the unused repository_client import from diff_normalizer.py,
leaving the remaining imports and implementation unchanged.
In `@application/utils/harvester/file_filter.py`:
- Around line 25-40: Update FileFilter.__init__ to precompile each exclude
pattern with re.compile and store allowed_extensions as a tuple, preserving the
configured defaults. Change is_excluded_by_pattern to use the compiled pattern
objects and change is_allowed_extension to pass the tuple directly to
file_path.endswith instead of iterating with any.
🪄 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: 4a212f95-eb0a-4747-a72b-a6c38a884f69
📒 Files selected for processing (45)
.gitignoreapplication/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/diff_normalizer_test.pyapplication/tests/harvester_test/diff_parser_test.pyapplication/tests/harvester_test/diff_pipeline_test.pyapplication/tests/harvester_test/diff_retriever_test.pyapplication/tests/harvester_test/file_filter_test.pyapplication/tests/harvester_test/filtering_benchmark_test.pyapplication/tests/harvester_test/filtering_metrics_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/diff_normalizer.pyapplication/utils/harvester/diff_parser.pyapplication/utils/harvester/diff_retriever.pyapplication/utils/harvester/exclude_patterns.txtapplication/utils/harvester/file_filter.pyapplication/utils/harvester/filtering_benchmark.pyapplication/utils/harvester/filtering_metrics.pyapplication/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
| if line.startswith("+++"): | ||
| continue | ||
|
|
||
| if line.startswith("---"): | ||
| continue | ||
|
|
||
| if line.startswith("@@"): | ||
| continue | ||
|
|
||
| if line.startswith("+") and not line.startswith("+++"): | ||
| added_lines.append(line[1:]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix parsing bug that silently drops added lines.
Checking line.startswith("+++") correctly skips the +++ b/file diff header, but it also incorrectly skips any newly added code that starts with ++ (e.g., ++i; or markdown ++ text), which appear as +++i; in the diff. The same applies to ---.
Match the exact header prefixes to safely ignore headers while preserving valid added code.
🛠️ Proposed fix
- if line.startswith("+++"):
+ if line.startswith("+++ b/") or line.startswith("+++ /dev/null"):
continue
- if line.startswith("---"):
+ if line.startswith("--- a/") or line.startswith("--- /dev/null"):
continue
if line.startswith("@@"):
continue
- if line.startswith("+") and not line.startswith("+++"):
+ if line.startswith("+"):
added_lines.append(line[1:])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if line.startswith("+++"): | |
| continue | |
| if line.startswith("---"): | |
| continue | |
| if line.startswith("@@"): | |
| continue | |
| if line.startswith("+") and not line.startswith("+++"): | |
| added_lines.append(line[1:]) | |
| if line.startswith("+++ b/") or line.startswith("+++ /dev/null"): | |
| continue | |
| if line.startswith("--- a/") or line.startswith("--- /dev/null"): | |
| continue | |
| if line.startswith("@@"): | |
| continue | |
| if line.startswith("+"): | |
| added_lines.append(line[1:]) |
🤖 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/diff_parser.py` around lines 47 - 57, Update the
diff-line header checks in the parser’s surrounding function to skip only exact
`+++` and `---` file-header lines, including their expected delimiter or path
format, rather than any line sharing those prefixes. Preserve added and removed
code whose diff payload begins with additional `+` or `-` characters, and keep
the existing hunk-header and line collection behavior unchanged.
|
Updated the benchmark to compare HEAD -1 against HEAD instead of fixed commit SHAs so it remains valid as the upstream repository evolves. |
d4bca65 to
8dea1c7
Compare
|
hey guys, if you are reviewing this, pls hold for a bit, i will update on slack regarding the failing CI test, been doing this for a while now, so i will tackle this tomorrow, need some sleep right now :( |
|
Status: not mergeable (no push from us). CI `Test` is red. Before re-review:
|
|
Still blocked: CI `Test` red, no updates addressing textacy / network tests / parser / rev hardening, and still on the stale stack. Rebase after #986 once that is fixed. |
8dea1c7 to
79080d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application/tests/harvester_test/git_repository_client_test.py (1)
141-154: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winIsolate
test_clone_runs_git_commandwith a temp directory.Unlike other tests in this file, this one doesn't override
local_path, so_clone_atomically's realmkdtemp/os.replacecalls create an actual.harvester_cache/owasp/asvs/maindirectory on disk with no cleanup — polluting the working tree and masking rerun behavior (leftover state changesself.local_path.exists()outcome silently).🛠️ Proposed fix
`@patch`("application.utils.harvester.git_repository_client.subprocess.run") def test_clone_runs_git_command(self, mock_run): - client = GitRepositoryClient( - owner="OWASP", - repository="ASVS", - ) - - with ( - patch.object(client, "verify_repository_integrity", return_value=False), - patch.object(client, "is_valid_repository", return_value=True), - ): - client.clone() - - mock_run.assert_called() + with tempfile.TemporaryDirectory() as tmpdir: + client = GitRepositoryClient( + owner="OWASP", + repository="ASVS", + local_path=Path(tmpdir) / "repo", + ) + + with ( + patch.object(client, "verify_repository_integrity", return_value=False), + patch.object(client, "is_valid_repository", return_value=True), + ): + client.clone() + + mock_run.assert_called()🤖 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 141 - 154, Update test_clone_runs_git_command to use a temporary directory by overriding the GitRepositoryClient local_path (or the relevant cache root) within the test, ensuring _clone_atomically's mkdtemp and os.replace operations remain isolated and are cleaned up after execution. Preserve the existing verify_repository_integrity and is_valid_repository patches and mock_run assertion.application/utils/harvester/git_repository_client.py (1)
54-99: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
sync()can never repair an invalid/corrupted local repo.
_clone_atomicallyskips replacingself.local_pathwhenever it merely exists (Line 81), without checking whether it's valid. Butsync()only callsclone()whenverify_repository_integrity()already returnedFalsefor an existing path — i.e. exactly the case where the directory needs replacing. The freshly-validatedtemp_pathclone is discarded and the broken repo is left in place, silently, with no exception. Every subsequentsync()call repeats this no-op forever.This isn't covered by any test: the integration test that breaks a repo's origin (
test_verify_repository_integrity_rejects_wrong_origin) never callssync()/clone()afterward to verify repair.🐛 Proposed fix
if not self.is_valid_repository(temp_path): raise RuntimeError("Temporary clone failed integrity verification") - if self.local_path.exists(): - shutil.rmtree(temp_path) - return + if self.local_path.exists(): + if self.is_valid_repository(self.local_path): + shutil.rmtree(temp_path) + return + shutil.rmtree(self.local_path) os.replace(temp_path, self.local_path)Consider adding a regression test: mark an existing cache path invalid (e.g. wrong remote, as in
test_verify_repository_integrity_rejects_wrong_origin), callsync(), and assert the repo is repaired (verify_repository_integrity()becomesTrue).🤖 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 54 - 99, Update _clone_atomically so an existing self.local_path is replaced when it is invalid, while preserving it when it remains valid; ensure the validated temporary clone is atomically promoted for corrupted repositories. Add a regression test covering sync() repairing an existing cache with an invalid remote and asserting verify_repository_integrity() returns True.
🧹 Nitpick comments (6)
application/utils/harvester/__init__.py (1)
29-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__alphabetically.Static analysis flags
__all__as unsorted.♻️ Proposed fix
__all__ = [ - "build_repository_cache_path", "ChunkingConfig", "ConfigLoaderError", "DiffRetriever", - "GitRepositoryClient", "FileFilter", - "FilteringMetricsCollector", "FilteringBenchmark", "FilteringBenchmarkResult", + "FilteringMetricsCollector", + "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 29 - 47, Sort the entries in the module-level __all__ list alphabetically, preserving every existing exported symbol and changing only their order.Source: Linters/SAST tools
application/utils/harvester/checkpoint_store.py (1)
73-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve exception context on the
IntegrityErrorbranch.
raise ValueError(...)inside theexcept IntegrityError:block drops the original exception's traceback/context.♻️ Proposed fix
- except IntegrityError: + except IntegrityError as exc: session.rollback() - raise ValueError("duplicate canonical source identity") + raise ValueError("duplicate canonical source identity") from exc🤖 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/checkpoint_store.py` around lines 73 - 80, Update the IntegrityError handler around session.commit in the checkpoint store so the ValueError for duplicate canonical source identity preserves the original exception context. Keep the rollback behavior unchanged and explicitly chain the new ValueError from the caught IntegrityError, while leaving the generic exception branch unchanged.Source: Linters/SAST tools
application/utils/harvester/diff_normalizer.py (2)
20-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping blocks that become fully empty after normalization.
If every line in
block.added_linesnormalizes to empty,normalizestill emits aDiffBlockwithadded_lines=[]. Filtering these out would avoid passing empty blocks to downstream chunking/ingestion.♻️ Optional fix
normalized.append( - DiffBlock( - file_path=block.file_path, - added_lines=cleaned_lines, - repository=block.repository, - commit_sha=block.commit_sha, - committed_at=block.committed_at, - ) - ) + DiffBlock( + file_path=block.file_path, + added_lines=cleaned_lines, + repository=block.repository, + commit_sha=block.commit_sha, + committed_at=block.committed_at, + ) + ) if cleaned_lines else None🤖 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/diff_normalizer.py` around lines 20 - 47, Update normalize so it only appends a DiffBlock after normalization when cleaned_lines contains at least one line; skip blocks whose added_lines all normalize to empty, while preserving the existing field mapping for retained blocks.
3-3: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the unused
repository_clientimport fromdiff_normalizer.py.
repository_client.pyexists, so the module will not fail to import, butDiffNormalizerdoes not userepository_client, so keep the import block minimal.🤖 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/diff_normalizer.py` at line 3, Remove the unused repository_client import from diff_normalizer.py, leaving the remaining imports unchanged.application/database/db.py (2)
353-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant local
datetime/timezoneimports shadow the new module-level import.Line 11 adds
from datetime import datetime, timezoneat module scope, yet_normalize_utc_datetime(Line 354),create_artifact_ingest_event(Line 422), andcreate_ingest_chunk(Line 453) each re-import the same names locally. These local imports are now dead weight since the module-level import already covers them.Also applies to: 410-471
🤖 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/database/db.py` around lines 353 - 360, Remove the redundant local datetime/timezone imports from _normalize_utc_datetime, create_artifact_ingest_event, and create_ingest_chunk, reusing the existing module-level import while preserving all current datetime handling.
261-347: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInconsistent timestamp timezone-awareness across new tables.
ArtifactIngestEvent.observed_at/created_at(Lines 278-279) andIngestChunk.created_at(Line 309) use plainsqla.DateTime, whileHarvesterCheckpoint.created_at/updated_at(Lines 328-337), added in the same commit, usesqla.DateTime(timezone=True). Since_normalize_utc_datetimeconverts values to UTC-aware but the column itself is timezone-naive, the tz metadata is silently dropped on write for these two tables. Recommend usingDateTime(timezone=True)consistently for all new timestamp columns.♻️ Proposed fix
- observed_at = sqla.Column(sqla.DateTime, nullable=False) - created_at = sqla.Column(sqla.DateTime, nullable=False) + observed_at = sqla.Column(sqla.DateTime(timezone=True), nullable=False) + created_at = sqla.Column(sqla.DateTime(timezone=True), nullable=False)- created_at = sqla.Column(sqla.DateTime, nullable=False) + created_at = sqla.Column(sqla.DateTime(timezone=True), nullable=False)🤖 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/database/db.py` around lines 261 - 347, Update the timestamp columns in ArtifactIngestEvent and IngestChunk to use sqla.DateTime(timezone=True), matching HarvesterCheckpoint.created_at and updated_at. Apply this to observed_at, both created_at columns, and preserve their existing nullability and other column settings.
🤖 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/database/db.py`:
- Around line 410-469: Update create_artifact_ingest_event and
create_ingest_chunk to catch SQLAlchemy IntegrityError around
session.add/commit, roll back sqla.session, and re-raise a clear
domain-appropriate error identifying the duplicate run/artifact or
artifact-event/chunk key. Follow the existing rollback-and-rethrow pattern used
by CheckpointStore.save and add_node/add_cre, while preserving successful
creation and return behavior.
In `@application/utils/harvester/diff_normalizer.py`:
- Line 1: Declare the runtime textacy dependency in requirements.txt so the
import used by diff_normalizer.py is available in production installations. Add
it with the approved version constraint, preserving the existing dependency
formatting.
In `@application/utils/harvester/diff_parser.py`:
- Around line 27-45: Reset current_file and added_lines after appending the
previous DiffBlock and before parsing each new header in the diff parsing loop.
Ensure a failed match for quoted or otherwise unsupported paths leaves
current_file unset and uses a fresh added_lines list, preventing subsequent
additions from mutating the already-emitted block; preserve normal parsing for
successfully matched headers.
In `@application/utils/harvester/diff_retriever.py`:
- Around line 83-99: Update _resolve_commit to catch subprocess failures from
rev-parse, log the failure with the commit value and resolution context using
the same logging pattern as get_diff, then re-raise the original exception
unchanged.
In `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py`:
- Around line 1-7: Update the migration module docstring’s “Revises” value to
match the down_revision value declared in the migration module. Keep the
existing revision identifier and all migration logic unchanged.
In `@requirements-dev.txt`:
- Line 81: Remove the textacy dependency from requirements-dev.txt, or obtain
and document explicit approval before retaining it; do not leave the CI
dependency addition unapproved.
---
Outside diff comments:
In `@application/tests/harvester_test/git_repository_client_test.py`:
- Around line 141-154: Update test_clone_runs_git_command to use a temporary
directory by overriding the GitRepositoryClient local_path (or the relevant
cache root) within the test, ensuring _clone_atomically's mkdtemp and os.replace
operations remain isolated and are cleaned up after execution. Preserve the
existing verify_repository_integrity and is_valid_repository patches and
mock_run assertion.
In `@application/utils/harvester/git_repository_client.py`:
- Around line 54-99: Update _clone_atomically so an existing self.local_path is
replaced when it is invalid, while preserving it when it remains valid; ensure
the validated temporary clone is atomically promoted for corrupted repositories.
Add a regression test covering sync() repairing an existing cache with an
invalid remote and asserting verify_repository_integrity() returns True.
---
Nitpick comments:
In `@application/database/db.py`:
- Around line 353-360: Remove the redundant local datetime/timezone imports from
_normalize_utc_datetime, create_artifact_ingest_event, and create_ingest_chunk,
reusing the existing module-level import while preserving all current datetime
handling.
- Around line 261-347: Update the timestamp columns in ArtifactIngestEvent and
IngestChunk to use sqla.DateTime(timezone=True), matching
HarvesterCheckpoint.created_at and updated_at. Apply this to observed_at, both
created_at columns, and preserve their existing nullability and other column
settings.
In `@application/utils/harvester/__init__.py`:
- Around line 29-47: Sort the entries in the module-level __all__ list
alphabetically, preserving every existing exported symbol and changing only
their order.
In `@application/utils/harvester/checkpoint_store.py`:
- Around line 73-80: Update the IntegrityError handler around session.commit in
the checkpoint store so the ValueError for duplicate canonical source identity
preserves the original exception context. Keep the rollback behavior unchanged
and explicitly chain the new ValueError from the caught IntegrityError, while
leaving the generic exception branch unchanged.
In `@application/utils/harvester/diff_normalizer.py`:
- Around line 20-47: Update normalize so it only appends a DiffBlock after
normalization when cleaned_lines contains at least one line; skip blocks whose
added_lines all normalize to empty, while preserving the existing field mapping
for retained blocks.
- Line 3: Remove the unused repository_client import from diff_normalizer.py,
leaving the remaining imports unchanged.
🪄 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 Plus
Run ID: 9650aacd-8918-4f73-aa74-28478cb93fc5
📒 Files selected for processing (31)
.gitignoreapplication/database/db.pyapplication/tests/harvester_test/change_detector_test.pyapplication/tests/harvester_test/checkpoint_store_test.pyapplication/tests/harvester_test/diff_normalizer_test.pyapplication/tests/harvester_test/diff_parser_test.pyapplication/tests/harvester_test/diff_pipeline_test.pyapplication/tests/harvester_test/diff_retriever_test.pyapplication/tests/harvester_test/file_filter_test.pyapplication/tests/harvester_test/filtering_benchmark_test.pyapplication/tests/harvester_test/filtering_metrics_test.pyapplication/tests/harvester_test/git_repository_client_integration_test.pyapplication/tests/harvester_test/git_repository_client_test.pyapplication/tests/import_run_test.pyapplication/utils/harvester/__init__.pyapplication/utils/harvester/change_detector.pyapplication/utils/harvester/checkpoint_store.pyapplication/utils/harvester/diff_normalizer.pyapplication/utils/harvester/diff_parser.pyapplication/utils/harvester/diff_retriever.pyapplication/utils/harvester/exclude_patterns.txtapplication/utils/harvester/file_filter.pyapplication/utils/harvester/filtering_benchmark.pyapplication/utils/harvester/filtering_metrics.pyapplication/utils/harvester/git_repository_client.pyapplication/utils/harvester/models.pyapplication/utils/harvester/repository_cache.pyapplication/utils/harvester/repository_lock.pymigrations/versions/6a9d0d62ef41_add_harvester_checkpoint_table.pymigrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.pyrequirements-dev.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- application/tests/harvester_test/filtering_benchmark_test.py
- application/tests/harvester_test/filtering_metrics_test.py
- application/utils/harvester/filtering_metrics.py
- .gitignore
| def create_artifact_ingest_event( | ||
| *, | ||
| run_id: str, | ||
| artifact_id: str, | ||
| harvest_mode: str, | ||
| event_type: str, | ||
| source_json: Any, | ||
| locator_json: Any, | ||
| artifact_json: Any, | ||
| harvest_json: Any, | ||
| observed_at: Any, | ||
| ) -> ArtifactIngestEvent: | ||
| from datetime import datetime, timezone | ||
|
|
||
| observed_at = _normalize_utc_datetime(observed_at) | ||
|
|
||
| event = ArtifactIngestEvent( | ||
| id=generate_uuid(), | ||
| run_id=run_id, | ||
| artifact_id=artifact_id, | ||
| harvest_mode=harvest_mode, | ||
| event_type=event_type, | ||
| source_json=_serialize_json_value(source_json), | ||
| locator_json=_serialize_json_value(locator_json), | ||
| artifact_json=_serialize_json_value(artifact_json), | ||
| harvest_json=_serialize_json_value(harvest_json), | ||
| observed_at=observed_at, | ||
| created_at=_normalize_utc_datetime(datetime.now(timezone.utc)), | ||
| ) | ||
| sqla.session.add(event) | ||
| sqla.session.commit() | ||
| return event | ||
|
|
||
|
|
||
| def create_ingest_chunk( | ||
| *, | ||
| artifact_event_id: str, | ||
| chunk_id: str, | ||
| text: str, | ||
| char_count: int, | ||
| span_json: Any, | ||
| delta_json: Optional[Any] = None, | ||
| ) -> IngestChunk: | ||
| from datetime import datetime, timezone | ||
|
|
||
| chunk = IngestChunk( | ||
| id=generate_uuid(), | ||
| artifact_event_id=artifact_event_id, | ||
| chunk_id=chunk_id, | ||
| text=text, | ||
| char_count=char_count, | ||
| span_json=_serialize_json_value(span_json), | ||
| delta_json=( | ||
| _serialize_json_value(delta_json) if delta_json is not None else None | ||
| ), | ||
| created_at=_normalize_utc_datetime(datetime.now(timezone.utc)), | ||
| ) | ||
| sqla.session.add(chunk) | ||
| sqla.session.commit() | ||
| return chunk |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unhandled IntegrityError on the new unique constraints.
Neither create_artifact_ingest_event nor create_ingest_chunk catches IntegrityError from uq_artifact_ingest_event_run_artifact / uq_ingest_chunk_artifact_chunk. A retried/duplicate ingest (same run_id+artifact_id, or same artifact_event_id+chunk_id) will raise uncaught, leaving the session in a state requiring rollback before further use. CheckpointStore.save() and add_node/add_cre in this same file already establish the pattern of catching IntegrityError, rolling back, and re-raising a clear error.
🐛 Proposed fix
sqla.session.add(event)
- sqla.session.commit()
+ try:
+ sqla.session.commit()
+ except IntegrityError:
+ sqla.session.rollback()
+ raise
return event(same pattern for create_ingest_chunk)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def create_artifact_ingest_event( | |
| *, | |
| run_id: str, | |
| artifact_id: str, | |
| harvest_mode: str, | |
| event_type: str, | |
| source_json: Any, | |
| locator_json: Any, | |
| artifact_json: Any, | |
| harvest_json: Any, | |
| observed_at: Any, | |
| ) -> ArtifactIngestEvent: | |
| from datetime import datetime, timezone | |
| observed_at = _normalize_utc_datetime(observed_at) | |
| event = ArtifactIngestEvent( | |
| id=generate_uuid(), | |
| run_id=run_id, | |
| artifact_id=artifact_id, | |
| harvest_mode=harvest_mode, | |
| event_type=event_type, | |
| source_json=_serialize_json_value(source_json), | |
| locator_json=_serialize_json_value(locator_json), | |
| artifact_json=_serialize_json_value(artifact_json), | |
| harvest_json=_serialize_json_value(harvest_json), | |
| observed_at=observed_at, | |
| created_at=_normalize_utc_datetime(datetime.now(timezone.utc)), | |
| ) | |
| sqla.session.add(event) | |
| sqla.session.commit() | |
| return event | |
| def create_ingest_chunk( | |
| *, | |
| artifact_event_id: str, | |
| chunk_id: str, | |
| text: str, | |
| char_count: int, | |
| span_json: Any, | |
| delta_json: Optional[Any] = None, | |
| ) -> IngestChunk: | |
| from datetime import datetime, timezone | |
| chunk = IngestChunk( | |
| id=generate_uuid(), | |
| artifact_event_id=artifact_event_id, | |
| chunk_id=chunk_id, | |
| text=text, | |
| char_count=char_count, | |
| span_json=_serialize_json_value(span_json), | |
| delta_json=( | |
| _serialize_json_value(delta_json) if delta_json is not None else None | |
| ), | |
| created_at=_normalize_utc_datetime(datetime.now(timezone.utc)), | |
| ) | |
| sqla.session.add(chunk) | |
| sqla.session.commit() | |
| return chunk | |
| def create_artifact_ingest_event( | |
| *, | |
| run_id: str, | |
| artifact_id: str, | |
| harvest_mode: str, | |
| event_type: str, | |
| source_json: Any, | |
| locator_json: Any, | |
| artifact_json: Any, | |
| harvest_json: Any, | |
| observed_at: Any, | |
| ) -> ArtifactIngestEvent: | |
| from datetime import datetime, timezone | |
| observed_at = _normalize_utc_datetime(observed_at) | |
| event = ArtifactIngestEvent( | |
| id=generate_uuid(), | |
| run_id=run_id, | |
| artifact_id=artifact_id, | |
| harvest_mode=harvest_mode, | |
| event_type=event_type, | |
| source_json=_serialize_json_value(source_json), | |
| locator_json=_serialize_json_value(locator_json), | |
| artifact_json=_serialize_json_value(artifact_json), | |
| harvest_json=_serialize_json_value(harvest_json), | |
| observed_at=observed_at, | |
| created_at=_normalize_utc_datetime(datetime.now(timezone.utc)), | |
| ) | |
| sqla.session.add(event) | |
| try: | |
| sqla.session.commit() | |
| except IntegrityError: | |
| sqla.session.rollback() | |
| raise | |
| return event | |
| def create_ingest_chunk( | |
| *, | |
| artifact_event_id: str, | |
| chunk_id: str, | |
| text: str, | |
| char_count: int, | |
| span_json: Any, | |
| delta_json: Optional[Any] = None, | |
| ) -> IngestChunk: | |
| from datetime import datetime, timezone | |
| chunk = IngestChunk( | |
| id=generate_uuid(), | |
| artifact_event_id=artifact_event_id, | |
| chunk_id=chunk_id, | |
| text=text, | |
| char_count=char_count, | |
| span_json=_serialize_json_value(span_json), | |
| delta_json=( | |
| _serialize_json_value(delta_json) if delta_json is not None else None | |
| ), | |
| created_at=_normalize_utc_datetime(datetime.now(timezone.utc)), | |
| ) | |
| sqla.session.add(chunk) | |
| try: | |
| sqla.session.commit() | |
| except IntegrityError: | |
| sqla.session.rollback() | |
| raise | |
| return chunk |
🤖 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/database/db.py` around lines 410 - 469, Update
create_artifact_ingest_event and create_ingest_chunk to catch SQLAlchemy
IntegrityError around session.add/commit, roll back sqla.session, and re-raise a
clear domain-appropriate error identifying the duplicate run/artifact or
artifact-event/chunk key. Follow the existing rollback-and-rethrow pattern used
by CheckpointStore.save and add_node/add_cre, while preserving successful
creation and return behavior.
| def _resolve_commit(self, commit: str) -> str: | ||
| result = subprocess.run( | ||
| [ | ||
| "git", | ||
| "-C", | ||
| str(self.repository_client.get_local_path()), | ||
| "rev-parse", | ||
| "--verify", | ||
| "--end-of-options", | ||
| f"{commit}^{{commit}}", | ||
| ], | ||
| check=True, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=60, | ||
| ) | ||
| return result.stdout.strip() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Log rev-parse failures before re-raising, consistent with get_diff's error handling.
_resolve_commit's subprocess.run has no try/except, so a failed commit resolution (e.g., invalid/garbage SHA from a corrupted checkpoint) propagates as a bare CalledProcessError with no context about which commit/step failed. The get_diff diff call right above it does log on failure (lines 64-69), and the PR's own git-error-handling design states rev-parse failures should be "logged and re-raised" — the same pattern isn't applied here.
🛠️ Proposed fix
def _resolve_commit(self, commit: str) -> str:
- result = subprocess.run(
- [
- "git",
- "-C",
- str(self.repository_client.get_local_path()),
- "rev-parse",
- "--verify",
- "--end-of-options",
- f"{commit}^{{commit}}",
- ],
- check=True,
- capture_output=True,
- text=True,
- timeout=60,
- )
+ try:
+ result = subprocess.run(
+ [
+ "git",
+ "-C",
+ str(self.repository_client.get_local_path()),
+ "rev-parse",
+ "--verify",
+ "--end-of-options",
+ f"{commit}^{{commit}}",
+ ],
+ check=True,
+ capture_output=True,
+ text=True,
+ timeout=60,
+ )
+ except subprocess.CalledProcessError as exc:
+ logger.error("Failed to resolve commit %s: %s", commit, exc.stderr)
+ raise
return result.stdout.strip()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _resolve_commit(self, commit: str) -> str: | |
| result = subprocess.run( | |
| [ | |
| "git", | |
| "-C", | |
| str(self.repository_client.get_local_path()), | |
| "rev-parse", | |
| "--verify", | |
| "--end-of-options", | |
| f"{commit}^{{commit}}", | |
| ], | |
| check=True, | |
| capture_output=True, | |
| text=True, | |
| timeout=60, | |
| ) | |
| return result.stdout.strip() | |
| def _resolve_commit(self, commit: str) -> str: | |
| try: | |
| result = subprocess.run( | |
| [ | |
| "git", | |
| "-C", | |
| str(self.repository_client.get_local_path()), | |
| "rev-parse", | |
| "--verify", | |
| "--end-of-options", | |
| f"{commit}^{{commit}}", | |
| ], | |
| check=True, | |
| capture_output=True, | |
| text=True, | |
| timeout=60, | |
| ) | |
| except subprocess.CalledProcessError as exc: | |
| logger.error("Failed to resolve commit %s: %s", commit, exc.stderr) | |
| raise | |
| return result.stdout.strip() |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 83-97: Command coming from incoming request
Context: subprocess.run(
[
"git",
"-C",
str(self.repository_client.get_local_path()),
"rev-parse",
"--verify",
"--end-of-options",
f"{commit}^{{commit}}",
],
check=True,
capture_output=True,
text=True,
timeout=60,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.0)
[error] 84-84: subprocess call: check for execution of untrusted input
(S603)
[error] 85-93: Starting a process with a partial executable path
(S607)
🤖 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/diff_retriever.py` around lines 83 - 99, Update
_resolve_commit to catch subprocess failures from rev-parse, log the failure
with the commit value and resolution context using the same logging pattern as
get_diff, then re-raise the original exception unchanged.
| """add artifact ingest event and chunk tables | ||
|
|
||
| Revision ID: 9f1a2b3c4d5e | ||
| Revises: e1f2a3b4c5d6 | ||
| Create Date: 2026-07-23 | ||
|
|
||
| """ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Docstring Revises: doesn't match down_revision.
Header comment says Revises: e1f2a3b4c5d6 but down_revision = "c7d8e9f0a1b2" (Line 14). Alembic uses the variable for chain resolution, so this won't break execution, but the stale docstring will mislead anyone reading migration history.
📝 Proposed fix
-Revises: e1f2a3b4c5d6
+Revises: c7d8e9f0a1b2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """add artifact ingest event and chunk tables | |
| Revision ID: 9f1a2b3c4d5e | |
| Revises: e1f2a3b4c5d6 | |
| Create Date: 2026-07-23 | |
| """ | |
| """add artifact ingest event and chunk tables | |
| Revision ID: 9f1a2b3c4d5e | |
| Revises: c7d8e9f0a1b2 | |
| Create Date: 2026-07-23 | |
| """ |
🤖 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 `@migrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py` around
lines 1 - 7, Update the migration module docstring’s “Revises” value to match
the down_revision value declared in the migration module. Keep the existing
revision identifier and all migration logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/diff_parser.py`:
- Around line 39-42: Update the diff-header parsing in the diff parser to
recognize Git-quoted paths, including filenames containing spaces or special
characters, so current_file is populated and additions are retained. If the
header cannot be parsed, raise an explicit error instead of silently setting
current_file to None; add a regression test covering a quoted filename with
spaces.
🪄 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 Plus
Run ID: c4817b1b-f078-424c-adba-3327d55dc695
📒 Files selected for processing (1)
application/utils/harvester/diff_parser.py
| match = re.match(r"diff --git a/(.+?) b/", line) | ||
|
|
||
| current_file = match.group(1) if match else None | ||
| added_lines = [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Avoid silently dropping valid quoted-path diffs.
The state reset fixes the stale-block mutation, but Git quotes headers for valid paths containing spaces or special characters, so this regex can fail. current_file then remains None, and all additions for that file are silently discarded. Parse quoted Git paths or raise an explicit error; add a regression test for a filename with spaces.
🤖 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/diff_parser.py` around lines 39 - 42, Update the
diff-header parsing in the diff parser to recognize Git-quoted paths, including
filenames containing spaces or special characters, so current_file is populated
and additions are retained. If the header cannot be parsed, raise an explicit
error instead of silently setting current_file to None; add a regression test
covering a quoted filename with spaces.
There was a problem hiding this comment.
good catch, the stale state issue has been fixed, supporting quoted git paths would require broadening the parser beyond the repositories currently harvested by us, so I’d prefer to keep that as a follow up enhancement if we encounter repositories that require it
There was a problem hiding this comment.
@ParthAggarwal16, understood—deferring quoted-path support is reasonable given the currently targeted repositories. The parser’s behavior for such paths remains a known limitation, so I’ll leave this finding open rather than treat it as resolved.
Would you like me to create a follow-up GitHub issue to track quoted Git diff-header parsing and its regression coverage?
✏️ Learnings added
Learnt from: ParthAggarwal16
Repo: OWASP/OpenCRE PR: 987
File: application/utils/harvester/diff_parser.py:39-42
Timestamp: 2026-07-30T13:39:23.227Z
Learning: For the Python harvester diff pipeline in `application/utils/harvester/diff_parser.py`, support for Git-quoted paths (such as filenames containing spaces or special characters) is intentionally deferred because the repositories currently harvested do not require it. Treat unsupported quoted diff headers as a known follow-up limitation unless the harvester’s repository scope expands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
@northdpole this one is good to go |
2 participants
Summary
(this PR is stacked on top of #986 )
This PR implements the initial diff processing pipeline for the harvester, introducing the components required to retrieve, parse, and normalize repository diffs before downstream document processing.
Changes
Diff Retrieval
DiffRetrieverfor retrieving git diffs between commits.Diff Parsing
DiffParserto convert unified git diffs into structuredDiffBlockobjects.Diff Normalization
DiffNormalizerto normalize extracted diff content.Models
DiffBlockas the intermediate representation for parsed diff content.Exports
DiffRetrieverthrough the harvester package.Tests
Added unit tests covering:
Why
These components establish the foundation of the incremental harvesting pipeline. Instead of processing entire repositories, the harvester can now retrieve repository diffs, extract newly added content, normalize it into a consistent format, and prepare it for subsequent embedding and indexing stages.
Testing
This diagram combines the independently implemented modules into the expected polling flow. The orchestration itself is not yet present in a production entry point.
smoke tests:
