neo4j-client correctness: read-only Cypher + bi-temporal supersession + merge timestamp#28
Merged
Merged
Conversation
stevepridemore
added a commit
that referenced
this pull request
May 10, 2026
… + merge timestamp fix (#28) Three correctness fixes to src/shared/neo4j-client.ts surfaced in the same code-review session, all touching the data-integrity layer. Cypher safety - New runReadOnly() method using session.executeRead() with a 30s transaction timeout. executeCypher() now uses runReadOnly(), making the graph_cypher MCP tool's "Enforced read-only via Neo4j executeRead()" claim actually true. Previously the tool description was a guarantee that didn't hold — admin LLM could execute DETACH DELETE despite the read-only contract. Bi-temporal supersession - createRelationship now invalidates predecessor edges of the same (from, type) pointing to a different target when valid_at is supplied. The dream prompts (dream-nightly.md, graph-capture.md) documented this behaviour but the code wasn't doing it — every "switched from X to Y" scenario silently produced two coexisting "current" edges (both invalid_at IS NULL). Trigger is gated on valid_at presence so multi-valued relationships (WORKS_ON, USES_TECH) still allow concurrent edges. - valid_at is now stored as a Neo4j datetime() rather than a raw ISO string in the property map. Same for invalid_at via the supersession path. Future temporal queries (WHERE r.valid_at < datetime()) now work correctly. merge() timestamp typo - Lines 1482 and 1516 set r.last_seen on consolidated relationships, but every other relationship code path uses r.last_confirmed and applyDecay() reads r.last_confirmed. Net: merged edges decayed as if never touched. Fixed to set r.last_confirmed. 13 new tests across read-only enforcement (5) and bi-temporal supersession (8). Coverage 65.29 → 65.88% statements, 67.29 → 67.78% lines. All existing tests still pass. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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
Three correctness fixes to
src/shared/neo4j-client.tssurfaced in the same code-review session, all bundled because they touch the same file's data-integrity layer. Closes the two HIGH-severity findings + one MEDIUM the reviews uncovered.Cypher safety (closes graph_cypher footgun)
The
graph_cypherMCP tool description claimed "Enforced read-only via Neo4j executeRead()" but the underlying code path (run()→ WRITE session) didn't enforce it. An admin LLM (or a prompt-injected admin session) could executeDETACH DELETEand wipe the graph despite the read-only contract.runReadOnly()method usingsession.executeRead()with a 30s transaction timeout. The timeout closes Gemini's "Cartesian product DoS" finding as a side benefit.executeCypher()now usesrunReadOnly(). The tool description is now accurate without changing.Independently flagged by Copilot and Gemini — two-reviewer confirmation.
Bi-temporal supersession (closes silent data corruption)
prompts/dream-nightly.md:189andprompts/graph-capture.md:59documented thatgraph_relateautomatically invalidates predecessor edges when a fact changes. The code wasn't doing it. Every "switched from X to Y" scenario silently produced two coexisting "current" edges (bothinvalid_at IS NULL), corrupting the bi-temporal model and skewing query results.createRelationshipnow invalidates sibling edges of the same(from, type)pointing to a different target whenvalid_atis suppliedvalid_atpresence so multi-valued relationships (WORKS_ON,USES_TECH) still allow concurrent edges — only explicit temporal updates trigger supersessionvalid_at(andinvalid_atvia the supersession path) now stored as Neo4jdatetime()instead of raw ISO strings, so future temporal queries workCaught by Copilot's review.
merge() timestamp typo
Lines 1482 and 1516 set
r.last_seenon consolidated relationships, but every other relationship path usesr.last_confirmedandapplyDecay()readsr.last_confirmed. Merged edges therefore decayed as if they'd never been touched.One-character fix per occurrence. Caught by Copilot.
Test plan
npm run buildcleannpm run test:coverage:local— 13 new tests passing (5 read-only enforcement + 8 bi-temporal supersession), no regressions on existing 133/graph-statsstill works (regression check on read paths) and a smallgraph_relatewrite succeedsWhy combined into one PR
Originally planned as two PRs but the parallel-builder run produced a working tree with both fixes already entangled. Both are correctness fixes to the same file's data-integrity layer; the granularity loss is minimal (one commit message vs two telling the same story). Tests prove they compose correctly.
🤖 Generated with Claude Code