Skip to content

fix(neo4j): add write fallback for memory repository (LISA-17)#208

Merged
TonyCasey merged 4 commits intomainfrom
lisa-17
Feb 5, 2026
Merged

fix(neo4j): add write fallback for memory repository (LISA-17)#208
TonyCasey merged 4 commits intomainfrom
lisa-17

Conversation

@TonyCasey
Copy link
Owner

@TonyCasey TonyCasey commented Feb 4, 2026

Summary

  • Added save() and saveBatch() methods to Neo4jMemoryRepository so it can serve as a write fallback when MCP/Graphiti is unavailable
  • Changed default routing rules: write fallback from zep to neo4j (IRepositoryRouter.ts)
  • Root cause: MCP uses FalkorDB internally (not the external Neo4j container), and Neo4j was read-only by design — leaving no working write path when MCP went down, causing a 12-day memory gap

Test plan

  • 19 new unit tests for save(), saveBatch(), supportsWrite()
  • All 1566 tests pass
  • Lint clean
  • Build successful
  • Manual: stop MCP container, verify lisa memory add falls back to Neo4j
  • Manual: verify facts written via Neo4j fallback appear in lisa memory load

Closes LISA-17

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Neo4j memory fallback now supports writes (single and batch saves), including UUID generation and per-item options.
    • Added commented LLM API key placeholders in the environment template.
  • Bug Fixes

    • Default write fallback routing updated to target Neo4j.
  • Tests

    • Added comprehensive unit tests covering save, batch save, UUID uniqueness, concurrency, and error propagation.

Neo4jMemoryRepository was read-only by design, leaving no fallback when
MCP was unavailable. This caused a 12-day memory gap (Jan 24 - Feb 4).
Added save/saveBatch methods and updated routing rules so Neo4j serves
as write fallback when MCP is down.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

Adds read-write support to the Neo4j memory repository (save and saveBatch with UUIDs and batching/concurrency), switches default write fallback to Neo4j, adds LLM API key placeholders in env template, and introduces unit tests validating write behavior, batching, UUID uniqueness, and error propagation.

Changes

Cohort / File(s) Summary
Neo4j Repository — read/write
src/lib/infrastructure/dal/repositories/neo4j/Neo4jMemoryRepository.ts, tests/unit/src/lib/infrastructure/dal/repositories/neo4j/Neo4jMemoryRepository.write.test.ts
Implements save() and saveBatch() with randomUUID generation, per-item Cypher writes, tags/name handling (including truncation), batching/concurrency control, and error propagation. Class now implements writer/expiration/quality interfaces and supportsWrite() returns true. Comprehensive unit tests added.
Routing & Interfaces
src/lib/domain/interfaces/dal/IRepositoryRouter.ts, src/lib/domain/interfaces/dal/IMemoryRepository.ts
Default write routing/fallback updated to target Neo4j (was Zep); minor comment removal in IMemoryRepositoryCapabilities.supportsWrite(). No public signature changes.
Changelog & Env template
CHANGELOG.md, src/project/.lisa/.env.template
Adds CHANGELOG entry for Neo4j memory fallback. Appends commented LLM API key placeholders (ANTHROPIC_API_KEY, OPENAI_API_KEY) to .env.template.

Sequence Diagram(s)

sequenceDiagram
  participant Client as Client
  participant Repo as Neo4jMemoryRepository
  participant Conn as Neo4jConnectionManager
  participant DB as Neo4jDB
  rect rgba(200,200,255,0.5)
    Client->>Repo: save(groupId, content, options?)
    Repo->>Conn: write(CYPHER, params {uuid, groupId, content, ...})
    Conn->>DB: execute write transaction
    DB-->>Conn: write result
    Conn-->>Repo: result
    Repo-->>Client: IMemoryItem
  end
  rect rgba(200,255,200,0.5)
    Client->>Repo: saveBatch(groupId, [facts], options?)
    Note over Repo,Conn: concurrency-controlled per-item writes -> Conn/DB per item
    Repo-->>Client: [IMemoryItem...]
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding write fallback capability to the Neo4j memory repository, which is the core objective of this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch lisa-17

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai bot added the nate Nate Plan: Memory improvement features label Feb 4, 2026
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9f491b734f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@src/lib/infrastructure/dal/repositories/neo4j/Neo4jMemoryRepository.ts`:
- Line 29: Move the Node.js built-in import for randomUUID to the top of the
import block so built-ins come before third-party and internal modules;
specifically, relocate "import { randomUUID } from 'node:crypto';" above other
imports in Neo4jMemoryRepository.ts (where the Neo4jMemoryRepository class and
its helpers are defined) to comply with the import ordering rule (built-ins,
then third-party, then internal).

In `@src/project/.lisa/.env.template`:
- Line 27: Add a single trailing newline at the end of the .env.template file so
the last line "# OPENAI_API_KEY=" is terminated with a newline character; edit
the file containing the "# OPENAI_API_KEY=" line and ensure the file ends with
one newline (no other content changes).

- Persist tags on Neo4j relationship so findByTags works on fallback writes
- Move node:crypto import above internal imports per import ordering rules
- Add trailing newline to .env.template

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@src/lib/infrastructure/dal/repositories/neo4j/Neo4jMemoryRepository.ts`:
- Around line 195-244: The save() implementation currently only stores
options.tags, causing lifecycle/confidence-derived tags used by expireByFilter()
and findByMinConfidence() to be missing; modify Neo4jMemoryRepository.save() to
compute the derived lifecycle/confidence tags (the same logic those filters
expect), merge them into the tags array before building params and writing the
node/relationship (ensure the cypher parameter $tags contains the merged list),
and return the merged tags in the IMemoryItem result so tests and consumers see
the persisted lifecycle/confidence tags; reference save(), expireByFilter(), and
findByMinConfidence() to reuse the same derivation logic to keep behavior
consistent.

In `@src/project/.lisa/.env.template`:
- Around line 18-21: The template currently enables all LLM features by default
via the LISA_LLM_FEATURES environment variable; change the .env template so it
does not enable all features by default by either commenting out the
LISA_LLM_FEATURES line or setting it to a minimal explicit default (e.g.,
"summarization" or an empty value) and add a short inline comment explaining how
to opt into additional features, so consumers must explicitly enable features
rather than inheriting "*".

- Derive lifecycle/confidence tags from save options and merge with
  explicit tags using Set for deduplication. Ensures expireByFilter and
  findByMinConfidence work on fallback-written facts.
- Revert LISA_LLM_FEATURES=* to commented in .env.template to avoid
  unintended cost for new users.
- 3 new tests for lifecycle/confidence tag derivation.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@src/lib/infrastructure/dal/repositories/neo4j/Neo4jMemoryRepository.ts`:
- Around line 244-251: The returned IMemoryItem.created_at is generated
client-side with new Date().toISOString(), which may mismatch the server-side
datetime() used in the Cypher query; update the return path in
Neo4jMemoryRepository (the function that constructs the memory object) to prefer
the DB-generated timestamp from the created node/record (e.g., use the
created_at property from the result record or node returned by the CREATE query)
and only fall back to new Date().toISOString() if the DB value is not present,
so returned created_at matches the authoritative DB value.

In `@src/project/.lisa/.env.template`:
- Line 25: Change the lowercase header "# llm api keys" to Title Case to match
the file's style; replace it with "# LLM API Keys" (capitalizing "LLM", "API",
and "Keys") so it aligns with other headers like "# Lisa Configuration" and "#
LLM Feature Toggles".

In
`@tests/unit/src/lib/infrastructure/dal/repositories/neo4j/Neo4jMemoryRepository.write.test.ts`:
- Around line 254-269: Add a brief comment inside the saveBatch implementation
(the method invoked by repo.saveBatch / Neo4jMemoryRepository.saveBatch)
documenting that it uses Promise.all and therefore fails fast: errors from an
individual write will reject the batch, earlier writes may already be committed,
and the caller will not be informed which items succeeded; note this
partial-commit behavior and that callers must handle or retry accordingly.

- Add comment noting client-side vs server-side timestamp approximation
- Fix section header capitalization in .env.template
- Document partial-failure behavior in saveBatch JSDoc

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/lib/infrastructure/dal/repositories/neo4j/Neo4jMemoryRepository.ts`:
- Around line 254-276: Extract the hard-coded CONCURRENCY_LIMIT in
Neo4jMemoryRepository.saveBatch into a configurable value by adding a
class-level constant or constructor parameter (e.g., this.batchConcurrency or a
static DEFAULT_CONCURRENCY) and use that instead of the local CONCURRENCY_LIMIT;
update the constructor to accept an optional concurrency value (or set the class
constant) so callers/tests can override it and replace references inside
saveBatch to the new property/constant.

@TonyCasey TonyCasey merged commit d386138 into main Feb 5, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nate Nate Plan: Memory improvement features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant