Conversation
Verifies that calling upsert_relations() twice with the same Relation produces exactly one edge (MERGE semantics, not CREATE). Closes #21
Wraps CoordinodeClient.vector_search() with label/property defaults,
returning [{id, node, distance}, ...] sorted by ascending distance.
Guards against empty query_vector to match server validation behaviour.
Adds two integration tests: one seeding a :LCSim node and verifying the
seeded node appears in top-k results, one verifying empty-vector returns [].
Closes #20
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded CoordinodeGraph.similarity_search(...) which delegates to CoordinodeClient.vector_search(...), returns [] for empty query vectors, sorts results by ascending distance, and maps results to dicts with keys Changes
Sequence Diagram(s)sequenceDiagram
participant LangChain as CoordinodeGraph
participant SDK as CoordinodeClient
participant DB as CoordiNodeDB
LangChain->>SDK: vector_search(label, property, vector, top_k)
SDK->>DB: VectorService RPC (query vector, params)
DB-->>SDK: results (nodes + distances)
SDK-->>LangChain: raw results
LangChain->>LangChain: sort by distance, map to {id, node, distance}
LangChain-->>Caller: return formatted list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds a LangChain-facing similarity_search() API on CoordinodeGraph and expands integration test coverage to ensure relationship upserts are idempotent.
Changes:
- Implement
CoordinodeGraph.similarity_search()as a thin wrapper overCoordinodeClient.vector_search()with sensible defaults. - Add LangChain integration tests for
similarity_search()(happy path + empty-vector guard). - Add LlamaIndex integration test verifying
upsert_relations()MERGE/idempotency semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
langchain-coordinode/langchain_coordinode/graph.py |
Adds similarity_search() wrapper around the SDK’s vector_search(). |
tests/integration/adapters/test_langchain.py |
Adds integration coverage for similarity_search() results + empty-vector behavior. |
tests/integration/adapters/test_llama_index.py |
Adds integration test to ensure upsert_relations() does not create duplicate edges. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@langchain-coordinode/langchain_coordinode/graph.py`:
- Around line 222-228: The similarity_search method promises results ordered by
ascending distance but currently returns backend order; after calling
self._client.vector_search (the results variable) sort results client-side by
the distance attribute (r.distance) in ascending order before mapping to the
returned list of dicts (the current list comprehension that yields {"id":
r.node.id, "node": r.node.properties, "distance": r.distance}) so the method is
deterministic regardless of backend ordering.
In `@tests/integration/adapters/test_langchain.py`:
- Around line 168-172: The test
test_similarity_search_empty_vector_returns_empty currently only asserts the
type of results from graph.similarity_search([], k=5); update it to also assert
the empty-vector contract by adding an equality assertion that results == [] so
the test fails if any items are returned, keeping the existing type check if you
want but ensuring an explicit empty-list equality check against
graph.similarity_search.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 92a6a461-1e85-4adc-933f-77351e06b86b
📒 Files selected for processing (3)
langchain-coordinode/langchain_coordinode/graph.pytests/integration/adapters/test_langchain.pytests/integration/adapters/test_llama_index.py
…n test - Sort vector_search results client-side by r.distance (ascending) to match the public contract promised in the docstring - Replace isinstance(results, list) with results == [] assertion to explicitly enforce the empty-vector guard contract
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/adapters/test_langchain.py`:
- Around line 151-163: The test stores the returned node object in
seeded_internal_id but similarity_search returns integer internal IDs; change
the assignment so seeded_internal_id is the node's internal ID (use
seed_rows[0]["nid"].id_) after the graph.query CREATE ... RETURN n call,
ensuring the assertion compares the same integer ID type when checking
any(r["id"] == seeded_internal_id for r in results); update references to
seeded_internal_id accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b7b5379e-d975-4149-abfb-6a471f1c558d
📒 Files selected for processing (2)
langchain-coordinode/langchain_coordinode/graph.pytests/integration/adapters/test_langchain.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tibility similarity_search() now uses len(query_vector) == 0 instead of truthiness to avoid ValueError on numpy.ndarray and other Sequence types.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@langchain-coordinode/langchain_coordinode/graph.py`:
- Around line 199-200: The default parameter k in the similarity search API is
set to 10 but the linked issue and intended behavior expect k=5; update the
function signature in graph.py (the parameter list containing query_vector:
list[float], k: int = 10) to use k: int = 5 and ensure any related docstrings,
tests, or examples that assume similarity_search(..., k=5) are updated to
reflect this default so the API and issue objective remain consistent.
In `@tests/integration/adapters/test_langchain.py`:
- Around line 163-167: The test currently only checks non-negativity for
results[0]["distance"] but must assert the ascending sort contract; update the
test that uses the results variable (and seeded_internal_id) to explicitly
assert ordering by checking adjacent distances are non-decreasing (e.g., assert
results[i]["distance"] <= results[i+1]["distance"] for the first couple of pairs
or across the whole list) so the search result list is verified to be ordered
ascending by distance.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e93e83a7-e449-4781-ba78-cc09499d83f9
📒 Files selected for processing (2)
langchain-coordinode/langchain_coordinode/graph.pytests/integration/adapters/test_langchain.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tocol and issue spec - Change query_vector type annotation from list[float] to Sequence[float] (matches existing code comment that explicitly documents numpy.ndarray support and aligns with CoordinodeClient.vector_search() signature) - Change default k from 10 to 5 (matches issue #20 acceptance criteria) - Strengthen test_similarity_search_returns_results: replace non-negativity check with full ascending-order assertion on adjacent distances - Use store.structured_query(param_map=...) in test_upsert_relations_idempotent instead of store._client.cypher() — keeps integration test stable vs internal API
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced Apr 12, 2026
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
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.
Summary
similarity_search(query_vector, k, label, property)toCoordinodeGraph(LangChain adapter), wrappingCoordinodeClient.vector_search()with label/property defaultstest_upsert_relations_idempotentintegration test for LlamaIndex adapter, verifying exactly 1 edge after doubleupsert_relations()call (MERGE semantics)Technical Details
similarity_search():[{"id": ..., "node": ..., "distance": ...}, ...]sorted by ascending cosine distancequery_vector(server returnsINVALID_ARGUMENTfor empty vectors):LCSimnode found in top-k results, empty vector returns[]Idempotency test:
upsert_relations()twice with the sameRelationcount(r) == 1via Cypher — MERGE semantics confirmedTest Plan
Closes #20
Closes #21