mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
37a73df0c0
* feat(memory): LLM-based NER + importance signal with cheap-signals short-circuit (#708)
Adds an Ollama-backed entity extractor and an LLM-derived importance
score as a new signal in the admission gate. Builds on merged Phase 2
(#733) without changing its surface — additive only.
Why LLM (not GLiNER): zero new deps (reuses existing reqwest + async
infra), reuses the same Ollama setup openhuman uses for embeddings,
better per-entity quality scales with model choice, no native ONNX
runtime to ship. Latency cost (~100-300ms per chunk on a small model
like qwen2.5:0.5b) is amortised by the short-circuit.
The admission gate stays a hybrid - cheap deterministic signals
always run; the LLM is one signal among many, never the sole arbiter.
Backwards-compatible: with default SignalWeights the LLM weight is 0.0
and behaviour matches pre-LLM Phase 2 exactly.
What's new:
- src/openhuman/memory/tree/score/extract/llm.rs: LlmEntityExtractor
implementing the existing EntityExtractor trait. Posts a structured
JSON request to an Ollama-compatible /api/chat endpoint asking for
NER + an importance rating in one call. Span recovery via string
search; hallucinated entities (surface not in source text) dropped.
Soft fallback: HTTP failures log a warn and return empty extraction.
- ExtractedEntities gains llm_importance (Option<f32>) +
llm_importance_reason (Option<String>); merge() takes max importance.
- ScoreSignals gains llm_importance (f32, defaults to 0.0).
- SignalWeights gains llm_importance (default 0.0; with_llm_enabled()
helper sets it to 2.0 - comparable to metadata/source weights).
- signals::combine_cheap_only(): variant that excludes the LLM signal,
used for the short-circuit decision in score_chunk.
- ScoringConfig gains llm_extractor (Option<Arc<dyn EntityExtractor>>),
definite_keep_threshold (default 0.85), definite_drop_threshold
(default 0.15). New with_llm_extractor() constructor.
- score_chunk() pipeline:
1. Always-on regex extraction
2. Cheap signals + combine_cheap_only -> cheap_total
3. If cheap_total >= definite_keep OR <= definite_drop: skip LLM
4. Else (borderline band): run LLM extractor, merge results, recompute
5. Final combine + admission gate against drop_threshold
- mem_tree_score table gains nullable llm_importance + llm_importance_reason
columns via idempotent ALTER TABLE migration.
- ScoreRow + upsert/get persist the new column.
What's not changed:
- Existing JSON-RPC surface
- Default behaviour (LLM weight=0, no llm_extractor configured = same
outputs as before)
- mem_tree_chunks / mem_tree_entity_index schemas
Tests added:
- LLM extractor: prompt construction, JSON parsing, hallucination
drop, importance clamping, strict vs lenient unknown-kind handling
- Score pipeline: short-circuit on definite_keep/definite_drop skips
LLM call (verified via FakeLlm call counter); borderline band
consults LLM once; LLM failure falls back gracefully without erroring
LLM-NER work follows up on #708. Parent: #711.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(memory): address CI failures and CodeRabbit review on LLM-NER (#708)
CI was red on PR #775 due to 10 E0063 errors — existing test-only struct
literals for ExtractedEntities / ScoreSignals / ScoreRow weren't updated
when the new optional llm_importance fields landed. Local cargo check
on lib-only passed; cargo check --tests caught them, as does CI.
CodeRabbit also flagged four semantic issues:
1. LlmEntityExtractor::extract() returned Err on HTTP transport, non-2xx
status, and JSON parse failures. The soft-fallback contract (documented
in the module header) only worked because score_chunk catches errors;
any other caller got a hard error. Now split into extract_or_empty()
which logs a warn on each failure mode and returns
ExtractedEntities::default() unconditionally. The public extract()
stays on the async-trait surface but never returns Err.
2. find_char_span() always searched from byte offset 0, so when the LLM
returned the same surface twice (explicitly asked for duplicates in
the prompt), both entities got (start=0, end=len) — same span, not
distinct mentions. Added find_char_span_from(haystack, needle,
byte_from, char_from) that resumes from a cursor. into_extracted_entities
now tracks a per-surface (byte_after, char_after) cursor in a
HashMap<String, (usize, u32)>, so duplicate mentions get distinct
non-overlapping spans and over-claim (LLM returns 3 "Alice" when
source has 2) drops the excess entries with a debug log.
3. combine() always included w.llm_importance in the denominator even
when llm_importance=0.0 because the LLM didn't run. That artificially
lowered the total on short-circuit paths. score_chunk now branches on
llm_consulted: uses combine() when LLM ran (importance actually
contributes), combine_cheap_only() when LLM was skipped or failed
(denominator excludes the LLM weight). Observable in two new tests:
short_circuit_reports_cheap_only_total verifies r.total ==
combine_cheap_only(r.signals, weights) and strictly exceeds
combine(r.signals, weights) when llm_importance=0; llm_consulted_
reports_full_total verifies the opposite path.
4. Same issue from a different angle (CodeRabbit flagged both). Fixed
by the same llm_consulted branch.
Test-literal fixes:
- signals/ops.rs: ScoreSignals literals pick up llm_importance field;
make_entities helper uses ..Default::default() on ExtractedEntities.
- extract/types.rs: three test ExtractedEntities literals gain
llm_importance: None, llm_importance_reason: None.
- resolver.rs: canonicalise_batch_preserves_spans literal gains the
two fields.
- score/store.rs: sample_row gains signals.llm_importance and
llm_importance_reason.
New tests (7 added):
- find_char_span_from_advances_past_prior_match
- find_char_span_from_returns_none_after_exhaustion
- find_char_span_from_preserves_utf8
- find_char_span_from_rejects_non_char_boundary
- into_extracted_entities_gives_distinct_spans_to_duplicate_mentions
- into_extracted_entities_drops_extra_duplicate_when_source_only_has_one
- extract_soft_fallback_on_unreachable_endpoint (transport failure →
empty extraction, not Err)
- short_circuit_reports_cheap_only_total
- llm_consulted_reports_full_total
cargo check --lib clean; cargo fmt clean. The integration-test rmeta
errors visible locally are stale-metadata artifacts from incremental
builds with prior struct shapes; CI's clean build resolves them and
tests/*.rs files do not construct any of the touched types.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(memory): source trees + bucket-seal foundation (#709)
Phase 3a of the memory architecture (#711 umbrella). Lifts admitted leaves
into a per-source hierarchy so Phase 4 retrieval has a tree to walk.
## What's in this PR
* **Schema** — three new tables in `chunks.db` alongside Phase 1/2:
`mem_tree_trees`, `mem_tree_summaries`, `mem_tree_buffers`. Plus a
`parent_summary_id` backlink column on `mem_tree_chunks`. All migrations
are additive and idempotent (`ALTER TABLE ADD COLUMN`, same pattern as
Phase 2).
* **`source_tree/` module** — isolated subdir; does not touch the legacy
`openhuman::memory` layer.
- `types.rs` — `Tree`, `SummaryNode`, `Buffer`, `TreeKind` (Source/Topic/Global),
`TreeStatus`. Constants: `TOKEN_BUDGET = 10_000`, `DEFAULT_FLUSH_AGE_SECS = 7d`.
- `store.rs` — CRUD for all three tables via the shared `with_connection`
entry point. Writes are transactional; summary insert is idempotent
on primary key (INSERT OR IGNORE).
- `registry.rs` — `get_or_create_source_tree(scope)` — idempotent lookup
keyed on `(kind, scope)`.
- `bucket_seal.rs` — `append_leaf` + cascade: push a leaf into L0, seal
when `token_sum >= TOKEN_BUDGET`, recurse upward. One seal = one
transaction; children get a parent backlink (leaves via
`parent_summary_id`, summaries via `parent_id`). Tree's `max_level`
and `root_id` bump on root split.
- `flush.rs` — `flush_stale_buffers(max_age)` force-seals any buffer
whose `oldest_at` is older than `max_age`. Primitive only; wiring
into a scheduler is out of scope.
- `summariser/` — `Summariser` trait + `InertSummariser` fallback
(concatenate children, union entities preserving first-seen order,
hard-truncate to budget). Trait is async; Ollama implementation slots
in later without breaking callers.
* **Ingest wiring** — `tree/ingest.rs::persist` now calls `append_leaf`
once per kept chunk after chunks + scores land. Failures are logged at
warn level and do not fail the ingest — leaves are already persisted
and the next flush can still rebuild the tree.
## Concurrency / LLM
The async summariser call happens OUTSIDE any DB transaction, so a slow
LLM never holds SQLite locks. Blocking DB calls inside `append_leaf` are
acceptable for Phase 3a because the Inert summariser does no real I/O;
when a networked summariser lands, wrap the DB calls in
`tokio::task::spawn_blocking`.
## Tests (23 new, all green)
* `types` — enum round-trips, buffer staleness predicates
* `store` — tree insert + unique scope, summary insert + idempotence,
buffer upsert/clear, stale-buffer ordering
* `registry` — `get_or_create` idempotence, distinct scopes yield distinct
trees, id prefix format
* `summariser/inert` — provenance-prefixed concat, entity union with
order preservation, budget truncation, empty-content skip
* `bucket_seal` — append-below-budget is buffered only (no seal);
12k-token cross-over produces an L1 summary with correct `child_ids`,
L0 cleared, L1 carries the new summary id, tree `max_level`/`root_id`
updated, leaf → parent backlink populated
* `flush` — stale buffer force-seals under budget; recent buffer is a no-op
Broader `memory::tree` suite: 160 tests pass (23 new + 137 existing Phase
1/2), no regressions.
## Stacked on #775
This PR applies on top of `feat/708-memory-llm-ner` (PR #775, Phase 2
LLM-NER follow-up). Merge #775 first; this branch will rebase cleanly
onto main.
## Out of scope (tracked separately)
* **3b — Global activity digest tree** — time-aligned cross-source recap
* **3c — Topic trees** — hotness-driven per-entity materialisation
* **Phase 4 (#710)** — retrieval / query / gate
Closes part of #709 (source-tree foundation).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* style(memory): cargo fmt on source_tree module (#709)
CI rustfmt caught a few multi-line function signatures and import
groups that rustfmt wants collapsed / reordered. No behaviour change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(memory): address CodeRabbit review on source-tree (#709)
Three correctness fixes from CodeRabbit on PR #789, plus a design
clarification for the InertSummariser. False-positive findings
(schema-out-of-sync, rustfmt — already in b7395a8) are not addressed
here because they are not real issues.
## Changes
### Idempotent buffer append (`bucket_seal.rs`)
`append_to_buffer` commits before the async seal/cascade runs, so a
retry after a failed seal (summariser timeout, crash, transient ingest
error) would re-push the same leaf and double-count tokens. Added a
dedup check on `item_ids` so the append is a true no-op for
already-buffered chunks.
### InertSummariser: honest stub, no entity propagation (`summariser/inert.rs`)
Summary-level entities and topics are **LLM-derived from the summary's
own synthesised content** by design, not a mechanical union of
children's labels. The networked summariser (future) does its own
extraction on its output. The inert fallback has no NER, so it emits
empty entities/topics — an honest stub rather than a misleading union.
Removed `union_preserve_order` helper and updated the corresponding
test.
### Forward-compat entity index on seal (`bucket_seal.rs`, `score/store.rs`)
Added `index_summary_entity_ids_tx` — a summary-specific indexer that
takes canonical ids only (matching the `Vec<String>` shape of
`SummaryOutput.entities`) and writes them into `mem_tree_entity_index`
with `node_kind='summary'`, placeholder `entity_kind`/`surface` (both
set to the canonical id), and the summary's score. Called from
`seal_one_level`'s write transaction. No-op today (InertSummariser
emits empty), but wired so the Ollama summariser lands as a
drop-in without touching bucket-seal.
### UNIQUE race in `get_or_create_source_tree` (`registry.rs`)
Two concurrent callers for the same scope could both pass the lookup
and then race on `insert_tree`; the loser got a UNIQUE violation
bubbled as an error instead of the pre-existing row. Now catches
`ConstraintViolation` (and the message-based fallback for chained
errors), re-queries by scope, and returns the winning row. New test
`get_or_create_recovers_from_unique_race` covers both the recovery
path and the `is_unique_violation` detector.
## Tests
161 passed / 0 failed across `memory::tree` (+1 new test vs the
prior baseline of 160). No regressions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* style(memory): cargo fmt on source_tree::registry (#709)
CI's rustfmt check flagged the race-recovery block in
get_or_create_source_tree — rustfmt prefers chaining
`?.ok_or_else(...)` on one line and wrapping the assert! macro.
No behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(memory): index_summary_entity_ids_tx writes parseable entity_kind (#709)
CodeRabbit on PR #789 caught a bug in the summary entity indexer I
added for Phase 3a. The helper was writing the full canonical id
(e.g. "email:alice@example.com") into the `entity_kind` column instead
of the kind prefix. `lookup_entity()` later runs `EntityKind::parse()`
on that column and would fail with `unknown EntityKind` the first time
a result set mixed leaf and summary hits — blocking any Phase 4 read
that needed summary-level entity resolution.
Fix: split the canonical id on `:` and store the prefix only. Falls
back to the full id with a `warn!` for malformed ids that lack `:`,
so bad data surfaces rather than silently stays latent.
Added regression test `summary_entity_index_kind_is_parseable` that:
- Indexes a leaf entity row (the existing happy path)
- Indexes two summary entity rows via index_summary_entity_ids_tx
- Runs lookup_entity for email, hashtag, and a cross-kind id
- Asserts each row comes back with a correctly-parsed EntityKind
Before the fix, the lookup_entity calls in this test would fail with
a FromSqlConversionFailure on the summary row's entity_kind column.
After the fix, mixed leaf+summary lookups round-trip cleanly.
Tests: 162 passed / 0 failed in memory::tree (+1 vs 161 baseline).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(memory): promote extracted topics to canonical entities (#709)
Addresses a design gap surfaced during PR #789 review: the Phase 3
plan promised topic trees would scope to "entities / topics" but the
implementation routed on canonical_entities only. A chunk saying
"Phoenix migration ships Friday" produced `r.extracted.topics =
["phoenix", "migration"]` but `canonical_entities = []` — meaning no
topic tree named Phoenix would ever spawn.
Fix (Shape A from the review discussion): extend the canonical entity
stream with topic rows. No new tables, no new routing path, no new
hotness counters.
## Changes
- `score/extract/types.rs`: add `EntityKind::Topic` with as_str/parse
round-trip. `non_exhaustive` enum so this is an additive change.
- `score/resolver.rs::canonicalise`: after emitting canonical entities
from `extracted.entities`, also emit one per `extracted.topics`
entry with `kind: Topic`, `canonical_id: "topic:<lowercased>"`.
Span fields are 0 (topics are chunk-level, not substring-scoped).
Dedups same-label topics internally; keeps `hashtag:launch` and
`topic:launch` as separate entities by design.
## Downstream effect (zero-touch)
- `score::store::index_entities_tx` indexes topic entities the same
way as email entities — rows land in `mem_tree_entity_index` with
`entity_kind = "topic"`.
- Phase 3c routing iterates `canonical_entities` — topics now trigger
topic-tree routing automatically. A chunk about "phoenix" starts
accumulating hotness for `topic:phoenix`; once the threshold
crosses, `get_or_create_topic_tree("topic:phoenix")` materialises
a dedicated topic tree. Backfill via `lookup_entity("topic:phoenix")`
then hydrates historic leaves.
- Phase 4 retrieval can filter `WHERE entity_kind = 'topic'` to
surface themes without mixing in people/emails.
## Tests
Five new tests in `resolver.rs`:
- topics → canonical entities with `kind: Topic`
- lowercase normalisation + dedup on label
- hashtag + topic with the same label coexist (different kind prefix)
- entities emitted first, topics appended
- `EntityKind::Topic` round-trips through `parse` / `as_str`
Broader memory::tree: 167 passed / 0 failed (162 baseline + 5 new).
No regressions.
## Caveats for follow-up
1. Topic noise — LLM-extracted themes are softer signal than regex
entities. Hotness threshold (TOPIC_CREATION_THRESHOLD=10.0) gates
tree creation, so transient topics won't spawn trees.
2. Topic/hashtag duplication — "Phoenix #phoenix" creates both
`topic:phoenix` and `hashtag:phoenix` trees. Tolerable (kinds are
semantically distinct) but a dedup policy may be worth revisiting
in a Phase 4 retrieval follow-up.
3. Topic normalisation is lowercase+trim only. Plurals / stemming
are deferred.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Steven Enamakel <31011319+senamakel@users.noreply.github.com>