Independent technical review: ran the suite, the evals, and probed the core (v1.2.5)

[rugbyvet]

Independent technical review: ran the suite, the evals, and probed the core (v1.2.5)

Heard you wanted constructive technical feedback, so here's an actual run of the code. Everything below was executed locally, offline, no API keys, no network calls to LLM providers. Clone at v1.2.5, editable install, Python 3.14.4.

Bottom line first: this is a serious, unusually well-disciplined codebase. 2,238 tests pass offline in ~63s, and every benchmark number I could reproduce offline came back exactly. The items below are ranked by leverage. Each is a gift, not an attack.

Setup

Zero friction. pip install -e . pulled exactly numpy + engraphis. engraphis-init writing a 0600 .env and printing per-client MCP snippets is genuinely good onboarding. Two nits: classifiers list 3.9–3.12 but the whole suite passes on 3.14, and the README's "first run downloads all-MiniLM-L6-v2" line could use one sentence clarifying that the plain install never downloads it (deterministic embedder is the silent default).

What I verified

• Six-term score: real (core/scoring.py:100-110, all six terms plus the staleness penalty, per-type weights at :40-45).
• 31 MCP tools: real (31 @mcp.tool decorators in mcp_server.py).
• Deterministic offline embedder works end-to-end with no sentence-transformers: recall, conflict resolution, grounded answers, unicode, clean ValidationError on empty queries.
• Grounded abstention works: off-topic question abstains (support 0.175 < 0.25 floor), on-topic answers with citation (0.543).
• Bi-temporal supersession correct, including backdated-write splicing and the predecessor guard (engine.py:463-508).
• Concurrency: 8 threads × 20 writes, 0 errors, 0 lost updates.
• Path traversal and SQL injection: properly guarded (realpath containment, parameterized queries throughout).

Eval reproduction (exact commands, exact outcomes)

python -m pytest tests/ -q --tb=no            # 2238 passed, 3 skipped (Windows/release-only)
python -m eval.chunking_eval                   # 73.0% reduction, reproduced exactly (808.8→218.4, 162.2→42.4)
python -m eval.grounded                        # 10/10 correct abstain/answer
python -m eval.performance --dataset eval/datasets/codemem.jsonl --k 5 --iterations 5
                                               # recall@5 1.000, warm p50 1.9ms
python -m eval.context_economy --dataset eval/datasets/codemem.jsonl --token-budget 512 --k 5
                                               # savings_ratio -0.167, break_even null (your own honest negative, reproduced)
python -m eval.harness --dataset eval/datasets/graph_multihop.jsonl --k 5
                                               # recall@5 0.333 (shipped as a gate, respect)

A repo that ships its own negative result and reproduces it on a stranger's machine is trustworthy in a way most OSS projects are not.

Ranked findings

F1 [High] Two live engines, divergent scoring, same package. Classic dashboard API (app.py:22, routes/memory.py:15-16) serves the legacy 3-term engine (engines/recall.py:46, engines/reweight.py:40) while MCP/Ledger serve the six-term hybrid (core/recall.py, core/scoring.py:100-110). Same SQLite file, different ranking. Worse: the two decay implementations disagree on stability=0.0: core/scoring.py:54's max(stability or 1.0, 1e-3) treats explicit zero as 1.0, while engines/reweight.py:40 makes it 0.01, so zero-stability memories decay 100× differently depending on which surface touched them. Fix: migrate Classic routes to the six-term engine behind a compat flag, or pin the divergence with a same-ranking test on a fixture corpus and document it.

F2 [Medium] The paraphrase conflict path is inert on the default offline embedder. core/resolve.py:41 needs cosine ≥ 0.90 to fire the paraphrase branch. I measured the deterministic hashing embedder on realistic pairs: max ≈ 0.63 ("auth migrated from JWT to PASETO" vs "authentication switched from jwt tokens to paseto" = 0.627; the "100 rpm vs five hundred requests" pair the comment explicitly names = 0.031). So on the advertised numpy-only install, paraphrased contradictions silently degrade to ADD: both facts kept, no supersession. Fix: per-embedder threshold (the hashing embedder needs ~0.5), a token-trigram fallback signal, or a README note that paraphrase detection needs a real embedder.

F3 [Medium] Default decay starves retention-only recall. R(t) = exp(-Δt/S) with default S=1.0 day: retention ≈ 0.0009 after a week untouched. recall_master ranks purely on retention × surprise, so any fact not touched for ~a week is effectively unreachable by proactive recall. The math is defensible; the default S and the pure-retention ranking are worth revisiting, and the forgetting curve deserves a README mention with the retention_class knobs.

F4 [Low-Med] engraphis_recall's score is min-max-normalized per query. Not comparable across queries; a pool of garbage still yields a top score near 1.0. grounded.py:152-158 already works around this correctly by recomputing absolute support, but the raw MCP field ships with no caveat. Fix: document it as rank-relative, or expose an absolute cosine alongside.

F5 [Low] The headline 98.2% LoCoMo number is from an unpinned-model run. Your own BENCHMARKS.md:75-76 says so, but the caveat sits in a collapsed <details> block ~20 lines below the chart. The 73.0%/73.9% chunking numbers I reproduced exactly; they're solid. Pin the revision, rerun, or move the caveat next to the headline.

F6 [Low] NumPy vector backend is O(n) full-scan. Honestly documented in its own docstring. Worth stating a scale envelope in the README ("comfortable to ~10k memories") so users know when to reach for sqlite-vec.

F7 [Low] Classifiers. Add 3.13/3.14; the suite passes on 3.14.

F8 [Medium] No red-team fixture proves the poisoning containment actually holds. Your containment design is genuinely good: fenced synthesis with an explicit ignore-directives system prompt (core/grounded.py:381-396), out-of-band vouching so forged extractor metadata can't mint trusted graph edges (core/engine.py:131-167), fan-out caps (engine.py:57), supersede-never-delete. But it's all containment, no detection, and nothing in the suite attacks it: tests/test_benchmark_adversarial.py is an eval-methodology fixture (off-topic exclusion from scoring denominators), not a red-team suite. The attack class to simulate is the delayed trigger (arXiv:2605.08442): a malicious instruction rides in through retrieved content, gets stored as a memory, and fires sessions later when an agent recalls it. In that paper's 5,040-run evaluation, five of five defense layers failed and only gating the memory tool held. A fixture that (a) stores delayed-trigger payloads, (b) runs recall and grounded synthesis against them, and (c) asserts the payload never executes and never steers the synthesized answer would both prove your fencing and catch regressions in it. Given OWASP's Agentic Top 10 lists memory poisoning as its own category (ASI06), shipping that test would also be a differentiator: almost nobody can demonstrate their containment holds, they'd only assert it.

Quick wins

1. Ollama quick-start (highest ROI, given where you posted). validate_llm_base_url (llm/client.py:29-68) already accepts http://localhost:11434/v1, I verified. But "Ollama" appears nowhere in README, docs, or .env.example. A 10-line section (ENGRAPHIS_LLM_PROVIDER=openai, ENGRAPHIS_LLM_BASE_URL=http://localhost:11434/v1, ENGRAPHIS_LLM_MODEL=qwen2.5-coder:latest) answers your Discord audience directly. Optionally an ollama convenience provider name.
2. One-paragraph "what conflict resolution can and can't detect offline" note (ties to F2).
3. Pin the LoCoMo run, move the caveat next to the chart, update classifiers.

What's genuinely impressive

1. The honesty. BENCHMARKS.md lists its non-claims first, flags its own LoCoMo run as noncanonical, ships a 0.333 multihop gate, and publishes a negative savings ratio. Most projects twice this size cherry-pick. Keep it.
2. _SerializedConnection (core/store.py:538-618): lock-pinned transaction ownership with rollback-on-failure is the correct answer to the shared-connection SQLite problem. You designed out a classic bug class.
3. Groundedness engineering (core/grounded.py): absolute support floor, extractive-only defaults, ordered-span verification that even catches "approved alpha, not beta" token-reuse reversal. Most RAG products never attempt this.
4. Determinism discipline: hashseed-independent sorts, id tie-breaks, 2,241 tests in 63s with zero network. This is why you can make reproducible claims at all.
5. The write path: claim-key identity + temporal predecessor splicing + the valid_from guard is subtle, correct bi-temporal engineering.

Happy to share raw command output for anything above.

[Coding-Dev-Tools]

Thanks for putting this together. Everything you flagged is fixed in the latest release. The dual engines are unified, per-embedder thresholds are in, decay defaults are bumped with ranking options, score normalization is documented, the scale envelope is in the README, classifiers are updated, and there's a red-team fixture covering the delayed-trigger case. Ollama quick-start is in the docs too.

Appreciate you actually running the suite and evals offline. If you rerun against the new version and find anything interesting please let me know!

[richardchen874-sys]

"Independent technical review: ran the suite, the evals, and probed the core (v1.2.5)" is a good agent-runtime surface because the real cost is whether the workflow completes cleanly across tools, retries, and provider behavior.

A compact provider matrix could track streaming, tool calls, structured output, usage reporting, timeout behavior, and fallback safety. That makes routing decisions easier to explain later.

I am testing a multi-model OpenAI-compatible API layer around official Chinese models, so the routing and usage-accounting parts of this are directly relevant to me. Which provider difference causes the most friction here: tools, streaming, usage reporting, or fallback behavior?