core: strict-mode entity filter reclassifies out-of-vocabulary entities instead of dropping them - #313
Conversation
|
🏭 Fabrik — stage: Implement Task checklist updated. Now let's emit the PR creation marker. Implemented reclassify-not-drop for strict-mode entities (mirroring ADR-0310's edge-side fix): out-of-vocabulary entity types now get the Used 100/100 turns, 0k input / 39k output tokens. |
![handarbeit-pruefer[bot]](https://avatars.githubusercontent.com/in/4408765?s=60&v=4){kind=small}
There was a problem hiding this comment.
Confirmed — this all lines up correctly. I'm done reviewing; I have no findings to report.
I reviewed the entity-side reclassify-not-drop implementation (crates/core/src/episode.rs, types.rs, handlers.rs), the new tests in ontology_integration.rs and ipc_parity.rs, README updates, and ADR-0312. The change mirrors the edge-side ADR-0310 pattern closely: original_entity_type is unconditionally cleared right after extraction (guarding against untrusted-JSON injection) before being legitimately re-set only in the strict-mode reclassify branch; the empty/"Entity" passthrough, in-vocabulary canonicalization, and out-of-vocabulary → Unclassified reclassification are all correctly ordered; the dead edges.clear() cascade is fully removed; the tally is counted once in Phase A with no double-counting risk (verified against the dedup-merge test, which intentionally still counts a merged-but-reclassified entity); and attributes.original_entity_type is only ever populated on the reclassified path. Cross-checked the ADR's claim that reclassified entities surface automatically via knowledge_reprocess_entity_types's OffOntology scope — confirmed in corrections.rs, which flags any label absent from the ontology's type-name set. Docs and tests are consistent with the implementation throughout. I found no bugs, correctness issues, or design problems worth flagging.
There was a problem hiding this comment.
Pull request overview
This PR updates strict ontology mode to reclassify out-of-vocabulary entities to Unclassified instead of dropping them, preserving the original extracted type in attributes.original_entity_type and exposing a new per-run tally (entities_reclassified_unclassified) via IPC. It also adds ADR-0312 and updates docs/tests to reflect and lock in the new behavior.
Changes:
- Rework strict-mode entity-type filtering in
add_episodeto reclassify rather than drop, and persist the original type in entityattributes. - Add
ExtractedEntity.original_entity_typeplusAddEpisodeResult.entities_reclassified_unclassified, and surface the tally inknowledge_process_chunk. - Add ADR-0312 + README updates, and expand integration tests to cover disposition, tallying, endpoint survival, passthrough, and dedup.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| specs/312-strict-mode-still-deletes/spec.md | Spec Kit feature spec capturing the reclassify-not-drop decision and acceptance scenarios. |
| README.md | Updates strict-mode ontology documentation to describe entity reclassification + preservation. |
| docs/adr/index.md | Adds ADR-0312 entry to the ADR index. |
| docs/adr/0312-entity-strict-mode-reclassifies-not-drops.md | Records the strict-mode entity reclassification decision and rationale. |
| crates/core/src/types.rs | Adds ExtractedEntity.original_entity_type for preserving rejected strict-mode types. |
| crates/core/src/episode.rs | Implements entity reclassification + attribute emission + new tally in AddEpisodeResult. |
| crates/core/src/handlers.rs | Surfaces entities_reclassified_unclassified in knowledge_process_chunk response JSON. |
| crates/core/src/extractor.rs | Updates mock extractor entity literals to include the new field. |
| crates/core/src/llm_router.rs | Updates tests to include the new field in extracted entities. |
| crates/core/tests/ontology_integration.rs | Replaces drop-expectation tests with reclassify/tally/endpoint/dedup/passthrough assertions. |
| crates/core/tests/ipc_parity.rs | Asserts entities_reclassified_unclassified is numeric in IPC response. |
| crates/core/tests/extraction_quality.rs | Updates test extractor outputs for new entity field. |
| crates/core/tests/edge_endpoint_resolution.rs | Updates helper extraction results for new entity field. |
| crates/core/tests/dedup_auto_heal_integration.rs | Updates test extractor outputs for new entity field. |
| crates/core/tests/cross_episode_dedup.rs | Updates test extraction results for new entity field. |
| crates/core/tests/concurrent_rw_integration.rs | Updates entity literals for new field in concurrency test. |
| crates/eval/tests/harness_integration.rs | Updates eval harness fixtures to include new entity field. |
| crates/eval/src/scoring.rs | Updates scoring tests’ entity construction for new field. |
| crates/eval/src/runner.rs | Updates runner tests’ entity construction for new field. |
| crates/eval/src/pairwise.rs | Updates pairwise tests’ entity construction for new field. |
| crates/eval/src/metrics.rs | Updates metrics tests’ entity construction for new field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Warning Review limit reached
Next review available in: 4 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 (22)
📝 WalkthroughWalkthroughStrict ontology mode retains unsupported non-empty entity types by relabeling them Assessment against linked issues
Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c25a08b-827d-46e8-b12e-69a0358465aa
📒 Files selected for processing (21)
README.mdcrates/core/src/episode.rscrates/core/src/extractor.rscrates/core/src/handlers.rscrates/core/src/llm_router.rscrates/core/src/types.rscrates/core/tests/concurrent_rw_integration.rscrates/core/tests/cross_episode_dedup.rscrates/core/tests/dedup_auto_heal_integration.rscrates/core/tests/edge_endpoint_resolution.rscrates/core/tests/extraction_quality.rscrates/core/tests/ipc_parity.rscrates/core/tests/ontology_integration.rscrates/eval/src/metrics.rscrates/eval/src/pairwise.rscrates/eval/src/runner.rscrates/eval/src/scoring.rscrates/eval/tests/harness_integration.rsdocs/adr/0312-entity-strict-mode-reclassifies-not-drops.mddocs/adr/index.mdspecs/312-strict-mode-still-deletes/spec.md
|
🏭 Fabrik — stage: Review Nothing to push since I made no changes. The implementation is already clean and complete. Review FindingsVerified
BlockingNone. Reviewed the entity reclassify-not-drop implementation for issue #312. Full local gate passes (fmt, 913 tests, clippy with Used 31/100 turns, 0k input / 6k output tokens. |
|
🏭 Fabrik — stage: Review (review feedback addressed) Confirmed — same two placeholders exist in the issue body. I'll produce the updated issue body with those references fixed, matching the Review Comment Findings — ResolutionFixed (real bugs, confirmed by code inspection):
Fixed (doc precision, quick wins):
Dismissed:
Not done: thread resolution via Both fixes pushed as separate commits ( Threads addressed:
Resolved 6 review thread(s) across 6 comment(s). |
There was a problem hiding this comment.
Confirms the claim. Reviewed everything thoroughly — implementation, tests, ADR, and README are internally consistent, with previous review findings already addressed. I found no correctness or security issues.
I reviewed the entity-side reclassify-not-drop change (crates/core/src/types.rs, crates/core/src/episode.rs), the new ADR-0312, README updates, and the test coverage in ontology_integration.rs/ipc_parity.rs. The implementation faithfully extends the ADR-0310 edge-side pattern to entities: the strict-mode filter reclassifies rather than drops, original_entity_type is unconditionally cleared before mode-specific filtering (defeating untrusted-JSON injection of that key), the dead edges.clear() cascade is fully removed, and edge cases (empty name before tally, "Entity"/case-variant passthrough not leaking into labels, merge-vs-insert tally semantics) are each covered by a dedicated test and matched by an explicit code comment. I did not find bugs, security issues, or design problems worth flagging — the one subtlety I traced (the reclassified-tally counting an entity that later merges away, discarding its Unclassified label/attributes) is deliberate, documented in the ADR's "Alternatives Considered" section, and asserted by strict_mode_reclassified_entity_dedups_against_declared_type.
|
🏭 Fabrik — stage: Validate All gates pass. Writing the validation report. Validation ReportRequirements: 8/8 passed
Also verified: SC-003 (no Test Suite: PASSED
Regressions: None detectedDiff is scoped exactly to the entity reclassify path, its call-site plumbing ( Code completenessNo TODO/FIXME/debug leftovers in the diff. All plan tasks (Implement + Review fix commits) reflected in Branch stateRebased onto latest Verdict: READY TO MERGEValidation passed. PR mergeable: MERGEABLE, mergeStateStatus: CLEAN. All 8 requirements (FR-001–008) verified against code and tests; full local gate clean (915 tests passed, fmt/clippy clean); no regressions; branch rebased and pushed. Used 21/100 turns, 0k input / 5k output tokens. |
8e76e22 to
c8147eb
Compare
There was a problem hiding this comment.
Everything is in order. This is a well-implemented, thoroughly tested PR with no issues found.
I reviewed the strict-mode entity reclassification change (episode.rs, types.rs, handlers.rs), the removed edges-clearing cascade, the ADR-0312 rationale, and the new tests in ontology_integration.rs/ipc_parity.rs. The implementation is symmetric with the existing edge-side reclassify-not-drop logic (ADR-0310), correctly clears original_entity_type before any mode-specific filtering (matching the untrusted-JSON discipline for the edge field), preserves the raw (non-normalized) out-of-vocabulary label in attributes, and counts the tally at the correct point in the pipeline given entities are never dropped downstream. Dedup/merge semantics, PascalCase vs SCREAMING_SNAKE_CASE label conventions, and the removal of the dead "no entities remain → clear edges" branch all check out against the code and are backed by targeted tests covering the tally, dedup-merge interaction, empty-name edge case, and edge survival with a reclassified endpoint. No remote/main ref was available in this checkout to produce a line-scoped diff, so I reviewed the current state of the relevant files directly against the PR description; I found no bugs, correctness issues, or design problems worth flagging.
|
🏭 Fabrik — stage: Validate All checks are consistent with an in-progress CI run (not a conflict). All local gates pass and requirements are verified. Writing the final report. Validation ReportRequirements: 8/8 passed
Also verified: no Test Suite: PASSED
Regressions: None detectedDiff ( Branch stateRebased onto latest Verdict: READY TO MERGERe-validated after the rebase that resolved the ADR-index conflict. All 8 requirements (FR-001–008) confirmed against code and tests; full local gate clean (925 tests, fmt/clippy clean); no regressions. PR #313 mergeable: MERGEABLE, mergeStateStatus: BLOCKED (CI still running on latest commit, not a conflict). Ready to merge once CI completes. Used 5/100 turns, 0k input / 8k output tokens. |
Mirrors ADR-0310's edge-side treatment: an out-of-vocabulary entity type
is rewritten to a new `Unclassified` sentinel label instead of being
dropped, with the original type preserved in `attributes` as
`{"original_entity_type": "<original>"}`. An empty/absent type (or the
literal "Entity") continues to resolve as a plain untyped Entity, not a
reclassification. The all-entities-empty cascade that used to clear
`extraction.edges` is removed, since entities are no longer removed for
their type. The per-run reclassification count is surfaced as
`entities_reclassified_unclassified` on `AddEpisodeResult` and in
`knowledge_process_chunk`'s response, mirroring
`edges_reclassified_unclassified` (issue #312).
…ures Mechanical follow-up to the new ExtractedEntity field — updates every existing struct-literal call site across crates/core and crates/eval tests/fixtures so they compile against the extended struct.
Replaces strict_mode_entity_type_still_drops_not_reclassifies with strict_mode_entity_type_reclassifies_not_drops, asserting the retained entity's Unclassified label and recoverable attributes. Updates the older strict_mode_entity_filtering_drops_out_of_vocab expectation to match. Adds coverage for: the per-run reclassification tally over N entities (User Story 3), an edge surviving with a reclassified endpoint (User Story 2), the empty/"Entity"-type passthrough (FR-007), and dedup against a pre-existing declared-type entity (FR-008/SC-006). Also asserts entities_reclassified_unclassified is numeric in the knowledge_process_chunk IPC response, mirroring the existing edges_reclassified_unclassified assertion.
Documents the decision to reclassify out-of-vocabulary entity types to Unclassified rather than dropping them, citing ADR-0310 as precedent and independently arguing the edge-endpoint-cascade and schema-closure reasons this holds for entities. Updates the ontology.yaml format example and the Modes table's strict entity-types cell to describe the new behavior instead of the old drop behavior.
…abel leak Two real bugs from Copilot/CodeRabbit review on issue #312: 1. entities_reclassified_unclassified could overcount: the strict-mode reclassify loop ran before the empty-name retain, so an out-of-vocab entity with an empty/whitespace name was tallied then dropped, never reaching storage. Move the empty-name retain before the reclassify loop so only persisted entities are counted. 2. The empty/"Entity" passthrough branch left entity_type un-normalized, so a case/separator variant (e.g. "entity", "ENTITY") bypassed reclassification correctly but then leaked as a spurious label via make_insert_row's raw-string check. Normalize entity_type on the passthrough path too. Adds regression tests for both.
- ADR-0312: reword a line so it doesn't start with "#310" (markdownlint MD018 false-positive heading trigger). - spec.md: replace the docs/adr/0312-<slug>.md placeholder with the actual ADR filename now that it exists, in both references. - README: qualify "never dropped" as "never dropped for its type alone" in both the strict-mode YAML comment and the Modes table, since edges can still be dropped later for unrelated reasons (self-referential, unresolvable endpoint).
c8147eb to
4875327
Compare
The docs-only rebase resolution updated docs/ontology.md's strict-mode description for issue #312 but left the generated llms-full.txt stale, tripping the docs-drift CI check.
|
🏭 Fabrik — stage: Validate
Validation ReportRequirements: 8/8 passed (re-confirmed against current code)
Test Suite: PASSED
Regressions: None detectedDiff scoped to the entity reclassify path, its fixture threading, docs, ADR, and (this round) the regenerated Issue found and fixed this roundThe "Docs drift check" workflow's Branch stateRebased onto latest Verdict: READY TO MERGEPre-completion gate: rebase clean against Validation passed after fixing a stale-docs CI failure: Used 17/100 turns, 0k input / 3k output tokens. |
There was a problem hiding this comment.
Not critical — I've already verified with build+clippy. I have enough to write the review.
Review
I reviewed the strict-mode entity reclassification change (crates/core/src/episode.rs, types.rs, handlers.rs), the removal of the "no entities remain → clear all edges" cascade, ADR-0312, and the accompanying tests in ontology_integration.rs/ipc_parity.rs. The implementation faithfully follows the edge-side precedent (ADR-0310): original_entity_type is unconditionally cleared before any mode-specific filtering to prevent hallucinated/injected JSON keys from surviving, the empty-name filter runs before the tally to keep entities_reclassified_unclassified in sync with what's actually persisted, and the reclassify logic correctly distinguishes "no type" from "in-vocabulary" from "out-of-vocabulary." I traced the label/attribute construction in make_insert_row, the dedup interaction (merge vs. insert), and the docs/ontology.md/ADR-0312 text against the code and found them consistent. cargo build and cargo clippy --all-targets -- -D warnings are clean. I did not find correctness bugs or security issues; only a couple of minor points worth a second look.
| false | ||
| e.original_entity_type = Some(e.entity_type.clone()); | ||
| e.entity_type = ENTITY_UNCLASSIFIED.to_string(); | ||
| entities_reclassified_unclassified += 1; |
There was a problem hiding this comment.
When a reclassified entity dedups by exact name-match into an existing entity (PhaseBResult::NameMatch), the Merge branch only updates summary, never labels/attributes — so entities_reclassified_unclassified counts this extraction event even though the Unclassified label/original_entity_type never reaches storage for it (confirmed by strict_mode_reclassified_entity_dedups_against_declared_type, which asserts the tally increments even on merge). This is called out and intentionally accepted in ADR-0312's consequences section, but it does mean the counter measures "reclassification decisions made during processing" rather than "entities persisted with the Unclassified label" — worth double-checking that downstream consumers of this IPC field (dashboards, alerts) are told which of the two it represents, since the name suggests the latter.
| e.name, e.entity_type | ||
| ); | ||
| false | ||
| e.original_entity_type = Some(e.entity_type.clone()); |
There was a problem hiding this comment.
Minor: if the model hallucinates the literal type string "Unclassified" for an entity type that is itself out of vocabulary, original_entity_type gets set to Some("Unclassified") and entity_type is rewritten to the same value — a no-op-looking reclassification whose attributes.original_entity_type is indistinguishable from the sentinel itself. Harmless, but slightly confusing when later inspecting attributes to recover the "real" original type.
There was a problem hiding this comment.
I've reviewed the implementation thoroughly. Now let me finalize my findings.
Reviewed the entity-side reclassify-not-drop change in episode.rs, types.rs, handlers.rs, the new ADR-0312, README/docs updates, and the new/updated tests in ontology_integration.rs. The implementation is careful and well-documented: original_entity_type is scrubbed from untrusted extractor JSON before any mode-specific logic runs (mirroring the edge-side discipline), the empty/"Entity" passthrough avoids spurious reclassification, the tally is counted at the correct point (no Phase C deferral needed since entities are never subsequently dropped for type), and the dead "clear all edges" cascade is gone. Tests cover the disposition, per-run tally, edge survival on a reclassified endpoint, and dedup-merge into a declared-type entity. Two minor points below are worth a look but neither blocks anything.
Additional findings (could not anchor to diff)
crates/core/src/episode.rs:550: Pre-existing type-conflict logging, but worth flagging in the context of this PR: when a reclassified entity (extracted.entity_type == "Unclassified") name-matches an existing entity stored under its correct declared type (per the PR description's own dedup scenario — "merging into a pre-existing entity stored under its correct declared type"), this comparison (existing.labels won't contain "Unclassified") will fire a false-positive type conflict warning on every such merge, even though this is expected, documented behavior under reclassify-not-drop. Consider special-casing Unclassified here so the log isn't misleading.
|
|
||
| The strict-mode entity filter in `episode.rs` never drops an entity for its `entity_type` alone. | ||
|
|
||
| 1. **An entity whose normalized type is empty or the literal `"Entity"` passes through |
There was a problem hiding this comment.
Neither this ADR nor the code adds any guard against an ontology author declaring an entity type literally named Unclassified in their vocabulary. If they do, a genuine in-vocabulary Unclassified-typed entity becomes indistinguishable from a reclassified out-of-vocabulary one (both get exactly the Unclassified label, and the genuine one won't have original_entity_type set, but there's no rejection/warning at ontology-load time to catch the collision early). Same gap likely exists for UNCLASSIFIED on the relation side from ADR-0310, so this may be an accepted pre-existing risk rather than new, but worth a one-line note in the ADR if so.
Closes #312
Summary
Completes the reclassify-not-drop symmetry ADR-0310 established for edges: under
strictontology mode, an entity extracted with a type outside the declared vocabulary is now retained with a newUnclassifiedlabel instead of being dropped, with its original type preserved and recoverable fromattributes.original_entity_type. This also removes the cascade where the strict-mode entity filter's "no entities remain" branch unconditionally cleared all edges for a chunk — since entities are never removed for their type anymore, that branch is now dead and has been deleted.Key changes
ExtractedEntitygets a neworiginal_entity_type: Option<String>field, unconditionally cleared before any mode-specific filtering (same untrusted-JSON discipline already applied to the edge-sideoriginal_relation_type).episode.rsreclassifies instead of dropping: an empty/absent type (or literal"Entity") resolves as a plain untypedEntity(unchanged, no reclassification); an in-vocabulary type is rewritten to canonical form as before; anything else getsentity_type = "Unclassified"with the original preserved.AddEpisodeResultgainsentities_reclassified_unclassified, tallied directly in the Phase A filter loop (no Phase C deferral needed, unlike the edge-side tally — an entity, once reclassified, is never subsequently dropped). Surfaced inknowledge_process_chunk's IPC response.make_insert_row'sattributesfield now emits{"original_entity_type": "<orig>"}when applicable, instead of always"{}".Test plan
cargo fmt --allcleancargo test— 913 passed, 8 ignored (full workspace suite)cargo clippy --all-targets -- -D warningscleancrates/core/tests/ontology_integration.rs: reclassify-not-drop disposition (labels + attributes), per-run tally over N out-of-vocabulary entities, edge survival with a reclassified endpoint, empty/"Entity"-type passthrough, and dedup against a pre-existing declared-type entitycrates/core/tests/ipc_parity.rsassertsentities_reclassified_unclassifiedis numeric in theknowledge_process_chunkresponse