From 534eaaa67482713b2ce2e111b1bfcf799c69f383 Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Sat, 18 Jul 2026 21:08:19 +0530 Subject: [PATCH 01/10] fix(harvester): improve filtering benchmark and sync behavior --- .../harvester_test/git_repository_client_test.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/application/tests/harvester_test/git_repository_client_test.py b/application/tests/harvester_test/git_repository_client_test.py index 941afd67e..1121c973f 100644 --- a/application/tests/harvester_test/git_repository_client_test.py +++ b/application/tests/harvester_test/git_repository_client_test.py @@ -89,6 +89,21 @@ def test_sync_fetches_when_repository_exists(self, mock_run): mock_fetch.assert_called_once() + mock_run.assert_called_once_with( + [ + "git", + "-C", + str(client.get_local_path()), + "reset", + "--hard", + "origin/main", + ], + check=True, + capture_output=True, + text=True, + timeout=300, + ) + @patch("application.utils.harvester.git_repository_client.subprocess.run") def test_fetch_runs_git_command(self, mock_run): client = GitRepositoryClient( From 0037cc90f838dca925e7bf34c348e608562fd1f4 Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Fri, 10 Jul 2026 13:47:17 +0530 Subject: [PATCH 02/10] feat(harvester): add git diff retrieval pipeline --- .gitignore | 2 + .../harvester_test/diff_retriever_test.py | 49 +++++++++++++++++++ application/utils/harvester/__init__.py | 3 ++ application/utils/harvester/diff_retriever.py | 39 +++++++++++++++ 4 files changed, 93 insertions(+) create mode 100644 application/tests/harvester_test/diff_retriever_test.py create mode 100644 application/utils/harvester/diff_retriever.py diff --git a/.gitignore b/.gitignore index 26b6ceb61..bc53fd244 100644 --- a/.gitignore +++ b/.gitignore @@ -85,3 +85,5 @@ tmp/ cres/* ### Local project management tooling project management scripts/ + +.harvester_cache/ diff --git a/application/tests/harvester_test/diff_retriever_test.py b/application/tests/harvester_test/diff_retriever_test.py new file mode 100644 index 000000000..e716f2452 --- /dev/null +++ b/application/tests/harvester_test/diff_retriever_test.py @@ -0,0 +1,49 @@ +import unittest +from unittest.mock import MagicMock +from unittest.mock import patch + +from application.utils.harvester.diff_retriever import ( + DiffRetriever, +) + + +class DiffRetrieverTests(unittest.TestCase): + @patch("application.utils.harvester.diff_retriever.subprocess.run") + def test_get_diff(self, mock_run): + mock_run.return_value = MagicMock( + stdout="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, + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/application/utils/harvester/__init__.py b/application/utils/harvester/__init__.py index eda28b629..9961aae16 100644 --- a/application/utils/harvester/__init__.py +++ b/application/utils/harvester/__init__.py @@ -19,6 +19,8 @@ from .repository_cache import build_repository_cache_path from .file_filter import FileFilter from .filtering_metrics import FilteringMetricsCollector +from .diff_retriever import DiffRetriever + from .filtering_benchmark import ( FilteringBenchmark, FilteringBenchmarkResult, @@ -28,6 +30,7 @@ "build_repository_cache_path", "ChunkingConfig", "ConfigLoaderError", + "DiffRetriever", "GitRepositoryClient", "FileFilter", "FilteringMetricsCollector", diff --git a/application/utils/harvester/diff_retriever.py b/application/utils/harvester/diff_retriever.py new file mode 100644 index 000000000..6067c9216 --- /dev/null +++ b/application/utils/harvester/diff_retriever.py @@ -0,0 +1,39 @@ +import logging +import subprocess + +from .git_repository_client import GitRepositoryClient + +logger = logging.getLogger(__name__) + + +class DiffRetriever: + def __init__(self, repository_client: GitRepositoryClient) -> None: + self.repository_client = repository_client + + def get_diff(self, base_commit: str, target_commit: str = "HEAD") -> str: + logger.info( + "Retrieving diff between %s and %s", + base_commit, + target_commit, + ) + + try: + result = subprocess.run( + [ + "git", + "-C", + str(self.repository_client.get_local_path()), + "diff", + base_commit, + target_commit, + ], + check=True, + capture_output=True, + text=True, + timeout=300, + ) + except subprocess.CalledProcessError as exc: + logger.error("Failed to retrieve diff: %s", exc.stderr) + raise + + return result.stdout From 31c2a7fdf029009fe909ed0697a04cc16a682287 Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Fri, 10 Jul 2026 14:00:26 +0530 Subject: [PATCH 03/10] feat(harvester): parse unified git diffs --- .../tests/harvester_test/diff_parser_test.py | 89 +++++++++++++++++++ application/utils/harvester/diff_parser.py | 43 +++++++++ application/utils/harvester/models.py | 6 ++ 3 files changed, 138 insertions(+) create mode 100644 application/tests/harvester_test/diff_parser_test.py create mode 100644 application/utils/harvester/diff_parser.py diff --git a/application/tests/harvester_test/diff_parser_test.py b/application/tests/harvester_test/diff_parser_test.py new file mode 100644 index 000000000..db001df4f --- /dev/null +++ b/application/tests/harvester_test/diff_parser_test.py @@ -0,0 +1,89 @@ +import unittest + +from application.utils.harvester.diff_parser import ( + DiffParser, +) + + +class DiffParserTests(unittest.TestCase): + def test_single_file_diff(self): + parser = DiffParser() + + diff = """diff --git a/test.md b/test.md +--- a/test.md ++++ b/test.md +@@ +-old ++new ++another +""" + + blocks = parser.parse(diff) + + self.assertEqual( + len(blocks), + 1, + ) + + self.assertEqual( + blocks[0].file_path, + "test.md", + ) + + self.assertEqual( + blocks[0].added_lines, + [ + "new", + "another", + ], + ) + + def test_multiple_files(self): + parser = DiffParser() + + diff = """diff --git a/a.md b/a.md +@@ ++one +diff --git a/b.md b/b.md +@@ ++two +""" + + blocks = parser.parse(diff) + + self.assertEqual( + len(blocks), + 2, + ) + + self.assertEqual( + blocks[0].file_path, + "a.md", + ) + + self.assertEqual( + blocks[1].file_path, + "b.md", + ) + + def test_deleted_lines_are_ignored(self): + parser = DiffParser() + + diff = """diff --git a/test.md b/test.md +@@ +-old ++new +""" + + blocks = parser.parse(diff) + + self.assertEqual( + blocks[0].added_lines, + [ + "new", + ], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/application/utils/harvester/diff_parser.py b/application/utils/harvester/diff_parser.py new file mode 100644 index 000000000..d9c8e9d32 --- /dev/null +++ b/application/utils/harvester/diff_parser.py @@ -0,0 +1,43 @@ +import re + +from .models import DiffBlock + + +class DiffParser: + def parse(self, diff: str) -> list[DiffBlock]: + blocks: list[DiffBlock] = [] + + current_file: str | None = None + added_lines: list[str] = [] + + for line in diff.splitlines(): + if line.startswith("diff --git"): + if current_file is not None: + blocks.append( + DiffBlock(file_path=current_file, added_lines=added_lines) + ) + + match = re.match(r"diff --git a/(.+?) b/", line) + + if match: + current_file = match.group(1) + added_lines = [] + + continue + + 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 current_file is not None: + blocks.append(DiffBlock(file_path=current_file, added_lines=added_lines)) + + return blocks diff --git a/application/utils/harvester/models.py b/application/utils/harvester/models.py index 227c0d64e..1a03db0cf 100644 --- a/application/utils/harvester/models.py +++ b/application/utils/harvester/models.py @@ -25,3 +25,9 @@ class FilteringMetrics(BaseModel): total_files: int retained_files: int filtered_files: int + + +@dataclass(slots=True) +class DiffBlock: + file_path: str + added_lines: list[str] From ce7b151a5b9600c7ae6f4751138d0081dc37d5e6 Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Fri, 10 Jul 2026 14:45:58 +0530 Subject: [PATCH 04/10] feat(harvester): normalize extracted diff content --- .../harvester_test/diff_normalizer_test.py | 105 ++++++++++++++++++ .../utils/harvester/diff_normalizer.py | 33 ++++++ 2 files changed, 138 insertions(+) create mode 100644 application/tests/harvester_test/diff_normalizer_test.py create mode 100644 application/utils/harvester/diff_normalizer.py diff --git a/application/tests/harvester_test/diff_normalizer_test.py b/application/tests/harvester_test/diff_normalizer_test.py new file mode 100644 index 000000000..19d3c8ec6 --- /dev/null +++ b/application/tests/harvester_test/diff_normalizer_test.py @@ -0,0 +1,105 @@ +import unittest + +from application.utils.harvester.diff_normalizer import ( + DiffNormalizer, +) + +from application.utils.harvester.models import ( + DiffBlock, +) + + +class DiffNormalizerTests(unittest.TestCase): + def test_whitespace_normalization(self): + normalizer = DiffNormalizer() + + blocks = [ + DiffBlock( + file_path="README.md", + added_lines=[ + " Hello World ", + "\t\tTabs\t\tEverywhere\t", + "", + " ", + "Unicode\u00a0Space", + "Mix\t of\t tabs and spaces", + " Multiple words together ", + "\u00a0\u00a0Leading unicode spaces\u00a0", + " ## Authentication ", + " - Use MFA ", + " `inline code` ", + " **Important** ", + ], + ) + ] + + result = normalizer.normalize(blocks) + + self.assertEqual( + result[0].added_lines, + [ + "Hello World", + "Tabs Everywhere", + "Unicode Space", + "Mix of tabs and spaces", + "Multiple words together", + "Leading unicode spaces", + "## Authentication", + "- Use MFA", + "`inline code`", + "**Important**", + ], + ) + + def test_remove_empty_lines(self): + normalizer = DiffNormalizer() + + blocks = [ + DiffBlock( + file_path="README.md", + added_lines=[ + "", + " ", + "Hello", + ], + ) + ] + + result = normalizer.normalize(blocks) + + self.assertEqual( + result[0].added_lines, + [ + "Hello", + ], + ) + + def test_multiple_blocks(self): + normalizer = DiffNormalizer() + + blocks = [ + DiffBlock( + file_path="a.md", + added_lines=[" One "], + ), + DiffBlock( + file_path="b.md", + added_lines=[" Two "], + ), + ] + + result = normalizer.normalize(blocks) + + self.assertEqual( + result[0].added_lines, + ["One"], + ) + + self.assertEqual( + result[1].added_lines, + ["Two"], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/application/utils/harvester/diff_normalizer.py b/application/utils/harvester/diff_normalizer.py new file mode 100644 index 000000000..babdf3508 --- /dev/null +++ b/application/utils/harvester/diff_normalizer.py @@ -0,0 +1,33 @@ +import textacy.preprocessing as prep + +from .models import DiffBlock + + +class DiffNormalizer: + def normalize_line(self, line: str) -> str: + line = prep.normalize.unicode(line) + line = prep.normalize.whitespace(line) + return line.strip() + + def normalize(self, blocks: list[DiffBlock]) -> list[DiffBlock]: + normalized: list[DiffBlock] = [] + + for block in blocks: + cleaned_lines: list[str] = [] + + for line in block.added_lines: + line = self.normalize_line(line) + + if not line: + continue + + cleaned_lines.append(line) + + normalized.append( + DiffBlock( + file_path=block.file_path, + added_lines=cleaned_lines, + ) + ) + + return normalized From f9b32071c9f4d460b1df88d071c97ac34298521d Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Fri, 10 Jul 2026 17:14:07 +0530 Subject: [PATCH 05/10] Enhance diff pipeline with metadata and normalization --- .../harvester_test/diff_normalizer_test.py | 12 +++++ .../tests/harvester_test/diff_parser_test.py | 52 ++++++++++++------- .../harvester_test/diff_pipeline_test.py | 50 ++++++++++++++++++ .../harvester_test/diff_retriever_test.py | 14 +++++ .../utils/harvester/diff_normalizer.py | 14 +++++ application/utils/harvester/diff_parser.py | 33 ++++++++++-- application/utils/harvester/diff_retriever.py | 39 +++++++++++++- application/utils/harvester/models.py | 8 +++ 8 files changed, 199 insertions(+), 23 deletions(-) create mode 100644 application/tests/harvester_test/diff_pipeline_test.py diff --git a/application/tests/harvester_test/diff_normalizer_test.py b/application/tests/harvester_test/diff_normalizer_test.py index 19d3c8ec6..04eb1ce3f 100644 --- a/application/tests/harvester_test/diff_normalizer_test.py +++ b/application/tests/harvester_test/diff_normalizer_test.py @@ -1,4 +1,5 @@ import unittest +from datetime import datetime from application.utils.harvester.diff_normalizer import ( DiffNormalizer, @@ -9,6 +10,13 @@ ) +DIFF_METADATA = { + "repository": "OWASP/ASVS", + "commit_sha": "abc123", + "committed_at": datetime(2026, 1, 1), +} + + class DiffNormalizerTests(unittest.TestCase): def test_whitespace_normalization(self): normalizer = DiffNormalizer() @@ -30,6 +38,7 @@ def test_whitespace_normalization(self): " `inline code` ", " **Important** ", ], + **DIFF_METADATA, ) ] @@ -62,6 +71,7 @@ def test_remove_empty_lines(self): " ", "Hello", ], + **DIFF_METADATA, ) ] @@ -81,10 +91,12 @@ def test_multiple_blocks(self): DiffBlock( file_path="a.md", added_lines=[" One "], + **DIFF_METADATA, ), DiffBlock( file_path="b.md", added_lines=[" Two "], + **DIFF_METADATA, ), ] diff --git a/application/tests/harvester_test/diff_parser_test.py b/application/tests/harvester_test/diff_parser_test.py index db001df4f..a4444e8d9 100644 --- a/application/tests/harvester_test/diff_parser_test.py +++ b/application/tests/harvester_test/diff_parser_test.py @@ -1,9 +1,14 @@ +from datetime import UTC, datetime import unittest from application.utils.harvester.diff_parser import ( DiffParser, ) +TEST_REPOSITORY = "OWASP/ASVS" +TEST_COMMIT_SHA = "abc123" +TEST_COMMITTED_AT = datetime.now(UTC) + class DiffParserTests(unittest.TestCase): def test_single_file_diff(self): @@ -18,13 +23,15 @@ def test_single_file_diff(self): +another """ - blocks = parser.parse(diff) - - self.assertEqual( - len(blocks), - 1, + blocks = parser.parse( + diff, + repository=TEST_REPOSITORY, + commit_sha=TEST_COMMIT_SHA, + committed_at=TEST_COMMITTED_AT, ) + self.assertEqual(len(blocks), 1) + self.assertEqual( blocks[0].file_path, "test.md", @@ -38,6 +45,10 @@ def test_single_file_diff(self): ], ) + self.assertEqual(blocks[0].repository, TEST_REPOSITORY) + self.assertEqual(blocks[0].commit_sha, TEST_COMMIT_SHA) + self.assertEqual(blocks[0].committed_at, TEST_COMMITTED_AT) + def test_multiple_files(self): parser = DiffParser() @@ -49,22 +60,20 @@ def test_multiple_files(self): +two """ - blocks = parser.parse(diff) - - self.assertEqual( - len(blocks), - 2, + blocks = parser.parse( + diff, + repository=TEST_REPOSITORY, + commit_sha=TEST_COMMIT_SHA, + committed_at=TEST_COMMITTED_AT, ) - self.assertEqual( - blocks[0].file_path, - "a.md", - ) + self.assertEqual(len(blocks), 2) - self.assertEqual( - blocks[1].file_path, - "b.md", - ) + self.assertEqual(blocks[0].file_path, "a.md") + self.assertEqual(blocks[1].file_path, "b.md") + + self.assertEqual(blocks[0].repository, TEST_REPOSITORY) + self.assertEqual(blocks[1].repository, TEST_REPOSITORY) def test_deleted_lines_are_ignored(self): parser = DiffParser() @@ -75,7 +84,12 @@ def test_deleted_lines_are_ignored(self): +new """ - blocks = parser.parse(diff) + blocks = parser.parse( + diff, + repository=TEST_REPOSITORY, + commit_sha=TEST_COMMIT_SHA, + committed_at=TEST_COMMITTED_AT, + ) self.assertEqual( blocks[0].added_lines, diff --git a/application/tests/harvester_test/diff_pipeline_test.py b/application/tests/harvester_test/diff_pipeline_test.py new file mode 100644 index 000000000..f27624170 --- /dev/null +++ b/application/tests/harvester_test/diff_pipeline_test.py @@ -0,0 +1,50 @@ +from datetime import UTC, datetime +import time +import unittest + +from application.utils.harvester.diff_normalizer import DiffNormalizer +from application.utils.harvester.diff_parser import DiffParser +from application.utils.harvester.diff_retriever import DiffRetriever +from application.utils.harvester.git_repository_client import GitRepositoryClient + + +class DiffPipelineBenchmark(unittest.TestCase): + """ + Simple benchmark to ensure the complete diff pipeline remains fast. + + This is not intended as a strict performance benchmark, only as a + regression guard against accidental slowdowns. + """ + + def test_pipeline_benchmark(self): + client = GitRepositoryClient( + "OWASP", + "ASVS", + "master", + ) + + retriever = DiffRetriever(client) + parser = DiffParser() + normalizer = DiffNormalizer() + + start = time.perf_counter() + + diff = retriever.get_diff( + "a79c0184", + "122d9e0969465a6041e16c806a0464b35deea444", + ) + + blocks = parser.parse( + diff, + repository="OWASP/ASVS", + commit_sha="122d9e0969465a6041e16c806a0464b35deea444", + committed_at=datetime.now(UTC), + ) + + normalizer.normalize(blocks) + + elapsed = time.perf_counter() - start + + print(f"\nPipeline took {elapsed:.3f}s") + + self.assertLess(elapsed, 5) diff --git a/application/tests/harvester_test/diff_retriever_test.py b/application/tests/harvester_test/diff_retriever_test.py index e716f2452..0e890a07f 100644 --- a/application/tests/harvester_test/diff_retriever_test.py +++ b/application/tests/harvester_test/diff_retriever_test.py @@ -44,6 +44,20 @@ def test_get_diff(self, mock_run): 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), + ) + + client = MagicMock() + client.get_local_path.return_value = "/tmp/repo" + + retriever = DiffRetriever(client) + + with self.assertRaises(ValueError): + retriever.get_diff("a", "b") + if __name__ == "__main__": unittest.main() diff --git a/application/utils/harvester/diff_normalizer.py b/application/utils/harvester/diff_normalizer.py index babdf3508..fe8773349 100644 --- a/application/utils/harvester/diff_normalizer.py +++ b/application/utils/harvester/diff_normalizer.py @@ -1,15 +1,26 @@ import textacy.preprocessing as prep +from application.utils.harvester import repository_client from .models import DiffBlock class DiffNormalizer: + """ + Normalizes extracted diff content. + + Whitespace is collapsed, Unicode normalized, + and empty lines removed. + """ + def normalize_line(self, line: str) -> str: line = prep.normalize.unicode(line) line = prep.normalize.whitespace(line) return line.strip() def normalize(self, blocks: list[DiffBlock]) -> list[DiffBlock]: + """ + Normalize every added line in each DiffBlock. + """ normalized: list[DiffBlock] = [] for block in blocks: @@ -27,6 +38,9 @@ def normalize(self, blocks: list[DiffBlock]) -> list[DiffBlock]: DiffBlock( file_path=block.file_path, added_lines=cleaned_lines, + repository=block.repository, + commit_sha=block.commit_sha, + committed_at=block.committed_at, ) ) diff --git a/application/utils/harvester/diff_parser.py b/application/utils/harvester/diff_parser.py index d9c8e9d32..8bdaeb42e 100644 --- a/application/utils/harvester/diff_parser.py +++ b/application/utils/harvester/diff_parser.py @@ -1,10 +1,23 @@ +from datetime import datetime import re from .models import DiffBlock class DiffParser: - def parse(self, diff: str) -> list[DiffBlock]: + """ + Parses unified git diffs into DiffBlock objects. + + Only added lines are extracted. + Deleted lines and diff metadata are ignored. + """ + + def parse( + self, diff: str, repository: str, commit_sha: str, committed_at: datetime + ) -> list[DiffBlock]: + """ + Convert a unified git diff into DiffBlock objects. + """ blocks: list[DiffBlock] = [] current_file: str | None = None @@ -14,7 +27,13 @@ def parse(self, diff: str) -> list[DiffBlock]: if line.startswith("diff --git"): if current_file is not None: blocks.append( - DiffBlock(file_path=current_file, added_lines=added_lines) + DiffBlock( + file_path=current_file, + added_lines=added_lines, + repository=repository, + commit_sha=commit_sha, + committed_at=committed_at, + ) ) match = re.match(r"diff --git a/(.+?) b/", line) @@ -38,6 +57,14 @@ def parse(self, diff: str) -> list[DiffBlock]: added_lines.append(line[1:]) if current_file is not None: - blocks.append(DiffBlock(file_path=current_file, added_lines=added_lines)) + blocks.append( + DiffBlock( + file_path=current_file, + added_lines=added_lines, + repository=repository, + commit_sha=commit_sha, + committed_at=committed_at, + ) + ) return blocks diff --git a/application/utils/harvester/diff_retriever.py b/application/utils/harvester/diff_retriever.py index 6067c9216..78c03bb13 100644 --- a/application/utils/harvester/diff_retriever.py +++ b/application/utils/harvester/diff_retriever.py @@ -7,10 +7,37 @@ class DiffRetriever: + MAX_DIFF_SIZE_BYTES = 50 * 1024 * 1024 + """ + + Retrieves unified git diffs between two commits. + + This class is responsible only for retrieving raw diff text. + + Parsing and normalization are handled by downstream components. + + """ + def __init__(self, repository_client: GitRepositoryClient) -> None: self.repository_client = repository_client def get_diff(self, base_commit: str, target_commit: str = "HEAD") -> str: + """ + Return the unified git diff between two commits. + + Args: + base_commit: + Base commit SHA. + target_commit: + Target commit SHA or branch. + + Raises: + subprocess.CalledProcessError: + If git diff fails. + + ValueError: + If the diff exceeds the configured size limit. + """ logger.info( "Retrieving diff between %s and %s", base_commit, @@ -36,4 +63,14 @@ def get_diff(self, base_commit: str, target_commit: str = "HEAD") -> str: logger.error("Failed to retrieve diff: %s", exc.stderr) raise - return result.stdout + diff = result.stdout + + diff_size = len(diff.encode("utf-8")) + + if diff_size > self.MAX_DIFF_SIZE_BYTES: + raise ValueError( + f"Diff size ({diff_size} bytes) exceeds " + f"maximum supported size ({self.MAX_DIFF_SIZE_BYTES} bytes)." + ) + + return diff diff --git a/application/utils/harvester/models.py b/application/utils/harvester/models.py index 1a03db0cf..0eca718c9 100644 --- a/application/utils/harvester/models.py +++ b/application/utils/harvester/models.py @@ -29,5 +29,13 @@ class FilteringMetrics(BaseModel): @dataclass(slots=True) class DiffBlock: + """ + Intermediate representation of normalized additions + extracted from a repository diff. + """ + file_path: str added_lines: list[str] + repository: str + commit_sha: str + committed_at: datetime | None = None From 8878178fd651a33540d096186290324073b4562d Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Sat, 18 Jul 2026 23:51:05 +0530 Subject: [PATCH 06/10] fix(harvester): address review feedback --- .../harvester_test/diff_pipeline_test.py | 24 ++++++++++++++++--- .../harvester_test/diff_retriever_test.py | 5 ++-- .../git_repository_client_test.py | 16 +------------ .../utils/harvester/checkpoint_store.py | 3 +++ application/utils/harvester/diff_parser.py | 6 ++--- application/utils/harvester/diff_retriever.py | 12 ++++++---- 6 files changed, 37 insertions(+), 29 deletions(-) diff --git a/application/tests/harvester_test/diff_pipeline_test.py b/application/tests/harvester_test/diff_pipeline_test.py index f27624170..5ce0a6fc2 100644 --- a/application/tests/harvester_test/diff_pipeline_test.py +++ b/application/tests/harvester_test/diff_pipeline_test.py @@ -1,4 +1,5 @@ from datetime import UTC, datetime +import subprocess import time import unittest @@ -22,6 +23,23 @@ def test_pipeline_benchmark(self): "ASVS", "master", ) + client.sync() + + head_commit = client.get_current_commit_sha() + + previous_commit = subprocess.run( + [ + "git", + "-C", + str(client.get_local_path()), + "rev-parse", + "HEAD~1", + ], + check=True, + capture_output=True, + text=True, + timeout=300, + ).stdout.strip() retriever = DiffRetriever(client) parser = DiffParser() @@ -30,14 +48,14 @@ def test_pipeline_benchmark(self): start = time.perf_counter() diff = retriever.get_diff( - "a79c0184", - "122d9e0969465a6041e16c806a0464b35deea444", + previous_commit, + head_commit, ) blocks = parser.parse( diff, repository="OWASP/ASVS", - commit_sha="122d9e0969465a6041e16c806a0464b35deea444", + commit_sha=head_commit, committed_at=datetime.now(UTC), ) diff --git a/application/tests/harvester_test/diff_retriever_test.py b/application/tests/harvester_test/diff_retriever_test.py index 0e890a07f..416499e5f 100644 --- a/application/tests/harvester_test/diff_retriever_test.py +++ b/application/tests/harvester_test/diff_retriever_test.py @@ -11,7 +11,7 @@ class DiffRetrieverTests(unittest.TestCase): @patch("application.utils.harvester.diff_retriever.subprocess.run") 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() @@ -39,7 +39,6 @@ def test_get_diff(self, mock_run): "def456", ], capture_output=True, - text=True, check=True, timeout=300, ) @@ -47,7 +46,7 @@ def test_get_diff(self, mock_run): @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), ) client = MagicMock() diff --git a/application/tests/harvester_test/git_repository_client_test.py b/application/tests/harvester_test/git_repository_client_test.py index 1121c973f..8bbff6ab8 100644 --- a/application/tests/harvester_test/git_repository_client_test.py +++ b/application/tests/harvester_test/git_repository_client_test.py @@ -89,21 +89,6 @@ def test_sync_fetches_when_repository_exists(self, mock_run): mock_fetch.assert_called_once() - mock_run.assert_called_once_with( - [ - "git", - "-C", - str(client.get_local_path()), - "reset", - "--hard", - "origin/main", - ], - check=True, - capture_output=True, - text=True, - timeout=300, - ) - @patch("application.utils.harvester.git_repository_client.subprocess.run") def test_fetch_runs_git_command(self, mock_run): client = GitRepositoryClient( @@ -130,6 +115,7 @@ def test_checkout_runs_git_command(self, mock_run): "-C", str(client.get_local_path()), "checkout", + "--", "main", ], check=True, diff --git a/application/utils/harvester/checkpoint_store.py b/application/utils/harvester/checkpoint_store.py index e9e3b62f4..ed757bb56 100644 --- a/application/utils/harvester/checkpoint_store.py +++ b/application/utils/harvester/checkpoint_store.py @@ -22,8 +22,10 @@ def load(self, repository_id: str) -> RepositoryCheckpoint | None: .filter_by(repository_id=repository_id) .first() ) + if record is None: return None + return RepositoryCheckpoint( repository_id=record.repository_id, last_processed_commit=record.last_processed_commit, @@ -53,6 +55,7 @@ def save(self, checkpoint: RepositoryCheckpoint) -> None: ) .first() ) + if canonical_conflict is not None: session.rollback() raise ValueError("duplicate canonical source identity") diff --git a/application/utils/harvester/diff_parser.py b/application/utils/harvester/diff_parser.py index 8bdaeb42e..d0f124bf9 100644 --- a/application/utils/harvester/diff_parser.py +++ b/application/utils/harvester/diff_parser.py @@ -44,16 +44,16 @@ def parse( continue - 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:]) if current_file is not None: diff --git a/application/utils/harvester/diff_retriever.py b/application/utils/harvester/diff_retriever.py index 78c03bb13..fce640d52 100644 --- a/application/utils/harvester/diff_retriever.py +++ b/application/utils/harvester/diff_retriever.py @@ -56,16 +56,18 @@ def get_diff(self, base_commit: str, target_commit: str = "HEAD") -> str: ], check=True, capture_output=True, - text=True, timeout=300, ) except subprocess.CalledProcessError as exc: - logger.error("Failed to retrieve diff: %s", exc.stderr) + logger.error( + "Failed to retrieve diff: %s", + exc.stderr.decode("utf-8", errors="replace"), + ) raise - diff = result.stdout + diff_bytes = result.stdout - diff_size = len(diff.encode("utf-8")) + diff_size = len(diff_bytes) if diff_size > self.MAX_DIFF_SIZE_BYTES: raise ValueError( @@ -73,4 +75,4 @@ def get_diff(self, base_commit: str, target_commit: str = "HEAD") -> str: f"maximum supported size ({self.MAX_DIFF_SIZE_BYTES} bytes)." ) - return diff + return diff_bytes.decode("utf-8", errors="replace") From 1c8ad1fc20122a2f0a7d04b8b78b4e1fa370b4d5 Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Thu, 30 Jul 2026 18:10:49 +0530 Subject: [PATCH 07/10] Harden diff retrieval and isolate network benchmark tests --- .../harvester_test/diff_pipeline_test.py | 5 ++ .../harvester_test/diff_retriever_test.py | 65 +++++++++++++++---- application/utils/harvester/diff_parser.py | 2 +- application/utils/harvester/diff_retriever.py | 21 ++++++ requirements-dev.txt | 1 + 5 files changed, 79 insertions(+), 15 deletions(-) diff --git a/application/tests/harvester_test/diff_pipeline_test.py b/application/tests/harvester_test/diff_pipeline_test.py index 5ce0a6fc2..07160f170 100644 --- a/application/tests/harvester_test/diff_pipeline_test.py +++ b/application/tests/harvester_test/diff_pipeline_test.py @@ -2,6 +2,7 @@ import subprocess import time import unittest +import os from application.utils.harvester.diff_normalizer import DiffNormalizer from application.utils.harvester.diff_parser import DiffParser @@ -18,6 +19,10 @@ class DiffPipelineBenchmark(unittest.TestCase): """ def test_pipeline_benchmark(self): + + if os.getenv("OPENCRE_RUN_NETWORK_TESTS") != "1": + self.skipTest("Network benchmark disabled") + client = GitRepositoryClient( "OWASP", "ASVS", diff --git a/application/tests/harvester_test/diff_retriever_test.py b/application/tests/harvester_test/diff_retriever_test.py index 416499e5f..502ac2d39 100644 --- a/application/tests/harvester_test/diff_retriever_test.py +++ b/application/tests/harvester_test/diff_retriever_test.py @@ -1,6 +1,7 @@ import unittest from unittest.mock import MagicMock from unittest.mock import patch +from unittest.mock import call from application.utils.harvester.diff_retriever import ( DiffRetriever, @@ -10,9 +11,11 @@ class DiffRetrieverTests(unittest.TestCase): @patch("application.utils.harvester.diff_retriever.subprocess.run") def test_get_diff(self, mock_run): - mock_run.return_value = MagicMock( - stdout=b"diff --git a/README.md b/README.md\n", - ) + mock_run.side_effect = [ + MagicMock(stdout="abc123\n"), + MagicMock(stdout="def456\n"), + MagicMock(stdout=b"diff --git a/README.md b/README.md\n"), + ] client = MagicMock() client.get_local_path.return_value = "/tmp/repo" @@ -29,18 +32,52 @@ def test_get_diff(self, mock_run): "diff --git a/README.md b/README.md\n", ) - mock_run.assert_called_once_with( + mock_run.assert_has_calls( [ - "git", - "-C", - "/tmp/repo", - "diff", - "abc123", - "def456", - ], - capture_output=True, - check=True, - timeout=300, + call( + [ + "git", + "-C", + "/tmp/repo", + "rev-parse", + "--verify", + "--end-of-options", + "abc123^{commit}", + ], + check=True, + capture_output=True, + text=True, + timeout=60, + ), + call( + [ + "git", + "-C", + "/tmp/repo", + "rev-parse", + "--verify", + "--end-of-options", + "def456^{commit}", + ], + check=True, + capture_output=True, + text=True, + timeout=60, + ), + call( + [ + "git", + "-C", + "/tmp/repo", + "diff", + "abc123", + "def456", + ], + check=True, + capture_output=True, + timeout=300, + ), + ] ) @patch("application.utils.harvester.diff_retriever.subprocess.run") diff --git a/application/utils/harvester/diff_parser.py b/application/utils/harvester/diff_parser.py index d0f124bf9..e8d98b9d3 100644 --- a/application/utils/harvester/diff_parser.py +++ b/application/utils/harvester/diff_parser.py @@ -44,7 +44,7 @@ def parse( continue - if line.startswith("+++ b/") or line.startswith("++/dev/null"): + if line.startswith("+++ b/") or line.startswith("+++ /dev/null"): continue if line.startswith("--- a/") or line.startswith("--- /dev/null"): diff --git a/application/utils/harvester/diff_retriever.py b/application/utils/harvester/diff_retriever.py index fce640d52..f40f1cbdf 100644 --- a/application/utils/harvester/diff_retriever.py +++ b/application/utils/harvester/diff_retriever.py @@ -44,6 +44,9 @@ def get_diff(self, base_commit: str, target_commit: str = "HEAD") -> str: target_commit, ) + base_commit = self._resolve_commit(base_commit) + target_commit = self._resolve_commit(target_commit) + try: result = subprocess.run( [ @@ -76,3 +79,21 @@ def get_diff(self, base_commit: str, target_commit: str = "HEAD") -> str: ) return diff_bytes.decode("utf-8", errors="replace") + + 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() diff --git a/requirements-dev.txt b/requirements-dev.txt index 4b8784bdc..283942164 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -78,6 +78,7 @@ types-PyYAML typing-inspect pycodestyle pyflakes +textacy # lint / test / typecheck black==24.4.2 From 04a49df10dc56eb12ca8752b2ca1a00d881bf07a Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Thu, 30 Jul 2026 19:01:24 +0530 Subject: [PATCH 08/10] Addressing code rabbit comment --- application/utils/harvester/diff_parser.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/application/utils/harvester/diff_parser.py b/application/utils/harvester/diff_parser.py index e8d98b9d3..12b14784c 100644 --- a/application/utils/harvester/diff_parser.py +++ b/application/utils/harvester/diff_parser.py @@ -38,9 +38,8 @@ def parse( match = re.match(r"diff --git a/(.+?) b/", line) - if match: - current_file = match.group(1) - added_lines = [] + current_file = match.group(1) if match else None + added_lines = [] continue From 67ea153daafc5ada8cb3b8af6367c3723587844c Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Thu, 6 Aug 2026 19:17:21 +0530 Subject: [PATCH 09/10] fix(harvester): use git checkout argument separator --- application/utils/harvester/git_repository_client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/application/utils/harvester/git_repository_client.py b/application/utils/harvester/git_repository_client.py index bed925d85..468be2665 100644 --- a/application/utils/harvester/git_repository_client.py +++ b/application/utils/harvester/git_repository_client.py @@ -162,6 +162,7 @@ def checkout(self, reference: str) -> None: "-C", str(self.local_path), "checkout", + "--", reference, ], check=True, From 6857ac0923d7adc707d559a65e907e98b237088e Mon Sep 17 00:00:00 2001 From: ParthAggarwal16 Date: Mon, 10 Aug 2026 12:42:36 +0530 Subject: [PATCH 10/10] fix(harvester): address review feedback --- application/utils/harvester/diff_normalizer.py | 8 ++++---- application/utils/harvester/diff_retriever.py | 3 ++- requirements-dev.txt | 1 - 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/application/utils/harvester/diff_normalizer.py b/application/utils/harvester/diff_normalizer.py index fe8773349..756d89147 100644 --- a/application/utils/harvester/diff_normalizer.py +++ b/application/utils/harvester/diff_normalizer.py @@ -1,6 +1,6 @@ -import textacy.preprocessing as prep +import re +import unicodedata -from application.utils.harvester import repository_client from .models import DiffBlock @@ -13,8 +13,8 @@ class DiffNormalizer: """ def normalize_line(self, line: str) -> str: - line = prep.normalize.unicode(line) - line = prep.normalize.whitespace(line) + line = unicodedata.normalize("NFKC", line) + line = re.sub(r"\s+", " ", line) return line.strip() def normalize(self, blocks: list[DiffBlock]) -> list[DiffBlock]: diff --git a/application/utils/harvester/diff_retriever.py b/application/utils/harvester/diff_retriever.py index f40f1cbdf..7efd45560 100644 --- a/application/utils/harvester/diff_retriever.py +++ b/application/utils/harvester/diff_retriever.py @@ -7,7 +7,6 @@ class DiffRetriever: - MAX_DIFF_SIZE_BYTES = 50 * 1024 * 1024 """ Retrieves unified git diffs between two commits. @@ -18,6 +17,8 @@ class DiffRetriever: """ + MAX_DIFF_SIZE_BYTES = 50 * 1024 * 1024 + def __init__(self, repository_client: GitRepositoryClient) -> None: self.repository_client = repository_client diff --git a/requirements-dev.txt b/requirements-dev.txt index 283942164..4b8784bdc 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -78,7 +78,6 @@ types-PyYAML typing-inspect pycodestyle pyflakes -textacy # lint / test / typecheck black==24.4.2