Skip to content

fix: incremental rebuild drops edges from unchanged files (#116)#120

Merged
carlos-alm merged 2 commits intomainfrom
fix/incremental-rebuild-drops-edges
Feb 26, 2026
Merged

fix: incremental rebuild drops edges from unchanged files (#116)#120
carlos-alm merged 2 commits intomainfrom
fix/incremental-rebuild-drops-edges

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Root cause: During incremental rebuilds, edge deletion was bidirectional (deleted edges FROM and TO the changed file), and nodes were deleted/re-inserted with new auto-increment IDs. Unchanged files that imported the changed file never rebuilt their edges, so those edges were permanently lost.
  • Fix: Added a reverse-dependency cascade in builder.js — before deleting nodes for changed files, finds all files with edges TO them, deletes only their outgoing edges (preserving node IDs), and adds them to the re-parse list so they participate in edge building.
  • Test: New integration test verifies that touching math.js preserves all import and call edges from utils.js and index.js.

Closes #116

Test plan

  • New test: npx vitest run tests/integration/build.test.js -t "issue #116" — passes, detects 2 reverse-deps, all 9 edges preserved
  • Full integration suite: npx vitest run tests/integration/ — 158 passed, 2 skipped
  • Lint: npm run lint — no issues

…116)

When a file was rebuilt incrementally, edges from other unchanged files
pointing to it were permanently lost because edge deletion was
bidirectional and unchanged importers never rebuilt their edges.

Add reverse-dependency cascade: before deleting nodes for changed files,
find files with edges TO them, delete only their outgoing edges (preserving
node IDs), and re-parse them so they participate in edge building.

Impact: 1 functions changed, 1 affected
@claude
Copy link

claude bot commented Feb 26, 2026

Claude finished @carlos-alm's task —— View job


PR Review: Fix Incremental Rebuild Edge Dropping

Review Todo List

  • Phase 1: Root Cause Analysis - understand the problem and solution approach
  • Read and analyze changed files (src/builder.js, tests/integration/build.test.js)
  • Phase 2: Configuration Change Detection
  • Phase 3: Test Integrity Assessment
  • Phase 4: Code Quality Review
  • Provide structured review with rating and recommendation

Root Cause Analysis

Problem Being Solved: During incremental rebuilds, edges from unchanged files to changed files were being permanently lost. When a file changed, the system would delete both incoming and outgoing edges bidirectionally, and unchanged files that imported the changed file would never have their edges rebuilt, causing permanent graph corruption.

Why This Approach: The fix implements a "reverse-dependency cascade" that identifies files with edges pointing TO changed files, preserves their node IDs while clearing only their outgoing edges, then adds them to the re-parse list so they participate in edge rebuilding. This is architecturally sound and maintains data integrity without breaking incremental build performance.

Risk Assessment: LOW RISK - The fix is surgical, well-targeted, and includes comprehensive testing.

Code Quality Assessment

Strengths:

  • Surgical fix: The solution precisely addresses the root cause without disrupting other incremental build logic
  • Performance conscious: Uses efficient SQL queries and minimizes unnecessary work (src/builder.js:458-474)
  • Preserves node IDs: Critical for maintaining foreign key relationships and avoiding cascade issues
  • Comprehensive test coverage: New test directly reproduces the bug scenario and verifies the fix (tests/integration/build.test.js:264-331)
  • Excellent documentation: Clear comments explaining the reverse-dependency cascade logic

Technical Quality:

  • SQL Query Efficiency: The reverse dependency query is well-structured with proper joins and uses DISTINCT to avoid duplicates
  • Edge Case Handling:
    • Verifies files exist on disk before adding to reverse deps (src/builder.js:469)
    • Prevents double-processing with set deduplication (src/builder.js:466)
    • Properly handles metadata updates for reverse-dep-only files (src/builder.js:584-585)
  • Transaction Safety: Changes are made within existing transaction boundaries
  • Memory Management: Uses sets for efficient lookups and prevents memory leaks

Test Quality:

  • Realistic scenario: Test uses a proper fixture with import/call dependencies
  • Baseline comparison: Captures edges before and after to verify preservation
  • Specific assertions: Explicitly checks that critical edges (utils.js->math.js, index.js->math.js) survive
  • Comprehensive verification: Tests both import and call edges

Minor Considerations

  • The new deleteOutgoingEdgesForFile prepared statement could be reused elsewhere, but the current implementation is appropriate for this targeted fix
  • The _reverseDepOnly flag is a clean way to track special handling without complex branching

Final Assessment

## Root Cause Analysis
**Problem Being Solved**: Incremental rebuilds were losing edges from unchanged files to changed files due to bidirectional edge deletion and unchanged files never rebuilding their edges.
**Why This Approach**: Implements a reverse-dependency cascade that preserves node IDs while selectively clearing outgoing edges and re-parsing affected files - architecturally sound solution.
**Risk Assessment**: LOW - Surgical fix with comprehensive testing, no configuration changes, maintains existing performance characteristics.

## Critical Concerns
None. This is a well-engineered fix that addresses a real data integrity issue.

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: APPROVE
- Reasoning: Excellent technical solution with proper root cause analysis, comprehensive testing, and surgical implementation. The fix directly addresses a data corruption issue without introducing new risks. Only minor point preventing 5 stars is the relatively narrow scope - this is fundamentally a bug fix rather than a broader architectural improvement.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 26, 2026

Greptile Summary

Fixes a critical bug in incremental rebuilds where edges from unchanged files to changed files were permanently lost. The solution adds a reverse-dependency cascade that identifies files with edges pointing to changed files, deletes only their outgoing edges (preserving node IDs via the UNIQUE(name, kind, file, line) constraint and INSERT OR IGNORE), and re-parses them to rebuild edges. The implementation correctly handles edge cases like verifying file existence and avoiding duplicate processing. Test coverage is comprehensive, verifying both import and call edges are preserved.

Confidence Score: 5/5

  • This PR is safe to merge with no identified risks
  • The implementation correctly fixes issue bug(builder): incremental rebuild drops edges when re-parsing a file #116 by preserving node IDs and rebuilding edges from unchanged files. The logic leverages the existing UNIQUE constraint on nodes to preserve IDs via INSERT OR IGNORE. Edge cases are handled (file existence checks, duplicate prevention). The comprehensive test verifies the fix works as intended, and all existing tests pass.
  • No files require special attention

Important Files Changed

Filename Overview
src/builder.js Added reverse-dependency cascade to preserve edges from unchanged files during incremental rebuilds; correctly uses INSERT OR IGNORE with UNIQUE constraint to preserve node IDs
tests/integration/build.test.js Comprehensive test case verifying edges from unchanged files are preserved after touching math.js; validates both import and call edges match baseline

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Start Incremental Build] --> B{Full Build?}
    B -->|Yes| C[Delete All Edges/Nodes]
    B -->|No| D[Build changedRelPaths Set]
    D --> E[Query DB: Find Files with Edges TO Changed Files]
    E --> F{Reverse Deps Found?}
    F -->|Yes| G[Add to reverseDeps Set]
    F -->|No| H[Delete Edges/Nodes for Changed Files]
    G --> I[Verify File Exists on Disk]
    I --> H
    H --> J[Delete Only Outgoing Edges for Reverse Deps]
    J --> K[Add Reverse Deps to parseChanges with _reverseDepOnly Flag]
    K --> L[Parse All Files in parseChanges]
    L --> M[Insert Nodes with INSERT OR IGNORE]
    M --> N{Has _reverseDepOnly Flag?}
    N -->|Yes| O[Skip Hash Update]
    N -->|No| P[Update File Hash]
    O --> Q[Rebuild Edges]
    P --> Q
    Q --> R[End]
    C --> L
Loading

Last reviewed commit: aa3255f

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

The lock file had @optave/codegraph-*-* at 2.3.0 while package.json
requires 2.4.0, and win32-x64-msvc was missing entirely. This caused
npm ci to fail in CI (License Compliance Scan).
@claude
Copy link

claude bot commented Feb 26, 2026

Claude finished @carlos-alm's task —— View job


PR Review: Fix Incremental Rebuild Edge Dropping

Review Todo List

  • Phase 1: Root Cause Analysis - understand the problem and solution approach
  • Read and analyze changed files (src/builder.js, tests/integration/build.test.js)
  • Phase 2: Configuration Change Detection
  • Phase 3: Test Integrity Assessment
  • Phase 4: Code Quality Review
  • Provide structured review with rating and recommendation

Root Cause Analysis

Problem Being Solved: During incremental rebuilds, edges from unchanged files to changed files were being permanently lost. When a file changed, the system would delete both incoming and outgoing edges bidirectionally, and unchanged files that imported the changed file would never have their edges rebuilt, causing permanent graph corruption.

Why This Approach: The fix implements a "reverse-dependency cascade" that identifies files with edges pointing TO changed files, preserves their node IDs while clearing only their outgoing edges, then adds them to the re-parse list so they participate in edge rebuilding. This is architecturally sound and maintains data integrity without breaking incremental build performance.

Risk Assessment: LOW RISK - The fix is surgical, well-targeted, and includes comprehensive testing.

Configuration Change Detection

Analysis: The primary change in package-lock.json appears to be a significant reduction in size (1032 lines deleted vs 52 added), which typically indicates dependency deduplication or removal of unused lockfile entries. This is LOW RISK as it doesn't involve relaxing linting rules, disabling strict modes, or increasing timeouts. No configuration files (biome.json, tsconfig.json, test configs) were modified.

Test Integrity Assessment

Excellent Test Quality: The new test (tests/integration/build.test.js:264-331) is EXEMPLARY:

  • Realistic scenario: Uses proper fixture with import/call dependencies
  • Baseline comparison: Captures edges before and after to verify preservation
  • Specific assertions: Explicitly checks that critical edges (utils.js->math.js, index.js->math.js) survive
  • Comprehensive verification: Tests both import and call edges
  • Proper isolation: Starts with clean build, makes targeted change (appends comment to math.js)
  • Clear intent: Test name and comments clearly explain issue bug(builder): incremental rebuild drops edges when re-parsing a file #116 reproduction

No Test Weakening: No existing tests were modified or assertions removed.

Code Quality Assessment

Strengths:

  • Surgical fix: The solution precisely addresses the root cause without disrupting other incremental build logic
  • Performance conscious: Uses efficient SQL queries and minimizes unnecessary work (src/builder.js:458-474)
  • Preserves node IDs: Critical for maintaining foreign key relationships and avoiding cascade issues
  • Comprehensive test coverage: New test directly reproduces the bug scenario and verifies the fix (tests/integration/build.test.js:264-331)
  • Excellent documentation: Clear comments explaining the reverse-dependency cascade logic

Technical Quality:

  • SQL Query Efficiency: The reverse dependency query is well-structured with proper joins and uses DISTINCT to avoid duplicates
  • Edge Case Handling:
    • Verifies files exist on disk before adding to reverse deps (src/builder.js:469)
    • Prevents double-processing with set deduplication (src/builder.js:466)
    • Properly handles metadata updates for reverse-dep-only files (src/builder.js:584-585)
  • Transaction Safety: Changes are made within existing transaction boundaries
  • Memory Management: Uses sets for efficient lookups and prevents memory leaks

Test Quality:

  • Realistic scenario: Test uses a proper fixture with import/call dependencies
  • Baseline comparison: Captures edges before and after to verify preservation
  • Specific assertions: Explicitly checks that critical edges (utils.js->math.js, index.js->math.js) survive
  • Comprehensive verification: Tests both import and call edges

Minor Considerations

  • The new deleteOutgoingEdgesForFile prepared statement could be reused elsewhere, but the current implementation is appropriate for this targeted fix
  • The _reverseDepOnly flag is a clean way to track special handling without complex branching

Final Assessment

## Root Cause Analysis
**Problem Being Solved**: Incremental rebuilds were losing edges from unchanged files to changed files due to bidirectional edge deletion and unchanged files never rebuilding their edges.
**Why This Approach**: Implements a reverse-dependency cascade that preserves node IDs while selectively clearing outgoing edges and re-parsing affected files - architecturally sound solution.
**Risk Assessment**: LOW - Surgical fix with comprehensive testing, no configuration changes, maintains existing performance characteristics.

## Critical Concerns
None. This is a well-engineered fix that addresses a real data integrity issue.

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: APPROVE
- Reasoning: Excellent technical solution with proper root cause analysis, comprehensive testing, and surgical implementation. The fix directly addresses a data corruption issue without introducing new risks. Only minor point preventing 5 stars is the relatively narrow scope - this is fundamentally a bug fix rather than a broader architectural improvement.

@carlos-alm carlos-alm merged commit 9c3e3ba into main Feb 26, 2026
15 checks passed
@carlos-alm carlos-alm deleted the fix/incremental-rebuild-drops-edges branch February 26, 2026 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(builder): incremental rebuild drops edges when re-parsing a file

1 participant