GSoC Module A : Week 3: feat(harvester): implement incremental change detection (stacked on #983)#985
GSoC Module A : Week 3: feat(harvester): implement incremental change detection (stacked on #983)#985ParthAggarwal16 wants to merge 17 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:
WalkthroughAdds harvester configuration schemas and validation, Git repository caching and synchronization, incremental change detection, checkpoint persistence, repository defaults, exclusion patterns, and artifact ingest persistence with unit and integration tests. ChangesHarvester foundation
Artifact ingest persistence
Estimated code review effort: 4 (Complex) | ~75 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: 1
🧹 Nitpick comments (5)
application/utils/harvester/__init__.py (1)
21-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__list.The static analysis tool Ruff flags that the
__all__list is not sorted. Using a standard ASCII sort (uppercase before lowercase) resolves this warning and follows standard Python conventions.♻️ Proposed fix
__all__ = [ - "build_repository_cache_path", "ChunkingConfig", "ConfigLoaderError", "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 21 - 34, Sort the __all__ entries in application/utils/harvester/__init__.py using standard ASCII ordering, placing uppercase names before lowercase names, without changing the exported symbols.Source: Linters/SAST tools
application/tests/harvester_test/git_repository_client_test.py (2)
32-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock the filesystem to prevent flaky tests.
This test asserts that the local repository path does not exist. However, if a developer runs this test locally after having previously run a harvest or sync, the
.harvester_cachedirectory might actually exist on their machine, causing the test to fail.Consider mocking
Path.existsto fully isolate the test from the local filesystem state.♻️ Proposed refactor
- def test_repository_exists_locally_false(self): + `@patch`("pathlib.Path.exists") + def test_repository_exists_locally_false(self, mock_exists): + mock_exists.return_value = False client = GitRepositoryClient( owner="OWASP", repository="ASVS", ) self.assertFalse(client.exists_locally())🤖 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 32 - 38, Update test_repository_exists_locally_false to mock pathlib.Path.exists so it deterministically reports that the repository path is absent, then assert GitRepositoryClient.exists_locally() returns False without consulting the developer’s filesystem.
27-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
.as_posix()to prevent cross-platform test failures.Comparing stringified
Pathobjects to a UNIX-style string path (.harvester_cache/owasp/asvs/main) can cause test failures on Windows, wherestr(Path)uses backslashes (\). Using.as_posix()ensures the output uses forward slashes on all platforms.♻️ Proposed refactor
- self.assertEqual( - str(client.get_local_path()), - ".harvester_cache/owasp/asvs/main", - ) + self.assertEqual( + client.get_local_path().as_posix(), + ".harvester_cache/owasp/asvs/main", + )🤖 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 27 - 30, Update the path assertion for client.get_local_path() to call .as_posix() before comparing with the expected UNIX-style path, preserving the existing expected value and test behavior across platforms.application/utils/harvester/change_detector.py (2)
47-60: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider returning commits in chronological order.
By default,
git logoutputs commits in reverse chronological order (newest first). If the orchestrator uses this list to process commits sequentially or incrementally replay history, it typically requires chronological order (oldest first).You can pass the
--reverseflag togit logto achieve this.💡 Proposed change
result = subprocess.run( [ "git", "-C", str(self.repository_client.get_local_path()), "log", + "--reverse", "--format=%H", f"{commit_sha}..HEAD", ], capture_output=True, text=True, check=True, timeout=60, )🤖 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 47 - 60, Update the git log invocation in the change-detection method to include the --reverse option, ensuring commits between commit_sha and HEAD are returned in chronological order while preserving the existing output and filtering behavior.
20-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
-zfor robust file path parsing.Git may escape or quote filenames containing special characters or newlines by default (e.g.,
"path/to/file with spaces.md"). Passing-ztogit diffproduces null-terminated output, which guarantees accurate parsing of all file names without interference from Git's quotation rules.♻️ Proposed refactor
result = subprocess.run( [ "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, )If you apply this, update the parsing logic on line 40 to split by
\0:files = [ file_path for file_path in result.stdout.split("\0") if file_path.strip() ]🤖 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 20 - 34, Update the git diff invocation in the change-detection flow to include the null-delimited output option, then update the parsing logic that consumes result.stdout to split on "\0" and ignore empty entries. Preserve accurate filenames, including spaces, quotes, and newlines, through the existing file-processing path.
🤖 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/checkpoint_store.py`:
- Around line 35-54: Update the checkpoint_store save method to serialize the
modified data into a temporary file in the checkpoint file’s directory, then
atomically replace the existing checkpoint file with it. Preserve the current
JSON content and encoding, and ensure temporary-file cleanup on write or
replacement failure.
---
Nitpick comments:
In `@application/tests/harvester_test/git_repository_client_test.py`:
- Around line 32-38: Update test_repository_exists_locally_false to mock
pathlib.Path.exists so it deterministically reports that the repository path is
absent, then assert GitRepositoryClient.exists_locally() returns False without
consulting the developer’s filesystem.
- Around line 27-30: Update the path assertion for client.get_local_path() to
call .as_posix() before comparing with the expected UNIX-style path, preserving
the existing expected value and test behavior across platforms.
In `@application/utils/harvester/__init__.py`:
- Around line 21-34: Sort the __all__ entries in
application/utils/harvester/__init__.py using standard ASCII ordering, placing
uppercase names before lowercase names, without changing the exported symbols.
In `@application/utils/harvester/change_detector.py`:
- Around line 47-60: Update the git log invocation in the change-detection
method to include the --reverse option, ensuring commits between commit_sha and
HEAD are returned in chronological order while preserving the existing output
and filtering behavior.
- Around line 20-34: Update the git diff invocation in the change-detection flow
to include the null-delimited output option, then update the parsing logic that
consumes result.stdout to split on "\0" and ignore empty entries. Preserve
accurate filenames, including spaces, quotes, and newlines, through the existing
file-processing path.
🪄 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: 45a49c90-33e4-4f20-9cac-25cd04822648
📒 Files selected for processing (31)
application/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/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/exclude_patterns.txtapplication/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
northdpole
left a comment
northdpole
left a comment
There was a problem hiding this comment.
Week 3 review — required before approval
This layer establishes durable incremental state, so checkpoint persistence should use Postgres now rather than introducing a JSON runtime contract that must be replaced later. Full multi-worker concurrency can be deferred, provided this version explicitly enforces a single harvester worker with no overlapping runs.
Required in this PR:
- replace JSON checkpoint persistence with the minimal Postgres model/migration described inline;
- enforce the single-worker/no-overlap execution constraint;
- replay commits oldest-first;
- use validated immutable base/target SHAs instead of mutable
HEAD; - add Postgres checkpoint tests and a deterministic temporary-Git integration test.
Please open separate follow-up tickets for multi-worker lease/CAS fencing, deletion/rename semantics, NUL-safe filenames, force-push/history-rewrite recovery, and broader failure/concurrency coverage. Every follow-up ticket should carry both the Module B and V1 labels so these are tracked as required next steps.
Verdict: changes are required before approval, but full concurrency support is not required in this PR.
| from .models import RepositoryCheckpoint | ||
|
|
||
|
|
||
| class CheckpointStore: |
There was a problem hiding this comment.
Blocker: replace the JSON runtime store with Postgres in this PR. Add a harvester_checkpoint SQLAlchemy model and an Alembic migration based on the current migration head after rebasing. The minimum schema should contain: immutable repository_id as the primary key, canonical source identity (provider/owner/repository/exact branch, with a unique canonical source key), nullable last_processed_commit, and timezone-aware created_at/updated_at. Replace this Path-based API with database load(repository_id) and transactional upsert behavior using the application SQLAlchemy session. Do not fall back to JSON on a database error; fail closed and leave the checkpoint unchanged. Full lease/CAS columns may be added in the labelled V1 follow-up, but this PR must explicitly enforce single-worker/no-overlap execution until then.
| "diff", | ||
| "--name-only", | ||
| commit_sha, | ||
| "HEAD", |
There was a problem hiding this comment.
Do not diff against mutable HEAD. Change the API to receive both a validated base SHA from the checkpoint and an immutable target SHA captured after synchronization, then invoke git diff <base> <target>. Validate each value as a full commit object before use (for example with rev-parse --verify --end-of-options <sha>^{commit}) and pass only the resolved SHA to subsequent Git commands. Add a temporary local-repository test where the branch advances after target capture and prove the detected range remains stable. Force-push/history-rewrite reconciliation can be handled in a Module B + V1 follow-up ticket, but this API should not bake in HEAD.
| str(self.repository_client.get_local_path()), | ||
| "log", | ||
| "--format=%H", | ||
| f"{commit_sha}..HEAD", |
There was a problem hiding this comment.
Git log is newest-first by default, which is unsafe for sequential replay and checkpoint progression. Add --reverse and use the explicit validated <base>..<target> range rather than <base>..HEAD, making the method contract oldest-first. Update the unit test to assert the complete Git argument list, and add a deterministic temporary-Git test with at least three commits that verifies the returned order.
|
|
||
| try: | ||
| store = CheckpointStore( | ||
| tmp_dir / "checkpoints.json", |
There was a problem hiding this comment.
These filesystem/JSON tests should be replaced with Postgres-backed persistence tests using the repository's application/SQLAlchemy test setup. Cover: create/load, update/upsert, two repository rows remaining isolated, duplicate canonical source identity rejection, immutable repository identity, null initial checkpoint, transaction rollback leaving the previous checkpoint intact, and database failure with no JSON fallback. Also add a migration check that upgrades a clean Postgres database and verifies the table, primary key, unique identity constraint, and timestamps. Multi-worker contention tests belong in the follow-up ticket labelled Module B and V1.
ccb515a to
6ff8c4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/utils/harvester/git_repository_client.py (1)
37-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLock
clone()andfetch()instead of relying onsync().
clone()andfetch()are publicRepositoryClientmethods, but onlysync()acquiresrepository_lock(self.local_path). A directclone()caller can race with anotherclone()in_clone_atomicallybetweenself.local_path.exists()andos.replace(), and aclone()can race with a laterfetch()called outsidesync(). Move the locking intoclone()/fetch()so every entry point is safe.🤖 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 37 - 51, Update the public RepositoryClient methods clone() and fetch() to acquire repository_lock(self.local_path) around their complete repository operations, including _clone_atomically and fetch logic. Remove reliance on sync() as the only locking boundary and avoid nested or duplicate locking there, while preserving existing behavior for all entry points.
🤖 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/change_detector.py`:
- Around line 27-28: Resolve and validate commit_sha before using it in Git
operations, using strict rev-parse verification with --end-of-options and the
^{commit} constraint, then replace it with the canonical object ID. Ensure
get_modified_files_since and the commit_sha..HEAD log range use only this
resolved value, and add a regression test confirming option-shaped input is
rejected before Git executes.
- Around line 25-26: Update the Git file-list command in the change detector to
include the NUL-delimited output option alongside “--name-only”. Parse the
command output by splitting on "\0" rather than using splitlines() or strip(),
while preserving valid filenames and filtering only the final empty delimiter
entry.
In `@application/utils/harvester/git_repository_client.py`:
- Around line 15-32: Update GitRepositoryClient.__init__ to reject branch values
beginning with "-" before assigning self.branch or deriving the cache path,
raising the same validation-style error used for invalid repository inputs.
Preserve normal branch handling and existing fetch/checkout behavior for valid
references.
In `@application/utils/harvester/repository_cache.py`:
- Around line 21-24: Validate the branch argument before the path construction
in the cache-path logic, rejecting exactly "." and ".." before calling quote or
creating candidate. Preserve existing validation for other branch names, and add
regression coverage confirming both values are rejected.
In `@application/utils/harvester/repository_lock.py`:
- Around line 11-19: Update repository_lock so lock_path appends the “.lock”
suffix to the complete repository_path name instead of replacing its final
suffix; preserve the existing parent-directory creation and locking flow while
ensuring dotted repository paths such as release-1.2.3 and release-1.2.4 produce
distinct lock files.
---
Outside diff comments:
In `@application/utils/harvester/git_repository_client.py`:
- Around line 37-51: Update the public RepositoryClient methods clone() and
fetch() to acquire repository_lock(self.local_path) around their complete
repository operations, including _clone_atomically and fetch logic. Remove
reliance on sync() as the only locking boundary and avoid nested or duplicate
locking there, while preserving existing behavior for all entry points.
🪄 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: 14a00380-614e-4b9f-ba2c-a4af79b9fec3
📒 Files selected for processing (21)
application/tests/harvester_test/change_detector_test.pyapplication/tests/harvester_test/checkpoint_store_test.pyapplication/tests/harvester_test/config_loader_test.pyapplication/tests/harvester_test/fixtures/duplicate_repo_ids_case.yamlapplication/tests/harvester_test/fixtures/duplicate_repositories_case.yamlapplication/tests/harvester_test/fixtures/invalid_overlap_tokens.yamlapplication/tests/harvester_test/fixtures/whitespace_branch.yamlapplication/tests/harvester_test/fixtures/whitespace_id.yamlapplication/tests/harvester_test/fixtures/whitespace_owner.yamlapplication/tests/harvester_test/fixtures/whitespace_repo.yamlapplication/tests/harvester_test/git_repository_client_integration_test.pyapplication/tests/harvester_test/git_repository_client_test.pyapplication/tests/harvester_test/repos_validator_test.pyapplication/tests/harvester_test/repository_cache_test.pyapplication/utils/harvester/change_detector.pyapplication/utils/harvester/checkpoint_store.pyapplication/utils/harvester/git_repository_client.pyapplication/utils/harvester/models.pyapplication/utils/harvester/repository_cache.pyapplication/utils/harvester/repository_lock.pyapplication/utils/harvester/schemas.py
🚧 Files skipped from review as they are similar to previous changes (6)
- application/tests/harvester_test/repos_validator_test.py
- application/tests/harvester_test/change_detector_test.py
- application/utils/harvester/models.py
- application/tests/harvester_test/checkpoint_store_test.py
- application/utils/harvester/schemas.py
- application/tests/harvester_test/git_repository_client_test.py
| "diff", | ||
| "--name-only", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)change_detector\.py$' || true
echo "== current references =="
if [ -f application/utils/harvester/change_detector.py ]; then
wc -l application/utils/harvester/change_detector.py
sed -n '1,120p' application/utils/harvester/change_detector.py | cat -n
else
echo "file not found"
fi
echo "== git diff name-only behavior probe =="
tmp="$(mktemp -d)"
cd "$tmp"
git init -q
git config user.email test@example.com
git config user.name Tester
printf 'normal\n' > normal.txt
printf '\nfile\n' > $'newline_in_name.txt'
mkdir -p $'space dir'
printf 'a\n' > $'space dir'/file.txt || true
git add -A
git commit -qm init -q
git checkout -qm test -q --orphan || true
git rm -r fq . && git clean -dfq || true
printf 'changed\n' > normal.txt
printf 'changed\n' > $'newline_in_name.txt'
printf 'changed\n' > $'space dir'/file.txt || true
git add -A
echo "== --name-only =="
git diff --name-only test..HEAD | cat -A
echo "== --name-only -z =="
git diff --name-only -z test..HEAD | sed -n '1p' | cat -A
cd /
rm -rf "$tmp"Repository: OWASP/OpenCRE
Length of output: 3201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git diff name-only newline filename probe =="
tmp="$(mktemp -d)"
cd "$tmp"
git init -q
git config user.email test@example.com
git config user.name Tester
printf 'normal\n' > normal.txt
printf 'content\n' > $'file\nwith\nnewlines\n'
git add -A
git commit -qm init -q --no-gpg-sign
printf 'changed\n' > normal.txt
printf 'content\n' > $'file\nwith\nnewlines\n'
git add -A
echo "name-only default:"
git diff --name-only HEAD^..HEAD 2>/tmp/diff_err | cat -A || { cat /tmp/diff_err; exit 128; }
echo "name-only with -z:"
git diff --name-only -z HEAD^..HEAD | od -An -tx1z -v
echo "line count via splitlines():"
git diff --name-only HEAD^..HEAD | python3 - <<'PY'
import sys
samples = sys.stdin.read().splitlines()
print("samples", samples)
print("count", len(samples))
PY
rm -rf "$tmp"Repository: OWASP/OpenCRE
Length of output: 414
🌐 Web query:
Git diff --name-only -z NUL delimited filenames newlines quoted characters man page
💡 Result:
When using git diff to list filenames, the default behavior and the NUL-delimited behavior handle special characters differently [1]. Default behavior (no -z): By default, Git identifies pathnames containing "unusual" characters (such as tabs, newlines, double quotes, or backslashes) by quoting them, as explained for the core.quotePath configuration variable [1]. This quoting mechanism wraps the pathname in double quotes and escapes special characters (e.g., \t, \n, ", \) [2]. However, pathnames containing only spaces are typically not quoted by default, which can sometimes lead to parsing challenges [2][3]. NUL-delimited behavior (-z): When the --name-only (or --raw, --numstat, --name-status) option is used in combination with the -z option, Git changes its output format to ensure reliable machine parsing [1][4]. In this mode: - Pathnames are output verbatim (no quoting or escaping is applied) [1][4]. - Each filename is terminated by a NUL (zero) byte instead of a newline [1][4]. - This format is specifically designed for integration with tools like xargs -0, which expects NUL-terminated input, allowing for robust handling of filenames containing spaces, newlines, or other special characters [5][6]. If you are writing scripts to process these filenames, using git diff -z --name-only is the recommended approach to avoid issues with filenames that contain special characters or whitespace [5].
Citations:
- 1: https://git-scm.com/docs/git-diff
- 2: https://public-inbox.org/git/201001011844.23571.agruen@suse.de/T/
- 3: http://public-inbox.org/git/alpine.LFD.0.98.0706201315310.3593@woody.linux-foundation.org/t/
- 4: https://git-scm.com/docs/git-diff/2.15.4
- 5: https://stackoverflow.com/questions/5605347/git-diff-name-only-and-copy-that-list
- 6: https://superuser.com/questions/518239/using-git-diff-with-xargs-0-spaces-in-file-names
🌐 Web query:
Git diff -z output newlines in filenames quoted
💡 Result:
Git handles special characters in filenames (including newlines, tabs, double quotes, and backslashes) in its diff output by quoting the entire pathname [1][2]. When a filename contains these "unusual" characters, Git represents them as escape sequences (\n for newline, \t for tab, " for double quote, and \ for backslash) and encloses the resulting string in double quotes [1][3][2]. This behavior is governed by the core.quotePath configuration variable [4][3]. If you need to work with filenames programmatically and want to avoid these parsing difficulties, the recommended approach is to use the -z option (e.g., git diff -z) [4][5][3]. When -z is provided, Git outputs filenames verbatim and separates them with a NUL byte rather than using escape sequences, which is a standard, robust practice for script consumption [4][5][3].
Citations:
- 1: https://public-inbox.org/git/201001011844.23571.agruen@suse.de/T/
- 2: https://git-scm.com/docs/diff-generate-patch/2.4.12
- 3: https://git-scm.com/docs/git-diff
- 4: https://man.archlinux.org/man/git-diff-files.1.en
- 5: https://stackoverflow.com/questions/28109520/how-to-cope-with-spaces-in-file-names-when-iterating-results-from-git-diff-nam
🌐 Web query:
Git mancore-quotePath quotePath true default output quotepath quoting filenames
💡 Result:
The core.quotepath configuration variable in Git controls how "unusual" characters in file paths are displayed by Git commands [1][2][3]. When core.quotepath is set to true (which is the default value), Git commands that output paths (such as git ls-files or git diff) will escape unusual characters [1][3]. This includes: - Non-printable characters (control characters) [1][3]. - Bytes with values larger than 0x80 [1][3]. - Characters that require escaping in C source code (e.g., backslashes and double-quotes) [1][3]. These characters are displayed by enclosing the path in double-quotes and using C-style backslash escaping (e.g., \t for TAB, \n for LF, or octal sequences for non-ASCII bytes) [1][4]. If you set core.quotepath to false, Git will output bytes with values higher than 0x80 verbatim rather than escaping them [1][3][5]. However, note that even with core.quotepath set to false, Git will still quote double-quotes, backslashes, and control characters to maintain output integrity [1][3]. Regardless of this setting, many Git commands provide a -z option, which allows pathnames to be output completely verbatim (often using a NUL character as a line terminator), which is generally the preferred method for machine-readable output [1][3].
Citations:
- 1: https://github.com/git/git/blob/master/Documentation/config/core.adoc
- 2: https://github.com/git/git/blob/v2.3.0/Documentation/config.txt
- 3: https://github.com/git/git/blob/bc22d845c4328f5bd896d019b3729f776ad4be4c/Documentation/config/core.txt
- 4: https://code.googlesource.com/git/+/refs/tags/v2.33.8/Documentation/config/core.txt
- 5: git/git@9378c16
Use NUL-delimited filenames from Git.
--name-only can quote filenames with newlines or other special characters, so splitlines()/strip() can split one file into multiple entries or drop whitespace-only filenames. Add -z and split on "\0" instead.
Proposed fix
"diff",
"--name-only",
+ "-z",
commit_sha,
"HEAD",
@@
- files = [
- file_path for file_path in result.stdout.splitlines() if file_path.strip()
- ]
+ files = [file_path for file_path in result.stdout.split("\0") if file_path]📝 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.
| "diff", | |
| "--name-only", | |
| "diff", | |
| "--name-only", | |
| "-z", | |
| commit_sha, | |
| "HEAD", |
🤖 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 25 - 26, Update
the Git file-list command in the change detector to include the NUL-delimited
output option alongside “--name-only”. Parse the command output by splitting on
"\0" rather than using splitlines() or strip(), while preserving valid filenames
and filtering only the final empty delimiter entry.
There was a problem hiding this comment.
Deferred intentionally. The maintainer requested NUL-safe filename handling be tracked as a Module B + V1 follow up rather than implemented in this PR.
There was a problem hiding this comment.
The PR description images show:
- Image 1: the
application/utils/harvesterpackage architecture:ChangeDetectorusesGitRepositoryClient.get_local_path()and invokes the git CLI throughsubprocess.run(git diff / git log);CheckpointStorepersistsRepositoryCheckpointdata to an on-diskcheckpoints.jsonfile via JSON reads/writes. - Image 2: the class model:
ChangeDetector,CheckpointStore,RepositoryChangeSet, andRepositoryCheckpoint, including the original one-argument change-detection signatures and the note thatRepositoryChangeSetis “conceptually related, not linked in code yet.” - Image 3: the original modified-files flow:
git diff --name-only sha HEAD→ “stdout lines” → “Filter blank lines” →sorted(set(files)). - Image 4: the commit-history flow:
git log --format=%H sha..HEAD→ filter blank lines → return commit SHAs. - Image 5:
CheckpointStore.save()/load()branching for absent or presentcheckpoints.jsonand repository IDs, with JSON parsing and persistence. - Image 6: the conceptual orchestrator flow: load checkpoint; if one exists, retrieve modified files and commits; otherwise do a full harvest; then save the latest checkpoint.
@ParthAggarwal16, acknowledged. NUL-safe filename handling remains a valid hardening item, but it is intentionally out of scope for this Module A PR and will not be treated as blocking here. If no follow-up already exists, would you like me to open a GitHub issue for the Module B + V1 work?
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/tests/harvester_test/change_detector_test.py`:
- Line 15: Replace every mocked "/tmp/repo" value in change_detector_test.py,
including the occurrences near client.get_local_path assignments, with the
neutral fixture path "repo-under-test" consistently, without changing the test
behavior.
🪄 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: a47dc0a6-eb7e-42c6-83f5-14838ddd4335
📒 Files selected for processing (3)
application/tests/harvester_test/change_detector_test.pyapplication/tests/harvester_test/git_repository_client_integration_test.pyapplication/utils/harvester/change_detector.py
🚧 Files skipped from review as they are similar to previous changes (1)
- application/tests/harvester_test/git_repository_client_integration_test.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 316-321: The _serialize_json_value function must preserve valid
JSON round-tripping for plain string inputs. Update its string branch to
JSON-encode ordinary strings with flask_json.dumps, or explicitly validate and
pass through only strings that already contain valid serialized JSON; ensure
downstream json.loads() succeeds for raw URIs, paths, and other string values.
- Around line 381-440: Wrap the add/commit flow in both
create_artifact_ingest_event and create_ingest_chunk with exception handling
that rolls back sqla.session when commit fails, then re-raise the original
exception so callers retain the failure behavior while the session is usable for
subsequent operations.
- Around line 324-331: Update _normalize_utc_datetime to reject datetime values
without tzinfo by raising an explicit error instead of returning them unchanged;
continue converting timezone-aware values to UTC and leaving non-datetime values
unchanged. Use timezone-aware SQLAlchemy DateTime columns for persisted
timestamps if needed to preserve offset metadata.
🪄 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: ca7cb048-edb3-4448-bdc8-8cd2d78b4ec3
📒 Files selected for processing (3)
application/database/db.pyapplication/tests/import_run_test.pymigrations/versions/9f1a2b3c4d5e_add_artifact_ingest_persistence.py
| def _normalize_utc_datetime(value: Any) -> Any: | ||
| from datetime import datetime, timezone | ||
|
|
||
| if isinstance(value, datetime): | ||
| if value.tzinfo is None: | ||
| return value | ||
| return value.astimezone(timezone.utc) | ||
| return value |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | grep -E '(^|/)application/database/db\.py$|(^|/)application/database/|gap-analysis|harvester|observations|import' || true
echo
echo "Relevant db.py outline:"
if [ -f application/database/db.py ]; then
ast-grep outline application/database/db.py --match _normalize_utc_datetime --view expanded || true
ast-grep outline application/database/db.py --match create_import_run --view expanded || true
wc -l application/database/db.py
echo
echo "Relevant lines 240-350:"
sed -n '240,350p' application/database/db.py | nl -ba -v240
echo
echo "Search observed_at/create_import_run/normalize_utc:"
rg -n "observed_at|created_at|_normalize_utc_datetime|create_import_run|astimezone|timezone=True|datetime\\(" application/database/db.py
fi
echo
echo "Search local timezone/utc assumptions in repo:"
rg -n "observed_at|created_at|astimezone\\(|timezone\\(\\)|UTC|naive|timezone-aware|utc" -g '*.py' -g '!*frontend*' .Repository: OWASP/OpenCRE
Length of output: 3770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Lines 240-350:"
sed -n '240,350p' application/database/db.py
echo
echo "All normalize_utc_datetime usages:"
python3 - <<'PY'
from pathlib import Path
p = Path('application/database/db.py')
text = p.read_text()
for i,line in enumerate(text.splitlines(), start=1):
if '_normalize_utc_datetime' in line or 'observed_at' in line or 'created_at' in line or 'create_import_run' in line:
print(f"{i}: {line}")
PY
echo
echo "All observed_at/create_import_run usages outside db.py:"
rg -n "observed_at|created_at|create_import_run|_normalize_utc_datetime" -g '*.py' application migrations scripts docsRepository: OWASP/OpenCRE
Length of output: 11277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "db.py lines 378-415:"
sed -n '378,415p' application/database/db.py
echo
echo "Harvester observed_at references:"
rg -n "observed_at|datetime.now|datetime.utcnow|timezone\\(|utc" application/utils/harvester -g '*.py'
echo
echo "Tests around record_artifact_event observed_at:"
sed -n '35,75p' application/tests/import_run_test.pyRepository: OWASP/OpenCRE
Length of output: 1204
Reject naive datetimes in UTC normalization.
artifact_ingest_event.observed_at is normalized through _normalize_utc_datetime, but naïve inputs are returned unchanged and persisted without validation. The public ingestion path currently passes tz-aware datetimes, so this is mainly about a future caller/local timestamp silently bypassing conversion. Raise an explicit error for naive inputs or document the UTC assumption, and consider sqla.DateTime(timezone=True) for timestamp columns so offset metadata is preserved.
🤖 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 324 - 331, Update
_normalize_utc_datetime to reject datetime values without tzinfo by raising an
explicit error instead of returning them unchanged; continue converting
timezone-aware values to UTC and leaving non-datetime values unchanged. Use
timezone-aware SQLAlchemy DateTime columns for persisted timestamps if needed to
preserve offset metadata.
There was a problem hiding this comment.
the RFC doesnt say, the ingestion pipeline already supplies timezone-aware UTC timestamps,
@northdpole @Pa04rth would like to know your thoughts on this one before i do it
| 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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No rollback on commit failure for either persistence helper.
Both create_artifact_ingest_event and create_ingest_chunk call sqla.session.add() followed directly by sqla.session.commit() with no try/except. A unique-constraint violation (duplicate run_id+artifact_id, or duplicate artifact_event_id+chunk_id — plausible on retries in an incremental harvester) raises an uncaught IntegrityError and leaves the session in a failed state, breaking subsequent operations on the same session until an explicit rollback() is issued.
🛠️ Suggested pattern (applies to both functions)
sqla.session.add(event)
- sqla.session.commit()
+ try:
+ sqla.session.commit()
+ except Exception:
+ sqla.session.rollback()
+ raise
return event🤖 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 381 - 440, Wrap the add/commit flow
in both create_artifact_ingest_event and create_ingest_chunk with exception
handling that rolls back sqla.session when commit fails, then re-raise the
original exception so callers retain the failure behavior while the session is
usable for subsequent operations.
|
I dont remember last time spending the whole day on a single PR, I have been reality checked after a while, so i guess i will continue this after getting you guy's thoughts on the stuff above, anyways, thanks for putting up with me, things are going slower than expected but i guess week4 should be a lot easier |
|
Status: not mergeable (no push from us). Please fix before re-review:
Outdated immutable-range threads were resolved; Postgres checkpoint + migration + remaining CodeRabbit items stay open. |
978bae8 to
41ae1a3
Compare
One note from local testing: after updating the checkout test to match the current implementation (git checkout -- ), the harvester tests passed. However, make test still fails locally due to an existing dataset issue unrelated to this change
|
1: will do after week2 gets merged |
2 participants
Summary
(this PR is stacked on top of #983 )
This PR introduces change detection and checkpoint persistence for the harvester pipeline.
It adds Git-based change detection utilities for discovering modified files and commits since a checkpoint, along with a lightweight checkpoint store for persisting repository processing state across pipeline runs.
Note: This PR is part of the Week 3 implementation and is intended to be reviewed after Week 2 (Repository Client), even though it targets
main.Added
ChangeDetectorfor Git-based incremental change detectionCheckpointStoreRepositoryCheckpointmodelRepositoryChangeSetmodelValidation Coverage
Change Detection
Checkpoint Store
Test Plan
Executed:
All tests passing.
Notes for Reviewers
ChangeDetectoris intentionally separated from the repository client so repository operations and incremental change detection remain independent concerns.1: High-Level Architecture (HLA)