fix(search): apply property_filters and validate group_id on write path

[danielchalef]

Summary

Fixes two related correctness/security issues in search-filter and partition-key (group_id) handling.

1. SearchFilters.property_filters was silently ignored.
The field was declared on SearchFilters but never read by node_search_filter_query_constructor or edge_search_filter_query_constructor, so searches always executed without the intended property filter and returned data callers believed was restricted. Both constructors now build parameterized Cypher fragments from property_filters via a shared property_filter_query_constructor helper (n.<prop>/e.<prop> with the value passed as a query parameter; IS NULL/IS NOT NULL emit no parameter). Because Cypher property keys cannot be parameterized, property_name is validated against a safe-identifier pattern (validate_property_name) both at the Pydantic layer and defensively inside the constructor, preventing query injection.

2. Write/read asymmetry on group_id.
The write path accepted arbitrary group_id values while the read/delete path rejects anything outside [a-zA-Z0-9_-], allowing records to be written that are permanently unreachable and undeletable via any API path. group_id is now validated on the core Node and Edge models and on the AddMessagesRequest / AddEntityNodeRequest server DTOs. GroupIdValidationError now also subclasses ValueError so Pydantic surfaces it as a normal validation error (HTTP 422) instead of a 500.

Type of Change

• Bug fix
• New feature
• Performance improvement
• Documentation/Tests

Objective

Restore the documented behavior of property_filters (operators rely on it to scope results) and close the write-path gap that let unreachable/undeletable records be persisted.

Testing

• Unit tests added/updated
• Integration tests added/updated
• All existing tests pass

New regression tests in tests/utils/search/test_search_security.py cover:

• node & edge property-filter query construction
• IS NULL/IS NOT NULL operators emitting no params
• rejection of unsafe property names (Pydantic + constructor bypass via model_construct)
• write-path group_id rejection on EntityNode / EntityEdge and acceptance of valid/default values

Existing search-security, node, edge, and attribute unit tests pass; make format, ruff check, and pyright are clean on the changed files.

Breaking Changes

• This PR contains breaking changes

Behavioral note (not a code-contract break): callers that were previously writing group_id values containing characters outside [a-zA-Z0-9_-] will now receive a validation error at write time instead of silently creating unreachable records. This aligns the write path with the already-enforced read/delete path.

Checklist

• Code follows project style guidelines (make lint passes)
• Self-review completed
• Documentation updated where necessary
• No secrets or sensitive information committed

Related Issues

N/A

🤖 Generated with Claude Code

---

Generated by Claude Code

[zep-cla-assistant]

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. For privacy information, see our Privacy Notice. You can sign the CLA by just posting a Pull Request Comment same as the below format.

---

I have read the CLA Document and I hereby sign the CLA behalf on myself, e-mail: example@example.com

or

I have read the CLA Document and I hereby sign the CLA behalf of my company, e-mail: example@example.com

Signature is valid for 6 months.

---

_{This bot will be retriggered when the Contributor License Agreement comment has been provided. Posted by the CLA Assistant Lite bot.}

[github-actions]

Claude finished @danielchalef's task in 3m 21s —— View job

---

PR Review: apply property_filters and validate group_id on write path

I reviewed the diff against origin/main plus the surrounding call sites (all driver search_ops, search_utils, hydration helpers, and add_episode). Overall this is a well-scoped, correct fix with good test coverage. One behavioral concern worth confirming before merge.

Findings

⚠️ Warning — group_id validation also fires on DB hydration (medium confidence)
nodes.py:108 / edges.py:56 — The new group_id field validator runs on every model construction, including deserialization from the DB (get_entity_node_from_record at nodes.py:1074 builds EntityNode(group_id=...) directly from the stored value). Since add_triplet, bulk ops, direct construction, and the pre-fix server DTO never validated group_id, a deployment can already hold records whose group_id violates [a-zA-Z0-9_-]. After this change a read that hydrates such a record raises ValidationError — reachable via cross-partition search (group_ids=None), where the query returns nodes/edges of any partition and then constructs them. The PR frames these records as "unreachable," but that only holds for group_id-filtered paths. Consider validating at write boundaries only (DTOs / save() / add_triplet) or hydrating via model_construct, or at least note this in the migration guidance. See inline comment.

💡 Suggestion — test coverage gaps (low)

• No test that the server DTOs (AddMessagesRequest / AddEntityNodeRequest) reject a bad group_id (the DTO validators are new code).
• is_not_null operator isn't exercised (only is_null).
• No coverage for the shared-filter semantics (a single property_filters list is applied as n.<prop> in node searches and e.<prop> in edge searches) — worth a line documenting this is intentional.

💡 Suggestion — dead helper (low, pre-existing)
cypher_to_opensearch_operator in search_filters.py has no callers, and property_filters is now wired only into the two Cypher constructors. That's fine — there's no separate OpenSearch filter path today — but confirm no backend (OpenSearch/aoss) silently bypasses these constructors, or property_filters would remain ignored there.

What I verified looks correct

• No parameter-name collisions: node_prop_* / edge_prop_* don't clash with $group_ids, $labels, $edge_types, $edge_uuids, or $valid_at_*/etc. Filter fragments are joined with AND in every driver.
• Injection safety: property values are always parameterized; property names are validated against SAFE_CYPHER_IDENTIFIER_PATTERN both at the Pydantic layer and defensively inside property_filter_query_constructor (covers the model_construct bypass, which is tested).
• IS NULL/IS NOT NULL correctly emit no parameter.
• GroupIdValidationError(GraphitiError, ValueError) — MRO is clean (matches existing NodeLabelValidationError), and this correctly makes Pydantic surface it as a 422 rather than a 500.
• Default group_ids ('' for Neo4j, '_' for FalkorDB) both pass validate_group_id, so the defaults aren't rejected.

Nice work overall — the core injection/asymmetry fixes are sound. The main thing to resolve is whether hydration of legacy group_id values should be able to hard-fail reads.

[github-actions]

Warning (medium confidence): This validator runs on every Node/EntityNode construction, including DB hydration — get_entity_node_from_record (nodes.py:1074) builds an EntityNode directly from the stored group_id. The equivalent edge path does the same.

The write path via add_episode already validated group_id (graphiti.py:1078), but add_triplet, direct model construction, bulk ops, and the server's AddEntityNodeRequest did not — so a deployment can hold legacy records whose group_id violates [a-zA-Z0-9_-]. After this change, any read that hydrates such a record will raise ValidationError, not just writes. This is reachable: fulltext/similarity searches only filter by group_id when group_ids is not None, so a cross-partition search (group_ids=None) returns and then tries to construct nodes of any group_id.

Net effect: the PR closes the write gap but converts previously "unreachable" legacy records into a hard read failure for cross-partition queries. Consider validating only at write boundaries (save()/DTOs/add_triplet) or using model_construct on the hydration path so deserialization of existing data can't break. At minimum, worth calling out in the migration note.

[danielchalef]

Thanks for the review — addressed in d38e1e9.

Hydration hard-fail (main finding): moved group_id validation off the Node/Edge field validators (which fired on every construction, including get_*_from_record hydration) to the write boundary, so reads of legacy records with non-conforming group_ids are tolerant again:

• Validate in EntityNode.save() and EntityEdge.save().
• Validate every element in add_nodes_and_edges_bulk_tx (the bulk path bypasses save(); this also covers add_triplet, which persists through it).
• add_episode/add_episode_bulk already validate at entry.
• Server DTOs now re-raise as ValueError so they surface a 422 regardless of whether the installed graphiti-core's GroupIdValidationError subclasses ValueError (the server depends on the published core, where it doesn't yet).

Suggestions:

• Added server DTO rejection/acceptance tests (server/tests/test_dto_validation.py), an is_not_null case, and a shared node/edge filter-semantics test; documented that a single property_filters list is intentionally applied to n (node search) and e (edge search).
• cypher_to_opensearch_operator is indeed dead and pre-existing; confirmed there's no OpenSearch/aoss driver — every search backend goes through the two Cypher constructors, so property_filters isn't silently bypassed. Left the dead helper alone to keep this diff scoped.

---

Generated by Claude Code

[github-actions]

Claude finished @danielchalef's task in 1m 46s —— View job

---

PR Review — apply property_filters and validate group_id on write path

I re-reviewed the latest state (commit d38e1e9), focused on whether the prior round's findings were resolved and whether the new write-boundary approach has gaps.

Prior findings — all resolved ✅

• Hydration hard-fail (main finding): Confirmed the group_id field validators were removed from Node/Edge (net diff has none), so get_entity_node_from_record and cross-partition (group_ids=None) reads no longer hard-fail on legacy records. Validation now lives at the write boundary: EntityNode.save() (nodes.py:546), EntityEdge.save() (edges.py:335), and add_nodes_and_edges_bulk_tx (bulk_utils.py:163). I traced add_triplet → add_nodes_and_edges_bulk (graphiti.py:1762) → add_nodes_and_edges_bulk_tx, so add_triplet is genuinely covered as claimed. New test_node_construction_is_tolerant_of_any_group_id locks in the tolerant-hydration behavior.
• Server DTO re-raise as ValueError: Correct — _validate_request_group_id wraps GroupIdValidationError in ValueError, so a 422 surfaces regardless of the installed core's class hierarchy. Covered by server/tests/test_dto_validation.py.
• Test-coverage gaps: All closed — DTO reject/accept, is_not_null, and the shared node/edge filter-semantics test are present, plus the intentional-sharing comment on property_filters.
• Dead helper: cypher_to_opensearch_operator left alone; confirmed no OpenSearch/aoss driver exists, so property_filters isn't silently bypassed.

What I verified this round

• Injection safety: SAFE_CYPHER_IDENTIFIER_PATTERN is ^[A-Za-z_][A-Za-z0-9_]*$ — fully anchored, so .match() rejects the group_id\) DETACH DELETE n //payload. Names validated both at the Pydantic layer and defensively insideproperty_filter_query_constructor(covers themodel_constructbypass). Values always parameterized.IS NULL/IS NOT NULL` emit no param.
• No param-name collision: node_prop_* / edge_prop_* don't clash with existing $group_ids/$edge_uuids/etc.
• GroupIdValidationError(GraphitiError, ValueError) MRO is clean and consistent with NodeLabelValidationError / the new PropertyNameValidationError.

Findings

💡 Suggestion — write-boundary validation only covers EntityNode/EntityEdge saves (low confidence, minor)
Only EntityNode.save() and EntityEdge.save() validate group_id. The other save() overrides — EpisodicNode.save() (nodes.py:334), CommunityNode.save() (nodes.py:696), SagaNode.save(), EpisodicEdge.save() (edges.py:144), CommunityEdge.save() (edges.py:581) — do not. In practice these are reached through validated entry points (add_episode validates at entry; the bulk path validates in add_nodes_and_edges_bulk_tx; community group_ids derive from already-validated member nodes), so I don't see a live gap today. But a direct EpisodicNode(...).save() / CommunityNode(...).save() call with a non-conforming group_id would still slip through, which is a latent version of the same write/read asymmetry this PR closes. Not blocking — worth either a one-line note that only entity writes are guarded at the model boundary, or lifting the validate_group_id(self.group_id) call into a shared helper the other saves also call.

Verdict

The core fixes (silent property_filters, injection hardening, write/read group_id asymmetry) are correct and now well-tested, and the previously-flagged hydration regression is properly resolved by moving validation to the write boundary. The one remaining item is a low-severity completeness note, not a correctness bug. Nice iteration.

[github-actions]

Claude finished @danielchalef's task in 1m 14s —— View job

---

PR Review — apply property_filters and validate group_id on write path

Re-reviewed the latest state (commit 7e95161), focused on the new shared-helper change that guards group_id on all save() overrides — the one open item from the prior round.

• Read full diff at latest commit
• Verify the shared group_id guard covers all save() overrides
• Check bulk path + hydration tolerance still intact
• Post review findings

Prior finding — resolved ✅

The last round's low-severity completeness note (only EntityNode/EntityEdge saves validated group_id) is now fixed the way suggested: a shared _validate_for_write() helper on both Node (nodes.py:108) and Edge (edges.py:56) base classes calls validate_group_id(self.group_id), and every concrete save() override invokes it as its first statement. I confirmed the full set is covered:

• Nodes: EpisodicNode (nodes.py:340), EntityNode (554), CommunityNode (701), SagaNode (890) — 4 concrete + abstract.
• Edges: EpisodicEdge (edges.py:150), EntityEdge (343), CommunityEdge (586), HasEpisodeEdge (702), NextEpisodeEdge (837) — 5 concrete + abstract.
• No save_bulk methods exist that would bypass this; the bulk path is still separately guarded in add_nodes_and_edges_bulk_tx (bulk_utils.py:162).
• The _validate_for_write() call sits before the if driver.graph_operations_interface: branch in each override, so both the interface and native-Cypher persistence paths are covered.
• New test test_non_entity_node_also_rejects_unsafe_group_id_on_save locks in the EpisodicNode case.

What I verified this round

• Hydration tolerance intact: validators still live only at the write boundary (no field validator on Node/Edge), so get_*_from_record and cross-partition (group_ids=None) reads of legacy records don't hard-fail. test_node_construction_is_tolerant_of_any_group_id confirms.
• Default group_ids still accepted: validate_group_id short-circuits on empty string (helpers.py:155) and '_' matches ^[a-zA-Z0-9_-]+$, so both the Neo4j ('') and FalkorDB ('_') defaults pass — community group_ids (derived from validated members) won't trip the new CommunityNode/CommunityEdge guards.
• Injection safety unchanged & correct: property_filter_query_constructor parameterizes all values and validates names via the fully-anchored SAFE_CYPHER_IDENTIFIER_PATTERN (helpers.py:39) both at the Pydantic layer and defensively in the constructor; IS NULL/IS NOT NULL emit no param; node_prop_*/edge_prop_* don't collide with existing params.
• Error hierarchy: GroupIdValidationError(GraphitiError, ValueError) and the new PropertyNameValidationError(GraphitiError, ValueError) have clean MROs consistent with NodeLabelValidationError; server DTOs additionally re-wrap as ValueError so a 422 surfaces regardless of the installed core's class hierarchy.

Findings

No new issues. The remaining item from the previous review is resolved, and I found no regressions introduced by the shared-helper refactor.

Verdict

All three original fixes (silently-ignored property_filters, Cypher injection hardening, and the write/read group_id asymmetry) are correct and well-tested, the hydration regression from round one stays resolved, and the write-boundary guard is now complete across every save() path. LGTM.

[danielchalef]

Addressed the remaining completeness note in 7e95161: lifted the write-boundary check into Node._validate_for_write() / Edge._validate_for_write() and call it from every concrete save() override (episodic/community/saga nodes, episodic/community/has-episode/next-episode edges), so a direct EpisodicNode(...).save() / CommunityNode(...).save() with a non-conforming group_id no longer slips through. Hydration still constructs models directly and stays tolerant. Added a regression test that a non-entity save (EpisodicNode) also rejects an unsafe group_id.

---

Generated by Claude Code

[Naseem77]

@danielchalef Verified this: property_filters passed into search were being silently ignored, and invalid group_ids were accepted on write. With your patch the filters generate real clauses and bad group_ids get rejected with a clear error. The 20 validation tests pass.