fix(graph): prune legacy target-resolved File identities - #1158
fix(graph): prune legacy target-resolved File identities#1158prondubuisi wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe graph updater now reconciles legacy target-resolved external ChangesLegacy external File identity pruning
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GraphUpdater
participant FileSystem
participant GraphDatabase
GraphUpdater->>FileSystem: resolve recorded relative path identity
FileSystem-->>GraphUpdater: return target or missing path
GraphUpdater->>GraphDatabase: query project-owned absolute paths
GraphDatabase-->>GraphUpdater: return matching paths
GraphUpdater->>GraphDatabase: delete verified stale File nodes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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 `@codebase_rag/graph_updater.py`:
- Around line 2119-2139: Update the outside-repository File handling in the
graph-updater flow to batch live legacy-target matches for project-ownership
verification before adding them to deletion candidates, rather than immediately
appending them from _is_live_legacy_target_resolved_file. Ensure only targets
confirmed as owned by the current project are deleted, and add a regression test
covering a live local symlink whose ownership query returns no matching path.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba2ecf79-915e-4be6-ba5a-335a604b5128
📒 Files selected for processing (3)
codebase_rag/constants/graph.pycodebase_rag/graph_updater.pycodebase_rag/tests/test_graph_updater_pruning.py
Greptile SummaryThis change extends incremental graph pruning for legacy File identities associated with external symlinks. A two-project reproduction confirmed that pruning a live external symlink in one project can globally remove a File identity still owned by another project, deleting that project’s containment edge and attached graph data. The deletion path needs to preserve shared File nodes until their final Project owner is removed. Confidence Score: 4/5Not safe to merge until pruning no longer globally deletes File identities that remain owned by another project. The failure was reproduced through the real pruning method with a live external symlink and two project owners, showing that the shared node and both ownership edges are removed. Files Needing Attention: codebase_rag/graph_updater.py needs an owner check or ownership-edge-only removal before invoking the global File deletion query; the related deletion query is in codebase_rag/constants/graph.py.
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if self._is_live_legacy_target_resolved_file( | ||
| path, abs_path, repo_abs | ||
| ): | ||
| orphans.append((path, abs_path)) |
There was a problem hiding this comment.
A live external symlink whose legacy File.absolute_path is its resolved target is queued for deletion here without checking whether another Project owns the same File. The subsequent globally keyed CYPHER_DELETE_FILE performs a DETACH DELETE, so pruning project A removes project B’s containment edge and all graph data attached to the shared node. Check for remaining Project owners before deleting the File, or remove only this project’s ownership relationship and delete the File only after its final owner is gone.
Artifacts
Focused two-project shared external File pruning reproduction source
- This executable reproduction creates project A and B as owners of one external File identity, runs the real pruning method for project A, and asserts the global delete removes the shared node and both owners.
Before pruning, the shared File has project A and project B owners
- The successful before run records that the File exists and is owned by both project-a and project-b before pruning.
After project A pruning, global File deletion removes both owners
- The successful after run exercises GraphUpdater pruning and records the global Cypher delete, no ownership query, and removal of the File plus both owner edges.
Prompt To Fix With AI
This is a comment left during a code review.
Path: codebase_rag/graph_updater.py
Line: 2134-2137
Comment:
**Live symlink pruning deletes shared File identities**
A live external symlink whose legacy `File.absolute_path` is its resolved target is queued for deletion here without checking whether another Project owns the same File. The subsequent globally keyed `CYPHER_DELETE_FILE` performs a `DETACH DELETE`, so pruning project A removes project B’s containment edge and all graph data attached to the shared node. Check for remaining Project owners before deleting the File, or remove only this project’s ownership relationship and delete the File only after its final owner is gone.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Thanks for picking up #1156 so quickly — the batched ownership verification ( Maintainer note on the residual Greptile P1 (shared File identity), so you can decide how far to take it:
Two ways to close it, your call:
Given how narrow this is, I lean 1 or 3. Whichever you choose, please add a regression test with two projects sharing one external-target File and assert the sibling's node survives. Nice work — this is close. |
2 participants
Summary
--update-graphnow sweeps pre-GHSA-85ggFilenodes that were keyed on a leaf-dereferenced external symlink target. Those nodes sit outside the repo, so the prune containment gate skipped them and they leaked forever.cached_resolve_posix(repo_path / path)equals the storedabsolute_pathand that path disagrees withcached_file_identity_posix(...).CONTAINS_PACKAGE|CONTAINS_FOLDER|CONTAINS_FILE*fromProject). Sibling-project files that share a relative path stay untouched.Type of Change
Related Issues
Fixes #1156
Test Plan
uv run pytest codebase_rag/tests/test_graph_updater_pruning.py— 21 passed, including the 16 existing prune tests)TestPruneLegacyTargetResolvedFileIdentity)make test-integration, requires Docker — not run locally; unit tests cover the prune decision table)RED verification (tests against unpatched
_prune_orphan_nodes):test_prune_removes_live_legacy_external_symlink_filefailed (0deletes)test_prune_removes_deleted_legacy_external_symlink_when_project_ownedfailed (0deletes)GREEN after the fix: 21/21 in
test_graph_updater_pruning.py. Also 48 passed across prune + incremental updater tests.Keep-safe coverage:
Filewith a missing relative path (sibling project) is not deletedPre-commit on the changed files: ruff, ruff format, ty, README generation, bandit.
Checklist
pre-commit run --fileson the three changed files)# type: ignore,cast(),Any, orobjecttype hintsAI assistance
AI assistance was used to locate the prune/identity code, implement the focused fix, and write the RED-verified regression tests. The final diff was reviewed against
CONTRIBUTING.md(constants, noAny, conventional commit, tests required) and run through the pre-commit hooks listed above.Why this is safe
Filenodes are unique onabsolute_path. Deleting an outside key cannot collapse an in-repo file. Deleting an in-repo identity mismatch would be unsafe (it is the real file's MERGE key), so that case is explicitly not pruned. Ownership is consulted only for missing-on-disk outside keys, sotest_prune_skips_other_projectsstill holds.Summary by CodeRabbit