Skip to content

fix(memory): keep Neo4j relationship types to a single identifier - #4837

Open
chopratejas wants to merge 7 commits into
mainfrom
fix/neo4j-relationship-type
Open

chopratejas wants to merge 7 commits into
mainfrom
fix/neo4j-relationship-type

Conversation

@chopratejas

@chopratejas chopratejas commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Description

DirectMem0Adapter._write_graph_to_neo4j builds its MERGE (source)-[r:<type>]->(dest) statement by concatenating the relationship label from memory_save (extracted_relationships[].relationship). Neo4j cannot bind a relationship type as a parameter, and the label was only lowercased with spaces replaced, so brackets, backticks or newlines in a label became part of the query text.

The label is now normalised to a single identifier: each non-word character becomes _ one for one (the same mapping spaces already had, so labels that were valid identifiers keep their stored type), and the result is emitted backtick-quoted. Nothing is stripped or truncated. An empty label is skipped with a warning. This was the only place in the memory backends where a value is interpolated into Cypher (node labels are constants; names, types and embeddings are already bound parameters).

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • headroom/memory/backends/direct_mem0.py: add _relationship_type() and use it when normalising relationships; quote the type in the MERGE.
  • tests/test_memory/test_direct_mem0.py: tests for the generated relationship type (plain, hyphenated, punctuated, leading/doubled underscores, long) and for skipping an empty one.

Testing

  • Unit tests pass (pytest)
  • Linting passes (ruff check .)
  • Type checking passes (mypy headroom)
  • New tests added for new functionality
  • Manual testing performed

Test Output

# new tests on origin/main (fix stashed)
$ pytest -q tests/test_memory/test_direct_mem0.py
FAILED ...test_write_graph_relationship_type_is_a_single_identifier[Works At-`works_at`]
FAILED ...test_write_graph_relationship_type_is_a_single_identifier[is-a-`is_a`]
FAILED ...test_write_graph_relationship_type_is_a_single_identifier[reports to (since 2020)-...]
FAILED ...test_write_graph_relationship_type_is_a_single_identifier[a`b]->(c)\nd-`a_b_c_d`]
FAILED ...test_write_graph_relationship_type_is_a_single_identifier[aaaa...]
FAILED ...test_write_graph_skips_relationship_with_no_usable_type
=================== 6 failed, 22 passed, 1 skipped ===================

# with the fix
$ pytest -q tests/test_memory/
=========== 720 passed, 2 skipped, 1681 warnings in 68.43s ===========

$ ruff check / ruff format --check (0.17.3) on changed files
All checks passed!
2 files already formatted
$ mypy headroom/memory/backends/direct_mem0.py --ignore-missing-imports
Success: no issues found in 1 source file

Real Behavior Proof

  • Environment: macOS, Python 3.12 venv, origin/main at c21b081 vs this branch; Neo4j driver replaced by a MagicMock session to capture the Cypher text sent.
  • Exact command / steps: call _write_graph_to_neo4j([], [{"source": "user", "relationship": raw, "destination": "x"}], "u1") for raw in "works at" and "a`b]->(c)\nd", print the MERGE (source)-[r:... line(s).
  • Observed result:
    • main: MERGE (source)-[r:works_at]->(dest); the punctuated label splits the statement into MERGE (source)-[r:ab]->(c)/d]->(dest)`.
    • this branch: MERGE (source)-[r:`works_at`]->(dest) and MERGE (source)-[r:`a_b_c_d`]->(dest).
  • Not tested: against a live Neo4j server.

Runtime Rollout Safety

  • Rollout-managed feature(s): none
  • Minimum rollout channel: n/a
  • Stable/default behavior changed: only labels containing characters other than letters, digits, _ and spaces are stored differently (those previously produced invalid Cypher). Plain labels such as works at keep the same stored type works_at.
  • Kill switch / disable path: n/a
  • Unsafe override required: no
  • Qualification impact: none
  • Rollback path: revert this PR

Review Readiness

  • I have performed a self-review
  • This PR is ready for human review

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I did not edit CHANGELOG.md

Additional Notes

No docs change needed. Fix adapted from a proposal by HetCreep (this version normalises instead of dropping labels such as is-a; later commits follow review feedback (no cap, one-for-one mapping)).

🤖 Generated with Claude Code

Reported by @JoaoMarcos44. Thank you for your contribution! 🩵

The direct Mem0 adapter wrote the relationship label from memory_save
into the Cypher text after only lowercasing and replacing spaces, so a
label with brackets, backticks or newlines changed the query itself.
Non-word characters now collapse to "_", the result is capped at 64
characters and backtick-quoted, and a label with nothing left is skipped.

Assay-Kind: bugfix
Assay-Ask: The memory_save relationship label was concatenated into Neo4j Cypher with only space
 normalisation, so punctuation in it altered the query.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chopratejas chopratejas added bug Something isn't working size: small At most 100 added/deleted lines. risk: low Scope estimate: documentation/tests-only change; verification still required. area: memory Touches memory files; triage routing, not a CODEOWNERS assignment. labels Nov 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR governance

This PR does not yet satisfy the required template fields:

  • Fill in Real Behavior Proof → Observed result.

Please update the PR body, or move the PR back to draft while it is still in progress.

@github-actions github-actions Bot added status: needs author action Pull request body or readiness checklist still needs author updates status: ci failing Required or reported CI checks are failing labels Nov 13, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The two existing compatibility findings remain unresolved. Saving relationship labels "a" * 64 + "x" and "a" * 64 + "y" between the same nodes should preserve distinct edge types; the new truncation generates the same type for both. Also, _works_at, __, and foo bar previously mapped to _works_at, __, and foo__bar; they now map to works_at, no write, and foo_bar. A recording-session reproduction against the base and this head confirms these generated-query differences. Please preserve valid existing identifiers and distinct long types, and add regression tests while retaining the injection protection. Live Neo4j behavior remains unverified.

… cap


Assay-Kind: bugfix
Assay-Ask: A relationship label cut at 64 characters could end in an underscore.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot removed the status: ci failing Required or reported CI checks are failing label Nov 13, 2026
@codecov-commenter

codecov-commenter commented Nov 13, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

… a cap


Collapsing runs, stripping underscores and capping at 64 characters
changed the stored type of labels that were already valid identifiers
(`_works_at`, `__`, `foo  bar`) and could merge two long labels into
one type. Each non-word character now maps to "_" one for one, which
matches the old space mapping, and nothing is stripped or truncated.
Only an empty label is skipped.

Assay-Kind: bugfix
Assay-Ask: The relationship-type normalisation changed already-valid labels and could merge
 distinct long labels into one edge type.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Picks up the dependency updates from #4835 so the audit checks pass.

Assay-Kind: deps
Assay-Ask: The dependency audit checks failed on this PR because the branch predates the dependency
 updates on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The updated source addresses both prior compatibility findings. A fresh isolated graph-writer check covers _works_at, __, foo bar, two labels differing after character 64, an injection-shaped label and an empty label: legacy types are preserved, long types stay distinct, unsafe punctuation stays inside one quoted identifier, and empty labels are skipped. This is query-generation proof, not a live Neo4j write/read. The current CI run fails only commitlint (run 43518907064, job 132207874190); please provide its error annotations or job excerpt and resolve or establish inheritance of that failure before final qualification. No new source defect was found.

@chopratejas

Copy link
Copy Markdown
Collaborator Author

Thanks Jerrett.

Source: agreed, no change needed. 055f289 (pushed before your review) already handles the length-64 collision and the existing-label cases you re-checked.

commitlint: the only failing commit is the branch's chore: merge main into ... commit (796a922f). Its body mentions a PR number, so the conventional-commits parser treats the trailer block as a footer, and the Assay-Ask: line there is over 100 characters (footer-max-line-length). Job log:

⧗   input: chore: merge main into fix/neo4j-relationship-type
✖   footer's lines must not be longer than 100 characters [footer-max-line-length]
✖   found 1 problems, 0 warnings

Every other commit on the branch passes the repo's .commitlintrc.json when checked locally (@commitlint/cli@19 + config-conventional@19):

f56cdab1 fix(memory): keep Neo4j relationship types to a single identifier => ok
5ece393e fix(memory): drop a trailing underscore left by the relationship-type cap => ok
055f289d fix(memory): map relationship labels one character at a time, without a cap => ok
796a922f chore: merge main into fix/neo4j-relationship-type => footer-max-line-length

This can't be cleared without rewriting branch history. The PR's own changes don't cause it, and a squash merge replaces that commit. If you prefer a clean check, I can reword that one commit with a force-push, but only with a maintainer's OK.

JerrettDavis added a commit that referenced this pull request Nov 14, 2026
@github-actions github-actions Bot added the status: ci failing Required or reported CI checks are failing label Nov 14, 2026

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The staged current-head commitlint log now establishes the outstanding cause: commit 796a922 has a 116-character Assay-Ask footer, exceeding the 100-character limit (run 43547244172, job 132301309078). That commit is absent from recorded main, so this is not an inherited main failure. Please reword that footer, for example Assay-Ask: Refresh dependencies from main to clear the audit checks., and obtain a passing commitlint check. A future squash does not clear the current check. The relationship-type source changes address the prior compatibility findings; live Neo4j write/read remains unverified.

@github-actions github-actions Bot removed the status: ci failing Required or reported CI checks are failing label Nov 14, 2026
@chopratejas

Copy link
Copy Markdown
Collaborator Author

Jerrett, the commitlint blocker is cleared. The footer-length rule fix (#4868) is merged into main, and I merged main into this branch, so commitlint now passes on this head. No code changes in this update.

@JerrettDavis
JerrettDavis force-pushed the fix/neo4j-relationship-type branch from 3b58c08 to 08ffe77 Compare November 14, 2026 05:21
@github-actions github-actions Bot added status: needs rebase Pull request branch is behind the base branch on files it also changes status: has conflicts Pull request has merge conflicts with the base branch labels Nov 14, 2026
@JerrettDavis

Copy link
Copy Markdown
Collaborator

Applied the requested mechanical fix: rewrote the PR-range commits to wrap the oversized Assay-Ask footer(s) to under the 100-character commitlint limit (content preserved, wrapped at word boundaries). Branch force-pushed; commitlint and CI are re-running at the new head.

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please integrate current main while retaining both test groups in tests/test_memory/test_direct_mem0.py. A merge currently conflicts where the relationship tests and main’s cross-user ownership test were appended. The prepared mechanical merge retains both; isolated checks pass eight relationship scenarios and reject cross-user mutations. The relationship fix addresses the prior compatibility findings. Current-head test CI is not staged, and live Neo4j write/read remains unverified.

Both sides appended tests at the end of tests/test_memory/test_direct_mem0.py;
kept the relationship-type tests and main's cross-user ownership test.

Assay-Kind: tests
Assay-Ask: The branch conflicted with main in the direct Mem0 adapter test file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chopratejas

Copy link
Copy Markdown
Collaborator Author

Thanks Jerrett. The conflict is resolved in 7b1f3f5 (Merge origin/main into fix/neo4j-relationship-type). Both sides had appended tests to the end of tests/test_memory/test_direct_mem0.py. I kept both groups unchanged: the relationship-type tests first, then main's test_update_and_delete_leave_another_users_memory_alone. No source file changed in the merge.

$ pytest -q tests/test_memory/test_direct_mem0.py -k "relationship or another_users"
10 passed, 23 deselected
$ pytest -q tests/test_memory
729 passed, 2 skipped
$ ruff check / ruff format --check (0.17.3) tests/test_memory/test_direct_mem0.py headroom/memory/backends/direct_mem0.py
All checks passed!
2 files already formatted

Test shard 3 currently also fails on main in two test_memory_tools_only_when_injected cases, which a separate change is fixing. If they show up here, they don't come from this PR.

@github-actions github-actions Bot removed status: needs rebase Pull request branch is behind the base branch on files it also changes status: has conflicts Pull request has merge conflicts with the base branch labels Nov 14, 2026

This branch was successfully deployed

1 active deployment
Preview — 7b1f3f55 Deployed Nov 14, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: memory Touches memory files; triage routing, not a CODEOWNERS assignment. bug Something isn't working risk: low Scope estimate: documentation/tests-only change; verification still required. size: small At most 100 added/deleted lines. status: code changes requested status: needs author action Pull request body or readiness checklist still needs author updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants