Skip to content

fix(knowledge_graph): use named column access instead of hardcoded integer indices - #89

Open
christauff wants to merge 3 commits into
MemPalace:developfrom
christauff:fix/kg-column-indices
Open

fix(knowledge_graph): use named column access instead of hardcoded integer indices#89
christauff wants to merge 3 commits into
MemPalace:developfrom
christauff:fix/kg-column-indices

Conversation

@christauff

Copy link
Copy Markdown

Problem

query_entity(), query_relationship(), and timeline() access JOIN query results using hardcoded integer column indices (row[2], row[4], row[5], row[6], row[7], row[10], row[11]).

These indices are derived from the position of columns in SELECT t.*, ... — which means any schema migration (adding a column, reordering columns, changing a JOIN) silently corrupts results without raising an exception. The bug would surface as wrong data returned from graph queries.

Example from query_entity() outgoing direction:

results.append({
    "subject": name,
    "predicate": row[2],   # triples.predicate — but fragile
    "object": row[10],     # obj_name from JOIN — breaks if schema changes
    ...
})

Fix

Use cursor.description to build a column name → value dict after every execute() call. All field access now uses named keys:

cur = conn.execute(query, params)
cols = [d[0] for d in cur.description]
for raw in cur.fetchall():
    row = dict(zip(cols, raw))
    results.append({
        "predicate": row["predicate"],
        "object": row["obj_name"],
        ...
    })

This is schema-safe: the column name contract is expressed in the SQL AS alias, not in a magic integer offset.

Test plan

  • kg.query_entity("Alice", direction="both") returns correct subject/predicate/object names
  • kg.query_relationship("child_of") returns correct sub_name/obj_name
  • kg.timeline() returns facts in chronological order with correct field values
  • Add a column to triples table schema and verify queries still return correct data

🤖 Generated with Claude Code

christauff and others added 3 commits April 7, 2026 10:47
query_entity(), query_relationship(), and timeline() accessed JOIN query
results by hardcoded integer indices (row[10], row[11], row[2], etc.).
Any schema migration or column reorder silently corrupts the returned
data with no error signal.

Use cursor.description to build a column-name map, then access results
by name (row["predicate"], row["obj_name"], etc.). The output dict
keys are unchanged — this is a safe internal refactor with no API
impact.
The conn.close() was called before the list comprehension that calls
cur.fetchall(), causing sqlite3.ProgrammingError on a closed database.
Move conn.close() after the results are materialized.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers query_entity (outgoing/incoming/both/temporal), query_relationship,
timeline, timeline with entity filter, invalidate, and stats. Verifies
named fields are populated correctly after cursor.description refactor.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GoodOlClint added a commit to GoodOlClint/mempalace that referenced this pull request Apr 7, 2026
Replaces hardcoded integer indices (row[10], row[11]) with named
dict access via cursor.description. Prevents silent data corruption
if the JOIN column order changes.

Upstream: MemPalace#89

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@bensig

bensig commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Clean fix. CI lint is failing — can you run ruff check . && ruff format --check . and fix any issues?

@adv3nt3 adv3nt3 mentioned this pull request Apr 7, 2026

@web3guru888 web3guru888 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.

🔧 Review of #89fix(knowledge_graph): use named column access instead of hardcoded integer indices

Scope: +180/−41 · 2 file(s) · touches core

  • ⚠️ mempalace/knowledge_graph.py (modified: +50/−41)
  • tests/test_knowledge_graph.py (added: +130/−0)

Technical Analysis

  • 🕸️ Knowledge graph changes — verify temporal triple ordering and connection lifecycle

Issues

  • ⚠️ Touches mempalace/knowledge_graph.py — Core KG — threading and concurrency sensitive

Suggestions

  • Magic number(s) 2015, 2025 — consider extracting to named constant(s)

Strengths

  • ✅ Includes test coverage

🟡 Needs attention — touches guarded files and has items to address.


🏛️ Reviewed by MemPalace-AGI · Autonomous research system with perfect memory · Showcase: Truth Palace of Atlantis

@bensig
bensig changed the base branch from main to develop April 11, 2026 22:23
@igorls igorls added bug Something isn't working area/kg Knowledge graph labels Apr 14, 2026
@igorls

igorls commented May 8, 2026

Copy link
Copy Markdown
Member

Hi, thanks for the contribution.

This PR has merge conflicts with develop, and the branch has not been updated in over 7 days, which puts it before our most recent release. The conflicts are likely against work that landed in that release.

Could you rebase onto develop so we can take another look?

If this change is no longer relevant, feel free to close the PR.

(This message is part of a periodic backlog pass, sent to all open PRs that match this state.)

@igorls igorls added the needs-rebase PR has merge conflicts with develop and needs rebase label May 8, 2026
igorls pushed a commit that referenced this pull request Aug 2, 2026
… audit found

hnsw_capacity_status() (chroma.py) exists precisely to preflight the
#1222 SIGSEGV/pyo3-panic class before anything touches the HNSW
segment, but repo-wide it was wired into only 4 call sites while raw
count()/collection.count() is called at 20+ others -- a bare
except Exception around count() cannot catch a native crash, since
the process dies regardless of any Python try/except. This wires the
existing, already-tested probe into the 7 remaining call sites the
audit identified as CRITICAL:

- #89 palace.py::_enforce_embedder_identity -- the universal
  get_collection() chokepoint every tool passes through, previously
  guarded only by except Exception. Highest leverage: skips this
  bookkeeping-only check on divergence instead of risking count().
- #90 migrate.py::migrate -- routes straight to the same
  SQLite-extraction fallback the except branch already used, instead
  of ever reaching col.count() when diverged.
- #91 repair.py::scan_palace / prune_corrupt -- both abort with the
  existing from-sqlite recovery guidance instead of opening the
  collection.
- #10 repair.py::rebuild_index -- preflights divergence alongside its
  existing sqlite-integrity and poisoned-max-seq-id preflights, before
  opening the collection.
- #13 repair.py::rebuild_index never rebuilt or reported on the
  closets collection -- now warns when closets is still diverged
  after a drawers-only rebuild, pointing at --mode from-sqlite instead
  of letting 'Repair complete' stand unqualified.
- #92 dedup.py::get_source_groups -- takes an optional palace_path
  (threaded from both callers) to preflight before count(); omitted by
  existing tests, which keep their pre-existing behavior.
- #93 miner.py::status -- preflights before the ChromaDB-client
  fallback path (used when the direct sqlite read is unavailable).

7 new regression tests, each confirmed failing against the pre-fix
code (via git stash of the source files only) and passing after the
fix. One existing dedup.py test updated for the new palace_path kwarg
in its call-signature assertion. Full suite: 3154 passed, 1 unrelated
pre-existing flake (test_mcp_server.py peer-writer-lock module-global
state leaking across test files in full-suite ordering -- this diff
never touches mcp_server.py).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/kg Knowledge graph bug Something isn't working needs-rebase PR has merge conflicts with develop and needs rebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants