Skip to content

feat(age-integration): 6-phase mempalace knowledge base ↔ AGE integration - #101

Merged
jphein merged 24 commits into
mainfrom
feat/age-kg-parity
May 22, 2026
Merged

feat(age-integration): 6-phase mempalace knowledge base ↔ AGE integration#101
jphein merged 24 commits into
mainfrom
feat/age-kg-parity

Conversation

@jphein

@jphein jphein commented May 17, 2026

Copy link
Copy Markdown
Collaborator

What

Six-phase plan to fully integrate the mempalace knowledge base with Apache AGE so the metaphor of the AI walking into a palace and finding wings, rooms, and drawers becomes real Cypher traversal — not a narrative device.

Phase What Commit
1 KnowledgeGraphAGE API parity (add_entity, invalidate, query_entity, query_relationship, timeline, seed_from_entity_facts) ff7187d
2 Write-through middleware on PostgresCollection.add/upsert — every drawer write extracts entities + creates :MENTIONS edges in AGE 3321d83
3 Palace structure as native AGE nodes (Wing → CONTAINS → Room → CONTAINS → Drawer, plus Wing -[SHARED_VIA]- Wing tunnels) ff583c0
4 backfill_age — restartable, checkpointed one-shot to populate AGE from existing drawer table (~22h projected for production 274K palace) b3f0206
6 mempalace_walk_palace MCP tool — agent-facing walk primitive (start_wing / start_room / start_entity × depth) 8022ecb
docs FORK_CHANGELOG entries + .gemini/config.yaml 07f63ba, c35c74e

Phase 5 (read-side fusion in palace-daemon /search) lives on techempower-org/palace-daemon:feat/age-fused-search.

Why

The 2026-05-17 AGE write-through spike on n=200 git-derived probes showed graph signal adds +9pp R@5 over vector-only (graph_only beats vector by +5pp; RRF fusion adds another +4pp on top). The architecture before this PR couldn't realize that lift because:

  1. KnowledgeGraphAGE was skeleton-only — 5 methods, no query surface (independently confirmed by nakata-app on MemPalace/mempalace/discussions/1384#discussioncomment-16951226)
  2. AGE graph state was unpopulated — 2 placeholder vertices + 1 placeholder edge total
  3. No write path connected drawer writes to entity-graph population
  4. No read path consulted AGE during search

This PR fixes (1), (2), (3), and (6). Companion PR on techempower-org/palace-daemon fixes (4).

Unified graph schema

Wing  -[:CONTAINS]->  Room  -[:CONTAINS]->  Drawer  -[:MENTIONS]->  Entity
  ↑                                                                    ↑
  +-[:SHARED_VIA {via_room}]- Wing (tunnels)        (auto-populated on every write)

An agent now walks the palace via Cypher:

walk_palace(start_wing="memorypalace", depth=3)
# → [{wing, room, drawer, entity}] rows for every reachable leaf

AGE Cypher dialect gaps documented

Three real limitations in AGE 1.6.0 we worked around during the build:

  1. No multi-column RETURN inside cypher(...) dollar-quoted form — workaround: return one column, query twice for additional projections
  2. No list literals (RETURN [a, b] errors) — workaround: query columns one at a time
  3. No MERGE ... ON CREATE SET, no SET on edge properties inline, no coalesce() in SET — workaround: truncate fresh + plain CREATE in three passes (entities → drawers → edges)

All three are encoded in mempalace/knowledge_graph_age.py workarounds.

Test plan

  • Smoke-tested each phase on sme_lme_bench against the production postgres+pgvector+AGE stack
  • backfill_age validated on sme_lme_bench (5344 drawers, 58948 entities created, 26 min, 0 errors → projects to ~22h for prod 274K palace)
  • AGE write-through bench on n=200 git-probes: graph_only beats vector by +5pp R@5; fusion (RRF) adds +4pp on top
  • mempalace_walk_palace returns navigable structured walks across all three modes (start_wing / start_room / start_entity)
  • Full pytest suite — should pass (no changes to existing code paths beyond the additive write-through hook in PostgresCollection._insert_rows, which is gated by set_kg_writethrough(hook) registration; default is zero overhead)
  • Gemini Code Assist review

Operational rollout

Backfill against mempalace_2026_05_13 (production palace) takes ~22h overnight. Phases 1-3 + 6 are safe to deploy without backfill — the write-through middleware adds new graph state as drawers come in, and walk_palace returns empty walks gracefully when graph isn't populated yet. Phase 4 backfill can run independently when ready.

Cross-references

  • Companion PR: techempower-org/palace-daemon/search/age-fused endpoint
  • Spike result: techempower-org/multipass-structural-memory-eval@28ae3f1
  • Independent cross-fork audit (nakata-app side): MemPalace/mempalace/discussions/1384#discussioncomment-16951226

🤖 Generated with Claude Code

jphein and others added 14 commits May 16, 2026 14:46
RESEARCH FEATURE supporting code — no behavior change in production.

Add ``mempalace/rrf.py`` with three functions:

* ``rrf_scores(rank_lists, key=, k=60)`` — Cormack-2009 RRF score
  aggregation across N ranked lists.
* ``rrf_fuse(rank_lists, ...)`` — score + sort + return fused
  triples (identity, score, representative). Configurable
  representative selector for callers that want metadata
  preference rules.
* ``explain_fusion(...)`` — diagnostic: per-identity rank table
  across lists, sorted by RRF score.

Pure module, no deps. Used by the multi-encoder retrieval glue
(follow-up commit) which is gated behind
``PALACE_USE_MULTI_ENCODER_RRF``.

Refs #82.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ch feature)

RESEARCH FEATURE — default off, gated by
``PALACE_USE_MULTI_ENCODER_RRF=1``. Refs #82.

Add ``mempalace/multi_encoder.py``: read an encoder roster from env,
for each encoder encode the query → query its bound palace → RRF-fuse
the rank lists into a chromadb-shaped result dict. Downstream code
(closet boost, hybrid rerank, BM25 fallback) is unchanged because
the fused result has the same shape as a single chromadb query
returns.

Encoders:

* ``default`` — built-in ONNX MiniLM, no model path needed.
* Any other name — local ``SentenceTransformer`` directory passed via
  ``PALACE_RRF_ENCODER_PATHS``.

Each encoder may have its own palace via ``PALACE_RRF_PALACES``. The
production shape is "N parallel mines, one per encoder" because
FT-encoded queries against ONNX-encoded drawer vectors are noise.
When an encoder has no palace, the call falls back to the
call-site palace with a one-shot warning (useful only for
single-palace benchmarks).

Defensive: one bad encoder palace or loader doesn't sink the whole
query — that encoder's contribution is skipped with a warning;
remaining encoders fuse normally. When every encoder fails, the
caller receives an empty result and the existing BM25-fallback path
fills in.

``mempalace/searcher.py`` gets a single hook at the
``drawers_col.query(...)`` call site: when ``is_enabled()`` is true,
route through ``fused_query``. Default path is byte-identical to
before. Hook is inside the existing try/except so a fan-out failure
falls through to the existing SQLite/BM25 fallback rather than
hard-failing.

Tunables:

* ``PALACE_RRF_K=60`` — Cormack smoothing constant.
* ``PALACE_RRF_OVERFETCH=3`` — per-encoder pool multiplier.

Costs: query latency ≈Nx; storage ≈Nx (one palace per encoder);
ingest ≈Nx. These costs make this a research lever, not a
flip-the-default candidate without further evaluation.

Tests cover env parsing, RRF promotion of consensus winners,
collapse of duplicate (source_file, chunk_index) across encoder
palaces, one-encoder-fails-gracefully, all-encoders-fail-empty,
overfetch tunable, and the env-flag hook through
``search_memories`` end-to-end.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… RRF

Add ``scripts/eval_multi_encoder_rrf.py``: mine the same corpus once
per encoder into temp palaces, then run a probe set through the
production ``search_memories`` path twice — once with
``PALACE_USE_MULTI_ENCODER_RRF`` unset (single-encoder baseline) and
once with it on. Report MRR@K, Recall@5, Recall@10, plus deltas.

Differs from ``scripts/verify_rrf_ftcode5k.py`` by exercising the
real production code path (not a surrogate min-rank fusion), so the
numbers it produces are what we'd actually ship.

Defaults to the 3-encoder roster from issue #82
(default + FT-Code-1000 + FT-Code-5000) but accepts arbitrary
``--encoders`` / ``--model-paths`` so 2-way and other rosters are
one-flag changes.

Add ``docs/research/2026-05-15-multi-encoder-rrf.md`` explaining
why this lever exists, what this PR ships vs. doesn't, the operator
surface, costs, and open questions.

Refs #82.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The built-in chromadb ONNX MiniLM embedding function returns
``[np.ndarray(dtype=float32), ...]`` from a batched call, and the
shape ``list(vecs[0])`` produces is a Python list whose elements
are ``numpy.float32`` scalars.

chromadb's ``collection.query(query_embeddings=...)`` validates the
input shape against ``list[float] | list[list[float]] | numpy
array``. A list of numpy scalars is the one shape it doesn't
recognize and raises:

    Expected embeddings to be a list of floats or ints, a list of
    lists, a numpy array, or a list of numpy arrays, got
    [[np.float32(-0.0807703), …]]

The first end-to-end eval against the n=200 git-derived probe set
silently skipped the default encoder for every query because of
this — the FT-Code SentenceTransformers happened to be fine since
they go through ``.tolist()`` already.

Fix: prefer ``ndarray.tolist()`` (the canonical numpy → Python
floats conversion), fall back to ``[float(x) for x in vec]`` for
EF implementations that return plain lists. Regression test stubs
the EF with the chromadb-shaped numpy output and asserts the
encoder yields plain ``float`` elements.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…pipeline

End-to-end eval of multi-encoder RRF against the n=200 git-derived
probe set, routed through ``search_memories`` (closet boost +
hybrid BM25 rerank already on).

Headline:

| Path                                | MRR    | R@5   | R@10  | qps  |
|-------------------------------------|-------:|------:|------:|-----:|
| Single-encoder baseline (default)   | 0.4042 | 46.5% | 49.0% | 3.85 |
| 3-way RRF (default + ft-1k + ft-5k) | 0.4033 | 45.5% | 49.5% | 2.78 |
| Δ                                   | −0.0008 | −1.00 pp | +0.50 pp | 0.72x |

Per-probe breakdown:
* 2 rescued, 1 regressed
* 5 better rank, 10 worse rank
* 82 tied hits, 100 tied misses

The +0.0841 raw-vector lift from issue #82 (and nakata-app's MemPalace#1384)
does not survive the hybrid pipeline. Most likely explanation: the
encoder-orthogonality signal that RRF exploits on raw vector
retrieval is already largely captured by the production path's
closet boost + BM25 rerank — whatever the encoders disagree on,
BM25 catches as verbatim terminology match. Same shape of finding
as the HyDE diagnosis (familiar.realm.watch#6): retrieval
techniques targeting vocabulary-bridging produce smaller wins on
top of an already-doing-vocabulary-bridging hybrid path than they
do on raw vector retrieval.

Persisted full per-probe JSON to
``docs/research/2026-05-15-rrf-eval-3way.json`` for reproducibility
and follow-up analysis.

Implication for the feature flag: leave default off; do not flip in
production. The code lands as a research artifact — reproducible,
inspectable, gated — but the measured lift on real call paths
doesn't justify 3x query latency or Nx storage.

Refs #82.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three small lint fixes so #85 can merge clean on the rebased branch:

- scripts/verify_rrf_ftcode5k.py: f-string with no placeholders → plain
  string (F541)
- scripts/chunk_strategy_ablation.py + scripts/verify_rrf_ftcode5k.py:
  ruff format trailing-comma adjustments
- tests/test_multi_encoder.py: the `type(x) is float` check was
  deliberate (isinstance accepts np.float32 because numpy scalars
  subclass float — that defeats the coercion test). Reworked to
  collect type names into a set and compare strings, dodging E721
  without losing the test's structural intent.

All RRF tests pass (40/40 across test_rrf.py + test_multi_encoder.py).

Co-Authored-By: jphein <jphein@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…uery_relationship, timeline, seed_from_entity_facts

Brings KnowledgeGraphAGE to API parity with the SQLite KnowledgeGraph.
Previously only had: add_triple, query_triples, stats, clear. The 5
missing methods make AGE a drop-in replacement for the SQLite backend
without requiring callsite changes.

All methods mirror SQLite semantics exactly:
- add_entity: MERGE pattern with type + properties (no ON CREATE SET
  in AGE — last-write-wins semantics, acceptable for write-through use)
- invalidate: SET valid_to on every active matching triple, with
  inverted-interval guard
- query_entity: outgoing/incoming/both direction filter + as_of
  temporal filter
- query_relationship: filter by predicate + as_of
- timeline: chronological ORDER BY, default limit 100
- seed_from_entity_facts: bulk-load from ENTITY_FACTS dict
- _entity_id: name → canonical id helper, matches SQLite KG derivation

All built on the existing _run_cypher infrastructure with the inlined-
parameter approach that works around AGE's prepared-statement
incompatibility. AGE Cypher dialect gaps respected:
- No ON CREATE SET → unconditional SET on MERGE
- No multi-column RETURN inside cypher() — works here because each
  method's RETURN uses AS-aliased columns that _extract_return_aliases
  parses
- No list literals — not used anywhere in these implementations

Smoke-tested end-to-end against sme_lme_bench:
- add 3 triples (Atakan -[works_on]-> adaptmem/mempalace-PRs, FT-300
  -[trained_by]-> Atakan)
- query_entity('Atakan', 'outgoing') → 2 results, both current=True
- query_entity('Atakan', 'incoming') → 1 result (FT-300 trained_by)
- invalidate('Atakan', 'works_on', 'mempalace-PRs') → 1 affected, then
  re-query shows valid_to=2026-05-17, current=False
- timeline() → 3 rows ordered by valid_from (NULL at end)
- stats → entities=4, triples=3, current_facts=2, expired_facts=1,
  relationship_types=['trained_by', 'works_on']

Phase 1 of the larger AGE-integration plan toward "agent walks the
palace" via real Cypher traversal.
…ry drawer write

Phase 2 of /goal: AGE integration. Every PostgresCollection upsert/add now
optionally fires a KG write-through hook that extracts entities from
the document and creates (drawer_id -[mentions]-> entity) triples in AGE.

Plumbing:
- PostgresCollection._insert_rows: after the row commits, calls
  self._kg_writethrough(drawer_id, document, metadata) if registered.
  Hook errors caught + logged, never raised — KG enrichment is
  opportunistic, never blocks writes.
- PostgresCollection.set_kg_writethrough(hook): registration API.
  Default (no hook) is zero overhead — vector-only behavior unchanged.

New module mempalace/kg_writethrough.py:
- make_age_writethrough(kg, extractor) — canonical hook factory. For
  each drawer, extracts entities, caps at max_entities_per_drawer
  (default 100), creates one add_triple per entity.
- make_null_writethrough() — no-op for tests / disabling.
- make_writethrough_from_env() — env-var-driven config:
    MEMPALACE_KG_WRITETHROUGH=1 + MEMPALACE_KG_EXTRACTOR=regex|null
- _builtin_regex_extractor — fallback when SME's regex extractor isn't
  importable. Captures capitalized proper nouns + hyphenated
  identifiers + version strings.

Extractor is pluggable: any callable matching (text) -> list[Entity]
where Entity has .name works. Tested with sme.extractors.regex.extract.

Smoke test on sme_lme_bench:
- Fresh AGE graph + fresh PostgresCollection + registered hook
- coll.upsert(2 drawers about Atakan/FT-300/AGE/mempalace-Phase-2)
- KG state after: 10 entities, 8 mentions edges, all current
- d1 → mentions → {atakan, longmemeval, apache, nakata-app, ft-300}
- d2 → mentions → {age, postgrescollection, phase-2}

Phase 2 done. Next: Phase 3 — palace structure (Wing/Room/Drawer) as
AGE nodes so /graph queries become Cypher MATCH on real graph structure.
Phase 3 of /goal: AGE integration. Mirrors mempalace.palace_graph's
SQL-aggregation pattern into AGE so Cypher MATCH can walk the palace
structure natively — no SQL aggregation per query.

New module mempalace/palace_graph_age.py:

- populate_from_postgres(kg, dsn, table_name, skip_drawers, skip_tunnels):
  Reads drawer table, builds Wing/Room/Drawer/SHARED_VIA in AGE.
  Idempotent via MERGE. Three-pass design (wings → rooms+contains →
  drawers+contains → tunnels) so skip_drawers can give a fast
  "high-level palace map" without per-drawer cost on huge palaces.

- walk_wing(kg, wing, depth): Returns a structured walk of the wing's
  contents. depth=1 returns rooms; depth=2 adds drawers; depth=3 joins
  MENTIONS edges back to entities from the kg_writethrough layer.
  This is the "agent walks the palace" primitive.

- list_wings, list_rooms_in_wing, list_drawers_in_room,
  tunnels_from_wing: read-side helpers, idiomatic Cypher patterns
  ready for MCP tool wiring in Phase 6.

Cypher schema:
  Wing  -[:CONTAINS]-> Room  -[:CONTAINS]-> Drawer  -[:MENTIONS]-> Entity
  Wing  -[:SHARED_VIA {via_room}]- Wing  (tunnels: rooms in multiple wings)

The CONTAINS + MENTIONS edges connect the palace structure (Phase 3) to
the KG triples (Phase 1) and the kg_writethrough layer (Phase 2) into
one unified graph that an agent or RLM can navigate.

Smoke-test on sme_lme_bench (5344 drawers across 2 wings: code/docs):
- populate(skip_drawers=True) → wings=2, rooms=237, contains_edges=238,
  shared_via_edges=1 (one tunnel — the 'cli' room appears in both wings)
- list_wings() → ['code', 'docs']
- list_rooms_in_wing('code')[:10] → ['__init__', '__main__', '_stdio', 'base', 'basic_mining', ...]
- tunnels_from_wing('code') → [{'to_wing': 'docs', 'via_room': 'cli'}]

Next: Phase 4 — backfill production palace (274K drawers) AGE graph.
Builds out Phase 4 (backfill script) and tightens Phases 1-3:

KnowledgeGraphAGE.add_mention():
  New method that connects Drawer nodes (palace structure) to Entity
  nodes via :MENTIONS edges, distinct from add_triple's
  Entity-RELATION-Entity pattern. Lets the write-through layer
  (Phase 2) populate the kg in the same shape the read-side walk
  (walk_wing, MCP tools) expects.

  AGE 1.6.0 dialect gaps documented + worked around:
  - No SET on edge properties inline (errors at '=')
  - No ON CREATE SET
  - No coalesce() in SET
  - No edge-type union [:A|B] in MATCH
  CREATE-always edge semantics matches the SQLite KG's triples-table
  behavior (no upsert on triples; multiple add_triple calls create
  parallel rows). Aggregate at read time via count(r).

palace_graph_age.walk_wing():
  Fixed to use :RELATION + property filter instead of [:MENTIONS|RELATION]
  union — AGE doesn't parse the union syntax.

kg_writethrough.make_age_writethrough():
  Switched from add_triple to add_mention so write-through populates
  the Drawer-MENTIONS-Entity graph the read side expects.

backfill_age.py (Phase 4):
  CLI + library entry point for one-shot AGE-graph population from
  existing drawer table. Restartable via mempalace_kg_backfill_state
  checkpoint table. Stream-based via psycopg2 named cursor (bounded
  memory). Configurable scope (--wing, --skip-palace, --skip-entities,
  --extractor). Default extractor is SME's regex extractor (with
  builtin fallback when SME isn't importable).

Smoke test on sme_lme_bench:
- populate_from_postgres(skip_drawers=True) → 2 wings, 237 rooms,
  238 CONTAINS, 1 SHARED_VIA tunnel
- backfill (docs wing only, max 20 entities/drawer) → 1181 drawers
  processed → 6015 entities + 13721 mentions in 5.85 min
  (~3.4 drawers/s; projects to ~22h for the prod 274K palace)
- add_mention parallel-edge test: 3 calls (1 duplicate) → 2 edges for
  Atakan, 1 for adaptmem; aggregate count(r) recovers true count
Phase 6 of /goal: AGE integration. The "agent walks into the palace
finding wings, rooms, drawers" metaphor becomes a real MCP tool over
the unified palace+entity graph (Wing → Room → Drawer → MENTIONS →
Entity) built in Phases 1-4.

Three traversal modes, exactly one anchor required:

  start_wing="memorypalace" → walks DOWN the structure
    depth=1: rooms in this wing
    depth=2: + drawers in those rooms
    depth=3: + entities those drawers mention

  start_room="problems" → walks DOWN from a room (across all wings)
    depth=1: drawers in this room (any wing)
    depth=2: + entities mentioned

  start_entity="pgvector" → walks UP from an entity (inverse walk)
    depth=1: drawers that mention this entity
    depth=2: + the rooms+wings containing those drawers

Returns:
  {
    "start": {"wing": ..., "room": ..., "entity": ...},
    "depth": N,
    "walk": [{wing, room, drawer, entity}, ...],
    "stats": {wings_touched, rooms_touched, drawers_touched, entities_touched}
  }

Requires MEMPALACE_BACKEND=postgres and the AGE graph populated via
mempalace.kg_writethrough or mempalace.backfill_age.

Smoke-tested on sme_lme_bench:
- walk_palace(start_entity='pgvector', depth=2) returns the 3 drawers
  mentioning it (postgres.py, CHANGELOG.md, BENCHMARKS.md), plus their
  containing rooms+wings (postgres+code, CHANGELOG+docs, BENCHMARKS+docs)
- walk_palace(start_room='postgres', depth=2) returns the postgres.py
  drawer plus its 3 mentioned entities (pgvector, hnsw, Apache AGE)
- walk_palace(start_wing='docs', depth=3) walks into the docs wing's
  rooms, drawers, and entity layer

This completes the 6-phase plan that started from today's spike result
(+9pp R@5 from AGE entity-overlap fused with vector). The metaphor of
the AI walking the palace is now real Cypher traversal — anyone with
the MCP tool can ask "where does pgvector get discussed?" or "what's
in the memorypalace wing?" and get a structured answer.
Documents the 5 fork-side commits in feat/age-kg-parity branch in the
canonical fork-changes.yaml + regenerated FORK_CHANGELOG.md. Each phase
gets a full body explaining what landed, why it matters, what was
tested, and which files changed:

- Phase 1 (ff7187d): KnowledgeGraphAGE API parity
- Phase 2 (3321d83): write-through middleware in PostgresCollection
- Phase 3 (ff583c0): palace structure as native AGE nodes
- Phase 4 (b3f0206): add_mention + backfill_age + walk_wing fix
- Phase 6 (8022ecb): mempalace_walk_palace MCP tool

Phase 5 (search-side fusion) lives on the palace-daemon repo; its
changelog gets a parallel entry there.

Note: render-docs.py regenerated FORK_CHANGELOG.md from fork-changes.yaml
per the file's own instructions.
…facts)

The LongMemEval cleaned datasets (longmemeval_s_cleaned.json ~261MB,
longmemeval_oracle.json ~15MB) get pulled per-environment when running
benchmarks/longmemeval_bench.py. They should never be committed —
git LFS or off-band data hosting is the right home, and the
download instructions live in the bench script docstring.
…ini Code Assist

Copilot Code Review has been failing 10+ times in last 20 runs since
weekly Copilot credit ran out. Gemini Code Assist is free for public
repos (which all three techempower-org forks are) and already actively
reviews on MemPalace upstream (saw it on PR MemPalace#1525).

Config is tuned for an active research fork:
- MEDIUM severity threshold — filter low-severity noise
- summary + code_review on PR open; help disabled
- have_fun: false — no emoji in review comments
- Ignore patterns: benchmark result JSONs, generated FORK_CHANGELOG.md
  (yaml is source of truth), v33x snapshot dirs, dashboards, lock
  files, build artifacts

Install Gemini Code Assist at https://github.com/apps/gemini-code-assist
on techempower-org org; config picks up automatically.

Disable Copilot Code Review in repo Settings → Code & automation →
Copilot review to stop the failure notification flood.

AGENTS.md (already in repo at /AGENTS.md) provides project context
to Gemini for code review — no separate GEMINI.md needed.
Copilot AI review requested due to automatic review settings May 17, 2026 21:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a comprehensive integration of Apache AGE for graph-based knowledge management and a research feature for multi-encoder retrieval using Reciprocal Rank Fusion (RRF). Key additions include write-through middleware for real-time KG population, a backfill utility, and an MCP tool for palace traversal. Feedback highlights critical security vulnerabilities regarding SQL injection in Cypher and SQL queries. Additionally, high-priority improvements are needed for fragile parameter replacement logic, performance bottlenecks due to excessive database commits in loops, and the use of unstable hashing for cross-process identity mapping. Other suggestions focus on optimizing database connection reuse and removing redundant JSON parsing.

Comment on lines +746 to +749
cur.execute(
f"SELECT * FROM cypher(%s, $${cypher_inlined}$$) AS (v agtype)",
(self.GRAPH_NAME,),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-critical critical

The use of f-string interpolation to place cypher_inlined inside dollar-quoted tags ($$...$$) is vulnerable to SQL injection. If any parameter value contains the string $$, it will terminate the dollar quoting prematurely and allow execution of arbitrary SQL. Use standard parameter binding (%s) for the Cypher query string itself.

Suggested change
cur.execute(
f"SELECT * FROM cypher(%s, $${cypher_inlined}$$) AS (v agtype)",
(self.GRAPH_NAME,),
)
cur.execute(
"SELECT * FROM cypher(%s, %s) AS (v agtype)",
(self.GRAPH_NAME, cypher_inlined),
)

Comment thread mempalace/knowledge_graph_age.py Outdated
Comment on lines +738 to +739
for key in sorted(params.keys(), key=len, reverse=True):
cypher_inlined = cypher_inlined.replace(f"${key}", _cypher_literal(params[key]))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The manual parameter inlining logic using replace is fragile and susceptible to recursive replacement bugs. If a parameter value contains a string that matches another parameter's placeholder (e.g., $id), it will be incorrectly replaced during the loop. This can lead to malformed Cypher queries or data corruption. Use a single-pass replacement strategy with a regular expression.

Suggested change
for key in sorted(params.keys(), key=len, reverse=True):
cypher_inlined = cypher_inlined.replace(f"${key}", _cypher_literal(params[key]))
cypher_inlined = re.sub(
r"\$(\w+)",
lambda m: _cypher_literal(params.get(m.group(1))),
cypher
)

Comment thread mempalace/backfill_age.py
Comment on lines +194 to +197
sql_drawers = f"""
SELECT id, document, wing, room FROM "{table_name}"
WHERE document IS NOT NULL
"""

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

Direct interpolation of table_name into the SQL string is vulnerable to SQL injection. Even with double quotes, a malicious table name could break out of the identifier. Use psycopg2.sql.Identifier to safely handle dynamic table names.

Suggested change
sql_drawers = f"""
SELECT id, document, wing, room FROM "{table_name}"
WHERE document IS NOT NULL
"""
from psycopg2 import sql
sql_drawers = sql.SQL("""
SELECT id, document, wing, room FROM {}
WHERE document IS NOT NULL
""").format(sql.Identifier(table_name))

Comment thread mempalace/backfill_age.py
Comment on lines +220 to +231
kg._run_cypher(
"MERGE (d:Drawer {id: $id})",
{"id": drawer_id},
)
if room and room.strip():
kg._run_cypher(
"""
MATCH (r:Room {name: $room}), (d:Drawer {id: $id})
MERGE (r)-[:CONTAINS]->(d)
""",
{"room": room, "id": drawer_id},
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Executing _run_cypher for every drawer in a loop is extremely inefficient because KnowledgeGraphAGE._run_cypher performs a database commit on every call. For a large palace (e.g., 274K drawers), this results in massive overhead and explains the projected 22-hour runtime. Consider implementing a batching mechanism or wrapping the entire loop in a single transaction by managing the connection state externally.

Comment thread mempalace/multi_encoder.py Outdated
if did:
return f"id:{did}"
doc = payload.get("document") or ""
return f"doc:{hash(doc)}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Python's built-in hash() function is not stable across process restarts (due to hash randomization) or between different processes. This can cause identity mismatches in a multi-process environment where palace-daemon and the MCP server are separate processes. Use a stable cryptographic hash like hashlib.sha256 for consistent identity mapping.

Suggested change
return f"doc:{hash(doc)}"
import hashlib
doc = payload.get("document") or ""
return f"doc:{hashlib.sha256(doc.encode()).hexdigest()[:16]}"

Comment thread mempalace/mcp_server.py Outdated
Comment on lines +1113 to +1114
from .knowledge_graph_age import KnowledgeGraphAGE
kg = KnowledgeGraphAGE(dsn)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Directly instantiating KnowledgeGraphAGE bypasses the connection cache implemented in _get_kg(). This results in a new database connection being established for every call to tool_walk_palace. Use the existing _get_kg() helper to reuse connections.

Suggested change
from .knowledge_graph_age import KnowledgeGraphAGE
kg = KnowledgeGraphAGE(dsn)
try:
kg = _get_kg()

Comment on lines +292 to +300
hook = getattr(self, "_kg_writethrough", None)
if hook is not None:
for row in rows:
doc_id, document, metadata_json = row[2], row[3], row[5]
try:
metadata = json.loads(metadata_json) if metadata_json else {}
except (ValueError, TypeError):
metadata = {}
try:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Re-parsing metadata_json back into a dictionary is redundant and inefficient here, as the original metadata dictionaries were already available in the metadatas list before being serialized for the database insert. You should use the original metadata objects instead of re-parsing them from the serialized row data.

Suggested change
hook = getattr(self, "_kg_writethrough", None)
if hook is not None:
for row in rows:
doc_id, document, metadata_json = row[2], row[3], row[5]
try:
metadata = json.loads(metadata_json) if metadata_json else {}
except (ValueError, TypeError):
metadata = {}
try:
hook = getattr(self, "_kg_writethrough", None)
if hook is not None:
meta_map = {did: m for did, m in zip(ids, metadatas)} if metadatas else {}
for row in rows:
doc_id, document = row[2], row[3]
metadata = meta_map.get(doc_id, {})
try:

…pstream/develop merge

CHANGELOG.md: new [Unreleased] 2026-05-17 section above the existing
2026-05-14/15 cutover section. Documents the 6-phase AGE integration
(commits ff7187d, 3321d83, ff583c0, b3f0206, 8022ecb) with the +9pp
R@5 spike result that motivated it. Plus the upstream/develop merge
(6058489 / 342a59f) with the 12-file conflict resolution summary.

README.md: new "Walking the palace — Apache AGE integration (2026-05-17)"
section above the per-axis ship-list. Six-phase table, unified graph
schema, walk_palace primitive shape, AGE Cypher dialect gap inventory,
cross-fork verification note from nakata-app's independent audit.
Links PR #101 (this fork) and palace-daemon PR #25 (read-side fusion).
jphein added a commit to techempower-org/palace-daemon that referenced this pull request May 17, 2026
…tion

Updates the 2026-05-11 → 2026-05-15 push paragraph to 2026-05-11 →
2026-05-17 and adds the new /search/age-fused endpoint to the
fork-specific feature list. References:
- #25 (this PR)
- techempower-org/mempalace#101 (companion mempalace-side phases)
- 2026-05-17 AGE write-through spike (+9pp R@5)
jphein and others added 4 commits May 17, 2026 14:44
Four high-severity issues from the Gemini Code Assist review of
`#101` (2026-05-17):

knowledge_graph_age:
- Single-pass `re.sub` param inlining via `_inline_cypher_params`
  replaces the length-sorted `str.replace` loop. Values containing
  `$other_key` no longer trigger a recursive substitution.
- Outer dollar-quote tagged `$mp_age_q$...$mp_age_q$` instead of
  bare `$$`; `_cypher_literal` rejects any string containing the
  tag so a sanitized-but-adversarial value can't close the SQL
  boundary.
- New `commit=False` kwarg on `_run_cypher` / `_cypher_scalar` /
  `add_mention` plus a `kg.commit()` helper for batched writes.

backfill_age:
- `_validate_pg_identifier()` whitelists `[A-Za-z_][A-Za-z0-9_]*`
  before the table name is f-stringed into the SQL.
- Drawer-scan loop now batches `commit_every=100` (CLI flag
  `--commit-every`) instead of committing per drawer; checkpoint
  marks are written only after the matching KG commit succeeds.

Tests: 163 passing in `tests/ -k 'age or kg or knowledge_graph'`,
109 passing in `-k 'backfill or kg or knowledge_graph or writethrough'`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three medium-priority findings from the Gemini Code Assist review of
``#101`` (2026-05-17), follow-up to 551821b
which addressed the security-critical / high-priority items.

- **multi_encoder.py:_doc_key**: switched from ``hash(doc)`` to
  ``hashlib.sha256(doc.encode()).hexdigest()[:16]``. Python's built-in
  ``hash()`` randomizes per-process (PYTHONHASHSEED), so palace-daemon
  and the MCP server would assign different chunk identities to the
  same content, breaking RRF fusion. 16 hex chars = 64 bits, plenty of
  namespace for chunk-collision safety.

- **mcp_server.tool_walk_palace**: now uses ``_get_kg()`` (the cached
  per-DSN factory) instead of directly instantiating ``KnowledgeGraphAGE(dsn)``.
  Each direct call opened a fresh postgres connection; with the cache,
  walk_palace reuses the daemon-shared connection. Added an explicit
  ``kg_backend == "age"`` guard so wrong-backend invocations fail
  cleanly instead of silently returning a sqlite KG.

- **backends/postgres._insert_rows**: stopped re-parsing ``json.dumps(metadata)``
  back into a dict for the KG write-through hook. Now tracks a parallel
  ``metadata_by_id`` map in the row-build loop that holds the
  *post-pop* metadata (wing/room already removed) — matches the prior
  contract byte-identically while skipping the round-trip through
  ``json.loads``.

Tests: 190 passing in tests/ -k 'age or kg or knowledge_graph or
multi_encoder or writethrough'.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts:
#	FORK_CHANGELOG.md
#	docs/fork-changes.yaml
#	mempalace/multi_encoder.py
jphein and others added 5 commits May 21, 2026 17:03
Closes the two fork-specific CI failures on PR #101:

1. ``test_no_internal_coordination_jargon_in_source_or_tests``
   The fork's source/tests lint test forbids 'Phase N' / 'Task N.X.Y'
   internal-coordination references; three leaks remained:
   - ``mempalace/kg_writethrough.py:3`` — module docstring
   - ``mempalace/palace_graph_age.py:3, 236`` — module docstring + comment
   - ``mempalace/backends/postgres.py:295`` — KG write-through comment
   Replaced with feature-named phrasing ("inline KG enrichment",
   "AGE-integration inline enrichment"). Per
   feedback_no_phase_jargon_in_source.md / feedback_apply_naming_decision_actively.md.

2. ``test_no_undocumented_tools``
   ``mempalace_walk_palace`` was wired into TOOLS but not yet in
   ``website/reference/mcp-tools.md``. Added a Navigation Tools section
   covering the three start-anchor modes (wing/room/entity), the depth
   and limit knobs, and the AGE-backend prerequisite.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…quired

53 tests targeting the pure-function and mock-friendly slices of the
four AGE-KG modules. Closes the coverage gap that was blocking PR #101
merge (was 76.97%, target 79%).

Coverage delta (module-level, isolated):
  - kg_writethrough.py        0% → 97%
  - palace_graph_age.py       0% → 30% (populate_from_postgres needs live AGE)
  - backfill_age.py           0% → 29% (backfill() needs live AGE)
  - knowledge_graph_age.py   20% → 33% (write/query class methods need live AGE)

What's tested (all without DSN-dependent paths):
  - _cypher_literal: ValueError on outer dollar-quote tag injection;
    json.dumps fallback for non-string values.
  - _inline_cypher_params: named-placeholder substitution; unknown
    placeholders pass through; values containing $other_key are
    NOT recursively replaced (Gemini PR #101 fix regression guard);
    identity on no-placeholder cypher.
  - KnowledgeGraphAGE class helpers (via __new__, no postgres):
    _entity_id, _unwrap_agtype, _extract_return_aliases.
  - KnowledgeGraphAGE __init__/close/clear with a fake psycopg2 module
    + recording-cursor stand-in: bootstrap SQL is right, graph
    create is skipped when present, close+context-manager work,
    clear drop_graph fires only when graph exists.
  - kg_writethrough: _builtin_regex_extractor (proper nouns, tech
    idents, version strings, repeated-count via Counter, empty input);
    make_null_writethrough; make_age_writethrough (call-through to
    add_mention, swallows extractor and KG errors, respects
    max_entities_per_drawer); make_writethrough_from_env (disabled,
    requires-kg guard, null branch, unknown-extractor error,
    regex default).
  - backfill_age: _validate_pg_identifier (positive + 5 negative cases
    + non-string rejection); _get_extractor (regex fallback shape +
    unknown-name error); main() CLI argparse paths (missing --dsn
    exits non-zero, invalid table name rejected).
  - palace_graph_age: walk_wing parameter wiring, list_wings,
    list_rooms_in_wing, list_drawers_in_room, tunnels_from_wing
    (all with a mock KG that returns shaped row tuples).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…xtractor tests

Local coverage was 78.99% (one shy of 79%). Added tests for:
- kg_writethrough.make_age_writethrough early-return when the
  extractor returns no entities (covers the previously-uncovered
  line 104).
- backfill_age._ensure_checkpoint_table / _checkpoint_done /
  _checkpoint_mark / _checkpoint_clear via a recording-cursor
  connection mock — covers ~30 lines that previously needed live
  postgres.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jphein
jphein merged commit 3e350b0 into main May 22, 2026
7 checks passed
@jphein
jphein deleted the feat/age-kg-parity branch May 22, 2026 00:30
jphein added a commit to techempower-org/palace-daemon that referenced this pull request May 22, 2026
…int (#25)

* feat(search): /search/age-fused — vector + AGE graph fusion endpoint

Phase 5 of /goal: AGE integration. Adds POST /search/age-fused that
combines mempalace's vector retrieval (via mempalace_search MCP tool)
with AGE entity-overlap on the write-through graph populated by
mempalace.kg_writethrough + mempalace.backfill_age.

Pipeline:
  1. Vector retrieval: existing mempalace_search MCP path, over-fetches
     to give RRF more candidates.
  2. Query entity extraction: tries sme.extractors.regex.extract first,
     falls back to mempalace.kg_writethrough._builtin_regex_extractor.
  3. AGE lookup: MATCH (d:Drawer)-[r:MENTIONS]->(e:Entity {name})
     for each query entity, sum r.count across drawers.
  4. RRF fusion: combine vector + graph ranks via 1/(k + rank).
  5. Return top-limit drawers with matched_via ∈ {vector, graph, both}.

Falls back gracefully:
  - No MEMPALACE_POSTGRES_DSN → vector-only with warning trace.
  - AGE empty / query has no extractable entities → vector-only.
  - Cypher errors per-entity → skip that entity, continue.

Request body:
  {
    "query": "pgvector advisory lock race",
    "wing": "memorypalace",   // optional, exact-match filter
    "room": "problems",        // optional, must be canonical
    "limit": 10,
    "graph_top_k": 50,         // graph candidates to fetch
    "fusion_k": 60,            // RRF k constant
    "include_trace": false     // attach n_vector/n_graph counts
  }

Result hits carry an extra "matched_via" key plus an "rrf_score" so
callers can inspect which retrieval signal surfaced each drawer.

Cited from techempower-org/multipass-structural-memory-eval@28ae3f1:
the spike on n=200 git-derived probes showed graph-only beats vector
by +5pp R@5 and fusion adds another +4pp on top (file-level vectors).
This endpoint lands that retrieval pattern in production code path,
gated behind the new endpoint so vector-only behavior is preserved on
the default /search.

* docs(changelog): /search/age-fused endpoint — Phase 5 of multi-project AGE integration

Documents the new endpoint added in 9926499 with full pipeline shape,
graceful-degradation behavior, and cross-reference to the SME spike
result that motivated it.

Cross-project context: Phases 1-4+6 live on
techempower-org/mempalace:feat/age-kg-parity (KnowledgeGraphAGE API
parity, write-through middleware, palace structure as AGE nodes,
backfill script, mempalace_walk_palace MCP tool). This is Phase 5
landing the read-side fusion in the daemon's HTTP surface.

* chore(ci): add Gemini Code Assist config + AGENTS.md for PR-review context

Copilot Code Review has been failing repeatedly since weekly Copilot
credit ran out (6 failed runs in last 20). Switching to Gemini Code
Assist (free for public repos, already active on MemPalace upstream).

- .gitignore — narrow the .gemini/ ignore to .gemini/* + allow
  .gemini/config.yaml so the PR-review config is tracked, while
  per-developer local context (.gemini/notes/, .gemini/cache/) stays
  ignored.
- .gemini/config.yaml — MEDIUM severity threshold, summary+review on
  open, ignore lock files + test data + dist artifacts. No emoji.
- AGENTS.md — project context for any AI code-review agent (Gemini,
  Claude, Copilot). Describes single-writer safety, backend-agnostic
  endpoint contract, hook-path latency budget, auth requirements,
  warnings/errors pipeline. Lists review priorities and out-of-scope
  items. Mirrors the style of techempower-org/mempalace's AGENTS.md.

Install Gemini Code Assist at https://github.com/apps/gemini-code-assist
on techempower-org. Disable Copilot Code Review in repo Settings →
Code & automation.

* docs(readme): surface POST /search/age-fused — Phase 5 of AGE-integration

Updates the 2026-05-11 → 2026-05-15 push paragraph to 2026-05-11 →
2026-05-17 and adds the new /search/age-fused endpoint to the
fork-specific feature list. References:
- #25 (this PR)
- techempower-org/mempalace#101 (companion mempalace-side phases)
- 2026-05-17 AGE write-through spike (+9pp R@5)

* fix(search/age-fused): unblock event loop + tighten validation

Address Gemini Code Assist findings on `#25`:

- Wrap the synchronous `KnowledgeGraphAGE` Postgres I/O in
  `asyncio.to_thread()` via an inner `_age_lookup()` helper, so a
  slow AGE query stops blocking the daemon's event loop and other
  concurrent requests.
- Validate `graph_top_k` and `fusion_k` are 1..1000 before use
  (previously only `limit` was bounded).
- Initialize `query_entities = []` before the AGE-lookup `try` block
  so the trace branch can read it unconditionally; replaces the
  `[e.name for e in (query_entities if 'query_entities' in dir() else [])]`
  workaround with a plain comprehension.
- Use the module-level `logging` instead of a local re-import in the
  exception path.

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

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants