Skip to content

fix(graph): address Qodo code-review findings on REFERENCES edges - #3

Merged
prom3theu5 merged 1 commit into
mainfrom
fix/qodo-review
Jun 1, 2026
Merged

fix(graph): address Qodo code-review findings on REFERENCES edges#3
prom3theu5 merged 1 commit into
mainfrom
fix/qodo-review

Conversation

@prom3theu5

Copy link
Copy Markdown
Member

Follow-up to the reference-edges feature (#1), addressing the seven Qodo review findings. Each is verified with a test and/or on a real repo.

Correctness

  1. Case-insensitive store traversals — the seed lookup is case-insensitive, but symbol_references, related_to_symbol's declaration lookup, and symbol_type_relations matched names case-sensitively. So --symbol widget seeded on Widget but returned no usages. Ladybug now uses WHERE lower(name) = $n (lowercased param); memory uses eq_ignore_ascii_case. Also fixes the same latent issue in the pre-existing type-relation queries.
  2. Segment-safe same-project checkfile_path.starts_with(project_prefix) treated src2/foo as same-project as src. Added same_project_dir() (a strip_prefix + /-boundary check) used in both resolve_references and resolve_supertypes.
  3. remove_file edge cleanup — previously pruned only edges originating from the removed file, leaking edges that point at a symbol the file declared and inflating the user-visible referenceEdges stat. Now retains only edges whose both endpoints still exist (applied to inherits/implements too).

Determinism & perf

  1. N+1 queriesresolve_references preloads each file's declarations once (name -> symbols) for the from lookup and caches to candidates per name across files, instead of two store queries per reference.
  2. Deterministic from selection — same-name/same-file declarations are sorted by (start_line, end_line, id) before picking, rather than relying on arbitrary order.

Maintainability & visibility

  1. Single source of truth — extracted collect_references(); both symbol_references and related_to_symbol call it (was duplicated inline).
  2. Surface extraction failures at warn (was debug) in extract / extract_supertypes / extract_references, so silent undercounts are visible. Goes to stderr, so the stdout=data contract holds.

Tests

New: case-insensitive reference lookup, segment-safe same-project resolution (src vs src2), and remove_file pruning incoming reference edges. All existing tests pass; cargo fmt --check, cargo clippy --all-targets, and cargo test (78 tests) all green. Verified case-insensitive lookups end-to-end on a real C# repo — lowercase and PascalCase queries return identical reference sites.

Follow-up to the reference-edges feature. Seven review findings:

1. Case-insensitive store traversals. The seed lookup is case-insensitive
   (eq_ignore_ascii_case) but symbol_references, related_to_symbol's
   declaration lookup, and symbol_type_relations matched names case-sensitively,
   so `--symbol widget` seeded on `Widget` but returned no usages. Ladybug now
   uses `WHERE lower(name) = $n` with a lowercased param; memory uses
   eq_ignore_ascii_case. Applies to references AND the pre-existing type
   relations.
2. Segment-safe same-project check. `file_path.starts_with(project_prefix)`
   treated `src2/foo` as same-project as `src`. Added same_project_dir() (a
   strip_prefix + '/'-boundary check) and used it in both resolve_references
   and resolve_supertypes.
3. Single source of truth for the memory reference traversal: extracted
   collect_references(); related_to_symbol and symbol_references both call it.
4. Surface extraction failures at warn (was debug) in extract / extract_supertypes
   / extract_references, so silent undercounts are visible. Goes to stderr, so
   the stdout=data contract is preserved.
5. remove_file now prunes symbol edges whose *either* endpoint no longer exists
   (was: only edges originating from the file), so reference_edges isn't
   inflated by dangling edges into a removed declaration. Applied to
   inherits/implements too.
6. resolve_references no longer does N+1 store queries: it preloads each file's
   declarations once (name -> symbols) for the `from` lookup and caches `to`
   candidates per name across files.
7. Deterministic `from` selection: same-name/same-file declarations are sorted
   by (start_line, end_line, id) before picking, instead of relying on
   arbitrary order.

Tests: case-insensitive reference lookup, segment-safe same-project resolution
(src vs src2), and remove_file pruning incoming reference edges. All existing
tests pass; fmt + clippy clean. Verified case-insensitive lookups end-to-end on
a real C# repo (lowercase and PascalCase queries return identical reference
sites).
@qodo-code-review

Copy link
Copy Markdown

Review Summary by Qodo

Fix case-insensitive lookups and same-project resolution in reference edges

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Fix case-insensitive symbol lookups across all stores (Ladybug/memory)
  - Seed lookup was case-insensitive but reference/type-relation queries were case-sensitive
  - Now uses lower(name) in Ladybug and eq_ignore_ascii_case in memory store
• Implement segment-safe same-project directory matching
  - Previous starts_with() check treated src2/foo as same-project as src
  - New same_project_dir() function enforces /-boundary checks
• Fix remove_file to prune incoming reference edges
  - Previously only removed edges originating from deleted file
  - Now removes edges whose either endpoint no longer exists
• Eliminate N+1 queries in resolve_references
  - Preload file declarations once per file and cache target candidates by name
• Improve determinism and maintainability
  - Sort same-name declarations by (start_line, end_line, id) for deterministic selection
  - Extract collect_references() as single source of truth for both stores
  - Elevate extraction failures from debug to warn level for visibility
Diagram
flowchart LR
  A["Symbol Lookup"] -->|case-insensitive| B["Ladybug Store"]
  A -->|case-insensitive| C["Memory Store"]
  B -->|lower name| D["Query Results"]
  C -->|eq_ignore_ascii_case| D
  E["File Removal"] -->|prune both endpoints| F["Reference Edges"]
  G["Reference Resolution"] -->|segment-safe check| H["Same-Project Filter"]
  G -->|preload & cache| I["Deterministic Selection"]

Loading

Grey Divider

File Changes

1. src/graph/ladybug_store.rs 🐞 Bug fix +25/-16

Make Ladybug store queries case-insensitive

• Convert symbol_type_relations queries to use case-insensitive WHERE lower(s.name) = $n
 matching
• Update symbol_references query to use case-insensitive name matching
• Update related_to_symbol query to use case-insensitive name matching
• Lowercase symbol name parameter before passing to queries

src/graph/ladybug_store.rs


2. src/graph/memory_store.rs 🐞 Bug fix +50/-52

Refactor memory store for case-insensitive lookups and edge cleanup

• Extract new collect_references() function as single source of truth for reference lookups
• Update symbol_references to call collect_references() instead of duplicating logic
• Update symbol_type_relations to use eq_ignore_ascii_case for name matching
• Update related_to_symbol to use eq_ignore_ascii_case and call collect_references()
• Fix remove_file to prune edges whose either endpoint no longer exists (not just originating
 edges)

src/graph/memory_store.rs


3. src/indexer/mod.rs ✨ Enhancement +63/-28

Optimize reference resolution and fix same-project checks

• Add same_project_dir() function to safely check project directory membership with /-boundary
 validation
• Update resolve_supertypes to use same_project_dir() instead of unsafe starts_with()
• Refactor resolve_references to preload file declarations once per file and cache target
 candidates by name
• Sort same-name declarations by (start_line, end_line, id) for deterministic selection
• Use same_project_dir() in reference resolution ambiguity policy

src/indexer/mod.rs


View more (2)
4. src/indexer/tree_sitter.rs Error handling +3/-3

Surface extraction failures at warn level

• Elevate extraction failure logging from debug to warn in extract() function
• Elevate supertype extraction failure logging from debug to warn in extract_supertypes()
• Elevate reference extraction failure logging from debug to warn in extract_references()

src/indexer/tree_sitter.rs


5. tests/unit.rs 🧪 Tests +150/-0

Add tests for case-insensitive lookups and edge cleanup

• Add reference_lookup_is_case_insensitive test verifying lowercase queries find PascalCase
 declarations
• Add reference_same_project_is_segment_safe test ensuring src/ and src2/ are treated as
 different projects
• Add remove_file_prunes_incoming_reference_edges test verifying dangling edges are cleaned up

tests/unit.rs


Grey Divider

Qodo Logo

@qodo-code-review

qodo-code-review Bot commented Jun 1, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

Grey Divider


Remediation recommended

1. Wrong enclosing symbol pick 🐞 Bug ≡ Correctness
Description
resolve_references() now deterministically selects the first same-named declaration by
(start_line, end_line, id), which tends to pick the outermost declaration instead of the smallest
enclosing declaration that Reference.from is defined to represent. This can attach REFERENCES
edges to the wrong symbol id when name collisions exist in-file (e.g., C# class Foo vs
constructor Foo).
Code

src/indexer/mod.rs[R452-480]

Evidence
resolve_references() sorts same-name declarations by earliest start_line and then picks
v.first(), which selects the outermost declaration when names collide. This conflicts with
Reference.from’s definition as the smallest enclosing declaration, and C# extraction/symbolization
makes such name collisions plausible (constructors share their class name and are both considered
enclosing decls and extracted as symbols).

src/indexer/mod.rs[452-480]
src/indexer/tree_sitter.rs[1445-1456]
src/indexer/tree_sitter.rs[206-225]
src/indexer/tree_sitter.rs[1496-1507]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`resolve_references()` builds `from_by_name` and then selects `v.first()` after sorting by `(start_line, end_line, id)`. But `Reference.from` is defined as the *smallest* enclosing declaration containing the usage; when multiple declarations share the same name in a file (e.g. C# class name equals constructor name), sorting by smallest `start_line` will prefer the outer declaration and violate the extractor’s intended semantics.

### Issue Context
- `Reference.from` is documented as the “smallest declaration containing the usage”.
- C# extraction explicitly includes both `constructor_declaration` and `class_declaration` as enclosing declaration kinds, and symbol extraction also emits constructor symbols.

### Fix Focus Areas
- src/indexer/mod.rs[452-480]

### Suggested fix approach
When multiple `from` candidates share the same name, prefer the most-specific/innermost declaration deterministically, e.g.:
- sort by span ascending: `(end_line - start_line)` (smaller first), then `start_line` (larger first), then `id`; and then pick first, **or**
- add a kind preference (e.g., Method/Constructor over Class/Struct) before applying a stable tie-breaker.

Keep behavior deterministic, but align it better to the “smallest enclosing declaration” contract.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@prom3theu5
prom3theu5 merged commit e9bcabd into main Jun 1, 2026
1 check passed
@prom3theu5
prom3theu5 deleted the fix/qodo-review branch June 1, 2026 20:43
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.

1 participant