Constrain edge endpoints to the extracted entity set, and salvage the rest instead of dropping them - #285
Conversation
…hema Extracts the control-char-stripping/trim/empty-drop logic already used by edge_user_prompt into a standalone, dedup-adding helper so the upcoming tool-schema enum (FR-001) can reuse identical sanitization.
…y set Adds build_edge_tool_schema, which puts an enum on source_name/target_name built from the sanitized entity list, so the Anthropic tool-use call enforces the endpoint contract at the schema level (FR-001) rather than relying on prompt text the model may ignore. Skips the edge-extraction HTTP call entirely when the sanitized name list is empty, since an empty enum is invalid schema.
…y test extract_text.txt/extract_message.txt/extract_json.txt each banned "abstract concepts" outright while the closed ontology (mod.rs) offers a Concept entity type — the header ban silently won on unchunked input, discarding the very entity the edge pass hubs its facts on (#281). Reword the ban to distinguish a vague standalone feeling (never extract) from a specific, named concept passing the existing "Wikipedia article" specificity test (extract as Concept), and add a worked example to each prompt.
…mmit Pre-lock edge validation used to permanently drop any edge whose endpoint wasn't in the batch or a persisted-entity lookup, before Phase C's lock-held resolution (which also checks the persisted graph) ever got a chance to rescue it — two independent, easily-desynced resolution passes. Pre-lock now only filters self-referential edges (always correct) and salvages an off-list endpoint by cosine-matching its name embedding against the batch's own entity name embeddings (reusing DEDUP_THRESHOLD), rewriting the edge to the matched entity's canonical name. Anything left unsalvaged passes through to Phase C, which becomes the sole authoritative point that resolves against the persisted graph or finally drops an edge — now counted as edges_dropped_unresolvable on AddEpisodeResult.
…hunk Surfaces the count of edges dropped for unresolvable endpoints (FR-004) in the JSON result, rather than leaving it observable only via stderr.
…ting Adds integration coverage for the salvage/deferred-drop rework: an off-list endpoint whose name embedding closely matches a batch entity is salvaged rather than dropped; a below-threshold adversarial near-miss (e.g. carbon dioxide vs carbon monoxide) is not force-matched; and a genuinely unresolvable edge is dropped and surfaces in edges_dropped_unresolvable. Extends ipc_parity's process_chunk test to assert the new field is present and numeric.
Documents why pre-lock edge validation is now advisory-only (self- referential filtering plus salvage), why the #209 pre-lock persisted- entity lookup was removed rather than kept alongside salvage, and why Phase C is the sole authoritative drop/count point — including why this is what makes FR-006 hold on the OpenAI-compatible path, which has no tool-use schema to constrain.
|
🏭 Fabrik — stage: Implement Everything is committed and pushed, local branch matches remote, working tree clean. All plan tasks are complete. Now let's emit the PR creation marker and complete the stage. Implemented all three fixes from the plan: an Used 2/100 turns, 0k input / 2k output tokens. |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughEdge extraction now sanitizes entity names, constrains tool-schema endpoints to those names, and permits specific named abstract concepts. Episode ingestion salvages off-batch endpoints through embedding similarity, defers final endpoint resolution to the commit phase, drops unresolved relationships there, and reports inserted and dropped edge counts. The new counter is propagated through the ingestion response and documented. Tests cover schema generation, empty inputs, salvage behavior, unresolved edges, self-loop prevention, and IPC output. Possibly related issues
Comment |
![handarbeit-pruefer[bot]](https://avatars.githubusercontent.com/in/4408765?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have what I need. Let me finalize the review.
I reviewed the diff-equivalent changes across crates/core/src/extractor.rs (FR-001 schema enum), crates/core/src/prompts/mod.rs and the three extract_*.txt prompts (FR-002 concept-ban rewording), crates/core/src/episode.rs (FR-003/005 salvage + deferred drop), crates/core/src/handlers.rs (FR-004 reporting), the new integration tests in edge_endpoint_resolution.rs, and ADR-0051. Since this sandbox's git history only contains the PR head commit with no reachable main/parent objects, I inspected the current state of these files directly rather than a line diff — findings below are anchored to the current file content, which should map onto the PR's diff lines.
The overall design is sound and well-documented (ADR-0051 clearly explains the two-pass-to-one-authoritative-pass consolidation), and the schema-enum + salvage-then-defer combination is a good fix for the underlying defect. I found one real correctness gap worth addressing before merge: the pre-lock self-referential-edge filter runs before the salvage rewrite, so a salvage match that rewrites one or both endpoints to the same canonical entity name can produce a self-loop edge that nothing downstream catches (Phase C has no self-loop check either). This is plausible in practice — e.g. an edge like ("Acme", "Acme Corp", ...) where "Acme" is off-list and salvage-matches back to "Acme Corp" — and isn't covered by the new adversarial/salvage tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/core/src/episode.rs (1)
244-347: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftParallelize salvage embedding lookups — the
missing_namesloop serializes every off-batch endpoint embed before Phase B even starts. The embedder already supports concurrent requests, so batching these withtry_join_all/similar would cut tail latency on chunks with many salvageable endpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 653bf16f-a246-463a-8da1-e76cc84b6a40
📒 Files selected for processing (13)
README.mdcrates/core/src/episode.rscrates/core/src/extractor.rscrates/core/src/handlers.rscrates/core/src/prompts/extract_json.txtcrates/core/src/prompts/extract_message.txtcrates/core/src/prompts/extract_text.txtcrates/core/src/prompts/mod.rscrates/core/tests/edge_endpoint_resolution.rscrates/core/tests/ipc_parity.rsdocs/adr/0051-edge-endpoint-salvage-and-deferred-drop.mddocs/adr/index.mdspecs/281-constrain-edge-endpoints-to/spec.md
Salvage can rewrite an off-list endpoint to match the entity name already used by the edge's other endpoint (e.g. "Global Warming" salvaging to "Climate Change" when the edge already targets "Climate Change"), producing a self-loop that the earlier pre-salvage self-referential filter can't see and Phase C has no guard against. Re-run the filter after salvage so such edges are dropped instead of persisted as self-referential relationships.
|
🏭 Fabrik — stage: Review Everything is committed, pushed, and the working tree is clean. Review complete. Review FindingsFixed
Verified
BlockingNone. Reviewed the #281 implementation end-to-end and found one real bug: salvage rewriting an off-list edge endpoint could collide with the edge's other endpoint, producing an unguarded self-referential edge that would have been silently persisted. Fixed by re-running the self-referential filter after salvage, added a regression test, and confirmed with a full local gate (fmt, 800 tests, clippy -D warnings, all clean). Fix is committed and pushed; PR #285 is ready for human review. Used 43/100 turns, 11k input / 11k output tokens. |
CodeRabbit flagged that the comment claimed Phase C reuses db_shared's Arc, but Phase C actually reloads its own handle (db_c) independently right before the write lock. Clarify the comment so a future contributor doesn't "deduplicate" that reload as redundant and reintroduce a stale-handle risk around a concurrent clear_all.
|
🏭 Fabrik — stage: Review (review feedback addressed) Both review threads resolved. No issue-body changes are needed — this was PR-level review feedback, not a spec change. Summary
Threads addressed:
Resolved 2 review thread(s) across 2 comment(s). |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/core/src/episode.rs (1)
607-618: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject self-loops after UUID resolution.
The name-level check misses Phase B dedup merges: two distinct names can map to one UUID in
name_to_uuid, causing Line 618 to persistsrc_uuid == dst_uuid. Check resolved UUIDs before insertion and add a dedup-merge regression test.Proposed fix
let (src_uuid, dst_uuid) = match (src_uuid, dst_uuid) { (Some(s), Some(d)) => (s, d), (src, dst) => { // ... } }; + if src_uuid == dst_uuid { + eprintln!( + "liminis-context-graph: dropping self-referential edge at commit: '{}' → '{}'", + edge.source_name, edge.target_name + ); + continue; + } conn.insert_relates_to_edge(&RelatesToEdge {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab1df856-5171-4aa7-a222-a398c3a64367
📒 Files selected for processing (2)
crates/core/src/episode.rscrates/core/tests/edge_endpoint_resolution.rs
There was a problem hiding this comment.
Good, this confirms the design. I have what I need to write the review now.
Summary
Reviewed the salvage/deferred-drop rework in episode.rs, the build_edge_tool_schema/sanitize_entity_names additions in extractor.rs/prompts/mod.rs, the prompt wording changes, and the new/extended integration tests (edge_endpoint_resolution.rs, ipc_parity.rs). The core design is sound and well-documented (ADR-0051 clearly explains the rationale for making Phase C the sole drop authority), and the test coverage for salvage, adversarial non-collapse, and the post-salvage self-referential recheck is solid. I found one real correctness gap worth addressing: the sanitize_entity_names transform used to build the schema enum (and the prompt's entity list) can diverge from the actual ExtractedEntity.name values stored in extraction.entities, because control characters are stripped by deletion rather than by replacement-with-space, and dedup is exact-string rather than the same trim+lowercase normalization used everywhere else in episode.rs. For entities whose extracted name contains stray control characters, this can make a batch-local edge endpoint that should exact-match fail the entity_name_set/name_to_uuid lookups and fall through to salvage (or the persisted-graph fallback) unnecessarily — undermining part of what FR-001 is meant to guarantee. This is a narrow edge case, not a blocker, but worth a small fix (e.g. replace control chars with a space, or normalize with the same trim/lowercase key used elsewhere) given the PR is specifically about not losing edges to endpoint-name mismatches.
|
🏭 Fabrik — stage: Validate Validation ReportRequirements: 6/6 FRs verified
SC-003's regression coverage (off-list endpoint semantically equivalent to a batch entity resolves rather than drops) and the drop-count assertion are both present in Test Suite: PASSED
Regressions: None detectedNo TODO/FIXME left in the diff; changed file set matches the Plan's file list exactly (episode.rs, extractor.rs, handlers.rs, three prompt files, prompts/mod.rs, two test files, ADR-0051, README). Branch State
Issues FoundNone. Verdict: READY TO MERGEValidation passed: all 6 functional requirements verified against code, full local gate clean (fmt, 800 tests, clippy -D warnings), no regressions, branch rebased and pushed. PR #285 mergeable: MERGEABLE, mergeStateStatus: BLOCKED (pending in-progress CI test job, not a conflict). Used 27/100 turns, 15k input / 5k output tokens. |
There was a problem hiding this comment.
I reviewed the diff for PR #285 (schema-enum constraint on extract_edges, the concept-ban prompt reword, the pre-lock salvage/deferred-drop rework in episode.rs, and the edges_dropped_unresolvable IPC threading), read ADR-0051, and ran the affected test suites (edge_endpoint_resolution, ipc_parity, prompts::) plus a full workspace build — all green. The core mechanism is sound: the enum constraint and empty-list HTTP skip in extractor.rs are correctly gated, the salvage step in episode.rs correctly reuses batch-only entity embeddings (never touching the persisted graph, per the ADR's stated rationale), the post-salvage self-loop re-check closes the collision case the tests exercise, and Phase C is unambiguously the sole drop/count authority. Two minor things worth a look before merge: an observability gap around self-referential drops, and an unbounded schema-size consideration for very large entity batches.
| // (write-lock held), which is now the *sole* point that resolves an endpoint — falling back | ||
| // to the persisted graph — or finally drops the edge, making `edges_dropped_unresolvable` | ||
| // authoritative (FR-003, FR-005) instead of one of two independent, easily-desynced passes. | ||
| extraction.edges.retain(|edge| { |
There was a problem hiding this comment.
Self-referential edges (both the pre-salvage drop here and the post-salvage collision drop at line 352) are only reported via eprintln! — there's no counter for them, unlike edges_dropped_unresolvable. Since this PR's stated purpose (FR-004) is making drop counts observable to a caller instead of only on stderr, a batch where salvage collapses several distinct off-list endpoints onto the same entity (producing several self-loops) will silently under-report total edge loss in the knowledge_process_chunk JSON result — edges_extracted + edges_dropped_unresolvable won't account for edges lost via this path. Consider a documented note on AddEpisodeResult (or a small additional counter) clarifying that self-referential drops aren't included in either existing counter.
| "required": ["edges"] | ||
| } | ||
| }); | ||
| let edge_tool = build_edge_tool_schema(&sanitized_names); |
There was a problem hiding this comment.
The source_name/target_name enum is built from the full sanitized entity-name list with no size cap. On a concept-heavy, unchunked page with a very large entity count — the PR's own motivating scenario — this could produce a very large extract_edges tool schema on every edge-extraction call, inflating request size/cost per call. Not a correctness bug, but worth confirming there's an upstream chunk-size or entity-count ceiling that keeps this bounded in practice, since nothing in this PR caps it.
…point matching sanitize_entity_names strips control characters before an entity name reaches the model (in the edge prompt's entity list and the tool schema's enum), but episode.rs's entity_name_set/name_to_uuid lookups keyed off the original, unsanitized name (trim+lowercase only). An entity name containing a control character therefore couldn't be matched by the sanitized form the model echoes back, defeating part of FR-001's batch-local matching guarantee. Adds prompts::normalize_name as the single control-strip+trim+lowercase key used on both sides of every entity/edge-endpoint name comparison in episode.rs, and a regression test covering the control-char case.
| // persisted Entity table (e.g. a recurring hub entity created in an earlier ingest | ||
| // batch that survived Site 1's validation via the same fallback) (FR-002, FR-003). | ||
| let src_uuid = match name_to_uuid.get(&edge.source_name.trim().to_lowercase()) { | ||
| let src_uuid = match name_to_uuid.get(&normalize_name(&edge.source_name)) { |
There was a problem hiding this comment.
Removing the pre-lock globally_resolved lookup (per ADR-0051) means every off-batch, unsalvaged edge endpoint now triggers get_entity_by_name_ci here, inside Phase C's spawn_blocking closure, while the exclusive write lock (_write_guard) is held. Previously this DB round-trip happened pre-lock (outside the critical section) for edges that failed the batch-local check. For the concept-heavy/unchunked pages this PR specifically targets (up to 97.8% of edges previously dropped, now flowing through to this fallback), the write-lock hold time could grow meaningfully with edge count, serializing concurrent add_episode callers for longer than before. The ADR's "Consequences" section acknowledges the timing shift but frames it as free ("doesn't add a new query, only moves it later") without addressing that moving it under the lock is exactly the kind of change AD-4's three-phase split was designed to avoid. Worth a benchmark on a batch with many unresolved edges before merge, or at minimum flagging as a known follow-up alongside the DEDUP_THRESHOLD caveat already called out.
| } | ||
| } | ||
|
|
||
| fn strip_control_chars(s: &str) -> String { |
There was a problem hiding this comment.
strip_control_chars deletes control characters outright instead of replacing them with a separator, so e.g. "Bob\ncontrol\tchars" becomes "Bobcontrolchars" (asserted by the test at line ~301/311 in this same file). This is used both for the sanitized names shown to the model (entity list + schema enum) and, via normalize_name, as the matching key for batch-local and salvage edge-endpoint resolution. Gluing tokens together on either side of a stripped control character is a plausible (if low-probability) source of an unintended name collision — two distinct entities could normalize to the same concatenated string, or a legitimate entity name could accidentally collide with a glued fragment of another. Substituting a space for control characters (then re-collapsing whitespace) would avoid the concatenation while still meeting the "no newlines in the bullet list / JSON enum" requirement that motivated this function.
1 participant
Closes #281
Summary
Fixes the defect where an edge-extraction endpoint that the entity pass never produced was silently discarded — up to 97.8% of edges on an unchunked, concept-heavy page. Three independent-but-complementary changes:
build_edge_tool_schemanow puts anenumof the batch's sanitized entity names onsource_name/target_namein theextract_edgestool schema, so a compliant model literally cannot name an off-list endpoint. The edge-extraction HTTP call is skipped entirely when the sanitized name list is empty (an emptyenumis invalid schema).extract_text.txt/extract_message.txt/extract_json.txtbanned "abstract concepts" outright while the ontology offers aConcepttype. Reworded the ban to a vagueness/specificity test (a vague feeling like "joy" is still excluded; a specific, named, Wikipedia-articleable concept like "climate change" is now extracted asConcept), with a worked example added to each prompt.episode.rsno longer permanently drops an edge for an unresolvable endpoint. It now only filters self-referential edges (always correct) and salvages an off-list endpoint by cosine-matching its name embedding against the batch's own entity embeddings (reusingDEDUP_THRESHOLD = 0.85), rewriting the edge to the matched entity's canonical name. Anything left unsalvaged passes through to Phase C (write-lock held), which is now the sole authoritative point that resolves against the persisted graph or finally drops an edge. This also removes the redundant pre-lockglobally_resolvedDB lookup added in Edges to already-existing entities silently dropped — resolve edge endpoints against the global entity table #209/PR fix(episode): resolve edge endpoints against persisted Entity table (#209) #218.edges_dropped_unresolvableis threaded from Phase C ontoAddEpisodeResultand intoknowledge_process_chunk's JSON result, rather than being observable only viaeprintln!.FR-006 (OpenAI-compatible path) falls out for free since the salvage/defer logic lives in
episode.rs, downstream of eitherExtractorimplementation — only the schemaenum(FR-001) is Anthropic-specific.Behavior change to flag for review
edges_extractedonAddEpisodeResult/ in theknowledge_process_chunkIPC result now means "edges actually inserted" rather than "edges surviving the pre-lock filter." This is a public IPC field; values should only go up relative to prior behavior (edges Phase C's fallback could have resolved were previously pre-dropped), never down.Documentation
knowledge_process_chunkdescription now mentions the salvage attempt andedges_dropped_unresolvable.Known limitations (flagged per the Plan's risk list)
DEDUP_THRESHOLD = 0.85salvage threshold is verified only against controlled test vectors (NameMapEmbedder), not real embedding output — a fast follow-up if production salvage proves too aggressive or too conservative.Test plan
cargo fmt --all -- --checkcleancargo test— 799 passed, 8 ignored (full debug suite)cargo clippy --all-targets -- -D warnings— no issuessanitize_entity_namesunit tests (control chars, dedup, empty-after-sanitize)build_edge_tool_schemaenum-contents test + empty-list HTTP-skip testedge_endpoint_resolution.rsipc_parity.rs'stest_knowledge_process_chunk_okto assertedges_dropped_unresolvableis present and numericGlobal_warming/LEMON/Capacitive_Micromachinedfixture pages to confirm the ≤5% drop-rate bound (SC-001/002) and at least oneConceptentity (SC-004) — recommended for Validate