CLA-87: Fix Neo4j info field deserialization (JSON string → dict) + gateway round-trip test - #2
CLA-87: Fix Neo4j info field deserialization (JSON string → dict) + gateway round-trip test#20xjackyang wants to merge 7 commits into
Conversation
Bug fixes in neo4j_community.py: 1. _parse_node: sources[idx][0] == "}" → sources[idx][-1] == "}" (was always False, preventing json.loads deserialization of sources) 2. _parse_nodes: same fix in batch retrieval path 3. add_node: removed double-serialization of sources (_prepare_node_metadata already serializes sources; doing it twice produces nested JSON strings that can never be decoded correctly) Also adds .env config (not committed — gitignored): - ENABLE_CHAT_API=true (fixes 503 on /product/chat/* endpoints) - CHAT_MODEL_LIST: OpenRouter claude-sonnet-4-6 wired as chat LLM Integration test (tests/test_cla81_integration.py): - TestCrossSessionRecall: write in session_a → recall in session_b (7 tests) - TestChatHandler: chat/complete not 503, responds - TestNeo4jSourcesDeserialization: sources round-trip as list not string - TestServiceHealth: search + scheduler reachable 7/7 tests passing
AC1: Chat handler 503 — already resolved; ENABLE_CHAT_API=true and
CHAT_MODEL_LIST properly configured in .env. Verified: chat/complete
returns memories without explicit model param.
AC2: Fix Neo4j CypherTypeError: Map{} (two root causes)
Bug 1 — neo4j.py _prepare_node_metadata:
'metadata["sources"]' -> KeyError when sources=None (excluded by
model_dump exclude_none=True), silently dropping ALL nodes.
Fix: use metadata.get('sources') and guard isinstance(item, dict)
to avoid double-serializing already-converted strings.
Bug 2 — manager.py _add_memories_batch:
model_dump() produces nested dicts for fields like 'history'
(list[ArchivedTextualMemory]) which Neo4j cannot store as node
properties (Map{} error).
Fix: new _sanitize_neo4j_metadata() helper — JSON-stringifies any
dict value or list-of-dicts value. Also ensures 'sources' key exists
so downstream _prepare_node_metadata never KeyErrors.
Applied to both working_metadata and metadata_dict paths.
Verified:
- /product/add -> memory_id 9a3f4a24 stored in both Qdrant AND Neo4j
- /product/search returns MANGO-TANGO as top result
- /product/chat/complete recalls passphrase correctly
launchd PATH does not include /usr/sbin, so 'lsof' silently fails and the pre-start port kill never runs. Result: [Errno 48] address already in use on every LaunchAgent restart loop. Fix: use full path /usr/sbin/lsof in all lsof calls.
CLA-81 added _sanitize_neo4j_metadata() which JSON-stringifies dicts
(including info) before Neo4j write. On the read path, Pydantic expects
metadata.info as a dict but receives a str → validation fails →
memory silently dropped from search results.
Fix: add field_validator('info', mode='before') to TextualMemoryMetadata
that parses JSON strings back to dicts. Handles both dict (cloud/new data)
and str (Neo4j-sanitized existing data) transparently.
Verified:
- AC1: _parse_info validator in TextualMemoryMetadata
- AC2: existing memories with stringified info recalled (info type: dict)
- AC3: MANGO-TANGO recalled at 0.771 relevance (10 results)
- AC4: zero memos-cloud errors in gateway.err.log
- AC5: zero TextualMemoryItem validation errors post-restart
Add _parse_info field_validator on TextualMemoryMetadata.info to
auto-parse JSON-stringified dicts back to dicts on the read path.
CLA-81's _sanitize_neo4j_metadata() JSON-stringifies the info dict
before writing to Neo4j. On read, Pydantic rejected strings where
it expected a dict, causing 290 silent ValidationErrors and dropping
every memory with a non-null info field.
Fixes:
- TextualMemoryMetadata._parse_info: JSON str → dict (mode='before')
- Handles: empty {}, invalid JSON, non-dict JSON (returns {})
- Inherited by TreeNodeTextualMemoryMetadata automatically
Verified:
- 10/10 unit tests + 3/3 live recall tests (MANGO-TANGO, PAPAYA-SUNSET,
CRIMSON-FALCON) all passing
- 0 post-restart validation errors in memos-server.log (was 290)
- PAPAYA-SUNSET memory (info={'agentId':'main'}) now surfaces correctly
Per Jack's constraint: test must use the exact gateway plugin payload
including info={sessionKey, agentId} — not a clean curl without info.
Changes:
- Add _flatten_memories() helper (refactor from inline)
- Add test_gateway_plugin_payload_roundtrip:
* Posts /product/add with exact plugin format (info as dict, async_mode=sync)
* Searches after add completes (sync = no poll loop needed)
* Asserts info comes back as a dict with agentId='main'
* Proves field_validator works on the FULL Neo4j write→read round-trip
Why this matters: a clean curl without info passes even if the fix is
broken — info=null nodes never hit the validation code path.
Summary of ChangesHello @jacksclaw, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the MemOS system where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical deserialization issue with the info field in Neo4j, preventing data loss on memory recall. The fix, which involves adding a Pydantic field_validator to handle JSON string parsing, is well-implemented. The accompanying unit and integration tests, especially the gateway round-trip test, are thorough and effectively validate the fix. My review focuses on some improvements for the newly added shell scripts to enhance their security and portability.
| set -euo pipefail | ||
|
|
||
| PORT=8001 | ||
| VENV_UVICORN="/Users/jack_family_office/.openclaw/workspace/projects/MemOS/.venv/bin/uvicorn" |
There was a problem hiding this comment.
This line contains a hardcoded absolute path, which makes the script non-portable and will cause it to fail on any other machine or for any other user. The path to the virtual environment should be determined dynamically relative to the script's location.
| VENV_UVICORN="/Users/jack_family_office/.openclaw/workspace/projects/MemOS/.venv/bin/uvicorn" | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| VENV_UVICORN="$SCRIPT_DIR/.venv/bin/uvicorn" |
| QUIET="${1:-}" | ||
| ENDPOINT="http://localhost:8001/product/search" | ||
| PAYLOAD='{"query":"healthcheck","limit":1,"user_id":"openclaw-user"}' | ||
| TIMEOUT=5 | ||
|
|
||
| response=$(curl -s -m "$TIMEOUT" -o /tmp/memos-health.json -w "%{http_code}" \ | ||
| -X POST "$ENDPOINT" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "$PAYLOAD" 2>/dev/null) | ||
|
|
||
| http_code="$response" | ||
|
|
||
| if [ "$http_code" = "200" ]; then | ||
| code=$(python3 -c "import json,sys; d=json.load(open('/tmp/memos-health.json')); print(d.get('code',''))" 2>/dev/null) | ||
| if [ "$code" = "200" ]; then | ||
| [ -z "$QUIET" ] && echo "✅ MemOS healthy (HTTP 200, code=200, port 8001)" | ||
| exit 0 |
There was a problem hiding this comment.
Using a hardcoded file path in /tmp is insecure and can lead to race conditions. It's better to use mktemp to create a secure, unique temporary file. Additionally, a trap should be used to ensure the temporary file is cleaned up on script exit, even in case of errors.
| QUIET="${1:-}" | |
| ENDPOINT="http://localhost:8001/product/search" | |
| PAYLOAD='{"query":"healthcheck","limit":1,"user_id":"openclaw-user"}' | |
| TIMEOUT=5 | |
| response=$(curl -s -m "$TIMEOUT" -o /tmp/memos-health.json -w "%{http_code}" \ | |
| -X POST "$ENDPOINT" \ | |
| -H "Content-Type: application/json" \ | |
| -d "$PAYLOAD" 2>/dev/null) | |
| http_code="$response" | |
| if [ "$http_code" = "200" ]; then | |
| code=$(python3 -c "import json,sys; d=json.load(open('/tmp/memos-health.json')); print(d.get('code',''))" 2>/dev/null) | |
| if [ "$code" = "200" ]; then | |
| [ -z "$QUIET" ] && echo "✅ MemOS healthy (HTTP 200, code=200, port 8001)" | |
| exit 0 | |
| QUIET="${1:-}" | |
| ENDPOINT="http://localhost:8001/product/search" | |
| PAYLOAD='{"query":"healthcheck","limit":1,"user_id":"openclaw-user"}' | |
| TIMEOUT=5 | |
| TMP_FILE=$(mktemp) | |
| trap 'rm -f "$TMP_FILE"' EXIT | |
| response=$(curl -s -m "$TIMEOUT" -o "$TMP_FILE" -w "%{http_code}" \ | |
| -X POST "$ENDPOINT" \ | |
| -H "Content-Type: application/json" \ | |
| -d "$PAYLOAD" 2>/dev/null) | |
| http_code="$response" | |
| if [ "$http_code" = "200" ]; then | |
| code=$(python3 -c "import json,sys; d=json.load(open('$TMP_FILE')); print(d.get('code',''))" 2>/dev/null) | |
| if [ "$code" = "200" ]; then | |
| [ -z "$QUIET" ] && echo "✅ MemOS healthy (HTTP 200, code=200, port 8001)" | |
| exit 0 |
| TMP_SEARCH="/tmp/memos-hc-search.json" | ||
| TMP_WRITE="/tmp/memos-hc-write.json" | ||
| TMP_RECALL="/tmp/memos-hc-recall.json" |
There was a problem hiding this comment.
Using hardcoded file paths in /tmp is insecure and can lead to race conditions. It's better to use mktemp to create secure, unique temporary files. Please also add a trap command to ensure these temporary files are cleaned up when the script exits, even on error.
| TMP_SEARCH="/tmp/memos-hc-search.json" | |
| TMP_WRITE="/tmp/memos-hc-write.json" | |
| TMP_RECALL="/tmp/memos-hc-recall.json" | |
| TMP_SEARCH=$(mktemp) | |
| TMP_WRITE=$(mktemp) | |
| TMP_RECALL=$(mktemp) | |
| trap 'rm -f "$TMP_SEARCH" "$TMP_WRITE" "$TMP_RECALL"' EXIT |
1 participant
Problem
CLA-81's
_sanitize_neo4j_metadata()JSON-stringifies theinfodict before Neo4j write. On read, Pydantic rejected strings where it expected a dict, causing 290 silentValidationErrors and dropping every memory with a non-nullinfofield. 1,629 memories written, zero recallable.Fix
Added
_parse_infofield_validatoronTextualMemoryMetadata.info(mode='before') that auto-parses JSON strings back to dicts on the read path.Key Test: Gateway Plugin Round-Trip
Per Jack's constraint — test with the exact gateway plugin payload (including
infofield), not a clean curl. A clean curl withoutinfopasses even if the fix is broken.test_gateway_plugin_payload_roundtrip:info={sessionKey: ..., agentId: main}infocomes back as a dict withagentId='main'— not a stringAcceptance Criteria
validation error for TextualMemoryItempost-restart (was 290)Test Results