Conversation
- Implement add_graph_documents() which was inherited as a no-op from GraphStore base class; upserts nodes via MERGE and creates edges via MATCH+CREATE with fallback when WHERE NOT guard is unsupported - Fix refresh_schema() relationship enrichment query: labels()/type() Cypher functions return null in CoordiNode; use a.__label__ and r.__type__ internal properties instead - Add _cypher_ident() helper to safely escape label/type names Closes #14
- Fix upsert_relations(): CoordiNode does not support MERGE for edge patterns; replace MATCH+MERGE with MATCH+MATCH+CREATE - Fix get_triplets(): type(r) returns null; use r.__type__ instead. Also use `or "RELATED"` guard (not default arg) since null differs from missing key - Fix get_rel_map(): same __type__ fix for variable-length path results where relationship data arrives as a list of dicts Closes #14
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughReplaced CoordiNode-incompatible Cypher metadata calls with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CoordinodeGraph
participant CypherBackend
Client->>CoordinodeGraph: add_graph_documents(documents)
activate CoordinodeGraph
loop per document
loop per node
CoordinodeGraph->>CypherBackend: MERGE (n:Label {name:$name}) SET n += $props
CypherBackend-->>CoordinodeGraph: OK
end
loop per relationship
CoordinodeGraph->>CypherBackend: MATCH (src),(dst) WHERE src.name=$s AND dst.name=$d
CypherBackend-->>CoordinodeGraph: src,dst
alt props non-empty
CoordinodeGraph->>CypherBackend: CREATE (src)-[r:TYPE]->(dst) SET r += $props
else
CoordinodeGraph->>CypherBackend: CREATE (src)-[r:TYPE]->(dst)
end
CypherBackend-->>CoordinodeGraph: Relationship created
end
opt include_source
CoordinodeGraph->>CypherBackend: MERGE (__Document__ {name:$doc_id}) SET d += $props
CypherBackend-->>CoordinodeGraph: OK
CoordinodeGraph->>CypherBackend: CREATE (d)-[:MENTIONS]->(entity)
CypherBackend-->>CoordinodeGraph: Mention created
end
end
CoordinodeGraph->>CoordinodeGraph: invalidate cached schema
deactivate CoordinodeGraph
CoordinodeGraph-->>Client: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…dapters - Add tests/integration/adapters/test_langchain.py: CoordinodeGraph connect, schema, query, add_graph_documents (upsert nodes, create relationships, idempotency), schema invalidation - Add tests/integration/adapters/test_llama_index.py: CoordinodePropertyGraphStore connect, schema, structured_query, upsert_nodes, upsert_relations, get_triplets, get_rel_map, delete (by id and name) - Fix namespace package declarations in llama_index/__init__.py and llama_index/graph_stores/__init__.py: use pkgutil.extend_path so llama_index.core remains importable when package is installed editable - Add llama-index-core and langchain-community to dev dependencies so adapter tests can be collected and run from the workspace root - 19/19 integration tests pass against localhost:17080 All 19 adapter tests pass. Relates to #14.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
llama-index-coordinode/llama_index/graph_stores/coordinode/base.py (3)
220-239:⚠️ Potential issue | 🟠 MajorPreserve idempotence in
upsert_relations().This change turns an upsert API into unconditional inserts. A repeated call with the same relation now creates duplicates, which is especially risky for retries and re-indexing flows. Please keep the guarded
WHERE NOT ... CREATEpattern here too, with unconditionalCREATEonly as a compatibility fallback.Suggested direction
- cypher = ( - f"MATCH (src {{id: $src_id}}) MATCH (dst {{id: $dst_id}}) " - f"CREATE (src)-[r:{label}]->(dst) SET r += $props" - ) + cypher = ( + f"MATCH (src {{id: $src_id}}) MATCH (dst {{id: $dst_id}}) " + f"WHERE NOT (src)-[:{label}]->(dst) " + f"CREATE (src)-[r:{label}]->(dst) SET r += $props" + )Keep the current unconditional
CREATEpath only as the fallback for servers that reject the guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py` around lines 220 - 239, The upsert_relations method currently always CREATEs edges and can produce duplicates; change it so it first attempts a guarded upsert using a pattern like MATCH (src {id: $src_id}) MATCH (dst {id: $dst_id}) WHERE NOT (src)-[r:Label]->(dst) CREATE (src)-[r:Label]->(dst) SET r += $props (use the computed label from _cypher_ident(rel.label) and same params: src_id, dst_id, props) and only fall back to the existing unconditional CREATE+SET if the guarded cypher is rejected by the server (catch the error from self._client.cypher and retry with the original CREATE statement). Ensure you preserve the props merging semantics (SET r += $props) and apply this change inside upsert_relations for each rel.
179-186:⚠️ Potential issue | 🟠 MajorReplace
type(rel)withrel.__type__in the ignore filter.Line 180 still uses
type(rel), which returns null on CoordiNode. This makes the ignore filter non-functional. The rest of the file has already migrated to__type__(see lines 137–142, 201–202); this location must be updated for consistency.Suggested fix
- ignore_clause = " AND NONE(rel IN r WHERE type(rel) IN $ignored_rels)" + ignore_clause = " AND NONE(rel IN r WHERE rel.__type__ IN $ignored_rels)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py` around lines 179 - 186, The ignore filter in the cypher construction uses type(rel) which is null on CoordiNode; update the ignore clause generation so it checks rel.__type__ instead of type(rel) (i.e., when building ignore_clause and params for the MATCH/WHERE block that references ignored_rels, use "NONE(rel IN r WHERE rel.__type__ IN $ignored_rels)") so the ignored list is applied consistently with other uses of __type__ (refer to the variables ignored, ignore_clause, params, and the cypher string that forms MATCH (n)-[r*1..{depth}]->(m)).
129-144:⚠️ Potential issue | 🟠 MajorEnforce
relation_namesrequirement or update the API contract.The
relation_namesparameter is optional inget_triplets(), but the implementation requires it: when omitted, the code silently emits a wildcard[r]pattern that returns no results on CoordiNode (as documented in the code comment on lines 137–139). This creates a silent failure: the method returns an empty list instead of raising an error or providing a fallback.The README.md example at line 121 demonstrates the broken path:
triplets = store.get_triplets(entity_names=["Alice"])This call will silently return no results because
relation_namesis not supplied.Either require
relation_namesat the signature level (raiseNotImplementedErrorif omitted, similar to the properties/ids check), or implement wildcard support in CoordiNode and update the type annotations accordingly. Do not leave the default path broken and undocumented at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py` around lines 129 - 144, The get_triplets() implementation currently treats relation_names as optional but uses a wildcard rel_pattern "[r]" which returns no results on CoordiNode; enforce the requirement by checking relation_names in get_triplets and raising a clear error (e.g., NotImplementedError or ValueError) when relation_names is None or empty, mirroring the existing properties/ids checks, or alternatively implement proper wildcard support; update the logic around rel_pattern and the surrounding comment so callers of get_triplets (and callers relying on the rel_pattern generation) receive a deterministic error instead of silent empty results.
🤖 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 105-110: The MERGE uses name = node.id but
props.setdefault("name", node.id) allows an incoming properties["name"] to
differ and overwrite the stored name during SET n += $props; update the code
around label, props, and the self._client.cypher call so the name used for MERGE
always wins — e.g., remove/override any "name" in props by assigning
props["name"] = node.id (or pop an incoming name) before calling
self._client.cypher (keeping MERGE (n:{label} {{name: $name}}) and
params={"name": node.id, "props": props} intact) so MATCHes by
rel.source.id/rel.target.id remain consistent.
- Around line 140-145: Replace the unstable fallback str(id(doc.source)) with a
deterministic stable ID: add a helper function _stable_document_id(source) that
computes a stable hash (e.g., SHA256) over the source's page_content plus its
sorted metadata items (canonicalized) and returns that hex string; in the block
inside include_source where you compute src_id (getattr(doc.source, "id", None)
or ...), call _stable_document_id(doc.source) instead of str(id(...)) and use
that src_id in the existing self._client.cypher MERGE call so re-ingests produce
the same __Document__ id.
---
Outside diff comments:
In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py`:
- Around line 220-239: The upsert_relations method currently always CREATEs
edges and can produce duplicates; change it so it first attempts a guarded
upsert using a pattern like MATCH (src {id: $src_id}) MATCH (dst {id: $dst_id})
WHERE NOT (src)-[r:Label]->(dst) CREATE (src)-[r:Label]->(dst) SET r += $props
(use the computed label from _cypher_ident(rel.label) and same params: src_id,
dst_id, props) and only fall back to the existing unconditional CREATE+SET if
the guarded cypher is rejected by the server (catch the error from
self._client.cypher and retry with the original CREATE statement). Ensure you
preserve the props merging semantics (SET r += $props) and apply this change
inside upsert_relations for each rel.
- Around line 179-186: The ignore filter in the cypher construction uses
type(rel) which is null on CoordiNode; update the ignore clause generation so it
checks rel.__type__ instead of type(rel) (i.e., when building ignore_clause and
params for the MATCH/WHERE block that references ignored_rels, use "NONE(rel IN
r WHERE rel.__type__ IN $ignored_rels)") so the ignored list is applied
consistently with other uses of __type__ (refer to the variables ignored,
ignore_clause, params, and the cypher string that forms MATCH
(n)-[r*1..{depth}]->(m)).
- Around line 129-144: The get_triplets() implementation currently treats
relation_names as optional but uses a wildcard rel_pattern "[r]" which returns
no results on CoordiNode; enforce the requirement by checking relation_names in
get_triplets and raising a clear error (e.g., NotImplementedError or ValueError)
when relation_names is None or empty, mirroring the existing properties/ids
checks, or alternatively implement proper wildcard support; update the logic
around rel_pattern and the surrounding comment so callers of get_triplets (and
callers relying on the rel_pattern generation) receive a deterministic error
instead of silent empty results.
🪄 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: eda65a4f-5d94-4ef6-91db-fd12e75b4873
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
langchain-coordinode/langchain_coordinode/graph.pyllama-index-coordinode/llama_index/graph_stores/coordinode/base.py
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 112-117: The idempotency check currently asserts result[0]["cnt"]
>= 1 which won't fail if duplicates are created; update the assertion to require
exactly one node to catch duplicates by changing the check on the graph.query
result (the query against label LCIdempotent using params {"name":
f"Idempotent-{unique_tag}"}) to assert result[0]["cnt"] == 1 so the test fails
if more than one node exists for that name.
- Around line 121-123: Remove the unused local variable by deleting the
assignment to schema_before; specifically, remove the line "schema_before =
graph.schema" (used after forcing a refresh with graph._schema = None) so no
unused variable remains, or if the previous schema value is actually needed
later, replace the unused variable with a direct use of graph.schema where
appropriate; target the statements around graph._schema and graph.schema in
tests/integration/adapters/test_langchain.py to resolve the Ruff F841 error.
- Around line 84-103: The test test_add_graph_documents_creates_relationship
currently only asserts the source node exists; update it to also verify the
Relationship of type "LC_RESEARCHES" was created by calling
graph.add_graph_documents([doc]) and then running a Cypher query that matches
the relationship between the two created nodes (e.g. MATCH (a:LCPerson2 {name:
$a_name})-[r:LC_RESEARCHES]->(b:LCConcept {name: $b_name}) RETURN r) using the
same f"Charlie-{unique_tag}" and f"GraphRAG-{unique_tag}" names as params, and
assert the query returns at least one result to confirm the relationship was
persisted.
In `@tests/integration/adapters/test_llama_index.py`:
- Around line 118-120: test_get_rel_map currently only checks type; strengthen
it by asserting the returned list is non-empty and contains expected
structure/content: after calling store.get_rel_map([src], depth=1, limit=10)
verify len(result) > 0, that every item is a dict (isinstance(item, dict)), and
that required keys exist (e.g., 'source' or 'relations' — match the actual keys
returned by get_rel_map). Also add an assertion that at least one relation
references the provided src (e.g., any(src in item.get('source', '') or src in
item.get('relations', []))). This will ensure the relation map is actually
produced and contains expected fields.
- Around line 10-16: The import ordering is incorrect (third-party import split)
causing Ruff I001; consolidate the separated import by moving "from
llama_index.graph_stores.coordinode import CoordinodePropertyGraphStore" into
the same third-party import block with "from llama_index.core.graph_stores.types
import EntityNode, Relation" so all llama_index imports are contiguous and
correctly ordered (EntityNode, Relation, CoordinodePropertyGraphStore) to
satisfy import sorting.
- Around line 63-71: The test test_upsert_nodes_idempotent currently only checks
len(found) >= 1 and doesn't ensure no duplicate nodes were created; change the
assertion to assert exactly one matching entity is present (e.g., assert
len(found) == 1) after calling store.upsert_nodes([node]) twice, keeping
references to the test function name and the calls to store.upsert_nodes and
store.get so you validate strict idempotency rather than just existence.
🪄 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: 9fb435e7-2e30-4d79-9421-4225f40ca8f0
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
llama-index-coordinode/llama_index/__init__.pyllama-index-coordinode/llama_index/graph_stores/__init__.pypyproject.tomltests/integration/adapters/__init__.pytests/integration/adapters/test_langchain.pytests/integration/adapters/test_llama_index.py
- Remove unused schema_before variable (F841) - Fix unsorted imports in test_llama_index.py (I001)
- props["name"] = node.id overwrites incoming properties["name"] so the MERGE predicate and stored name always match; prevents duplicate nodes and broken relationship MATCH on re-ingest - Add _stable_document_id(): SHA-256(page_content + metadata) makes include_source=True idempotent across Python processes test(adapters): tighten integration assertions for idempotency and rels - test_add_graph_documents_creates_relationship: assert LC_RESEARCHES edge exists, not just source node presence - test_add_graph_documents_idempotent: cnt == 1 to catch duplicates - test_upsert_nodes_idempotent: len == 1 to catch duplicates - test_get_rel_map: assert len >= 1 to verify non-empty result
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 124-140: The current broad except around self._client.cypher in
graph.py risks duplicating edges on transient errors; narrow the catch to the
specific client/syntax exception (e.g., the driver's SyntaxError/ClientError or
whatever the neo4j/neo4jrest client exposes) instead of Exception, log the
caught exception when falling back, and add a cached boolean flag on the Graph
instance (e.g., self._guard_supported) that is set to False only after a
confirmed unsupported-syntax error so the unguarded CREATE fallback is used only
once per session thereafter; reference the existing guarded call to
self._client.cypher, the fallback call, and the rel/source/target variables when
implementing these changes.
- Around line 87-169: The add_graph_documents method is overly complex and
should be refactored by extracting the repeated logic into helpers: implement
_upsert_node(node) to perform the MERGE/SET for a node (used in the nodes loop),
implement _create_relationship_idempotent(src_label, dst_label, rel_type,
src_id, dst_id, props) to contain the MATCH + guarded CREATE with the try/except
fallback (replace the relationship creation block), and implement
_link_document_to_entities(doc_id, nodes) to create the __Document__ MERGE and
the MENTIONS edges using the same guarded-CREATE-with-fallback logic; update
add_graph_documents to call these helpers (and clear
self._schema/self._structured_schema at the end) to reduce cognitive complexity
and remove duplicated try/except blocks.
- Around line 217-222: The _cypher_ident function uses an explicit character
class [A-Za-z0-9_] to validate identifiers; replace that with the more concise
\w in the regex (i.e., use r"^[A-Za-z_]\w*$") to simplify the pattern and keep
the existing "not starting with digit" check, and if you need ASCII-only
behavior add the re.ASCII flag to the re.match call to avoid matching Unicode
word characters. Ensure you update the re.match invocation inside _cypher_ident
accordingly and keep the backtick-escaping fallback unchanged.
In `@tests/integration/adapters/test_langchain.py`:
- Around line 22-25: The module-scoped fixture `graph` creates a shared
CoordinodeGraph instance (CoordinodeGraph) that allows state to leak between
tests like `test_query_count`; change the fixture to use scope="function" or add
explicit per-test cleanup in the `graph` fixture (e.g., clear/delete nodes and
relationships after each yield) so each test gets a fresh graph; locate the
`graph` fixture and update its decorator or add teardown logic, and ensure tests
that rely on `unique_tag` (and `test_query_count`) will only see their own data.
- Around line 1-8: Run the ruff formatter on the module to fix formatting CI
failures: execute the suggested command (uv run ruff format
tests/integration/adapters/test_langchain.py) or run ruff format against that
file, then stage and commit the resulting changes so the module docstring and
file formatting meet the linter requirements.
In `@tests/integration/adapters/test_llama_index.py`:
- Around line 1-8: This test module's formatting doesn't match ruff's
expectations; run the ruff formatter (e.g., uv run ruff format) on the
test_llama_index.py test module to auto-fix formatting, then re-run the tests/CI
and commit the updated file so the integration test file's top-level docstring
and formatting match the project's ruff rules.
🪄 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: 80f570ad-b716-477d-8a46-c4932010571a
📒 Files selected for processing (3)
langchain-coordinode/langchain_coordinode/graph.pytests/integration/adapters/test_langchain.pytests/integration/adapters/test_llama_index.py
There was a problem hiding this comment.
Pull request overview
Fixes CoordiNode (v0.4.3) OpenCypher compatibility issues in the LangChain and LlamaIndex adapters and adds/expands integration coverage to prevent regressions.
Changes:
- Implement LangChain
CoordinodeGraph.add_graph_documents()usingMERGEfor nodes andMATCH+CREATEfor edges, with compatibility fallback behavior. - Replace unsupported Cypher functions (
labels(n),type(r)) with CoordiNode-specific internal properties (__label__,__type__) in adapter queries. - Add integration tests for both adapters and update dev dependencies / lockfile accordingly.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
langchain-coordinode/langchain_coordinode/graph.py |
Implements add_graph_documents() and updates schema refresh relationship query to use __label__ / __type__. |
llama-index-coordinode/llama_index/graph_stores/coordinode/base.py |
Updates relationship type retrieval to __type__ and changes relation upsert to MATCH+CREATE. |
tests/integration/adapters/test_langchain.py |
Adds adapter-level integration coverage for LangChain (schema/query/add_graph_documents). |
tests/integration/adapters/test_llama_index.py |
Adds adapter-level integration coverage for LlamaIndex (schema/query/upsert/get_triplets/get_rel_map/delete). |
pyproject.toml |
Adds LangChain + LlamaIndex dev dependencies needed for integration tests. |
uv.lock |
Locks updated workspace version and dev dependencies. |
llama-index-coordinode/llama_index/__init__.py |
Switches to pkgutil-style namespace package path extension. |
llama-index-coordinode/llama_index/graph_stores/__init__.py |
Switches to pkgutil-style namespace package path extension. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
llama-index-coordinode/llama_index/graph_stores/coordinode/base.py
Outdated
Show resolved
Hide resolved
llama-index-coordinode/llama_index/graph_stores/coordinode/base.py
Outdated
Show resolved
Hide resolved
LangChain adapter:
- Remove WHERE NOT (pattern) guard — CoordiNode returns 0 rows silently
instead of raising, so the guarded CREATE never executed
- Remove try/except fallback (no longer needed)
- Skip SET r += $props when props is empty — SET r += {} unsupported
LlamaIndex adapter:
- upsert_relations: same fix — skip SET r += $props when props is empty
- get_rel_map: replace [r*1..N] variable-length path with single-hop [r];
variable-length paths don't serialize correctly in RETURN position
- Adjust ignore_rels filter from NONE(rel IN r ...) to r.__type__ NOT IN
(compatible with single-hop [r] pattern)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
llama-index-coordinode/llama_index/graph_stores/coordinode/base.py (1)
129-144:⚠️ Potential issue | 🟠 MajorRaise NotImplementedError when
relation_namesis omitted.The current code falls back to
MATCH (n)-[r]->(m)whenrelation_namesis None, but the inline comment explicitly states this pattern returns no rows on CoordiNode. This causes silent failure (empty results) on valid API usage. The README documentation examplestore.get_triplets(entity_names=["Alice"])would be affected, making this a real usability issue. Raise an error instead of silently returning empty results.Proposed fix
if relation_names: # Escape each type name to prevent Cypher injection rel_filter = "|".join(_cypher_ident(t) for t in relation_names) rel_pattern = f"[r:{rel_filter}]" else: - rel_pattern = "[r]" + raise NotImplementedError( + "Coordinode requires relation_names for get_triplets(); untyped [r] patterns return no rows" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py` around lines 129 - 144, The code currently falls back to a wildcard rel_pattern when relation_names is omitted which returns no rows on CoordiNode; instead detect when relation_names is falsy and raise NotImplementedError with a clear message guiding callers to provide relation_names (e.g., in the section that computes rel_pattern/rel_filter and constructs cypher): replace the wildcard branch that sets rel_pattern = "[r]" with logic that raises NotImplementedError("CoordiNode does not support wildcard relation_names; please supply relation_names") so callers get a hard failure rather than silent empty results; ensure the error is raised before building the cypher variable.
🤖 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 92-102: Update the add_graph_documents(...) docstring to
accurately state that while nodes are upserted by id using MERGE,
relationships/edges are not deduplicated and re-ingesting the same GraphDocument
will create duplicate edges (including regular relationships and MENTIONS edges)
due to CoordiNode's lack of MERGE support for edges; mention the existing
implementation detail/workaround that uses unconditional CREATE for
relationships and clarify this limitation so callers don't assume idempotent
edge behavior.
In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py`:
- Around line 182-185: The method that promises multi-hop traversal but
currently ignores the depth parameter must explicitly reject unsupported depths:
in the method that contains the depth parameter (referenced as depth and
documented as "Get relationship map for a set of nodes up to ``depth`` hops" —
e.g., the get_relationship_map implementation in the Coordinode graph store),
replace the silent ignore ("_ = depth") with a guard that raises
NotImplementedError when depth != 1 (allow depth == 1 to proceed with the
existing single-hop logic) and update the error message to state that multi-hop
traversal is not yet implemented.
In `@tests/integration/adapters/test_langchain.py`:
- Around line 107-119: Update the test_add_graph_documents_idempotent test to
include at least one relationship in the GraphDocument (e.g., add a Relationship
referencing the Node and another entity) so it exercises edge creation in
add_graph_documents, and after calling graph.add_graph_documents([doc]) twice
assert both node count and the specific edge count (for relationships like
LC_RESEARCHES or MENTIONS) remain 1; locate the test function
test_add_graph_documents_idempotent, the GraphDocument/Node construction, and
the relationships=[] placeholder and replace it with a non-empty relationships
list and an assertion on the relationship edge count returned by graph.query.
In `@tests/integration/adapters/test_llama_index.py`:
- Around line 112-122: The test test_get_rel_map only checks depth=1 so it won't
catch the adapter ignoring depth; update the test to cover a 2-hop case by
creating three nodes (e.g., src, mid, dst), upserting two relations (src->mid
and mid->dst), then call store.get_rel_map([src], depth=2, limit=10) and assert
the returned map includes the 2-hop dst (and has length >=2 or contains expected
relation entries); alternatively, if the current contract should forbid depth>1
until fixed, change the test to assert that store.get_rel_map(..., depth=2, ...)
raises the appropriate exception; reference test_get_rel_map and
store.get_rel_map when locating where to modify.
---
Outside diff comments:
In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py`:
- Around line 129-144: The code currently falls back to a wildcard rel_pattern
when relation_names is omitted which returns no rows on CoordiNode; instead
detect when relation_names is falsy and raise NotImplementedError with a clear
message guiding callers to provide relation_names (e.g., in the section that
computes rel_pattern/rel_filter and constructs cypher): replace the wildcard
branch that sets rel_pattern = "[r]" with logic that raises
NotImplementedError("CoordiNode does not support wildcard relation_names; please
supply relation_names") so callers get a hard failure rather than silent empty
results; ensure the error is raised before building the cypher variable.
🪄 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: 377e46a1-8ec6-4fa3-a914-00d439ed0aa3
📒 Files selected for processing (4)
langchain-coordinode/langchain_coordinode/graph.pyllama-index-coordinode/llama_index/graph_stores/coordinode/base.pytests/integration/adapters/test_langchain.pytests/integration/adapters/test_llama_index.py
llama-index-coordinode/llama_index/graph_stores/coordinode/base.py
Outdated
Show resolved
Hide resolved
…terns - get_rel_map: query schema edge types, build typed [r:T1|T2|...] pattern instead of wildcard [r] which returns no results in CoordiNode - get_rel_map: raise NotImplementedError when depth != 1 - get_triplets: raise NotImplementedError when relation_names is None - add_graph_documents docstring: clarify edges use unconditional CREATE - _cypher_ident: use \w with re.ASCII instead of [A-Za-z0-9_] - tests: count(*) instead of count(r) — CoordiNode returns 0 for rel vars - tests: add relationship to idempotency test, assert edge count >= 1 - tests: assert depth=2 raises NotImplementedError - tests: fix example port 17080 -> 7080 in docstrings
polaz
commented
Apr 9, 2026
Member
Author
|
@coderabbitai Fixed (outside-diff item): |
|
Tip For best results, initiate chat on the files or code changes.
I'll record this CoordiNode limitation for future reference. (( ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
llama-index-coordinode/llama_index/graph_stores/coordinode/base.py (1)
164-180:⚠️ Potential issue | 🟠 MajorMake the default
depthvalue supported.Line 167 still defaults
depthto2, but Lines 177-180 now reject every value except1. That meansget_rel_map(graph_nodes)raisesNotImplementedErroron the default code path.Suggested fix
- depth: int = 2, + depth: int = 1,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py` around lines 164 - 180, The get_rel_map method currently defaults depth to 2 but immediately raises NotImplementedError for any depth != 1; change the default to depth: int = 1 in the get_rel_map signature (and update the method docstring to reflect depth=1 is supported) so calling get_rel_map(graph_nodes) no longer triggers the NotImplementedError in CoordinodePropertyGraphStore.get_rel_map.
🤖 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 75-77: refresh_schema() currently uses a wildcard relationship
pattern in the cypher call (self._client.cypher with "MATCH (a)-[r]->(b) ...")
which returns no rows on CoordiNode; fix it by replacing the single-pattern
query with one that UNWINDs node relationships to enumerate actual relationship
objects: first MATCH (a) WHERE size(relationships(a))>0, UNWIND relationships(a)
AS r, then MATCH (a)-[r]->(b) RETURN DISTINCT a.__label__ AS src, r.__type__ AS
rel, b.__label__ AS dst so structured["relationships"] is populated; update the
cypher string used in refresh_schema() accordingly.
In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py`:
- Around line 234-256: The upsert_relations() implementation now always CREATEs
edges which causes duplicate relations on retries; change it to be idempotent by
using MERGE for the relationship when the server supports it (or by first
checking for existence and only CREATE-ing when missing) instead of
unconditional CREATE. Update upsert_relations() to attempt a MERGE-based cypher
(or a MATCH ... WHERE NOT (src)-[:LABEL]->(dst) ... CREATE fallback behind the
existing compatibility branch), and ensure callers like get_triplets() and
get_rel_map() see a single logical relation; reference upsert_relations(),
_cypher_ident(), and the self._client.cypher calls when making the change.
---
Outside diff comments:
In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py`:
- Around line 164-180: The get_rel_map method currently defaults depth to 2 but
immediately raises NotImplementedError for any depth != 1; change the default to
depth: int = 1 in the get_rel_map signature (and update the method docstring to
reflect depth=1 is supported) so calling get_rel_map(graph_nodes) no longer
triggers the NotImplementedError in CoordinodePropertyGraphStore.get_rel_map.
🪄 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: 5fa6b1c3-618d-4b85-bc5f-440f3cefe7cd
📒 Files selected for processing (4)
langchain-coordinode/langchain_coordinode/graph.pyllama-index-coordinode/llama_index/graph_stores/coordinode/base.pytests/integration/adapters/test_langchain.pytests/integration/adapters/test_llama_index.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…strings - refresh_schema: build typed [r:T1|T2|...] pattern from rel_props keys (CoordiNode wildcard [r] returns no results) - get_rel_map: change default depth from 2 to 1 (depth != 1 now raises) - _stable_document_id docstring: clarify only __Document__ node ID is stable; MENTIONS edges are not deduplicated (unconditional CREATE) - upsert_relations: add comment explaining WHERE NOT (pattern) returns 0 rows silently in CoordiNode, hence unconditional CREATE is used
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 93-169: The add_graph_documents method has high cognitive
complexity due to nested loops and conditionals; extract helpers to simplify it:
implement _upsert_node(self, node) to perform the MERGE + SET logic (use
_cypher_ident and ensure props["name"]=node.id), implement _create_edge(self,
src_label, dst_label, rel_type, src_id, dst_id, props) to run the MATCH...CREATE
pattern and handle empty props vs non-empty props exactly as current code does,
and implement _link_document_to_entities(self, doc, src_id) to MERGE the
__Document__ node and CREATE MENTIONS edges for each node; then refactor
add_graph_documents to call these three helpers while preserving include_source
behavior and the final invalidation of self._schema and self._structured_schema.
In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py`:
- Around line 374-393: The function _parse_edge_types_from_schema has high
cognitive complexity due to inline line-parsing logic; extract the logic that
determines section start and parses a single line into a small helper (e.g.,
_is_edge_section_start(line: str) and/or _parse_edge_list_item(line: str) that
returns an optional name) and replace the nested checks inside
_parse_edge_types_from_schema with calls to those helpers so the main loop only
toggles in_edges, breaks on empty lines, and appends non-empty names returned by
the helper(s).
🪄 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: bb2d0e2b-cae3-48c9-ae3a-dacffe9c8fdf
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
langchain-coordinode/langchain_coordinode/graph.pyllama-index-coordinode/llama_index/__init__.pyllama-index-coordinode/llama_index/graph_stores/__init__.pyllama-index-coordinode/llama_index/graph_stores/coordinode/base.pypyproject.tomltests/integration/adapters/__init__.pytests/integration/adapters/test_langchain.pytests/integration/adapters/test_llama_index.py
…xity - graph.py: extract _upsert_node(), _create_edge(), _link_document_to_entities() from add_graph_documents() — reduces cognitive complexity from 23 to ~8 - graph.py: _stable_document_id() now uses json.dumps(sort_keys=True) instead of string concat with "|"/"=" — fixes delimiter ambiguity for nested metadata - base.py: _parse_edge_types_from_schema() refactored to use iter() with two for-loops instead of boolean flag — reduces complexity from 16 to 14 - base.py: get_triplets() docstring documents that relation_names is required
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 212-232: _stable_document_id currently calls json.dumps on
arbitrary metadata which raises TypeError for non-JSON-serializable values;
update _stable_document_id to first recursively normalize the metadata and
content into JSON-safe primitives (e.g., convert datetime/date to ISO format,
UUID and Path to str, bytes to base64 or str, set to list, Decimal to str, and
fall back to str() for unknown objects) before calling json.dumps with
sort_keys/separators/ensure_ascii=False, then compute the sha256 hex and return
the 32-char prefix as before; ensure you only change _stable_document_id and
preserve the canonical JSON options so behavior remains deterministic.
In `@llama-index-coordinode/llama_index/graph_stores/coordinode/base.py`:
- Around line 204-212: The Cypher query interpolates the user-provided limit
directly into the f-string (see the cypher variable built after rel_filter,
node_ids and params), which can be abused if non-integer input is passed to the
public get_rel_map() API; coerce and validate limit to an int before
constructing cypher (e.g., call int(limit) and ensure it's non-negative, raising
a TypeError/ValueError on failure) so the formatted query always contains a safe
integer and bad inputs fail closed.
🪄 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: 56cca4bb-3af0-428b-badf-f60baf3ec2ac
📒 Files selected for processing (2)
langchain-coordinode/langchain_coordinode/graph.pyllama-index-coordinode/llama_index/graph_stores/coordinode/base.py
llama-index-coordinode/llama_index/graph_stores/coordinode/base.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 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.
- refresh_schema(): remove broad except Exception — transport/query errors now propagate instead of silently producing an empty relationships list - _stable_document_id(): add default=str to json.dumps to handle non-JSON- serializable metadata values (datetime, UUID, Path, custom objects) - get_rel_map(): coerce limit to int() before Cypher f-string interpolation to prevent injection via non-integer caller input
|
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
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
E2E testing against v0.4.3 revealed four compatibility bugs. All verified with 15/15 e2e tests passing after fix.
langchain-coordinode
add_graph_documents()— was inherited as no-oppassfromGraphStore; now upserts nodes viaMERGEand creates edges viaMATCH+CREATEwith fallback for servers that don't supportWHERE NOTpattern guardsrefresh_schema()relationship query —labels(n)andtype(r)returnnullin CoordiNode; replaced withn.__label__andr.__type__llama-index-graph-stores-coordinode
upsert_relations()—MERGE (src)-[r:TYPE]->(dst)fails with "MERGE create from non-NodeScan pattern"; replaced withMATCH (src) MATCH (dst) CREATE (src)-[r]->(dst)get_triplets()/get_rel_map()—type(r)returnsnull; replaced withr.__type__Test Plan
ruff check .— all checks passedCloses #14