feat: incremental knowledge graph updates (closes #221)#789
Open
saij3b wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughThis PR introduces an incremental knowledge-graph update service that detects file changes between snapshots, cleans up affected graph nodes, and applies differential updates without full reconstruction. It includes change detection, Neo4j-backed cleanup operations, and filtered subgraph storage, with comprehensive unit tests validating hashing, change detection, and updater behaviors. ChangesIncremental Graph Updates
Sequence DiagramsequenceDiagram
participant Caller
participant IncrementalGraphUpdater
participant Neo4j
participant CodeGraphService
Caller->>IncrementalGraphUpdater: apply_changes(changeset, new_artifacts)
IncrementalGraphUpdater->>Neo4j: get_existing_file_snapshot()
Neo4j-->>IncrementalGraphUpdater: FILE node data
IncrementalGraphUpdater->>IncrementalGraphUpdater: detect affected files
IncrementalGraphUpdater->>Neo4j: cleanup_files(affected_paths)
Neo4j-->>IncrementalGraphUpdater: deleted node count
alt new_artifacts provided
IncrementalGraphUpdater->>IncrementalGraphUpdater: reconstruct filtered subgraph
IncrementalGraphUpdater->>CodeGraphService: _store_graph(filtered subgraph)
CodeGraphService->>Neo4j: persist nodes and edges
Neo4j-->>CodeGraphService: insertion stats
CodeGraphService-->>IncrementalGraphUpdater: node/edge counts
end
IncrementalGraphUpdater-->>Caller: stats dict (added, modified, deleted)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
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 `@app/modules/parsing/graph_construction/incremental_update_service.py`:
- Around line 119-142: get_existing_file_snapshot builds the snapshot from nodes
labeled FILE, but cleanup_files only removes nodes labeled NODE, so FILE-labeled
nodes can survive and reappear in future diffs; update the cleanup logic
(cleanup_files and any related deletion queries) to target the same file-backed
nodes used by get_existing_file_snapshot—i.e., delete nodes with the FILE label
(or both FILE and NODE) instead of only :NODE—so that removed files are actually
removed from the live graph and won't be included in subsequent snapshots.
- Around line 162-175: The delete-before-insert is not atomic: the session.run
deletion using self.code_graph_service.driver.session() must be combined with
the subsequent _store_subgraph()/_store_graph() call inside a single write
transaction (e.g., use a write_transaction or transaction context) so that both
deletion and reinsertion commit together, or implement a compensating rollback
path if the store fails; update the code paths around the MATCH...DETACH DELETE
query and the calls to _store_subgraph()/_store_graph() to run within the same
transactional unit provided by code_graph_service (or explicitly roll back and
restore deleted nodes on failure).
- Around line 38-50: FileSnapshot is only shallowly immutable because
frozen=True prevents rebinding but allows in-place mutation of the files dict;
modify FileSnapshot.from_files to wrap the copied dict in an immutable read-only
mapping (e.g., use types.MappingProxyType on dict(files)) and update the files
type annotation to a read-only mapping (e.g., Mapping[str, str]) so callers
cannot mutate snapshot.files and subsequent detect_changes() sees stable data.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d8b9ce4-b8d0-4536-aeba-9e224305171b
📒 Files selected for processing (2)
app/modules/parsing/graph_construction/incremental_update_service.pytests/unit/parsing/test_incremental_update.py
Comment on lines
+38
to
+50
| @dataclass(frozen=True) | ||
| class FileSnapshot: | ||
| """Content-hashed view of a set of files at a point in time. | ||
|
|
||
| ``files`` maps repo-relative file path → SHA-256 hex digest of the | ||
| file contents. Two snapshots are comparable via :func:`detect_changes`. | ||
| """ | ||
|
|
||
| files: Dict[str, str] = field(default_factory=dict) | ||
|
|
||
| @classmethod | ||
| def from_files(cls, files: Dict[str, str]) -> "FileSnapshot": | ||
| return cls(files=dict(files)) |
Contributor
There was a problem hiding this comment.
FileSnapshot is only shallowly immutable.
frozen=True prevents rebinding files, but callers can still mutate snapshot.files[...] in place. That can silently skew later detect_changes() results if a snapshot is reused. Wrap the mapping in a read-only container during construction.
Proposed fix
+from types import MappingProxyType
-from typing import Dict, Iterable, List, Optional, Set
+from typing import Dict, Iterable, List, Mapping, Optional, Set
@@
`@dataclass`(frozen=True)
class FileSnapshot:
@@
- files: Dict[str, str] = field(default_factory=dict)
+ files: Mapping[str, str] = field(default_factory=dict)
+
+ def __post_init__(self) -> None:
+ object.__setattr__(self, "files", MappingProxyType(dict(self.files)))
@@
`@classmethod`
def from_files(cls, files: Dict[str, str]) -> "FileSnapshot":
- return cls(files=dict(files))
+ return cls(files=files)📝 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.
Suggested change
| @dataclass(frozen=True) | |
| class FileSnapshot: | |
| """Content-hashed view of a set of files at a point in time. | |
| ``files`` maps repo-relative file path → SHA-256 hex digest of the | |
| file contents. Two snapshots are comparable via :func:`detect_changes`. | |
| """ | |
| files: Dict[str, str] = field(default_factory=dict) | |
| @classmethod | |
| def from_files(cls, files: Dict[str, str]) -> "FileSnapshot": | |
| return cls(files=dict(files)) | |
| `@dataclass`(frozen=True) | |
| class FileSnapshot: | |
| """Content-hashed view of a set of files at a point in time. | |
| ``files`` maps repo-relative file path → SHA-256 hex digest of the | |
| file contents. Two snapshots are comparable via :func:`detect_changes`. | |
| """ | |
| files: Mapping[str, str] = field(default_factory=dict) | |
| def __post_init__(self) -> None: | |
| object.__setattr__(self, "files", MappingProxyType(dict(self.files))) | |
| `@classmethod` | |
| def from_files(cls, files: Dict[str, str]) -> "FileSnapshot": | |
| return cls(files=files) |
🤖 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 `@app/modules/parsing/graph_construction/incremental_update_service.py` around
lines 38 - 50, FileSnapshot is only shallowly immutable because frozen=True
prevents rebinding but allows in-place mutation of the files dict; modify
FileSnapshot.from_files to wrap the copied dict in an immutable read-only
mapping (e.g., use types.MappingProxyType on dict(files)) and update the files
type annotation to a read-only mapping (e.g., Mapping[str, str]) so callers
cannot mutate snapshot.files and subsequent detect_changes() sees stable data.
Comment on lines
+119
to
+142
| def get_existing_file_snapshot(self, project_id: str) -> FileSnapshot: | ||
| """Reconstruct a :class:`FileSnapshot` from the live graph. | ||
|
|
||
| Each ``FILE`` node carries a ``file_path`` and ``text`` property | ||
| (the file's source). We hash the text to produce the snapshot | ||
| entry. Files without a ``text`` property fall back to an empty | ||
| hash sentinel so they still participate in deletion detection. | ||
| """ | ||
| snapshot: Dict[str, str] = {} | ||
| with self.code_graph_service.driver.session() as session: | ||
| result = session.run( | ||
| """ | ||
| MATCH (n:FILE {repoId: $project_id}) | ||
| RETURN n.file_path AS file_path, | ||
| COALESCE(n.text, '') AS text | ||
| """, | ||
| project_id=project_id, | ||
| ) | ||
| for record in result: | ||
| path = record["file_path"] | ||
| if not path: | ||
| continue | ||
| snapshot[path] = FileSnapshot.hash_content(record["text"] or "") | ||
| return FileSnapshot(files=snapshot) |
Contributor
There was a problem hiding this comment.
Snapshot/delete label mismatch can keep deleted files in future diffs.
get_existing_file_snapshot() rebuilds state from :FILE nodes, but cleanup_files() only deletes :NODE. If FILE is not also labeled NODE, modified/deleted files survive in the next snapshot and future diffs will treat them as still present. Cleanup should target the same file-backed nodes that snapshot reconstruction reads from.
One possible fix
- MATCH (n:NODE {repoId: $project_id})
+ MATCH (n {repoId: $project_id})
WHERE n.file_path IN $pathsAlso applies to: 147-175
🤖 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 `@app/modules/parsing/graph_construction/incremental_update_service.py` around
lines 119 - 142, get_existing_file_snapshot builds the snapshot from nodes
labeled FILE, but cleanup_files only removes nodes labeled NODE, so FILE-labeled
nodes can survive and reappear in future diffs; update the cleanup logic
(cleanup_files and any related deletion queries) to target the same file-backed
nodes used by get_existing_file_snapshot—i.e., delete nodes with the FILE label
(or both FILE and NODE) instead of only :NODE—so that removed files are actually
removed from the live graph and won't be included in subsequent snapshots.
Comment on lines
+162
to
+175
| with self.code_graph_service.driver.session() as session: | ||
| result = session.run( | ||
| """ | ||
| MATCH (n:NODE {repoId: $project_id}) | ||
| WHERE n.file_path IN $paths | ||
| WITH n, count(*) AS _ | ||
| DETACH DELETE n | ||
| RETURN count(n) AS deleted | ||
| """, | ||
| project_id=project_id, | ||
| paths=paths, | ||
| ) | ||
| record = result.single() | ||
| deleted = int(record["deleted"]) if record else 0 |
Contributor
There was a problem hiding this comment.
Delete and reinsert are not atomic.
The cleanup commits before _store_subgraph() runs. If _store_graph() fails, the affected files stay deleted and the project graph is left partially updated. That breaks the rollback/partial-failure requirement called out in the PR objective. Run both steps in a single write transaction or add a compensating rollback path.
Also applies to: 213-223, 284-285
🤖 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 `@app/modules/parsing/graph_construction/incremental_update_service.py` around
lines 162 - 175, The delete-before-insert is not atomic: the session.run
deletion using self.code_graph_service.driver.session() must be combined with
the subsequent _store_subgraph()/_store_graph() call inside a single write
transaction (e.g., use a write_transaction or transaction context) so that both
deletion and reinsertion commit together, or implement a compensating rollback
path if the store fails; update the code paths around the MATCH...DETACH DELETE
query and the calls to _store_subgraph()/_store_graph() to run within the same
transactional unit provided by code_graph_service (or explicitly roll back and
restore deleted nodes on failure).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #221.
Adds an incremental knowledge-graph update system: FileSnapshot content-hash view, detect_changes() differential analyzer (added/modified/deleted), and IncrementalGraphUpdater doing scoped neo4j DETACH DELETE only for affected file_paths (preserving untouched nodes/relationships/inferences) and writing the filtered subgraph via the existing CodeGraphService._store_graph path. Includes unit tests for snapshot hashing, change-detection partitions, no-op short-circuits, blank-path filtering, and scoped-cleanup guarantees.
Summary by CodeRabbit
New Features
Tests